Move IDBTransaction to WebIDL
Categories (Core :: Storage: IndexedDB, defect) Core ▾Core
Shared components used by Firefox and other Mozilla software, including handling of Web content; Gecko, HTML, CSS, layout, DOM, scripts, images, networking, etc. Issues with web page layout probably go here, while Firefox user interface issues belong in the
Firefoxproduct. (
More info)
Storage: IndexedDB ▾Core :: Storage: IndexedDB
IndexedDBimplementation, exposed through self.indexedDB.
Webcompat Score Accessibility Severity Size Estimate Performance Impact a11y-review Webcompat Priority Tracking Status relnote-firefox thunderbird_esr115 thunderbird_esr128 thunderbird_esr140 firefox-esr115 firefox-esr128 firefox-esr140 firefox140 firefox141 firefox142 People (Reporter: Ms2ger, Assigned: baku)Reset Assignee to default
Details (Keywords: addon-compat, dev-doc-complete, site-compat)Bug Flags:
in-testsuite + in-testsuite behind-pref firefox-backlog sec-bounty sec-bounty-hof in-qa-testsuite qe-verifyThis bug is publicly visible.
Attachments (1 file, 3 obsolete files)Assignee: nobody → amarchesini
Comment on
attachment 772924 [details] [diff] [review]patch Review of
attachment 772924 [details] [diff] [review]: ----------------------------------------------------------------- It looks good overall, requesting feedback from bent on the style in IDB webidl files. ::: dom/indexedDB/IDBTransaction.cpp @@ +586,5 @@
> { > NS_ASSERTION(NS_IsMainThread(), "Wrong thread!"); > NS_ASSERTION(aRequest, "This is undesirable."); > > + mozilla::ErrorResult rv;this builds fine w/o the prefix on mac, is this needed only on windows ? you could add a new "using" for that since it is needed more than once @@ +688,5 @@
> } > > uint32_t count = arrayOfNames->Length(); > for (uint32_t index = 0; index < count; index++) { > + if (!list->Add(arrayOfNames->ElementAt(index))) {add NS_WARNING() here ? @@ +721,5 @@
> } > > nsRefPtr<IDBObjectStore> objectStore = > GetOrCreateObjectStore(aName, info, false); > + if (!objectStore) {add NS_WARNING() here ? @@ +1282,5 @@
> +JSObject* > +IDBTransaction::WrapObject(JSContext* aCx, JS::Handle<JSObject*> aScope) > +{ > + return IDBTransactionBinding::Wrap(aCx, aScope, this); > +}Nit: I would move this before GetMode() ::: dom/indexedDB/IDBTransaction.h @@ +29,1 @@
>Nit: move before mozilla/dom/indexedDB/IDBDatabase.h @@ +214,5 @@
> { > return mSerialNumber; > } > #endif >I think it would be cleaner to use following order and comments: // nsWrapperCache virtual JSObject* WrapObject(JSContext* aCx, JS::Handle<JSObject*> aScope) MOZ_OVERRIDE; // WebIDL nsPIDOMWindow* GetParentObject() const { return GetOwner(); } IDBTransactionMode GetMode(ErrorResult& aRv) const; ... @@ +216,5 @@
> } > #endif > > + // WebIDL > + nsPIDOMWindow* GetParentObject() const;you can inline this @@ +221,5 @@
> + > + virtual JSObject* > + WrapObject(JSContext* aCx, JS::Handle<JSObject*> aScope) MOZ_OVERRIDE; > + > + IDBTransactionMode GetMode(ErrorResult& aRv) const;return value on separate line, dtto for other methods, I know there are some methods in this file which don't follow it, but we are slowly converging to the new style :) @@ +223,5 @@
> + WrapObject(JSContext* aCx, JS::Handle<JSObject*> aScope) MOZ_OVERRIDE; > + > + IDBTransactionMode GetMode(ErrorResult& aRv) const; > + > + nsIIDBDatabase* Db() const;you can inline this too (including the main thread assertion) this method isn't virtual anymore, so it makes sense to inline it here @@ +230,5 @@
> + > + already_AddRefed<nsIIDBObjectStore> > + ObjectStore(const nsAString& aName, ErrorResult& aRv); > + > + void Abort(ErrorResult& aRv);inline this one too ::: dom/indexedDB/ipc/IndexedDBParent.cpp @@ +812,5 @@
> > + ErrorResult rv; > + nsCOMPtr<nsIIDBObjectStore> store = mTransaction->ObjectStore(name, rv); > + if (rv.Failed()) { > + return false;You should add a warning here (NS_WARNING) ::: dom/webidl/IDBTransaction.webidl @@ +14,5 @@
> + "readonly", > + "readwrite", > + "versionchange" > +}; > +I'm not sure what style we want to use for IDB webidl interfaces. The style in this patch is obviously the style used in HTML*.webidl and the IndexedDB spec uses it too. nsIIDB*.idl used a different style and IDBFactory.webidl uses it too. Personally, I'd prefer the nsIIDB*.idl style, but I'll let bent to be the judge. Also, all the interface comments got removed in this patch, some of them could stay I think. For example these (slightly modified): // Don't commit the transaction. // Event handler that fires when the transaction is completed successfully. // Event handler that fires when the transaction is aborted. ::: js/xpconnect/src/nsXPConnect.cpp @@ +43,1 @@
>Nit: what about moving this before IDBVersionChangeEventBinding.h @@ +484,5 @@
> // Init WebIDL binding constructors wanted on all XPConnect globals. > if (!IDBVersionChangeEventBinding::GetConstructorObject(aJSContext, global) || > !TextDecoderBinding::GetConstructorObject(aJSContext, global) || > !TextEncoderBinding::GetConstructorObject(aJSContext, global) || > + !IDBTransactionBinding::GetConstructorObject(aJSContext, global) ||Nit: move before IDBVersionChangeEventBinding.h too ?
(In reply to Jan Varga [:janv] from
comment #2)
> ::: dom/webidl/IDBTransaction.webidl > @@ +14,5 @@ > > + "readonly", > > + "readwrite", > > + "versionchange" > > +}; > > + > > I'm not sure what style we want to use for IDB webidl interfaces.We want to match the spec.
(In reply to Jan Varga [:janv] from
comment #2)
> ::: dom/indexedDB/IDBTransaction.h > @@ +29,1 @@ > > > > Nit: move before mozilla/dom/indexedDB/IDBDatabase.h >that was for: +#include "mozilla/dom/IDBTransactionBinding.h"
Comment on
attachment 781008 [details] [diff] [review]transaction.patch Review of
attachment 781008 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/indexedDB/IDBTransaction.cpp @@ +35,5 @@
> #include "ipc/IndexedDBChild.h" > > #define SAVEPOINT_NAME "savepoint" > > +using namespace mozilla;hrm, I meant "using mozilla::ErrorResult", not entire mozilla namespace and put it after "using mozilla::dom::quota::QuotaManager;" please @@ +689,5 @@
> > uint32_t count = arrayOfNames->Length(); > for (uint32_t index = 0; index < count; index++) { > + if (!list->Add(arrayOfNames->ElementAt(index))) { > + NS_WARNING("Error adding a new element to a nsDOMStringList");Nit: What about just: "Failed to add element!" @@ +723,5 @@
> > nsRefPtr<IDBObjectStore> objectStore = > GetOrCreateObjectStore(aName, info, false); > + if (!objectStore) { > + NS_WARNING("objectStore doesn't exist");Nit: What about: "Failed to get or create object store!" ::: dom/indexedDB/IDBTransaction.h @@ +22,5 @@
> #include "nsRefPtrHashtable.h" > > #include "mozilla/dom/indexedDB/IDBDatabase.h" > #include "mozilla/dom/indexedDB/IDBWrapperCache.h" > +#include "mozilla/dom/indexedDB/IndexedDatabase.h"please don't do this, see
https://bugzilla.mozilla.org/show_bug.cgi?id=860731#c11@@ +27,2 @@
> #include "mozilla/dom/indexedDB/FileInfo.h" > +#include "mozilla/dom/IDBTransactionBinding.h"Nit: I meant to just move |#include "mozilla/dom/IDBTransactionBinding.h"| *before* |#include "mozilla/dom/indexedDB/IDBDatabase.h"| ::: dom/indexedDB/ipc/IndexedDBParent.cpp @@ +797,5 @@
> > + ErrorResult rv; > + nsCOMPtr<nsIIDBObjectStore> store = mTransaction->ObjectStore(name, rv); > + if (rv.Failed()) { > + NS_WARNING("Didn't get the object we expected!");Nit: what about just: "Failed to get object store!" ::: js/xpconnect/src/nsXPConnect.cpp @@ +35,5 @@
> #include "XPCQuickStubs.h" > > #include "mozilla/dom/BindingUtils.h" > #include "mozilla/dom/IDBVersionChangeEventBinding.h" > +#include "mozilla/dom/IDBTransactionBinding.h"Nit: *before* IDBVersionChangeEventBinding.h @@ +544,5 @@
> MOZ_ASSERT(js::GetObjectClass(global)->flags & JSCLASS_DOM_GLOBAL); > > // Init WebIDL binding constructors wanted on all XPConnect globals. > if (!IDBVersionChangeEventBinding::GetConstructorObject(aJSContext, global) || > + !IDBTransactionBinding::GetConstructorObject(aJSContext, global) ||Nit: *before* IDBVersionChangeEventBinding.h
Comment on
attachment 781961 [details] [diff] [review]patch Review of
attachment 781961 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/indexedDB/IDBTransaction.cpp @@ +36,5 @@
> > #define SAVEPOINT_NAME "savepoint" > > using namespace mozilla::dom; > +using namespace mozilla::ErrorResult;why *namespace* ? if you fix it, move it after "using mozilla::dom::quota::QuotaManager;" please @@ +723,5 @@
> > nsRefPtr<IDBObjectStore> objectStore = > GetOrCreateObjectStore(aName, info, false); > + if (!objectStore) { > + NS_WARNING("Failed to get or create object store");Nit: missing "!" ::: dom/indexedDB/IDBTransaction.h @@ +21,5 @@
> #include "nsInterfaceHashtable.h" > #include "nsRefPtrHashtable.h" > > +#include "mozilla/dom/IDBTransactionBinding.h" > +#include "mozilla/dom/indexedDB/IndexedDatabase.h"Did you read the comment in the other bug? Please keep |#include "mozilla/dom/indexedDB/IndexedDatabase.h"| at the original position/line. ::: dom/indexedDB/ipc/IndexedDBParent.cpp @@ +797,5 @@
> > + ErrorResult rv; > + nsCOMPtr<nsIIDBObjectStore> store = mTransaction->ObjectStore(name, rv); > + if (rv.Failed()) { > + NS_WARNING("Failed to get object store");Nit: missing "!", see all other warnings in the module
We need to update the documentation because IDBTransaction.READ_WRITE, IDBTransaction.READ_ONLY and IDBTransaction.VERSION_CHANGE have been removed. Can we check if this breaks some addons?
(In reply to Andrea Marchesini (:baku) from
comment #15)
> Can we check if this breaks some addons?They were marked as deprecated a long time ago (and they cause Error Console warnings if you use them) so I personally think it's fine to proceed here.
Status: NEW → RESOLVED
Closed: 12 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
RetroSearch is an open source project built by @garambo | Open a GitHub Issue
Search and Browse the WWW like it's 1997 | Search results from DuckDuckGo
HTML:
3.2
| Encoding:
UTF-8
| Version:
0.7.4