Use WebIDBKeyPath type in WebKit API, implement IndexedDBKeyPath type.

Use WebIDBKeyPath type in WebKit API, implement IndexedDBKeyPath type.

IndexedDB allows key paths to be strings or arrays of strings (or null, 
for object stores). Implement the new WebKit API that uses a dedicated type
instead of strings, and implement appropriate IPC plumbing. This change is 
dependent on: https://webkit.org/b/84631

R=darin@chromium.org,tkent@chromium.org,dgrogan@chromium.org,michaeln@chromium.org
BUG=112308
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=134581

[michaeln, 2012-04-23 23:26:18]

on the surface, plumbing looks correct to me

http://codereview.chromium.org/10204003/diff/1/content/common/indexed_db/inde...
File content/common/indexed_db/indexed_db_key_path.cc (right):

http://codereview.chromium.org/10204003/diff/1/content/common/indexed_db/inde...
content/common/indexed_db/indexed_db_key_path.cc:54: WebIDBKeyPath key_path =
*this;
this innocent looking method looks like it may be expensive, how is this method
used?

http://codereview.chromium.org/10204003/diff/1/content/common/indexed_db/inde...
File content/common/indexed_db/indexed_db_key_path.h (right):

http://codereview.chromium.org/10204003/diff/1/content/common/indexed_db/inde...
content/common/indexed_db/indexed_db_key_path.h:18: IndexedDBKeyPath(); //
Defaults to WebKit::WebIDBKeyPath::NullType.
nit: two spaces prior to  // per style guide

[jsbell, 2012-04-23 23:33:21]

http://codereview.chromium.org/10204003/diff/1/content/common/indexed_db/inde...
File content/common/indexed_db/indexed_db_key_path.cc (right):

http://codereview.chromium.org/10204003/diff/1/content/common/indexed_db/inde...
content/common/indexed_db/indexed_db_key_path.cc:54: WebIDBKeyPath key_path =
*this;
On 2012/04/23 23:26:19, michaeln wrote:
> this innocent looking method looks like it may be expensive, how is this
method
> used?

Once the final patches have landed, only during calls to
IDBDatabase.createObjectStore() / IDBObjectStore.createIndex() - which are the
only places where IDBKeyPaths are creating. If the path is not valid, the
creation will fail and the IDBKeyPath will be discarded. There is no need to
call it later since invalid paths won't make it past that step, except for
testing.

That said, the WebIDBKeyPath implementation determines the validity on creation
and stores the result in a local member, so the call is cheap.

http://codereview.chromium.org/10204003/diff/1/content/common/indexed_db/inde...
File content/common/indexed_db/indexed_db_key_path.h (right):

http://codereview.chromium.org/10204003/diff/1/content/common/indexed_db/inde...
content/common/indexed_db/indexed_db_key_path.h:18: IndexedDBKeyPath(); //
Defaults to WebKit::WebIDBKeyPath::NullType.
On 2012/04/23 23:26:19, michaeln wrote:
> nit: two spaces prior to  // per style guide

Done.

[michaeln, 2012-04-24 01:30:39]

http://codereview.chromium.org/10204003/diff/1/content/common/indexed_db/inde...
File content/common/indexed_db/indexed_db_key_path.cc (right):

http://codereview.chromium.org/10204003/diff/1/content/common/indexed_db/inde...
content/common/indexed_db/indexed_db_key_path.cc:54: WebIDBKeyPath key_path =
*this;
On 2012/04/23 23:33:21, jsbell wrote:
> Once the final patches have landed, only during calls to
> IDBDatabase.createObjectStore() / IDBObjectStore.createIndex() - which are the
> only places where IDBKeyPaths are creating. If the path is not valid, the
> creation will fail and the IDBKeyPath will be discarded. There is no need to
> call it later since invalid paths won't make it past that step, except for
> testing.
> 
> That said, the WebIDBKeyPath implementation determines the validity on
creation
> and stores the result in a local member, so the call is cheap.

sgtm, the reason i was asking is that IsValid() constructs such an object per
call, but based on the usage pattern you described... moxnix

[michaeln, 2012-04-24 21:08:52]

modulo nits and a couple of questions, lgtm, but would be nice to wait for green
try runs after the webkit side has rolled in prior to committing

https://chromiumcodereview.appspot.com/10204003/diff/5001/content/browser/ind...
File content/browser/indexed_db/idbbindingutilities_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10204003/diff/5001/content/browser/ind...
content/browser/indexed_db/idbbindingutilities_browsertest.cc:384:
scoped_helper.CheckValuesForKeyPath(kId, serialized_values, key_path);
I don't follow the purpose of the change from !+Invalid to PropertyNotAvailable
strings in this test. Can you explain wassup? Or maybe the !+Invalid string
should be retained here?

https://chromiumcodereview.appspot.com/10204003/diff/5001/content/common/inde...
File content/common/indexed_db/indexed_db_key_path.cc (right):

https://chromiumcodereview.appspot.com/10204003/diff/5001/content/common/inde...
content/common/indexed_db/indexed_db_key_path.cc:43: if (keyPath.type() ==
WebIDBKeyPath::ArrayType) {
nit: might be more readable as a switch statement or
arr.clear(); str.clear(); if (arrType) {} else if (strType) {}

https://chromiumcodereview.appspot.com/10204003/diff/5001/content/common/inde...
content/common/indexed_db/indexed_db_key_path.cc:63: return
WebIDBKeyPath::create(static_cast<WebString>(string_));
Is this static_cast correct/needed?

Looks like you need to construct a real WebString here, and that this ctor will
do that automagically for you, WebString(const string16& s). Seems like you just
need WebIDBKeyPath::create(string_) and if that doesn't work
::create(WebString(string_)) could be a better choice.

https://chromiumcodereview.appspot.com/10204003/diff/5001/content/common/util...
File content/common/utility_messages.h (right):

https://chromiumcodereview.appspot.com/10204003/diff/5001/content/common/util...
content/common/utility_messages.h:65:
IPC_MESSAGE_CONTROL1(UtilityHostMsg_IDBKeysFromValuesAndKeyPath_Failed,
still needed?

https://chromiumcodereview.appspot.com/10204003/diff/5001/content/utility/uti...
File content/utility/utility_thread_impl.cc (left):

https://chromiumcodereview.appspot.com/10204003/diff/5001/content/utility/uti...
content/utility/utility_thread_impl.cc:104: Send(new
UtilityHostMsg_IDBKeysFromValuesAndKeyPath_Failed(id));
can this method be removed from the messages.h now?

[jsbell, 2012-04-24 22:17:47]

Other nits look good, I'll comment if necessary after testing the various build
permutations. But since there might be feedback on this reply...

https://chromiumcodereview.appspot.com/10204003/diff/5001/content/browser/ind...
File content/browser/indexed_db/idbbindingutilities_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10204003/diff/5001/content/browser/ind...
content/browser/indexed_db/idbbindingutilities_browsertest.cc:384:
scoped_helper.CheckValuesForKeyPath(kId, serialized_values, key_path);
On 2012/04/24 21:08:53, michaeln wrote:
> I don't follow the purpose of the change from !+Invalid to
PropertyNotAvailable
> strings in this test. Can you explain wassup? Or maybe the !+Invalid string
> should be retained here?

It's a great question. Since invalid key paths are being blocked at the creation
point, the follow-on WebCore patch adds an ASSERT(key_path.isValid()) key path
evaluation methods that this test calls, since invalid key paths should never
get evaluated unless there's a bug.

I waffled about retaining that ability for testing purposes, but decided that
the assertion was more important. The WebCore side will get additional unit
tests.

[jsbell, 2012-04-25 21:49:45]

http://codereview.chromium.org/10204003/diff/5001/content/common/indexed_db/i...
File content/common/indexed_db/indexed_db_key_path.cc (right):

http://codereview.chromium.org/10204003/diff/5001/content/common/indexed_db/i...
content/common/indexed_db/indexed_db_key_path.cc:43: if (keyPath.type() ==
WebIDBKeyPath::ArrayType) {
On 2012/04/24 21:08:53, michaeln wrote:
> nit: might be more readable as a switch statement or
> arr.clear(); str.clear(); if (arrType) {} else if (strType) {}

Done.

http://codereview.chromium.org/10204003/diff/5001/content/common/indexed_db/i...
content/common/indexed_db/indexed_db_key_path.cc:63: return
WebIDBKeyPath::create(static_cast<WebString>(string_));
On 2012/04/24 21:08:53, michaeln wrote:
> Is this static_cast correct/needed?
> 
> Looks like you need to construct a real WebString here, and that this ctor
will
> do that automagically for you, WebString(const string16& s). Seems like you
just
> need WebIDBKeyPath::create(string_) and if that doesn't work
> ::create(WebString(string_)) could be a better choice.

Done.

http://codereview.chromium.org/10204003/diff/5001/content/common/utility_mess...
File content/common/utility_messages.h (right):

http://codereview.chromium.org/10204003/diff/5001/content/common/utility_mess...
content/common/utility_messages.h:65:
IPC_MESSAGE_CONTROL1(UtilityHostMsg_IDBKeysFromValuesAndKeyPath_Failed,
On 2012/04/24 21:08:53, michaeln wrote:
> still needed?

Done.

http://codereview.chromium.org/10204003/diff/5001/content/utility/utility_thr...
File content/utility/utility_thread_impl.cc (left):

http://codereview.chromium.org/10204003/diff/5001/content/utility/utility_thr...
content/utility/utility_thread_impl.cc:104: Send(new
UtilityHostMsg_IDBKeysFromValuesAndKeyPath_Failed(id));
On 2012/04/24 21:08:53, michaeln wrote:
> can this method be removed from the messages.h now?

Done.

[jsbell, 2012-04-25 21:50:44]

On 2012/04/24 21:08:52, michaeln wrote:
> modulo nits and a couple of questions, lgtm, but would be nice to wait for
green
> try runs after the webkit side has rolled in prior to committing

Of course! I'll land it via the commit queue.

[jsbell, 2012-04-27 15:38:18]

win_rel failures are unrelated tests. I'll give it another few tries, though.
(One is an IDB test but looks like a hiccup in the test harness/logging system
not the test itself.)

darin@, tkent@ - OWNERS reviews please?

[jsbell, 2012-04-27 21:31:37]

+jam@ for content/ OWNERS review
+tony@ for webkit/ OWNERS review

please take a look

[tony, 2012-04-27 22:52:21]

webkit LGTM

http://codereview.chromium.org/10204003/diff/14001/webkit/glue/idb_bindings.h
File webkit/glue/idb_bindings.h (right):

http://codereview.chromium.org/10204003/diff/14001/webkit/glue/idb_bindings.h...
webkit/glue/idb_bindings.h:29: const WebKit:: WebIDBKeyPath& idb_key_path);
Nit: looks like a stray space after WebKit::.

[jsbell, 2012-04-27 23:46:26]

On 2012/04/27 22:52:21, tony wrote:
> Nit: looks like a stray space after WebKit::.

Oops - deleted.

[jam, 2012-04-30 16:01:29]

content lgtm with nits

http://codereview.chromium.org/10204003/diff/32001/content/common/indexed_db/...
File content/common/indexed_db/indexed_db_key_path.cc (right):

http://codereview.chromium.org/10204003/diff/32001/content/common/indexed_db/...
content/common/indexed_db/indexed_db_key_path.cc:46: {
nit: i think this tabbing isn't per the style guide, i.e. the brace bracket
should be on the previous line and the code in the case statement should be
moved left by 2 spaces

http://codereview.chromium.org/10204003/diff/32001/content/common/indexed_db/...
File content/common/indexed_db/indexed_db_key_path.h (right):

http://codereview.chromium.org/10204003/diff/32001/content/common/indexed_db/...
content/common/indexed_db/indexed_db_key_path.h:16: class CONTENT_EXPORT
IndexedDBKeyPath {
nit: please put this in the "content" namespace, that's where all new code in
content should go (and we need to migrate existing code)

http://codereview.chromium.org/10204003/diff/32001/content/common/indexed_db/...
File content/common/indexed_db/indexed_db_param_traits.cc (right):

http://codereview.chromium.org/10204003/diff/32001/content/common/indexed_db/...
content/common/indexed_db/indexed_db_param_traits.cc:169: {
ditto

[jsbell, 2012-04-30 18:08:23]

Filed http://crbug.com/125589 for migrating existing IDBKey* types (at least) to
content.

http://codereview.chromium.org/10204003/diff/32001/content/common/indexed_db/...
File content/common/indexed_db/indexed_db_key_path.cc (right):

http://codereview.chromium.org/10204003/diff/32001/content/common/indexed_db/...
content/common/indexed_db/indexed_db_key_path.cc:46: {
On 2012/04/30 16:01:29, John Abd-El-Malek wrote:
> nit: i think this tabbing isn't per the style guide, i.e. the brace bracket
> should be on the previous line and the code in the case statement should be
> moved left by 2 spaces

Done.

http://codereview.chromium.org/10204003/diff/32001/content/common/indexed_db/...
File content/common/indexed_db/indexed_db_key_path.h (right):

http://codereview.chromium.org/10204003/diff/32001/content/common/indexed_db/...
content/common/indexed_db/indexed_db_key_path.h:16: class CONTENT_EXPORT
IndexedDBKeyPath {
On 2012/04/30 16:01:29, John Abd-El-Malek wrote:
> nit: please put this in the "content" namespace, that's where all new code in
> content should go (and we need to migrate existing code)

Done.

http://codereview.chromium.org/10204003/diff/32001/content/common/indexed_db/...
File content/common/indexed_db/indexed_db_param_traits.cc (right):

http://codereview.chromium.org/10204003/diff/32001/content/common/indexed_db/...
content/common/indexed_db/indexed_db_param_traits.cc:169: {
On 2012/04/30 16:01:29, John Abd-El-Malek wrote:
> ditto

Done.

[commit-bot: I haz the power, 2012-04-30 18:34:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jsbell@chromium.org/10204003/39004

[commit-bot: I haz the power, 2012-04-30 20:18:34]

Change committed as 134581