Bug 84631 – [Chromium] IndexedDB: Use WebIDBKeyPath type for key paths in WebKit API

• Component: New Bugs
• Status: RESOLVED FIXED

[Joshua Bell]

[Chromium] IndexedDB: Use WebIDBKeyPath type for key paths in WebKit API

[Joshua Bell]

Created attachment 138428 [details]
Patch

[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.

[Joshua Bell]

dglazkov@ or fishd@ - r?

This is part 3 of 5, in a sequence:

(1) DONE - WebKit prep: keyPath -> keyPathString rename - http://webkit.org/b/84208
(2) DONE - Chromium prep: keyPath -> keyPathString rename - http://codereview.chromium.org/10041014/
(3) THIS - WebKit API change: WebIDBKeyPath type addition to the API, with compat shims
(4) Chromium use of the new API
(5) implementation in WebCore of the new IDB functionality (and drop shims) - http://webkit.org/b/84207

I probably put too many FIXME comments in this one, but a bunch of the code in this patch is just temporary scaffolding that will be removed in the last patch in the sequence.

[David Grogan]

Comment on attachment 138428 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=138428&action=review

> Source/WebKit/chromium/public/platform/WebKitPlatformSupport.h:60
> +class WebIDBKeyPath; // FIXME: Does this belong in platform?

Do you need both this and the include on line 34?

[Joshua Bell]

Comment on attachment 138428 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=138428&action=review

>> Source/WebKit/chromium/public/platform/WebKitPlatformSupport.h:60
>> +class WebIDBKeyPath; // FIXME: Does this belong in platform?
> 
> Do you need both this and the include on line 34?

Nope. (The #include can be deleted when the temporary overloads are removed, but the forward declaration is then needed. I'll drop it for now though.)

[Joshua Bell]

(In reply to comment #5)
> (The #include can be deleted when the temporary overloads are removed, but the forward declaration is then needed. I'll drop it for now though.)

Referencing the "final" bug in my ChangeLog confused webkit-patch; the patch with the above one-line modification. I'll re-up shortly.

However, something going awry in rebaselining the final patch (not this one) has led to a test failure (combo: cr-final + wk-final) which I need to track down, then re-verify combos (cr-final + wk-api-only) and (cr-current + wk-api-only). I'll hold off on re-upping the patch until all of that is sorted, just to be paranoid.

Any tentative r+ from fishd@ or glazkov@ in the meantime would be lovely. :)

[Joshua Bell]

Created attachment 138856 [details]
Patch

[Joshua Bell]

(In reply to comment #7)
> Created an attachment (id=138856) [details]
> Patch

Test failure was unrelated to this patch (side effect of splitting out webkit.org/b/84217 from the "final" patch).

[e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] - r?

[David Grogan]

Comment on attachment 138856 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=138856&action=review

> Source/WebKit/chromium/public/platform/WebKitPlatformSupport.h:34
> +#include "../WebIDBKeyPath.h" // FIXME: Remove with: http://webkit.org/b/84207

Interested to see what the WebKit API reviewer says about this.

[David Grogan]

LGTM

[James Robinson]

(In reply to comment #9)
> (From update of attachment 138856 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=138856&action=review
> 
> > Source/WebKit/chromium/public/platform/WebKitPlatformSupport.h:34
> > +#include "../WebIDBKeyPath.h" // FIXME: Remove with: http://webkit.org/b/84207
> 
> Interested to see what the WebKit API reviewer says about this.

This looks bad - WebKitPlatformSupport should not be getting any more dependencies on client things.  What is the long term plan for this?

[James Robinson]

(In reply to comment #11)
> (In reply to comment #9)
> > (From update of attachment 138856 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=138856&action=review
> > 
> > > Source/WebKit/chromium/public/platform/WebKitPlatformSupport.h:34
> > > +#include "../WebIDBKeyPath.h" // FIXME: Remove with: http://webkit.org/b/84207
> > 
> > Interested to see what the WebKit API reviewer says about this.
> 
> This looks bad - WebKitPlatformSupport should not be getting any more dependencies on client things.  What is the long term plan for this?

Another way to put it - is WebIDBKeyPath something that belongs in the Platform API or the Client API long term (is it something that the embedder is using to talk to WebKit, something that WebCore is going to use to talk back to the embedder, or both?).

[Joshua Bell]

 > Interested to see what the WebKit API reviewer says about this.
> > 
> > This looks bad - WebKitPlatformSupport should not be getting any more dependencies on client things.  What is the long term plan for this?

As soon as this lands, the Chromium side (http://codereview.chromium.org/10204003/) lands and this #include can be removed (tracked as part of http://webkit.org/b/84207). It's replaced by a forward declaration of class WebIDBKeyPath, just like all the other forward declarations in WebKitPlatformSupport (with requisite "// FIXME: Does this belong in platform?" comment). Which is slightly better but still...

> Another way to put it - is WebIDBKeyPath something that belongs in the Platform API or the Client API long term (is it something that the embedder is using to talk to WebKit, something that WebCore is going to use to talk back to the embedder, or both?).

It's a value type like WebIDBKey - there are APIs in WebKitPlatformSupport qhixh accept these types as arguments. Primarily, though, they are used by WebIDB{Database,ObjectStore,Index} interfaces implemented by embedders.

Longer term (i.e. as soon as we've caught up with the IDB spec) we have a big refactor planned, which includes removing these methods from WebKitPlatformSupport. (The they are implemented in Chromium, through several levels of indirection, by a utility process called via sync IPC which we want to eliminate for performance reasons; the refactor will let us do it within WebCore.)

[James Robinson]

Comment on attachment 138856 [details]
Patch

OK, then the public API changes seem fine to me. I would be happy to help you move whatever parts of WebIDB* into Platform/ if that would be useful.  I do not want to leave that #include in WebKitPlatformSupport.h for long

R=me

[WebKit Review Bot]

Comment on attachment 138856 [details]
Patch

Clearing flags on attachment: 138856

Committed r115262: <http://trac.webkit.org/changeset/115262>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Hin-Chung Lam]

Reverted r115262 for reason:

r115262 is causing link error in WebKit Win Builder (dbg)

Committed r115279: <http://trac.webkit.org/changeset/115279>

[Joshua Bell]

Created attachment 139031 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 139031 [details]
Patch for landing

Clearing flags on attachment: 139031

Committed r115339: <http://trac.webkit.org/changeset/115339>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.