IndexedDB: Treat count of 0 as unbounded for getAll() methods

IndexedDB: Treat count of 0 as unbounded for getAll() methods

After discussions w/ Moz and spec updates, modify the experimental
(behind a flag) IDBObjectStore/IDBIndex getAll()/getAllKeys() methods
to treat 0 the same as "not passed", and return as many results as
possible.

Also adds missing tests for IDBObjectStore.getAllKeys() and tweaks
the tests to avoid the deprecated assert_object_equals() function.

R=cmumford@chromium.org

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

[jsbell, 2015-08-20 23:28:03]

cmumford@ - please take a look?

[jsbell, 2015-08-21 17:16:38]

https://codereview.chromium.org/1303983002/diff/1/LayoutTests/storage/indexed...
File LayoutTests/storage/indexeddb/objectstore-getall.html (right):

https://codereview.chromium.org/1303983002/diff/1/LayoutTests/storage/indexed...
LayoutTests/storage/indexeddb/objectstore-getall.html:31: store.put('value-' +
letter, letter);
FYI, I made this change so that getAll and getAllKeys' behavior against this
store would be different.

[cmumford, 2015-08-21 17:32:40]

https://codereview.chromium.org/1303983002/diff/1/LayoutTests/storage/indexed...
File LayoutTests/storage/indexeddb/index-getall.html (right):

https://codereview.chromium.org/1303983002/diff/1/LayoutTests/storage/indexed...
LayoutTests/storage/indexeddb/index-getall.html:118:
assert_array_equals(data.map(e => e.upper), 'ABCDEFGHIJ'.split(''));
Yes, much better approach.

https://codereview.chromium.org/1303983002/diff/1/LayoutTests/storage/indexed...
LayoutTests/storage/indexeddb/index-getall.html:200: alphabet.map(e =>
e.toUpperCase()));
Nit: since it's used twice how about creating a global 'ALPHABET' object?

https://codereview.chromium.org/1303983002/diff/1/LayoutTests/storage/indexed...
File LayoutTests/storage/indexeddb/objectstore-getall.html (right):

https://codereview.chromium.org/1303983002/diff/1/LayoutTests/storage/indexed...
LayoutTests/storage/indexeddb/objectstore-getall.html:148:
assert_array_equals(data.map(e => e.id), [4, 5, 6]);
In general I really like the way you used map in this way. One downside I can
see, however, is that if (somehow) the object was returned with additional
properties - like not just 'ch' and 'id', but some others, then this test will
still pass whereas before it would fail. I'm not sure that's likely enough to
abandon this approach. What do you think?

https://codereview.chromium.org/1303983002/diff/1/LayoutTests/storage/indexed...
LayoutTests/storage/indexeddb/objectstore-getall.html:167:
assert_array_equals(evt.target.result, alphabet.map(c => 'value-' + c));
Do you want to keep this < 80 chars for w3c submission?

https://codereview.chromium.org/1303983002/diff/1/Source/modules/indexeddb/ID...
File Source/modules/indexeddb/IDBObjectStore.cpp (left):

https://codereview.chromium.org/1303983002/diff/1/Source/modules/indexeddb/ID...
Source/modules/indexeddb/IDBObjectStore.cpp:130:
exceptionState.throwTypeError(IDBDatabase::notValidMaxCountErrorMessage);
Since you're deleting all references to notValidMaxCountErrorMessage you can
remove it from IDBDatabase.*

[jsbell, 2015-08-21 17:48:33]

https://codereview.chromium.org/1303983002/diff/1/LayoutTests/storage/indexed...
File LayoutTests/storage/indexeddb/index-getall.html (right):

https://codereview.chromium.org/1303983002/diff/1/LayoutTests/storage/indexed...
LayoutTests/storage/indexeddb/index-getall.html:200: alphabet.map(e =>
e.toUpperCase()));
On 2015/08/21 17:32:40, cmumford wrote:
> Nit: since it's used twice how about creating a global 'ALPHABET' object?

Done.

https://codereview.chromium.org/1303983002/diff/1/LayoutTests/storage/indexed...
File LayoutTests/storage/indexeddb/objectstore-getall.html (right):

https://codereview.chromium.org/1303983002/diff/1/LayoutTests/storage/indexed...
LayoutTests/storage/indexeddb/objectstore-getall.html:148:
assert_array_equals(data.map(e => e.id), [4, 5, 6]);
On 2015/08/21 17:32:40, cmumford wrote:
> In general I really like the way you used map in this way. One downside I can
> see, however, is that if (somehow) the object was returned with additional
> properties - like not just 'ch' and 'id', but some others, then this test will
> still pass whereas before it would fail. I'm not sure that's likely enough to
> abandon this approach. What do you think?

IMHO we don't to be that paranoid everywhere, and keep these tests focused on
the getAll behavior. (Having dedicated tests that verify that key/value
retrieval doesn't include extra properties is good, pretty sure we have that
elsewhere.) 

The alternative would be comparing JSON.stringify() of the objects, which gets
wordy.

https://codereview.chromium.org/1303983002/diff/1/LayoutTests/storage/indexed...
LayoutTests/storage/indexeddb/objectstore-getall.html:167:
assert_array_equals(evt.target.result, alphabet.map(c => 'value-' + c));
On 2015/08/21 17:32:40, cmumford wrote:
> Do you want to keep this < 80 chars for w3c submission?

Done. (Here and elsewhere)

https://codereview.chromium.org/1303983002/diff/1/Source/modules/indexeddb/ID...
File Source/modules/indexeddb/IDBObjectStore.cpp (left):

https://codereview.chromium.org/1303983002/diff/1/Source/modules/indexeddb/ID...
Source/modules/indexeddb/IDBObjectStore.cpp:130:
exceptionState.throwTypeError(IDBDatabase::notValidMaxCountErrorMessage);
On 2015/08/21 17:32:40, cmumford wrote:
> Since you're deleting all references to notValidMaxCountErrorMessage you can
> remove it from IDBDatabase.*

Done.

[cmumford, 2015-08-21 18:01:48]

lgtm

[jsbell, 2015-08-21 18:02:11]

The CQ bit was checked by jsbell@chromium.org

[commit-bot: I haz the power, 2015-08-21 18:02:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1303983002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1303983002/20001

[commit-bot: I haz the power, 2015-08-21 18:48:24]

Committed patchset #2 (id:20001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=200999