Bug 84208 – [Chromium] IndexedDB: Prep for changing keyPath return type

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Joshua Bell]

[Chromium] IndexedDB: Prep for changing keyPath return type

[Joshua Bell]

Created attachment 137631 [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]

FYI, the Chromium side is at: https://chromiumcodereview.appspot.com/10041014

This will end up being a 5-part patch:
(1) WebKit prep: keyPath -> keyPathString rename
(2) Chromium prep: keyPath -> keyPathString rename
(3) 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)

[David Grogan]

Comment on attachment 137631 [details]
Patch

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

LGTM

> Source/WebKit/chromium/src/WebIDBIndexImpl.h:48
> +    virtual WebString keyPathString() const OVERRIDE;

You probably already know, but you can leave off the OVERRIDEs for WebKit API overrides if they make commit engineering difficult.

[James Robinson]

Comment on attachment 137631 [details]
Patch

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

> Source/WebKit/chromium/public/WebIDBIndex.h:2
> + * Copyright (C) 2012 Google Inc. All rights reserved.

we don't do this in WebKit. leave the date alone

>> Source/WebKit/chromium/src/WebIDBIndexImpl.h:48
>> +    virtual WebString keyPathString() const OVERRIDE;
> 
> You probably already know, but you can leave off the OVERRIDEs for WebKit API overrides if they make commit engineering difficult.

we typically don't use OVERRIDE for things in the WebKit API.  The hard rule is that if the declaration you are overriding is in a different repository, do not use OVERRIDE cos it'll make staging changes difficult. In this case, keyPathString() is in the same repo so that shouldn't be an issue - but we also say to favor consistency, which would be a strike against using OVERRIDE here.  So on balance I think you should not add OVERRIDE here unless you want to do it everywhere

[Joshua Bell]

Created attachment 137714 [details]
Patch

[Joshua Bell]

Oops, thanks. So eager to be done I was dotting t's and crossing i's. :)

Take another look?

[Joshua Bell]

@abarth or @fishd - can you take a look?

[Joshua Bell]

or dglazkov@ ?

[Kent Tamura]

Comment on attachment 137714 [details]
Patch

ok

[WebKit Review Bot]

Comment on attachment 137714 [details]
Patch

Clearing flags on attachment: 137714

Committed r114708: <http://trac.webkit.org/changeset/114708>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.