Separate WebIDBKey type into separate header

Separate WebIDBKey type into separate header

WebIDBKey has exported symbols that we don't want to call from any common or browser
code as well as a type enum that we want to reuse everywhere. This separates the
type enum into a standalone WebKit::WebIDBKeyType enum in its own header and
normalizes the names to standard blink public API conventions. This way we can
use DEPS to enforce that we're only using the types in common and browser code.

BUG=237267
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=154358

[jamesr, 2013-07-16 00:57:19]

https://codereview.chromium.org/19316002/diff/1/Source/web/AssertMatchingEnum...
File Source/web/AssertMatchingEnums.cpp (right):

https://codereview.chromium.org/19316002/diff/1/Source/web/AssertMatchingEnum...
Source/web/AssertMatchingEnums.cpp:504: // MinType is specific to the backing
store, and is not plumbed through the Platform API, so this match is not
required, and thus is purposefully absent.
this comment doesn't make sense to me. MinType is exposed in the API and used in
chromium

https://codereview.chromium.org/19316002/diff/1/public/platform/WebIDBKey.h
File public/platform/WebIDBKey.h (right):

https://codereview.chromium.org/19316002/diff/1/public/platform/WebIDBKey.h#n...
public/platform/WebIDBKey.h:68: WEBKIT_EXPORT WebIDBKeyType keyType() const;
this is an unfortunately stuttery name, but since there's a lot of chromium code
calling type() I can't change the return type without using a #define and
landing a prereq chromium patch with #ifdefs all over the place. with this, we
can land this patch first then a chromium patch to convert to the new enum +
function and then (if we want) add a type() function back to WebIDBKey and
convert chromium over to it.

[alecflett, 2013-07-16 17:06:34]

Thank you for tackling this, something josh and I have been meaning to do
ourselves!

So there are actually multiple enums (In WebIDBKeyPath, at least - there are
other enums that we'd like to use raw in chromium/browser as well) that we
wanted to move into the same file. What I'd prefer to see is for WebIDBKeyType.h
to be renamed something like WebIDB.h (I don't care about the actual name as
long as it consolidates them) 

We are trying to consolidate the 3 sets of enums, such as:
WebCore::IndexedDB::Direction
WebKit::WebIDBCursor::Direction
content::indexeddb::CursorDirection

We'd *like* to just use WebKit::WebIDBCursor::Direction everywhere... the blink
side is obvious to me but I'm not sure what the right convention is w.r.t.
accessing WebKit:: enums from content/

On 2013/07/16 00:57:19, jamesr wrote:
>
https://codereview.chromium.org/19316002/diff/1/Source/web/AssertMatchingEnum...
> File Source/web/AssertMatchingEnums.cpp (right):
> 
>
https://codereview.chromium.org/19316002/diff/1/Source/web/AssertMatchingEnum...
> Source/web/AssertMatchingEnums.cpp:504: // MinType is specific to the backing
> store, and is not plumbed through the Platform API, so this match is not
> required, and thus is purposefully absent.
> this comment doesn't make sense to me. MinType is exposed in the API and used
in
> chromium
> 

That comment (actually from me..) either predates, or was part of the backend
move from blink to chromium. I think removing the comment is the right thing... 

> https://codereview.chromium.org/19316002/diff/1/public/platform/WebIDBKey.h
> File public/platform/WebIDBKey.h (right):
> 
>
https://codereview.chromium.org/19316002/diff/1/public/platform/WebIDBKey.h#n...
> public/platform/WebIDBKey.h:68: WEBKIT_EXPORT WebIDBKeyType keyType() const;
> this is an unfortunately stuttery name, but since there's a lot of chromium
code
> calling type() I can't change the return type without using a #define and
> landing a prereq chromium patch with #ifdefs all over the place. with this, we
> can land this patch first then a chromium patch to convert to the new enum +
> function and then (if we want) add a type() function back to WebIDBKey and
> convert chromium over to it.

[jamesr, 2013-07-16 17:14:16]

On 2013/07/16 17:06:34, alecflett wrote:
> Thank you for tackling this, something josh and I have been meaning to do
> ourselves!
> 
> So there are actually multiple enums (In WebIDBKeyPath, at least - there are
> other enums that we'd like to use raw in chromium/browser as well) that we
> wanted to move into the same file. What I'd prefer to see is for
WebIDBKeyType.h
> to be renamed something like WebIDB.h (I don't care about the actual name as
> long as it consolidates them) 
> 
> We are trying to consolidate the 3 sets of enums, such as:
> WebCore::IndexedDB::Direction
> WebKit::WebIDBCursor::Direction
> content::indexeddb::CursorDirection
> 
> We'd *like* to just use WebKit::WebIDBCursor::Direction everywhere... the
blink
> side is obvious to me but I'm not sure what the right convention is w.r.t.
> accessing WebKit:: enums from content/

That sounds good.  Using WebKit:: enums in content/ is a-ok.  In the short term,
we just want to have a way to easily check (ideally with DEPS) that the only
things content/browser/ is using from WebKit:: are enums and PODs, so having
them in a separate header from actual API is convenient.  I can definitely put
all the IDB-related enums in public/platform/WebIDB.h (or WebIDBTypes.h,
perhaps?) and then try to use that consistently throughout.

[jamesr, 2013-07-16 23:11:33]

This does WebIDBKeyPath::Type as well and consolidates the enums in
WebIDBTypes.h.  The ugly thing about this patch is that since the new enum types
require new entry points I've added the slightly unfortunate method names
WebIDBKey::keyType() and WebIDBKeyPath::keyPathType().  This is also
inconsistent with the content:: equivalents :/  We can clean this up after
switching the chromium code over to the new types if we feel strongly about it.

[alecflett, 2013-07-17 01:30:38]

On 2013/07/16 23:11:33, jamesr wrote:
> This does WebIDBKeyPath::Type as well and consolidates the enums in
> WebIDBTypes.h.  The ugly thing about this patch is that since the new enum
types
> require new entry points I've added the slightly unfortunate method names
> WebIDBKey::keyType() and WebIDBKeyPath::keyPathType().  This is also
> inconsistent with the content:: equivalents :/  We can clean this up after
> switching the chromium code over to the new types if we feel strongly about
it.

lgtm

As I told james, it's likely I or josh or david can help with the cleanup, as we
also want to move some of the types like cursor direction as well, we can just
clean those in the same pass.

[commit-bot: I haz the power, 2013-07-17 01:32:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jamesr@chromium.org/19316002/11001

[commit-bot: I haz the power, 2013-07-17 03:34:12]

Change committed as 154358