Comment on attachment 773961 [details] [diff] [review] patch Review of attachment 773961 [details] [diff] [review]: ----------------------------------------------------------------- I wonder whether we need something like NS_ENSURE_SUCCESS/NS_ENSURE_TRUE that works with ErrorResult. bent: What do you think? Should we just add warnings everywhere for now ? ::: dom/indexedDB/IDBCursor.cpp @@ +317,5 @@ > + *aResult = PREV_UNIQUE; > + break; > + > + default: > + return NS_ERROR_TYPE_ERR; The caller will assert while assigning this nsresult to an ErrorResult (NS_ERROR_TYPE_ERR can't be assigned directly). Actually I don't think you need to return NS_ERROR_TYPE_ERR here, replace it with |MOZ_CRASH("Unknown direction!");| and change return value to void ::: dom/indexedDB/IDBCursor.h @@ +8,5 @@ > #define mozilla_dom_indexeddb_idbcursor_h__ > > #include "mozilla/dom/indexedDB/IndexedDatabase.h" > #include "mozilla/dom/indexedDB/IDBObjectStore.h" > +#include "mozilla/dom/IDBCursorBinding.h" Nit: it would be nice to fix the ordering while you are here: #include "mozilla/dom/indexedDB/IndexedDatabase.h" #include "nsIIDBCursorWithValue.h" #include "mozilla/dom/IDBCursorBinding.h" #include "nsCycleCollectionParticipant.h" #include "mozilla/dom/indexedDB/IDBObjectStore.h" #include "mozilla/dom/indexedDB/Key.h" @@ +116,5 @@ > > static nsresult ParseDirection(const nsAString& aDirection, > Direction* aResult); > > + static nsresult ParseDirection(IDBCursorDirection aDirection, Nit: return value on a separate line ::: dom/indexedDB/IDBObjectStore.cpp @@ +526,5 @@ > /* > * Not setting this will cause JS_CHECK_RECURSION to report false > * positives > */ > + JS_SetNativeStackQuota(mRuntime, 128 * sizeof(size_t) * 1024); AFAIK, we fix white space only when we touch that code @@ +1063,5 @@ > if (rv == NS_ERROR_STORAGE_CONSTRAINT && updateInfo.indexUnique) { > // If we're inserting multiple entries for the same unique index, then > // we might have failed to insert due to colliding with another entry for > // the same index in which case we should ignore it. > + revert this change (see the comment above) @@ +1590,5 @@ > } > NS_ConvertUTF16toUTF8 convName(name); > uint32_t convNameLength = SwapBytes(convName.Length()); > > + if (!JS_WriteBytes(aWriter, &lastModifiedDate, sizeof(lastModifiedDate)) || revert this change @@ +1631,5 @@ > > nsresult rv; > int32_t id = token.ToInteger(&rv); > NS_ENSURE_SUCCESS(rv, rv); > + revert this change @@ +1723,5 @@ > mAutoIncrement(false), > mActorChild(nullptr), > mActorParent(nullptr) > { > NS_ASSERTION(NS_IsMainThread(), "Wrong thread!"); Nit: add a new line here @@ +1811,5 @@ > > +already_AddRefed<IDBRequest> > +IDBObjectStore::AddOrPut(JSContext* aCx, JS::Handle<JS::Value> aValue, > + const Optional<JS::Handle<JS::Value> >& aKey, > + bool aOverwrite, mozilla::ErrorResult& aRv) add |using mozilla::ErrorResult| and remove the namespace prefix here and elsewhere @@ +1827,3 @@ > } > > + JS::Rooted<JS::Value> keyval(aCx, aKey.WasPassed() ? aKey.Value() : JSVAL_VOID); Nit: max 80 chars per line @@ +1839,4 @@ > } > > nsRefPtr<IDBRequest> request = GenerateRequest(this); > + if (!request) { NS_ENSURE_TRUE() warns add: NS_WARNING("Failed to generate request!"); @@ +1848,5 @@ > new AddHelper(mTransaction, request, this, cloneWriteInfo, key, > aOverwrite, updateInfo); > > + nsresult rv = helper->DispatchToTransactionPool(); > + if (NS_FAILED(rv)) { NS_ENSURE_SUCCESS() warns add: NS_WARNING("Failed to dispatch!"); @@ +2001,4 @@ > } > > nsRefPtr<IDBRequest> request = GenerateRequest(this); > + if (!request) { add a warning here @@ +2009,5 @@ > nsRefPtr<GetHelper> helper = > new GetHelper(mTransaction, request, this, aKeyRange); > > nsresult rv = helper->DispatchToTransactionPool(); > + if (NS_FAILED(rv)) { add a warning here @@ +2039,4 @@ > } > > nsRefPtr<IDBRequest> request = GenerateRequest(this); > + if (!request) { add a warning here @@ +2047,5 @@ > nsRefPtr<GetAllHelper> helper = > new GetAllHelper(mTransaction, request, this, aKeyRange, aLimit); > > nsresult rv = helper->DispatchToTransactionPool(); > + if (NS_FAILED(rv)) { add a warning here @@ +2084,4 @@ > } > > nsRefPtr<IDBRequest> request = GenerateRequest(this); > + if (!request) { add a warning here @@ +2092,5 @@ > nsRefPtr<DeleteHelper> helper = > new DeleteHelper(mTransaction, request, this, aKeyRange); > > nsresult rv = helper->DispatchToTransactionPool(); > + if (NS_FAILED(rv)) { add a warning here @@ +2125,4 @@ > } > > nsRefPtr<IDBRequest> request = GenerateRequest(this); > + if (!request) { add a warning here @@ +2132,5 @@ > > nsRefPtr<ClearHelper> helper(new ClearHelper(mTransaction, request, this)); > > nsresult rv = helper->DispatchToTransactionPool(); > + if (NS_FAILED(rv)) { add a warning here @@ +2161,4 @@ > } > > nsRefPtr<IDBRequest> request = GenerateRequest(this); > + if (!request) { add a warning here please check other NS_ENSURE_SUCCESS/NS_ENSURE_TRUE that got converted to NS_FAILED @@ +2370,5 @@ > } > > NS_IMPL_CYCLE_COLLECTION_TRACE_BEGIN(IDBObjectStore) > NS_IMPL_CYCLE_COLLECTION_TRACE_JSVAL_MEMBER_CALLBACK(mCachedKeyPath) > + NS_IMPL_CYCLE_COLLECTION_TRACE_PRESERVED_WRAPPER Nit: I would put this one first @@ +2382,5 @@ > NS_CYCLE_COLLECTION_NOTE_EDGE_NAME(cb, "mCreatedIndexes[i]"); > cb.NoteXPCOMChild(static_cast<nsIIDBIndex*>(tmp->mCreatedIndexes[i].get())); > } > + > + NS_IMPL_CYCLE_COLLECTION_TRAVERSE_SCRIPT_OBJECTS this one is already there, no? @@ +2396,5 @@ > if (tmp->mRooted) { > NS_DROP_JS_OBJECTS(tmp, IDBObjectStore); > tmp->mRooted = false; > } > + NS_IMPL_CYCLE_COLLECTION_UNLINK_PRESERVED_WRAPPER Nit: I would put this one first @@ +2517,3 @@ > { > NS_ASSERTION(NS_IsMainThread(), "Wrong thread!"); > + return AddOrPut(aCx, aValue, aKey, false, aRv); you can inline this @@ +2526,3 @@ > { > NS_ASSERTION(NS_IsMainThread(), "Wrong thread!"); > + return AddOrPut(aCx, aValue, aKey, true, aRv); dtto @@ +2614,5 @@ > + const Sequence<nsString >& aKeyPath, > + const IDBIndexParameters& aOptionalParameters, > + mozilla::ErrorResult& aRv) > +{ > + NS_PRECONDITION(NS_IsMainThread(), "Wrong thread!"); NS_PRECONDITION -> NS_ASSERTION @@ +2954,5 @@ > union { > double d; > uint64_t u; > } pun; > + revert this change @@ +4456,5 @@ > + return mTransaction; > +} > + > +JSObject* > +IDBObjectStore::WrapObject(JSContext* aCx, JS::Handle<JSObject*> aScope) Nit: I would move this before GetKeyPath() ::: dom/indexedDB/IDBObjectStore.h @@ +7,5 @@ > #ifndef mozilla_dom_indexeddb_idbobjectstore_h__ > #define mozilla_dom_indexeddb_idbobjectstore_h__ > > #include "mozilla/dom/indexedDB/IndexedDatabase.h" > +#include "mozilla/dom/indexedDB/IDBRequest.h" Nit: this should go before |#include "mozilla/dom/indexedDB/IDBTransaction.h"| @@ +15,5 @@ > #include "mozilla/dom/indexedDB/IDBTransaction.h" > #include "mozilla/dom/indexedDB/KeyPath.h" > +#include "mozilla/dom/IDBObjectStoreBinding.h" > +#include "mozilla/dom/IDBCursorBinding.h" > +#include "mozilla/dom/IDBIndexBinding.h" Nit: these three should go before |#include "nsCycleCollectionParticipant.h"| in alphabetical order @@ +245,5 @@ > SetInfo(ObjectStoreInfo* aInfo); > > static JSClass sDummyPropJSClass; > > + // WebIDL // nsWrapperCache virtual JSObject* WrapObject(JSContext* aCx, JS::Handle<JSObject*> aScope) MOZ_OVERRIDE; // WebIDL IDBTransaction* GetParentObject() const { return mTransaction; } void GetName(nsString& aName) const { NS_ASSERTION(NS_IsMainThread(), "Wrong thread!"); aName.Assign(mName); } ... @@ +247,5 @@ > static JSClass sDummyPropJSClass; > > + // WebIDL > + > + IDBTransaction* GetParentObject() const; you can inline this Nit: return value on a separate line here and elsewhere @@ +253,5 @@ > + virtual JSObject* > + WrapObject(JSContext* aCx, JS::Handle<JSObject*> aScope) MOZ_OVERRIDE; > + > + void GetName(nsString& aName) const > + { add the main thread assertion @@ +257,5 @@ > + { > + aName.Assign(mName); > + } > + > + JS::Value GetKeyPath(JSContext* aCx, ErrorResult& aRv); Nit: return value on a separate line, here and elsewhere @@ +261,5 @@ > + JS::Value GetKeyPath(JSContext* aCx, ErrorResult& aRv); > + > + already_AddRefed<nsIDOMDOMStringList> GetIndexNames(ErrorResult& aRv); > + > + indexedDB::IDBTransaction* Transaction() const is the namespace prefix really needed ? @@ +276,5 @@ > + > + already_AddRefed<IDBRequest> > + Put(JSContext* aCx, JS::Handle<JS::Value> aValue, > + const Optional<JS::Handle<JS::Value> >& aKey, > + ErrorResult& aRv); Nit: |ErrorResult& aRv| fits after |aKey, | @@ +281,5 @@ > + > + already_AddRefed<IDBRequest> > + Add(JSContext* aCx, JS::Handle<JS::Value> aValue, > + const Optional<JS::Handle<JS::Value> >& aKey, > + ErrorResult& aRv); dtto @@ +285,5 @@ > + ErrorResult& aRv); > + > + already_AddRefed<IDBRequest> > + Delete(JSContext* aCx, JS::Handle<JS::Value> aKey, > + ErrorResult& aRv); dtto @@ +289,5 @@ > + ErrorResult& aRv); > + > + already_AddRefed<IDBRequest> > + Get(JSContext* aCx, JS::Handle<JS::Value> aKey, > + ErrorResult& aRv); dtto @@ +301,5 @@ > + > + already_AddRefed<nsIIDBIndex> > + CreateIndex(JSContext* aCx, const nsAString& aName, const nsAString& aKeyPath, > + const IDBIndexParameters& aOptionalParameters, > + ErrorResult& aRv); dtto @@ +304,5 @@ > + const IDBIndexParameters& aOptionalParameters, > + ErrorResult& aRv); > + > + already_AddRefed<nsIIDBIndex> > + CreateIndex(JSContext* aCx, const nsAString& aName, const Sequence<nsString >& aKeyPath, Nit: 80 chars per line @@ +311,5 @@ > + > + already_AddRefed<nsIIDBIndex> > + Index(const nsAString& aName, ErrorResult &aRv); > + > + void DeleteIndex(const nsAString& aIndexName, ErrorResult& aRv); Nit: return value on a separate line @@ +335,5 @@ > > + already_AddRefed<IDBRequest> > + AddOrPut(JSContext* aCx, JS::Handle<JS::Value> aValue, > + const Optional<JS::Handle<JS::Value> >& aKey, > + bool aOverwrite, ErrorResult& aRv); Nit: |bool aOverWite| fits after |aKey,| @@ +340,5 @@ > + > + already_AddRefed<nsIIDBIndex> > + CreateIndex(JSContext* aCx, const nsAString& aName, KeyPath& aKeyPath, > + const IDBIndexParameters& aOptionalParameters, > + ErrorResult& aRv); Nit: you can join these two lines ::: dom/indexedDB/KeyPath.h @@ +6,5 @@ > > #ifndef mozilla_dom_indexeddb_keypath_h__ > #define mozilla_dom_indexeddb_keypath_h__ > > #include "mozilla/dom/indexedDB/IndexedDatabase.h" Nit: add an empty line here @@ +34,5 @@ > { > MOZ_COUNT_CTOR(KeyPath); > } > > + KeyPath(JSContext* aCx, const nsAString& aString) Why not: static nsresult Parse(JSContext* aCx, const nsAString& aString, KeyPath* aKeyPath); setting type to STRING and then to NONEXISTENT, if something fails, looks a bit odd @@ +44,5 @@ > + SetType(NONEXISTENT); > + } > + } > + > + KeyPath(JSContext* aCx, const Sequence<nsString >& aStrings) Why not: static nsresult Parse(JSContext* aCx, const Sequence<nsString>& aStrings, KeyPath* aKeyPath); ::: dom/indexedDB/ipc/IndexedDBParent.cpp @@ +1279,3 @@ > } > > + return !rv.Failed(); NS_ENSURE_SUCCESS() warns add: NS_WARNING("Failed to delete index!"); @@ +1322,3 @@ > } > > + if (rv.Failed()) { NS_ENSURE_SUCCESS() warns add: NS_WARNING("Failed to create index!"); @@ +1532,5 @@ > AutoSetCurrentTransaction asct(mObjectStore->Transaction()); > > + ErrorResult rv; > + request = mObjectStore->GetInternal(keyRange, rv); > + if (rv.Failed()) { NS_ENSURE_SUCCESS() warns add: NS_WARNING("Failed to get value!"); @@ +1574,5 @@ > AutoSetCurrentTransaction asct(mObjectStore->Transaction()); > > + ErrorResult rv; > + request = mObjectStore->MozGetAllInternal(keyRange, aParams.limit(), rv); > + if (rv.Failed()) { NS_ENSURE_SUCCESS() warns add: NS_WARNING("Failed to get all values!"); and I would keep the old method name: GetAllInternal @@ +1657,5 @@ > AutoSetCurrentTransaction asct(mObjectStore->Transaction()); > > + ErrorResult rv; > + request = mObjectStore->DeleteInternal(keyRange, rv); > + if (rv.Failed()) { NS_ENSURE_SUCCESS() warns add: NS_WARNING("Failed to delete value!"); @@ +1680,5 @@ > AutoSetCurrentTransaction asct(mObjectStore->Transaction()); > > + ErrorResult rv; > + request = mObjectStore->Clear(rv); > + if (rv.Failed()) { NS_ENSURE_SUCCESS() warns add: NS_WARNING("Failed to clear object store!"); @@ +1721,5 @@ > AutoSetCurrentTransaction asct(mObjectStore->Transaction()); > > + ErrorResult rv; > + request = mObjectStore->CountInternal(keyRange, rv); > + if (rv.Failed()) { NS_ENSURE_SUCCESS() warns add: NS_WARNING("Failed to count values!"); @@ +1764,5 @@ > AutoSetCurrentTransaction asct(mObjectStore->Transaction()); > > + ErrorResult rv; > + request = mObjectStore->OpenCursorInternal(keyRange, direction, rv); > + if (rv.Failed()) { NS_ENSURE_SUCCESS() warns add: NS_WARNING("Failed to open cursor!"); ::: dom/webidl/WebIDL.mk @@ +160,5 @@ > HTMLTimeElement.webidl \ > HTMLTitleElement.webidl \ > HTMLUListElement.webidl \ > HTMLVideoElement.webidl \ > + IDBIndex.webidl \ Nit: Move before IDBObjectStore.weibidl ? ::: js/xpconnect/src/nsXPConnect.cpp @@ +38,5 @@ > #include "mozilla/dom/IDBVersionChangeEventBinding.h" > #include "mozilla/dom/TextDecoderBinding.h" > #include "mozilla/dom/TextEncoderBinding.h" > #include "mozilla/dom/DOMErrorBinding.h" > +#include "mozilla/dom/IDBObjectStoreBinding.h" Nit: Don't forget to keep alphabetical order when you rebase this patch. @@ +486,5 @@ > if (!IDBVersionChangeEventBinding::GetConstructorObject(aJSContext, global) || > !TextDecoderBinding::GetConstructorObject(aJSContext, global) || > !TextEncoderBinding::GetConstructorObject(aJSContext, global) || > !IDBTransactionBinding::GetConstructorObject(aJSContext, global) || > + !IDBObjectStoreBinding::GetConstructorObject(aJSContext, global) || dtto

Actually, all those internal methods (e.g. GetInternal) could return nsresult (as they used to). I think it makes sense to use nsresult internally and ErrorResult for WebIDL interface If you do that, you don't have to add warnings (in place of original NS_ENSURE_SUCCESS) to internal methods nor to IndexedDBParent.cpp

Comment on attachment 773961 [details] [diff] [review] patch Review of attachment 773961 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/webidl/IDBObjectStore.webidl @@ +60,5 @@ > + > +partial interface IDBObjectStore { > + // Success fires IDBTransactionEvent, result == array of values for given keys > + [Throws] > + IDBRequest mozGetAll(optional any key, optional unsigned long limit); Nit: |IDBRequest mozGetAll (optional any key, optional unsigned long limit);|

Comment on attachment 773961 [details] [diff] [review] patch Review of attachment 773961 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/indexedDB/IDBCursor.h @@ +13,5 @@ > #include "mozilla/dom/indexedDB/Key.h" > > #include "nsIIDBCursorWithValue.h" > > #include "nsCycleCollectionParticipant.h" This should just be one sorted list without empty lines: #include "mozilla/dom/IDBCursorBinding.h" #include "mozilla/dom/indexedDB/IDBObjectStore.h" #include "mozilla/dom/indexedDB/IndexedDatabase.h" #include "mozilla/dom/indexedDB/Key.h" #include "nsCycleCollectionParticipant.h" #include "nsIIDBCursorWithValue.h"

(In reply to :Ms2ger from comment #6) > This should just be one sorted list without empty lines: oh, I see now why you stopped working on the LockedFile conversion

Comment on attachment 783102 [details] [diff] [review] patch Review of attachment 783102 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/indexedDB/IDBCursor.cpp @@ +318,5 @@ > + break; > + > + default: > + MOZ_CRASH("Unknown direction!"); > + break; no need for "break" ::: dom/indexedDB/IDBCursor.h @@ +6,5 @@ > > #ifndef mozilla_dom_indexeddb_idbcursor_h__ > #define mozilla_dom_indexeddb_idbcursor_h__ > > #include "mozilla/dom/indexedDB/IndexedDatabase.h" add an empty line here @@ +7,5 @@ > #ifndef mozilla_dom_indexeddb_idbcursor_h__ > #define mozilla_dom_indexeddb_idbcursor_h__ > > #include "mozilla/dom/indexedDB/IndexedDatabase.h" > +#include "nsIIDBCursorWithValue.h" add an empty line here @@ +9,5 @@ > > #include "mozilla/dom/indexedDB/IndexedDatabase.h" > +#include "nsIIDBCursorWithValue.h" > +#include "mozilla/dom/IDBCursorBinding.h" > +#include "nsCycleCollectionParticipant.h" add an empty line here ::: dom/indexedDB/IDBObjectStore.cpp @@ +50,5 @@ > > USING_INDEXEDDB_NAMESPACE > using namespace mozilla::dom; > using namespace mozilla::dom::indexedDB::ipc; > +using mozilla::ErrorResult; this should go after |using mozilla::dom::quota::FileOutputStream;| @@ +1834,4 @@ > } > > + JS::Rooted<JS::Value> keyval(aCx, aKey.WasPassed() > + ? aKey.Value() : JSVAL_VOID); ? aKey.Value() : JSVAL_VOID); would be nicer (":" just below "?") @@ +2511,5 @@ > > +already_AddRefed<IDBRequest> > +IDBObjectStore::MozGetAll(JSContext* aCx, > + const Optional<JS::Handle<JS::Value> >& aKey, > + const Optional<uint32_t >& aLimit, ErrorResult& aRv) no need for space before ">" @@ +2592,5 @@ > + return nullptr; > + } > + } > + > + IDBCursor::ParseDirection(aDirection, &direction); sorry I didn't notice this earlier, this should be: IDBCursor::Direction direction = IDBCursor::ConvertDirection(aDirection); ::: dom/indexedDB/IDBObjectStore.h @@ +10,5 @@ > #include "mozilla/dom/indexedDB/IndexedDatabase.h" > > +#include "mozilla/dom/IDBObjectStoreBinding.h" > +#include "mozilla/dom/IDBIndexBinding.h" > +#include "mozilla/dom/IDBCursorBinding.h" alphabetical order ? #include "mozilla/dom/IDBCursorBinding.h" #include "mozilla/dom/IDBIndexBinding.h" #include "mozilla/dom/IDBObjectStoreBinding.h" @@ +15,1 @@ > remove this empty line @@ +246,5 @@ > SetInfo(ObjectStoreInfo* aInfo); > > static JSClass sDummyPropJSClass; > > + // WebIDL "// nsWrapperCache" goes here @@ +247,5 @@ > > static JSClass sDummyPropJSClass; > > + // WebIDL > + remove this empty line @@ +250,5 @@ > + // WebIDL > + > + virtual JSObject* > + WrapObject(JSContext* aCx, JS::Handle<JSObject*> aScope) MOZ_OVERRIDE; > + "// WebIDL" goes here @@ +350,5 @@ > > + already_AddRefed<IDBRequest> > + AddOrPut(JSContext* aCx, JS::Handle<JS::Value> aValue, > + const Optional<JS::Handle<JS::Value> >& aKey, > + bool aOverwrite, ErrorResult& aRv); Nit: |bool aOverWite| fits after |aKey,| @@ +355,5 @@ > + > + already_AddRefed<nsIIDBIndex> > + CreateIndex(JSContext* aCx, const nsAString& aName, KeyPath& aKeyPath, > + const IDBIndexParameters& aOptionalParameters, > + ErrorResult& aRv); Nit: you can join these two lines ::: dom/indexedDB/ipc/IndexedDBParent.cpp @@ +1266,4 @@ > } > > + if (rv.Failed()) { > + NS_WARNING("Failed to delete index!"); why not |return false| ? ::: dom/webidl/WebIDL.mk @@ +163,5 @@ > HTMLTitleElement.webidl \ > HTMLTrackElement.webidl \ > HTMLUListElement.webidl \ > HTMLVideoElement.webidl \ > + IDBIndex.webidl \ should this really go first ?

Ok, so you just got r+ for the ENSURE_SUCCESS macro patch. It would be highly appreciated if you reworked IDBObjectStore patch on top of it.

you can also add this to Bindings.conf 'IDBObjectStore': { ... 'binaryNames': { 'mozGetAll': 'getAll' } }

Ready to land once bent gives his feedback and when bug 899546 is fixed.

Comment on attachment 783593 [details] [diff] [review] objectstore.patch Review of attachment 783593 [details] [diff] [review]: ----------------------------------------------------------------- I like these new macros, thanks for doing it! ::: dom/indexedDB/IDBObjectStore.cpp @@ +1834,4 @@ > } > > + JS::Rooted<JS::Value> keyval(aCx, aKey.WasPassed() > + ? aKey.Value() |? aKey.Value()| can go right after |aKey.WasPassed()| ":" should then be aligned below "?" @@ +1842,5 @@ > nsTArray<IndexUpdateInfo> updateInfo; > > JS::Rooted<JS::Value> value(aCx, aValue); > + aRv = GetAddInfo(aCx, value, keyval, cloneWriteInfo, key, updateInfo); > + ENSURE_SUCCESS(aRv, nullptr); there was no NS_ENSURE_SUCCESS here, change it to: if (aRv.Failed()) { return nullptr; } @@ +1856,5 @@ > new AddHelper(mTransaction, request, this, cloneWriteInfo, key, > aOverwrite, updateInfo); > > + nsresult rv = helper->DispatchToTransactionPool(); > + if (NS_FAILED(rv)) { this should be just ENSURE_SUCCESS() @@ +2019,5 @@ > nsRefPtr<GetHelper> helper = > new GetHelper(mTransaction, request, this, aKeyRange); > > nsresult rv = helper->DispatchToTransactionPool(); > + if (NS_FAILED(rv)) { this should be just ENSURE_SUCCESS() @@ +2058,5 @@ > nsRefPtr<GetAllHelper> helper = > new GetAllHelper(mTransaction, request, this, aKeyRange, aLimit); > > nsresult rv = helper->DispatchToTransactionPool(); > + if (NS_FAILED(rv)) { this should be just ENSURE_SUCCESS() @@ +2105,5 @@ > nsRefPtr<DeleteHelper> helper = > new DeleteHelper(mTransaction, request, this, aKeyRange); > > nsresult rv = helper->DispatchToTransactionPool(); > + if (NS_FAILED(rv)) { this should be just ENSURE_SUCCESS() @@ +2147,5 @@ > > nsRefPtr<ClearHelper> helper(new ClearHelper(mTransaction, request, this)); > > nsresult rv = helper->DispatchToTransactionPool(); > + if (NS_FAILED(rv)) { this should be just ENSURE_SUCCESS() @@ +2184,5 @@ > > nsRefPtr<CountHelper> helper = > new CountHelper(mTransaction, request, this, aKeyRange); > nsresult rv = helper->DispatchToTransactionPool(); > + if (NS_FAILED(rv)) { this should be just ENSURE_SUCCESS() @@ +2226,5 @@ > nsRefPtr<OpenCursorHelper> helper = > new OpenCursorHelper(mTransaction, request, this, aKeyRange, direction); > > nsresult rv = helper->DispatchToTransactionPool(); > + if (NS_FAILED(rv)) { this should be just ENSURE_SUCCESS() @@ +2316,5 @@ > nsRefPtr<CreateIndexHelper> helper = > new CreateIndexHelper(mTransaction, index); > > nsresult rv = helper->DispatchToTransactionPool(); > + if (NS_FAILED(rv)) { this should be just ENSURE_SUCCESS() @@ +2710,5 @@ > nsRefPtr<DeleteIndexHelper> helper = > new DeleteIndexHelper(mTransaction, this, aName); > > + nsresult rv = helper->DispatchToTransactionPool(); > + if (NS_FAILED(rv)) { this should be just ENSURE_SUCCESS() ::: dom/indexedDB/KeyPath.cpp @@ +237,5 @@ > +} > + > +//static > +nsresult > +KeyPath::Parse(JSContext* aCx, const Sequence<nsString>& aStrings, KeyPath* aKeyPath) Nit: 80 chars max ::: dom/indexedDB/ipc/IndexedDBParent.cpp @@ +1256,5 @@ > { > AutoSetCurrentTransaction asct(mObjectStore->Transaction()); > > + mObjectStore->DeleteIndex(aName, rv); > + ENSURE_SUCCESS(rv, false); |ENSURE_SUCCESS(rv, false);| should go in place of original |NS_ENSURE_SUCCESS(rv, false);| @@ +1261,3 @@ > } > > + return !rv.Failed(); this should be reverted back to |return true| @@ +1299,5 @@ > > + ErrorResult rv; > + nsCOMPtr<nsIIDBIndex> obj = mObjectStore->CreateIndexInternal(info, rv); > + index = static_cast<IDBIndex*>(obj.get()); > + ENSURE_SUCCESS(rv, false); you're probably going to remove |index = static_cast<IDBIndex*>(obj.get());| in some following patch, but |ENSURE_SUCCESS(rv, false);| should be placed right after CreateIndexInternal() call

(In reply to Jan Varga [:janv] from comment #13) > @@ +1856,5 @@ > > new AddHelper(mTransaction, request, this, cloneWriteInfo, key, > > aOverwrite, updateInfo); > > > > + nsresult rv = helper->DispatchToTransactionPool(); > > + if (NS_FAILED(rv)) { > > this should be just ENSURE_SUCCESS() I take this back, we don't have a macro for this yet

Ready to land.

Comment on attachment 783750 [details] [diff] [review] objectstore.patch r=me