Add getAllKeys to IDBObjectStore

RESOLVED FIXED in mozilla27

Status

()

Core
DOM: IndexedDB
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: Ben Turner (not reading bugmail, use the needinfo flag!), Assigned: Ben Turner (not reading bugmail, use the needinfo flag!))

Tracking

({dev-doc-complete})

Trunk
mozilla27
dev-doc-complete
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Created attachment 810015 [details] [diff] [review]
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.
Attachment #810015 - Flags: review?(Jan.Varga)

Comment 1

4 years ago
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 ?
Attachment #810015 - Flags: review?(Jan.Varga) → review+
Keywords: dev-doc-needed
Backed out because this depends on bug 920179 which had to be backed out.
https://hg.mozilla.org/integration/mozilla-inbound/rev/19b202186812
https://hg.mozilla.org/mozilla-central/rev/809c2ffacd34
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Will IDBObjectStore.mozGetAll be standardized as getAll?
(In reply to Kohei Yoshino [:kohei] from comment #6)
> Will IDBObjectStore.mozGetAll be standardized as getAll?

Yes.
(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.
You need to log in before you can comment on or make changes to this bug.