891944 - Move IDBCursor to WebIDL

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Comment 1

[Andrea Marchesini [:baku]]

Attachment: patch

Comment 2

[Jan Varga [:janv]]

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 3

[Andrea Marchesini [:baku]]

Attachment: cursor.patch

Comment 4

[Jan Varga [:janv]]

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 5

[Andrea Marchesini [:baku]]

Attachment: cursor.patch

Comment 6

[Jan Varga [:janv]]

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

Comment 7

[Andrea Marchesini [:baku]]

Attachment: cursor.patch

Comment 8

[Andrea Marchesini [:baku]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5737186129bb

Comment 9

[Andrea Marchesini [:baku]]

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.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5737186129bb

Comment 11

[Kohei Yoshino [:kohei]]

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