Closed
Bug 920633
Opened 10 years ago
Closed 10 years ago
Add getAllKeys to IDBObjectStore
Categories
(Core :: Storage: IndexedDB, defect)
Core
Storage: IndexedDB
Tracking
()
RESOLVED
FIXED
mozilla27
People
(Reporter: bent.mozilla, Assigned: bent.mozilla)
References
Details
(Keywords: dev-doc-complete)
Attachments
(1 file)
30.64 KB,
patch
|
janv
:
review+
|
Details | Diff | Splinter Review |
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•10 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+
Updated•10 years ago
|
Keywords: dev-doc-needed
Assignee | ||
Comment 2•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/24818d9b7470
Comment 3•10 years ago
|
||
Backed out because this depends on bug 920179 which had to be backed out. https://hg.mozilla.org/integration/mozilla-inbound/rev/19b202186812
Assignee | ||
Comment 4•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/809c2ffacd34
Comment 5•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/809c2ffacd34
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Comment 6•10 years ago
|
||
Will IDBObjectStore.mozGetAll be standardized as getAll?
Assignee | ||
Comment 7•10 years ago
|
||
(In reply to Kohei Yoshino [:kohei] from comment #6) > Will IDBObjectStore.mozGetAll be standardized as getAll? Yes.
Comment 8•10 years ago
|
||
(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•8 years ago
|
||
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
Keywords: dev-doc-needed → dev-doc-complete
You need to log in
before you can comment on or make changes to this bug.
Description
•