Indexed DB: Adopt BufferSource model for binary keys

Indexed DB: Adopt BufferSource model for binary keys

An experimental feature adds support for "binary" key types to IDB.
Previously these were restricted to Uint8Arrays in/out. Since then
the spec world has semi-converged on BufferSource as an input
type (typedef for ArrayBuffer or ArrayBufferView) and returning
raw ArrayBuffers rather than a specific view type.

After further discussions with Mozilla, adopt the BufferSource
model for IDB binary keys.

Draft spec:
https://w3c.github.io/IndexedDB/#key-construct

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

[jsbell, 2015-04-23 16:04:52]

jsbell@chromium.org changed reviewers:
+ bashi@chromium.org, cmumford@chromium.org

[jsbell, 2015-04-23 16:04:52]

cmumford@, bashi@ - please take a look?

Discussion is taking place in:

https://www.w3.org/Bugs/Public/show_bug.cgi?id=23332

This is behind an experimental flag so won't break the web. I won't land until
we have a bit more consensus, though.

[bashi, 2015-04-24 00:40:07]

LGTM but cmumford@ should confirm.

[haraken, 2015-04-24 04:57:31]

bindings/ LGTM

[cmumford, 2015-04-27 17:14:22]

cmumford@chromium.org changed reviewers:
+ haraken@chromium.org

[cmumford, 2015-04-27 17:14:23]

lgtm % nits.

https://codereview.chromium.org/1087923003/diff/1/LayoutTests/storage/indexed...
File LayoutTests/storage/indexeddb/key-type-binary.html (right):

https://codereview.chromium.org/1087923003/diff/1/LayoutTests/storage/indexed...
LayoutTests/storage/indexeddb/key-type-binary.html:11:
assert_equals(indexedDB.cmp(a, a), 0, message);
expected is first.

https://codereview.chromium.org/1087923003/diff/1/LayoutTests/storage/indexed...
LayoutTests/storage/indexeddb/key-type-binary.html:39:
assert_not_equals(req.result, null, 'cursor should be present');
param order.

https://codereview.chromium.org/1087923003/diff/1/LayoutTests/storage/indexed...
LayoutTests/storage/indexeddb/key-type-binary.html:43:
assert_key_equals(found_key, key,
Nit: Do you want to swap the expected/actual order so that they are congruent
with those built into testharness.js?

https://codereview.chromium.org/1087923003/diff/1/LayoutTests/storage/indexed...
File LayoutTests/storage/indexeddb/resources/testharness-helpers.js (right):

https://codereview.chromium.org/1087923003/diff/1/LayoutTests/storage/indexed...
LayoutTests/storage/indexeddb/resources/testharness-helpers.js:20:
assert_equals(indexedDB.cmp(a, b), 0, message);
expected is first argument.

https://codereview.chromium.org/1087923003/diff/1/Source/bindings/modules/v8/...
File Source/bindings/modules/v8/V8BindingForModules.cpp (right):

https://codereview.chromium.org/1087923003/diff/1/Source/bindings/modules/v8/...
Source/bindings/modules/v8/V8BindingForModules.cpp:202: return
IDBKey::createBinary(SharedBuffer::create(start, length));
How much do we care about overflow? length is going from unsigned -> signed w/o
a check.

[jsbell, 2015-04-27 22:39:48]

Did I misunderstand the param ordering feedback or is one of us mis-reading?

https://codereview.chromium.org/1087923003/diff/1/LayoutTests/storage/indexed...
File LayoutTests/storage/indexeddb/key-type-binary.html (right):

https://codereview.chromium.org/1087923003/diff/1/LayoutTests/storage/indexed...
LayoutTests/storage/indexeddb/key-type-binary.html:11:
assert_equals(indexedDB.cmp(a, a), 0, message);
On 2015/04/27 17:14:23, cmumford wrote:
> expected is first.

Not per testharness.js docs:

* https://github.com/w3c/testharness.js/blob/master/docs/api.md
* http://darobin.github.io/test-harness-tutorial/docs/using-testharness.html

Or the source:

* https://github.com/w3c/testharness.js/blob/master/testharness.js#L780

All list: assert_equals(actual, expected, description)

https://codereview.chromium.org/1087923003/diff/1/Source/bindings/modules/v8/...
File Source/bindings/modules/v8/V8BindingForModules.cpp (right):

https://codereview.chromium.org/1087923003/diff/1/Source/bindings/modules/v8/...
Source/bindings/modules/v8/V8BindingForModules.cpp:202: return
IDBKey::createBinary(SharedBuffer::create(start, length));
On 2015/04/27 17:14:23, cmumford wrote:
> How much do we care about overflow? length is going from unsigned -> signed
w/o
> a check.

Covered by the FIXME at:

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

(Lame answer, but that's the right place to fix it)

[cmumford, 2015-04-27 22:44:23]

https://codereview.chromium.org/1087923003/diff/1/LayoutTests/storage/indexed...
File LayoutTests/storage/indexeddb/key-type-binary.html (right):

https://codereview.chromium.org/1087923003/diff/1/LayoutTests/storage/indexed...
LayoutTests/storage/indexeddb/key-type-binary.html:11:
assert_equals(indexedDB.cmp(a, a), 0, message);
On 2015/04/27 at 22:39:47, jsbell wrote:
> On 2015/04/27 17:14:23, cmumford wrote:
> > expected is first.
> 
> Not per testharness.js docs:
> 
> * https://github.com/w3c/testharness.js/blob/master/docs/api.md
> * http://darobin.github.io/test-harness-tutorial/docs/using-testharness.html
> 
> Or the source:
> 
> * https://github.com/w3c/testharness.js/blob/master/testharness.js#L780
> 
> All list: assert_equals(actual, expected, description)

Yep - you're totally correct. I naively trusted the first Google search result
for that function. Silly me!

[jsbell, 2015-05-27 21:59:05]

The CQ bit was checked by jsbell@chromium.org

[jsbell, 2015-05-27 21:59:06]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, cmumford@chromium.org, bashi@chromium.org
Link to the patchset: https://codereview.chromium.org/1087923003/#ps40001
(title: "Update test's shared lib usage")

[commit-bot: I haz the power, 2015-05-27 22:00:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1087923003/40001

[commit-bot: I haz the power, 2015-05-27 23:16:39]

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=196017