920633 - Add getAllKeys to IDBObjectStore

Status: RESOLVED FIXED

(Core :: Storage: IndexedDB, defect)

Description

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch, v1

At the Google meetup a while back we all agreed that getAllKeys should exist on IDBObjectStore as well as IDBIndex.

Requires bug 920179 in order for the tests to work.

Comment 1

[Jan Varga [:janv]]

Comment on attachment 810015 [details] [diff] [review]
Patch, v1

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

What can I say, looks good!

::: dom/indexedDB/IDBObjectStore.cpp
@@ +454,5 @@
> +    mKeyRange(aKeyRange), mLimit(aLimit)
> +  { }
> +
> +  virtual nsresult DoDatabaseWork(mozIStorageConnection* aConnection)
> +                                  MOZ_OVERRIDE;

Nit: maybe put the return value on a separate line ? up to you

@@ +2123,5 @@
>  }
>  
>  already_AddRefed<IDBRequest>
> +IDBObjectStore::GetAllKeysInternal(IDBKeyRange* aKeyRange,
> +                                   uint32_t aLimit, ErrorResult& aRv)

Nit: aLimit fits on the line above

@@ +2129,5 @@
> +  MOZ_ASSERT(NS_IsMainThread());
> +
> +  if (!mTransaction->IsOpen()) {
> +    aRv.Throw(NS_ERROR_DOM_INDEXEDDB_TRANSACTION_INACTIVE_ERR);
> +    return nullptr;

This is not directly related to your patch, but I was comparing this method with IDBIndex::GetAllKeysInternal() side by side and I found out that IDBIndex::GetAllKeysInternal() is missing "return nullptr" there.
Maybe you could add it ?

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

Nit: aRv on the same line with aLimit ?

@@ +4449,5 @@
> +    rv = mKeyRange->BindToStatement(stmt);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +  }
> +
> +  mKeys.SetCapacity(std::min<uint32_t>(50, mLimit));

Nit: maybe you could do the same for GetAllKeysHelper::DoDatabaseWork in IDBIndex.cpp

@@ +4472,5 @@
> +nsresult
> +GetAllKeysHelper::GetSuccessResult(JSContext* aCx,
> +                                   JS::MutableHandle<JS::Value> aVal)
> +{
> +  MOZ_ASSERT(NS_IsMainThread());

Nit: maybe add this assertion to GetAllKeysHelper::GetSuccessResult in IDBIndex.cpp

@@ +4588,5 @@
> +GetAllKeysHelper::UnpackResponseFromParentProcess(
> +                                            const ResponseValue& aResponseValue)
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +  MOZ_ASSERT(!IndexedDatabaseManager::IsMainProcess());

Nit: maybe add those two assertions to GetAllKeysHelper::UnpackResponseFromParentProcess in IDBIndex.cpp ?

Comment 2

[Ben Turner (not reading bugmail, use the needinfo flag!)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/24818d9b7470

Comment 3

[Ryan VanderMeulen [:RyanVM]]

Backed out because this depends on bug 920179 which had to be backed out.
https://hg.mozilla.org/integration/mozilla-inbound/rev/19b202186812

Comment 4

[Ben Turner (not reading bugmail, use the needinfo flag!)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/809c2ffacd34

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/809c2ffacd34

Comment 6

[Kohei Yoshino [:kohei]]

Will IDBObjectStore.mozGetAll be standardized as getAll?

Comment 7

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to Kohei Yoshino [:kohei] from comment #6)
> Will IDBObjectStore.mozGetAll be standardized as getAll?

Yes.

Comment 8

[Kohei Yoshino [:kohei]]

(In reply to ben turner [:bent] (needinfo? encouraged) from comment #7)
> (In reply to Kohei Yoshino [:kohei] from comment #6)
> > Will IDBObjectStore.mozGetAll be standardized as getAll?
> 
> Yes.

Glad to hear that. I was afraid the method would be removed w/o standardization.

Comment 9

[Chris Mills [:cmills]]

Completed: 

https://developer.mozilla.org/en-US/docs/Web/API/IDBObjectStore/getAllKeys

Note also added to relevant release notes:

https://developer.mozilla.org/en-US/Firefox/Releases/27#InterfacesAPIsDOM