[Chromium] IndexedDB: Use WebIDBKeyPath type for key paths in WebKit API
Created attachment 138428 [details] Patch
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.
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 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 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.)
(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. :)
Created attachment 138856 [details] Patch
(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 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.
LGTM
(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?
(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?).
> 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 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 on attachment 138856 [details] Patch Clearing flags on attachment: 138856 Committed r115262: <http://trac.webkit.org/changeset/115262>
All reviewed patches have been landed. Closing bug.
Reverted r115262 for reason: r115262 is causing link error in WebKit Win Builder (dbg) Committed r115279: <http://trac.webkit.org/changeset/115279>
Created attachment 139031 [details] Patch for landing
Comment on attachment 139031 [details] Patch for landing Clearing flags on attachment: 139031 Committed r115339: <http://trac.webkit.org/changeset/115339>