Bug 84631

Summary: [Chromium] IndexedDB: Use WebIDBKeyPath type for key paths in WebKit API
Product: WebKit Reporter: Joshua Bell <jsbell@chromium.org>
Component: New BugsAssignee: Joshua Bell <jsbell@chromium.org>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth@webkit.org, alecflett@chromium.org, dglazkov@chromium.org, dgrogan@chromium.org, fishd@chromium.org, hclam@google.com, jamesr@chromium.org, tkent+wkapi@chromium.org, webkit.review.bot@gmail.com
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch for landing none

Description From 2012-04-23 14:00:15 PST
[Chromium] IndexedDB: Use WebIDBKeyPath type for key paths in WebKit API
------- Comment #1 From 2012-04-23 14:41:50 PST -------
Created an attachment (id=138428) [details]
Patch
------- Comment #2 From 2012-04-23 14:45:41 PST -------
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 #3 From 2012-04-23 14:46:33 PST -------
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.
------- Comment #4 From 2012-04-24 12:36:52 PST -------
(From update of attachment 138428 [details])
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?
------- Comment #5 From 2012-04-24 15:57:46 PST -------
(From update of attachment 138428 [details])
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.)
------- Comment #6 From 2012-04-25 08:57:36 PST -------
(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. :)
------- Comment #7 From 2012-04-25 12:35:07 PST -------
Created an attachment (id=138856) [details]
Patch
------- Comment #8 From 2012-04-25 12:36:44 PST -------
(In reply to comment #7)
> Created an attachment (id=138856) [details] [details]
> Patch

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

abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org - r?
------- Comment #9 From 2012-04-25 14:21:35 PST -------
(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.
------- Comment #10 From 2012-04-25 14:21:48 PST -------
LGTM
------- Comment #11 From 2012-04-25 14:36:21 PST -------
(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?
------- Comment #12 From 2012-04-25 14:39:30 PST -------
(In reply to comment #11)
> (In reply to comment #9)
> > (From update of attachment 138856 [details] [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?).
------- Comment #13 From 2012-04-25 14:48:23 PST -------
 > 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.)
------- Comment #14 From 2012-04-25 15:48:32 PST -------
(From update of attachment 138856 [details])
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
------- Comment #15 From 2012-04-25 17:06:54 PST -------
(From update of attachment 138856 [details])
Clearing flags on attachment: 138856

Committed r115262: <http://trac.webkit.org/changeset/115262>
------- Comment #16 From 2012-04-25 17:07:17 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #17 From 2012-04-25 18:55:12 PST -------
Reverted r115262 for reason:

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

Committed r115279: <http://trac.webkit.org/changeset/115279>
------- Comment #18 From 2012-04-26 11:28:08 PST -------
Created an attachment (id=139031) [details]
Patch for landing
------- Comment #19 From 2012-04-26 12:19:13 PST -------
(From update of attachment 139031 [details])
Clearing flags on attachment: 139031

Committed r115339: <http://trac.webkit.org/changeset/115339>
------- Comment #20 From 2012-04-26 12:19:19 PST -------
All reviewed patches have been landed.  Closing bug.