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

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)

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.

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

Created attachment 137714 [details]
Patch

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

Take another look?

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

or dglazkov@ ?

Comment on attachment 137714 [details]
Patch

ok

Comment on attachment 137714 [details]
Patch

Clearing flags on attachment: 137714

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

All reviewed patches have been landed.  Closing bug.