Bug 86676 – IndexedDB: Remove IDBIndex.storeName

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Joshua Bell]

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!

[Joshua Bell]

Created attachment 142354 [details]
Patch

[Joshua Bell]

dgrogan@ - can you take a look?

After this lands, https://chromiumcodereview.appspot.com/10383221 can land to remove the Chromium IPC plumbing for this.

[David Grogan]

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.

[Joshua Bell]

(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?

[Joshua Bell]

Hrm, bots don't like the patch, I may have to rebase.

[Joshua Bell]

Created attachment 142361 [details]
Patch

[Joshua Bell]

(In reply to comment #5)
> Hrm, bots don't like the patch, I may have to rebase.

Yeah, collided with r117334 - rebased. tony@ r?

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Tony Chang]

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.

[Joshua Bell]

Created attachment 142366 [details]
Patch

[Dimitri Glazkov (Google)]

Comment on attachment 142366 [details]
Patch

I approve!

[WebKit Review Bot]

Comment on attachment 142366 [details]
Patch

Clearing flags on attachment: 142366

Committed r117512: <http://trac.webkit.org/changeset/117512>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.