Bug 84631 - [Chromium] IndexedDB: Use WebIDBKeyPath type for key paths in WebKit API
Summary: [Chromium] IndexedDB: Use WebIDBKeyPath type for key paths in WebKit API
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Joshua Bell
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-04-23 14:00 PDT by Joshua Bell
Modified: 2012-04-26 12:19 PDT (History)
9 users (show)

See Also:


Attachments
Patch (21.94 KB, patch)
2012-04-23 14:41 PDT, Joshua Bell
no flags Details | Formatted Diff | Diff
Patch (21.48 KB, patch)
2012-04-25 12:35 PDT, Joshua Bell
no flags Details | Formatted Diff | Diff
Patch for landing (21.81 KB, patch)
2012-04-26 11:28 PDT, Joshua Bell
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joshua Bell 2012-04-23 14:00:15 PDT
[Chromium] IndexedDB: Use WebIDBKeyPath type for key paths in WebKit API
Comment 1 Joshua Bell 2012-04-23 14:41:50 PDT
Created attachment 138428 [details]
Patch
Comment 2 WebKit Review Bot 2012-04-23 14:45:41 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 3 Joshua Bell 2012-04-23 14:46:33 PDT
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 David Grogan 2012-04-24 12:36:52 PDT
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?
Comment 5 Joshua Bell 2012-04-24 15:57:46 PDT
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.)
Comment 6 Joshua Bell 2012-04-25 08:57:36 PDT
(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 Joshua Bell 2012-04-25 12:35:07 PDT
Created attachment 138856 [details]
Patch
Comment 8 Joshua Bell 2012-04-25 12:36:44 PDT
(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).

abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org - r?
Comment 9 David Grogan 2012-04-25 14:21:35 PDT
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.
Comment 10 David Grogan 2012-04-25 14:21:48 PDT
LGTM
Comment 11 James Robinson 2012-04-25 14:36:21 PDT
(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?
Comment 12 James Robinson 2012-04-25 14:39:30 PDT
(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?).
Comment 13 Joshua Bell 2012-04-25 14:48:23 PDT
 > 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 James Robinson 2012-04-25 15:48:32 PDT
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
Comment 15 WebKit Review Bot 2012-04-25 17:06:54 PDT
Comment on attachment 138856 [details]
Patch

Clearing flags on attachment: 138856

Committed r115262: <http://trac.webkit.org/changeset/115262>
Comment 16 WebKit Review Bot 2012-04-25 17:07:17 PDT
All reviewed patches have been landed.  Closing bug.
Comment 17 Hin-Chung Lam 2012-04-25 18:55:12 PDT
Reverted r115262 for reason:

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

Committed r115279: <http://trac.webkit.org/changeset/115279>
Comment 18 Joshua Bell 2012-04-26 11:28:08 PDT
Created attachment 139031 [details]
Patch for landing
Comment 19 WebKit Review Bot 2012-04-26 12:19:13 PDT
Comment on attachment 139031 [details]
Patch for landing

Clearing flags on attachment: 139031

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