Summary: | IndexedDB: Remove IDBIndex.storeName | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Joshua Bell <jsbell> | ||||||||
Component: | WebCore Misc. | Assignee: | Joshua Bell <jsbell> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | abarth, alecflett, dglazkov, dgrogan, fishd, jamesr, tkent+wkapi, tony, webkit.review.bot | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Joshua Bell
2012-05-16 14:36:39 PDT
Created attachment 142354 [details]
Patch
dgrogan@ - can you take a look? After this lands, https://chromiumcodereview.appspot.com/10383221 can land to remove the Chromium IPC plumbing for this. Comment on attachment 142354 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=142354&action=review LGTM > Source/WebCore/Modules/indexeddb/IDBIndexBackendImpl.cpp:-100 > - RefPtr<IDBObjectStoreBackendInterface> objectStore = transaction->objectStore(index->m_storeName, ec); This didn't rely on any logic in IDBTransactionBackendImpl::objectStore for anything useful did it? Seems unlikely but also seems weird that this was ever used instead of just making m_objectStoreBackend non-const like you do in this patch. (In reply to comment #3) > > Source/WebCore/Modules/indexeddb/IDBIndexBackendImpl.cpp:-100 > > - RefPtr<IDBObjectStoreBackendInterface> objectStore = transaction->objectStore(index->m_storeName, ec); > > This didn't rely on any logic in IDBTransactionBackendImpl::objectStore for anything useful did it? Seems unlikely but also seems weird that this was ever used instead of just making m_objectStoreBackend non-const like you do in this patch. Not that I can see. IDBTransactionBackendImpl::objectStore just has early exits that set ec (which would have asserted), and refs the IDBObjectStoreBackendImpl which the IDBCursorBackendImpl constructor will do anyway. The storeName property was originally exposed to script - see http://www.w3.org/TR/2010/WD-IndexedDB-20100105/#index-concept - but the actual object store was not, so it may have been the most convenient approach available at the time. tony@ - r? Hrm, bots don't like the patch, I may have to rebase. Created attachment 142361 [details]
Patch
(In reply to comment #5) > Hrm, bots don't like the patch, I may have to rebase. Yeah, collided with r117334 - rebased. tony@ r? Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI. Comment on attachment 142361 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=142361&action=review > Source/WebCore/ChangeLog:8 > + Reviewed by NOBODY (OOPS!). > + > + No new tests - no functional changes. Can you include some of the "why" in the ChangeLog? Basically what you wrote in comment #0. Created attachment 142366 [details]
Patch
Comment on attachment 142366 [details]
Patch
I approve!
Comment on attachment 142366 [details] Patch Clearing flags on attachment: 142366 Committed r117512: <http://trac.webkit.org/changeset/117512> All reviewed patches have been landed. Closing bug. |