Andrea Marchesini [:baku] Assignee
	Description • 11 years ago

      No description provided.

	Jan Varga [:janv]
	Comment 2 • 11 years ago

Andrea, you are unbelievable, great job on all the recent patches!

	Andrea Marchesini [:baku] Assignee
	Comment 3 • 11 years ago

https://tbpl.mozilla.org/?tree=Try&rev=a58e60dc6e44

	(no longer active)
	Comment 4 • 11 years ago

(In reply to comment #2)
> Andrea, you are unbelievable, great job on all the recent patches!

+1!

	Jan Varga [:janv]
	Comment 5 • 11 years ago

Comment on attachment 774096 [details] [diff] [review]
patch

Review of attachment 774096 [details] [diff] [review]:
-----------------------------------------------------------------

It seems to me that the internal methods should return nsresult in this patch as well.

::: dom/indexedDB/IDBIndex.cpp
@@ +403,5 @@
>    mUnique(false),
>    mMultiEntry(false),
>    mRooted(false)
>  {
>    NS_ASSERTION(NS_IsMainThread(), "Wrong thread!");

Nit: add an empty line

@@ +438,4 @@
>    }
>  
>    nsRefPtr<IDBRequest> request = GenerateRequest(this);
> +  if (!request) {

NS_ENSURE_TRUE() warns, but you can just change the return value back to nsresult

@@ +778,5 @@
>  }
>  
>  NS_IMPL_CYCLE_COLLECTION_TRACE_BEGIN(IDBIndex)
>    NS_IMPL_CYCLE_COLLECTION_TRACE_JSVAL_MEMBER_CALLBACK(mCachedKeyPath)
> +  NS_IMPL_CYCLE_COLLECTION_TRACE_PRESERVED_WRAPPER

Nit: I would put this one first

@@ +795,5 @@
>    if (tmp->mRooted) {
>      NS_DROP_JS_OBJECTS(tmp, IDBIndex);
>      tmp->mRooted = false;
>    }
> +  NS_IMPL_CYCLE_COLLECTION_UNLINK_PRESERVED_WRAPPER

Nit: I would put this one first

@@ +807,5 @@
>  NS_IMPL_CYCLE_COLLECTING_ADDREF(IDBIndex)
>  NS_IMPL_CYCLE_COLLECTING_RELEASE(IDBIndex)
>  
> +JS::Value
> +IDBIndex::GetKeyPath(JSContext* aCx, mozilla::ErrorResult& aRv)

add |using mozilla::ErrorResult| and remove the namespace prefix here and elsewhere

@@ +817,4 @@
>    }
>  
> +  aRv = GetKeyPath().ToJSVal(aCx, mCachedKeyPath);
> +  if (aRv.Failed()) {

NS_ENSURE_SUCCESS() warns

please add a warning here and elsewhere

@@ +830,5 @@
>  }
>  
> +already_AddRefed<IDBRequest>
> +IDBIndex::Get(JSContext* aCx, JS::Handle<JS::Value> aKey,
> +              mozilla::ErrorResult& aRv)

Nit: you can join these two lines

@@ +857,5 @@
>  }
>  
> +already_AddRefed<IDBRequest>
> +IDBIndex::GetKey(JSContext* aCx, JS::Handle<JS::Value> aKey,
> +                 mozilla::ErrorResult& aRv)

Nit: you can join these two lines

@@ +874,5 @@
> +    return nullptr;
> +  }
> +
> +  if (!keyRange) {
> +    // Must specify a key or keyRange for get().

Nit: // Must specify a key or keyRange for getKey().

@@ +886,5 @@
> +already_AddRefed<IDBRequest>
> +IDBIndex::MozGetAll(JSContext* aCx,
> +                    const Optional<JS::Handle<JS::Value> >& aKey,
> +                    const Optional<uint32_t >& aLimit,
> +                    mozilla::ErrorResult& aRv)

Nit: you can join these two lines

@@ +916,5 @@
> +already_AddRefed<IDBRequest>
> +IDBIndex::MozGetAllKeys(JSContext* aCx,
> +                        const Optional<JS::Handle<JS::Value> >& aKey,
> +                        const Optional<uint32_t >& aLimit,
> +                        mozilla::ErrorResult& aRv)

Nit: you can join these two lines

@@ +946,5 @@
> +already_AddRefed<IDBRequest>
> +IDBIndex::OpenCursor(JSContext* aCx,
> +                     const Optional<JS::Handle<JS::Value> >& aRange,
> +                     IDBCursorDirection aDirection,
> +                     mozilla::ErrorResult& aRv)

Nit: you can join these two lines

@@ +993,5 @@
> +already_AddRefed<IDBRequest>
> +IDBIndex::OpenKeyCursor(JSContext* aCx,
> +                        const Optional<JS::Handle<JS::Value> >& aRange,
> +                        IDBCursorDirection aDirection,
> +                        mozilla::ErrorResult& aRv)

Nit: you can join these two lines

@@ +2588,5 @@
>    return NS_OK;
>  }
> +
> +JSObject*
> +IDBIndex::WrapObject(JSContext* aCx, JS::Handle<JSObject*> aScope)

Nit: I would move this before GetKeyPath()

::: dom/indexedDB/IDBIndex.h
@@ +19,2 @@
>  
> +#include "mozilla/dom/IDBCursorBinding.h"

Nit: fixing the order according to the scheme would be nice

@@ +35,5 @@
>  
>  struct IndexInfo;
>  
> +class IDBIndex MOZ_FINAL : public nsISupports
> +                         , public nsWrapperCache

Nit: It seems we put comma after (not before) in this module

@@ +105,5 @@
>    }
>  
> +  already_AddRefed<IDBRequest>
> +  GetInternal(IDBKeyRange* aKeyRange,
> +              ErrorResult& aRv);

As I suggested in bug 888597, you could keep the original signature of internal methods, just remove |JSContext* aCx|

@@ +150,5 @@
>                              const SerializedStructuredCloneReadInfo& aCloneInfo,
>                              nsTArray<StructuredCloneFile>& aBlobs,
>                              IDBCursor** _retval);
>  
> +  // WebIDL

// nsWrapperCache
virtual JSObject*
WrapObject(JSContext* aCx, JS::Handle<JSObject*> aScope) MOZ_OVERRIDE;

// WebIDL
IDBObjectStore*
GetParentObject() const
{
  return mObjectStore;
}

void
GetName(nsString& aName) const
{
  NS_ASSERTION(NS_IsMainThread(), "Wrong thread!");
  aName.Assign(mName);
}

...

@@ +160,5 @@
> +
> +  virtual JSObject*
> +  WrapObject(JSContext* aCx, JS::Handle<JSObject*> aScope) MOZ_OVERRIDE;
> +
> +  void GetName(nsString& aName) const

Nit: return value on a separate line, here and elsewhere

@@ +218,5 @@
> +  already_AddRefed<IDBRequest>
> +  MozGetAllKeys(JSContext* aCx, const Optional<JS::Handle<JS::Value> >& aKey,
> +                const Optional<uint32_t >& aLimit, ErrorResult& aRv);
> +
> +

Nit: remove the extra empty line

::: dom/indexedDB/ipc/IndexedDBParent.cpp
@@ +1314,2 @@
>      {
> +      ErrorResult rv;

you can declare |rv| just before CreateIndexInternal() call

@@ +1316,3 @@
>        AutoSetCurrentTransaction asct(mObjectStore->Transaction());
>  
> +      index = mObjectStore->CreateIndexInternal(info, rv);

Nit: remove this empty line

@@ +1816,5 @@
>      AutoSetCurrentTransaction asct(mIndex->ObjectStore()->Transaction());
>  
> +    ErrorResult rv;
> +    request = mIndex->GetInternal(keyRange, rv);
> +    if (rv.Failed()) {

NS_ENSURE_SUCCESS() warns

please add a warning here and elsewhere or just make the internal method return nsresult

::: dom/webidl/IDBIndex.webidl
@@ +37,5 @@
> +    [Throws]
> +    IDBRequest count (optional any key);
> +};
> +
> +partial interface IDBIndex {

Nit: you could use the same indentation here at least
the same style would be even better

::: 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/IDBIndexBinding.h"

Nit: alphabetical order

@@ +488,5 @@
>          !TextDecoderBinding::GetConstructorObject(aJSContext, global) ||
>          !TextEncoderBinding::GetConstructorObject(aJSContext, global) ||
>          !IDBTransactionBinding::GetConstructorObject(aJSContext, global) ||
>          !IDBObjectStoreBinding::GetConstructorObject(aJSContext, global) ||
> +        !IDBIndexBinding::GetConstructorObject(aJSContext, global) ||

Nit: alphabetical order

	Andrea Marchesini [:baku] Assignee
	Comment 6 • 11 years ago

The ordering of the header files is still missing.

	Andrea Marchesini [:baku] Assignee
	Comment 7 • 11 years ago

Comment on attachment 783113 [details] [diff] [review]
patch

autocomplete does strange stuff...

	Jan Varga [:janv]
	Comment 9 • 11 years ago

Comment on attachment 783191 [details] [diff] [review]
index.patch

Review of attachment 783191 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/indexedDB/IDBIndex.cpp
@@ +448,5 @@
>    nsRefPtr<GetHelper> helper =
>      new GetHelper(transaction, request, this, aKeyRange);
>  
>    nsresult rv = helper->DispatchToTransactionPool();
> +  if (NS_FAILED(rv)) {

ENSURE_SUCCESS() here and elsewhere

@@ +848,5 @@
>  }
>  
> +already_AddRefed<IDBRequest>
> +IDBIndex::Get(JSContext* aCx, JS::Handle<JS::Value> aKey,
> +              ErrorResult& aRv)

Nit: |ErrorResult& aRv| fits after |aKey,|

@@ +876,5 @@
>  }
>  
> +already_AddRefed<IDBRequest>
> +IDBIndex::GetKey(JSContext* aCx, JS::Handle<JS::Value> aKey,
> +                 ErrorResult& aRv)

Nit: |ErrorResult& aRv| fits after |aKey,|

@@ +905,5 @@
>  
> +already_AddRefed<IDBRequest>
> +IDBIndex::MozGetAll(JSContext* aCx,
> +                    const Optional<JS::Handle<JS::Value> >& aKey,
> +                    const Optional<uint32_t >& aLimit, ErrorResult& aRv)

Nit: no need for space before ">"

@@ +935,5 @@
>  
> +already_AddRefed<IDBRequest>
> +IDBIndex::MozGetAllKeys(JSContext* aCx,
> +                        const Optional<JS::Handle<JS::Value> >& aKey,
> +                        const Optional<uint32_t >& aLimit, ErrorResult& aRv)

Nit: no need for space before ">"

@@ +985,5 @@
> +      NS_WARNING("Failed to get KeyRange from jsval!");
> +      return nullptr;
> +    }
> +
> +    IDBCursor::ParseDirection(aDirection, &direction);

this shouldn't depend on aRange.WasPassed(), no ?

put it after this block, something like:
IDBCursor::Direction direction = IDBCursor::ConvertDirection(aDirection);

@@ +1031,5 @@
> +      NS_WARNING("Failed to get KeyRange from jsval!");
> +      return nullptr;
> +    }
> +
> +    IDBCursor::ParseDirection(aDirection, &direction);

dtto

::: dom/indexedDB/IDBIndex.h
@@ +155,5 @@
> +  virtual JSObject*
> +  WrapObject(JSContext* aCx, JS::Handle<JSObject*> aScope) MOZ_OVERRIDE;
> +
> +  // WebIDL
> +

Nit: you can remove this empty line

@@ +220,5 @@
> +  }
> +
> +  already_AddRefed<IDBRequest>
> +  MozGetAll(JSContext* aCx, const Optional<JS::Handle<JS::Value> >& aKey,
> +            const Optional<uint32_t >& aLimit, ErrorResult& aRv);

Nit: no need for space before ">"

@@ +224,5 @@
> +            const Optional<uint32_t >& aLimit, ErrorResult& aRv);
> +
> +  already_AddRefed<IDBRequest>
> +  MozGetAllKeys(JSContext* aCx, const Optional<JS::Handle<JS::Value> >& aKey,
> +                const Optional<uint32_t >& aLimit, ErrorResult& aRv);

Nit: no need for space before ">"

::: dom/indexedDB/ipc/IndexedDBParent.cpp
@@ +1813,5 @@
>      AutoSetCurrentTransaction asct(mIndex->ObjectStore()->Transaction());
>  
> +    ErrorResult rv;
> +    request = mIndex->GetInternal(keyRange, rv);
> +    if (rv.Failed()) {

ENSURE_SUCCESS()

@@ +1840,5 @@
>      AutoSetCurrentTransaction asct(mIndex->ObjectStore()->Transaction());
>  
> +    ErrorResult rv;
> +    request = mIndex->GetKeyInternal(keyRange, rv);
> +    if (rv.Failed()) {

ENSURE_SUCCESS()

@@ +1881,5 @@
>      AutoSetCurrentTransaction asct(mIndex->ObjectStore()->Transaction());
>  
> +    ErrorResult rv;
> +    request = mIndex->MozGetAllInternal(keyRange, aParams.limit(), rv);
> +    if (rv.Failed()) {

ENSURE_SUCCESS()

@@ +1922,5 @@
>      AutoSetCurrentTransaction asct(mIndex->ObjectStore()->Transaction());
>  
> +    ErrorResult rv;
> +    request = mIndex->MozGetAllKeysInternal(keyRange, aParams.limit(), rv);
> +    if (rv.Failed()) {

ENSURE_SUCCESS()

@@ +1963,5 @@
>      AutoSetCurrentTransaction asct(mIndex->ObjectStore()->Transaction());
>  
> +    ErrorResult rv;
> +    request = mIndex->CountInternal(keyRange, rv);
> +    if (rv.Failed()) {

ENSURE_SUCCESS()

@@ +2047,5 @@
>      AutoSetCurrentTransaction asct(mIndex->ObjectStore()->Transaction());
>  
> +    ErrorResult rv;
> +    request = mIndex->OpenKeyCursorInternal(keyRange, direction, rv);
> +    if (rv.Failed()) {

ENSURE_SUCCESS()

	Jan Varga [:janv]
	Comment 10 • 11 years ago

you can also add this to Bindings.conf

'IDBIndex': {
    ...

    'binaryNames': {
        'mozGetAll': 'getAll',
        'mozGetAllKeys': 'getAllKeys',
    }
}

	Andrea Marchesini [:baku] Assignee
	Comment 11 • 11 years ago

https://tbpl.mozilla.org/?tree=Try&rev=019826217df8

	Jan Varga [:janv]
	Comment 12 • 11 years ago

Comment on attachment 783613 [details] [diff] [review]
index.patch

Review of attachment 783613 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/indexedDB/IDBIndex.cpp
@@ +893,5 @@
>  }
>  
> +already_AddRefed<IDBRequest>
> +IDBIndex::GetAll(JSContext* aCx,
> +                 const Optional<JS::Handle<JS::Value> >& aKey,

Nit: |const Optional<JS::Handle<JS::Value> >& aKey,| can be placed at the same line with |JSContext* aCx|

@@ +921,5 @@
>  
> +already_AddRefed<IDBRequest>
> +IDBIndex::GetAllKeys(JSContext* aCx,
> +                        const Optional<JS::Handle<JS::Value> >& aKey,
> +                        const Optional<uint32_t>& aLimit, ErrorResult& aRv)

Nit: indentation

::: dom/indexedDB/ipc/IndexedDBParent.cpp
@@ +1783,5 @@
>      AutoSetCurrentTransaction asct(mIndex->ObjectStore()->Transaction());
>  
> +    ErrorResult rv;
> +    request = mIndex->GetInternal(keyRange, rv);
> +    if (rv.Failed()) {

ENSURE_SUCCESS() ?

@@ +1810,5 @@
>      AutoSetCurrentTransaction asct(mIndex->ObjectStore()->Transaction());
>  
> +    ErrorResult rv;
> +    request = mIndex->GetKeyInternal(keyRange, rv);
> +    if (rv.Failed()) {

ENSURE_SUCCESS() ?

@@ +1851,5 @@
>      AutoSetCurrentTransaction asct(mIndex->ObjectStore()->Transaction());
>  
> +    ErrorResult rv;
> +    request = mIndex->GetAllInternal(keyRange, aParams.limit(), rv);
> +    if (rv.Failed()) {

ENSURE_SUCCESS() ?

@@ +1892,5 @@
>      AutoSetCurrentTransaction asct(mIndex->ObjectStore()->Transaction());
>  
> +    ErrorResult rv;
> +    request = mIndex->GetAllKeysInternal(keyRange, aParams.limit(), rv);
> +    if (rv.Failed()) {

ENSURE_SUCCESS() ?

@@ +1933,5 @@
>      AutoSetCurrentTransaction asct(mIndex->ObjectStore()->Transaction());
>  
> +    ErrorResult rv;
> +    request = mIndex->CountInternal(keyRange, rv);
> +    if (rv.Failed()) {

ENSURE_SUCCESS() ?

	Andrea Marchesini [:baku] Assignee
	Comment 13 • 11 years ago

Ready to land

	Jan Varga [:janv]
	Comment 14 • 11 years ago

Comment on attachment 783751 [details] [diff] [review]
index.patch

r=me

	Andrea Marchesini [:baku] Assignee
	Comment 15 • 11 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/afac636489de

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 16 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/afac636489de