Bug 86676

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 Flags
Patch
none
Patch
none
Patch none

Description Joshua Bell 2012-05-16 14:36:39 PDT
Possibly a remnant of an older version of the spec. Although it is not exposed in the IDL there is completely superfluous plumbing for IDBIndex.storeName - front-end accessor methods, WebKit API (and Chromium IPC plumbing), and back-end storage. Given that back-end indexes have handles to their parent stores, this is unnecessary clutter/code.

Nuke it!
Comment 1 Joshua Bell 2012-05-16 15:33:20 PDT
Created attachment 142354 [details]
Patch
Comment 2 Joshua Bell 2012-05-16 15:45:34 PDT
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 3 David Grogan 2012-05-16 15:50:13 PDT
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.
Comment 4 Joshua Bell 2012-05-16 15:57:17 PDT
(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?
Comment 5 Joshua Bell 2012-05-16 15:58:40 PDT
Hrm, bots don't like the patch, I may have to rebase.
Comment 6 Joshua Bell 2012-05-16 16:04:53 PDT
Created attachment 142361 [details]
Patch
Comment 7 Joshua Bell 2012-05-16 16:05:47 PDT
(In reply to comment #5)
> Hrm, bots don't like the patch, I may have to rebase.

Yeah, collided with r117334 - rebased. tony@ r?
Comment 8 WebKit Review Bot 2012-05-16 16:08:15 PDT
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 9 Tony Chang 2012-05-16 16:25:10 PDT
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.
Comment 10 Joshua Bell 2012-05-16 16:29:15 PDT
Created attachment 142366 [details]
Patch
Comment 11 Dimitri Glazkov (Google) 2012-05-17 15:39:56 PDT
Comment on attachment 142366 [details]
Patch

I approve!
Comment 12 WebKit Review Bot 2012-05-17 16:19:47 PDT
Comment on attachment 142366 [details]
Patch

Clearing flags on attachment: 142366

Committed r117512: <http://trac.webkit.org/changeset/117512>
Comment 13 WebKit Review Bot 2012-05-17 16:19:53 PDT
All reviewed patches have been landed.  Closing bug.