Comment on attachment 776475 [details] [diff] [review] patch Review of attachment 776475 [details] [diff] [review]: ----------------------------------------------------------------- hm, it seems test-indexed-db.js needs to be updated too you might need |using mozilla::ErrorResult| in IDBCursor.cpp too ::: dom/indexedDB/IDBCursor.cpp @@ +358,5 @@ > mRooted(false), > mContinueCalled(false), > mHaveValue(true) > { > NS_ASSERTION(NS_IsMainThread(), "Wrong thread!"); Nit: I would add a new line here @@ +444,5 @@ > NS_NOTREACHED("Unknown cursor type!"); > } > > nsresult rv = helper->DispatchToTransactionPool(); > + if (NS_FAILED(rv)) { ENSURE_SUCCESS() @@ +458,5 @@ > NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mRequest) > NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mTransaction) > NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mObjectStore) > NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mIndex) > + NS_IMPL_CYCLE_COLLECTION_TRAVERSE_SCRIPT_OBJECTS redundant declaration? @@ +473,5 @@ > NS_IMPL_CYCLE_COLLECTION_TRACE_JS_MEMBER_CALLBACK(mScriptOwner) > NS_IMPL_CYCLE_COLLECTION_TRACE_JSVAL_MEMBER_CALLBACK(mCachedKey) > NS_IMPL_CYCLE_COLLECTION_TRACE_JSVAL_MEMBER_CALLBACK(mCachedPrimaryKey) > NS_IMPL_CYCLE_COLLECTION_TRACE_JSVAL_MEMBER_CALLBACK(mCachedValue) > + NS_IMPL_CYCLE_COLLECTION_TRACE_PRESERVED_WRAPPER Nit: I would place this first @@ +480,5 @@ > NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(IDBCursor) > // Don't unlink mObjectStore, mIndex, or mTransaction! > tmp->DropJSObjects(); > NS_IMPL_CYCLE_COLLECTION_UNLINK(mRequest) > + NS_IMPL_CYCLE_COLLECTION_UNLINK_PRESERVED_WRAPPER Nit: I would place this first @@ +492,5 @@ > NS_IMPL_CYCLE_COLLECTING_ADDREF(IDBCursor) > NS_IMPL_CYCLE_COLLECTING_RELEASE(IDBCursor) > > DOMCI_DATA(IDBCursor, IDBCursor) > DOMCI_DATA(IDBCursorWithValue, IDBCursor) hmm, this should be removed too, no? @@ +501,5 @@ > NS_ASSERTION(NS_IsMainThread(), "Wrong thread!"); > > switch (mDirection) { > case NEXT: > + return IDBCursorDirection::Next; fully qualified, here and below? @@ +515,5 @@ > > case DIRECTION_INVALID: > default: > + aRv.Throw(NS_ERROR_UNEXPECTED); > + return IDBCursorDirection::Next; just MOZ_CRASH() ? @@ +528,5 @@ > > + nsCOMPtr<nsISupports> source; > + if (mType == OBJECTSTORE) { > + source = do_QueryInterface(mObjectStore); > + } else { Nit: else on a new line @@ +553,5 @@ > mRooted = true; > } > > + aRv = mKey.ToJSVal(aCx, mCachedKey); > + if (aRv.Failed()) { ENSURE_SUCCESS @@ +701,4 @@ > } > > +already_AddRefed<IDBRequest> > +IDBCursor::Update(JSContext* aCx, JS::Handle<JS::Value> aValue, ErrorResult& aRv) Nit: 80 chars max @@ -874,4 @@ > { > NS_ASSERTION(NS_IsMainThread(), "Wrong thread!"); > > - if (aCount < 1 || aCount > UINT32_MAX) { are you sure you can remove this entirely ? the method should throw when the count is zero and [EnforceRange] handles negative and "over the max" values only you will need to call ThrowTypeError() and add an entry to dom/bindings/Errors.msg ::: dom/indexedDB/IDBCursor.h @@ +17,2 @@ > #include "nsCycleCollectionParticipant.h" > +#include "nsWrapperCache.h" #include "mozilla/dom/indexedDB/IndexedDatabase.h" #include "mozilla/Attributes.h" #include "mozilla/dom/IDBCursorBinding.h" #include "mozilla/ErrorResult.h" #include "nsCycleCollectionParticipant.h" #include "nsWrapperCache.h" #include "mozilla/dom/indexedDB/IDBObjectStore.h" #include "mozilla/dom/indexedDB/Key.h" @@ +33,5 @@ > class IndexedDBCursorChild; > class IndexedDBCursorParent; > > +class IDBCursor MOZ_FINAL : public nsISupports > + , public nsWrapperCache Nit: comma after (not before) and trailing whitespace @@ +146,5 @@ > + void > + ContinueInternal(const Key& aKey, int32_t aCount, > + ErrorResult& aRv); > + > + // WebIDL // nsWrapperCache virtual JSObject* WrapObject(JSContext* aCx, JS::Handle<JSObject*> aScope) MOZ_OVERRIDE; // WebIDL IDBTransaction* GetParentObject() const { return mTransaction(); } ... @@ +148,5 @@ > + ErrorResult& aRv); > + > + // WebIDL > + > + IDBTransaction* GetParentObject() const Nit: return value on a separate line, here and elsewhere @@ +166,5 @@ > + JS::Value GetPrimaryKey(JSContext* aCx, ErrorResult& aRv); > + > + already_AddRefed<IDBRequest> > + Update(JSContext* aCx, JS::Handle<JS::Value> aValue, > + ErrorResult& aRv); Nit: one line for arguments is enough ::: dom/indexedDB/ipc/IndexedDBParent.cpp @@ +2094,5 @@ > AutoSetCurrentTransaction asct(mCursor->Transaction()); > > + ErrorResult rv; > + mCursor->ContinueInternal(aParams.key(), aParams.count(), rv); > + if (rv.Failed()) { ENSURE_SUCCESS()

Comment on attachment 783708 [details] [diff] [review] cursor.patch Review of attachment 783708 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/bindings/Errors.msg @@ +43,5 @@ > MSG_DEF(MSG_INVALID_VERSION, 0, "0 (Zero) is not a valid database version.") > MSG_DEF(MSG_INVALID_BYTESTRING, 2, "Cannot convert string to ByteString because the character" > " at index {0} has value {1} which is greater than 255.") > MSG_DEF(MSG_NOT_DATE, 1, "{0} is not a date.") > +MSG_DEF(MSG_NOT_ZERO, 1, "{0} can't be zero.") this would probably translate into "0 can't be zero." which is not very useful what about: MSG_DEF(MSG_INVALID_ADVANCE_COUNT, 0, "0 (Zero) is not a valid advance count.") but check with someone else, English is not my mother tongue :) ::: dom/indexedDB/IDBCursor.cpp @@ +34,2 @@ > USING_INDEXEDDB_NAMESPACE > +using mozilla::ErrorResult; Nit: move after |using namespace mozilla::dom::indexedDB::ipc;| @@ +441,5 @@ > NS_NOTREACHED("Unknown cursor type!"); > } > > nsresult rv = helper->DispatchToTransactionPool(); > + if (NS_FAILED(rv)) { add a warning @@ +508,5 @@ > > case DIRECTION_INVALID: > default: > + MOZ_CRASH("Unknown direction!"); > + return mozilla::dom::IDBCursorDirection::Next; It compiles even w/o |return mozilla::dom::IDBCursorDirection::Next;| on mac at least @@ +742,5 @@ > } > else { > JS::Rooted<JS::Value> keyVal(aCx); > + aRv = objectKey.ToJSVal(aCx, &keyVal); > + if (aRv.Failed()) { ENSURE_SUCCESS @@ +914,2 @@ > } > sorry, forgot to point out that WrapObject() could go before GetDirection() ::: dom/webidl/IDBCursor.webidl @@ +17,5 @@ > +interface IDBCursor { > + // This should be: readonly attribute (IDBObjectStore or IDBIndex) source; > + readonly attribute nsISupports source; > + > + [Throws] hm, I think this doesn't need [Throws] anymore

Comment on attachment 783862 [details] [diff] [review] cursor.patch Review of attachment 783862 [details] [diff] [review]: ----------------------------------------------------------------- ok, I'm done :) thanks! ::: dom/indexedDB/IDBCursor.cpp @@ +443,5 @@ > } > > nsresult rv = helper->DispatchToTransactionPool(); > + if (NS_FAILED(rv)) { > + NS_ERROR("Failed to dispatch!"); this should be NS_WARNING

With this patch IDBCursor.NEXT, IDBCursor.NEXT_NO_DUPLICATE, IDBCursor.PREV and IDBCursor.PREV_NO_DUPLICATE are removed completely. We need to update the documentation.

https://developer.mozilla.org/en-US/docs/Web/API/IDBCursor#Constants https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/25/Site_Compatibility