IndexedDB: Added IDBObjectStore.getAll() implementation.

IndexedDB: Added IDBObjectStore.getAll() implementation.

This is the experimental IDBObjectStore.getAll implementation for
retrieving multiple values in a single request as proposed in
https://www.w3.org/2008/webapps/wiki/IndexedDatabaseFeatures

In order to use run Chrome with: --enable-experimental-web-platform-features

BUG=457450
Committed: https://crrev.com/b86405eb7d5712e42f96012724b975fbad6a74bb
Cr-Commit-Position: refs/heads/master@{#329848}

[jsbell, 2015-04-08 18:23:52]

jsbell@chromium.org changed reviewers:
+ jsbell@chromium.org

[jsbell, 2015-04-08 18:23:52]

Initial notes

https://codereview.chromium.org/1074493002/diff/1/content/browser/indexed_db/...
File content/browser/indexed_db/indexed_db_database.cc (right):

https://codereview.chromium.org/1074493002/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/indexed_db_database.cc:768: } while
(found_values.size() < static_cast<size_t>(max_count));
What's our behavior if the result size is > max IPC size?

For cursor prefetching we use both the requested # limit and monitor the IPC
limit. See IndexedDBCursor::CursorPrefetchIterationOperation

We'd need to communicate back in the results that we could not return all the
requested results - I'm not sure what that API should look like.

(For now a TODO and crbug is probably okay, maybe being safe and simply dropping
the extra values on the floor rather than overloading IPC...?)

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

https://codereview.chromium.org/1074493002/diff/1/content/browser/indexed_db/...
File content/browser/indexed_db/indexed_db_database.cc (right):

https://codereview.chromium.org/1074493002/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/indexed_db_database.cc:768: } while
(found_values.size() < static_cast<size_t>(max_count));
On 2015/04/08 at 18:23:52, jsbell wrote:
> What's our behavior if the result size is > max IPC size?
> 
> For cursor prefetching we use both the requested # limit and monitor the IPC
limit. See IndexedDBCursor::CursorPrefetchIterationOperation
> 
> We'd need to communicate back in the results that we could not return all the
requested results - I'm not sure what that API should look like.
> 
> (For now a TODO and crbug is probably okay, maybe being safe and simply
dropping the extra values on the floor rather than overloading IPC...?)

I added the size check & TODO for the next patch, but would just prefer to go
ahead and implement a solution to that problem in this CL. I guess we could do
one of the following:

1. Override the success event handler to provide that error data.
2. Add a property to the response (Array).
3. Silently fail (undesired).
4. Fill in the truncated response values with undefined.
5. throw a DOMException of type (DataError???) New exception?:
   a. Num values we have space to transport.
   b. Deliver the values to the error handler.

I'm sure there are other alternatives I'm missing. My preference is 5b.

[jsbell, 2015-04-28 00:06:54]

https://codereview.chromium.org/1074493002/diff/20001/content/browser/indexed...
File content/browser/indexed_db/indexed_db_callbacks.cc (right):

https://codereview.chromium.org/1074493002/diff/20001/content/browser/indexed...
content/browser/indexed_db/indexed_db_callbacks.cc:297:
&params->values.at(i).blob_or_file_info,
Why at() rather than [] ?

https://codereview.chromium.org/1074493002/diff/20001/content/browser/indexed...
File content/browser/indexed_db/indexed_db_database.cc (right):

https://codereview.chromium.org/1074493002/diff/20001/content/browser/indexed...
content/browser/indexed_db/indexed_db_database.cc:749: const size_t
max_size_estimate = 10 * 1024 * 1024;
Can you add a TODO here to replace this with IPC-based limit?

In indexed_db_dispatcher we try and cap a single value being WRITTEN using
IPC::Channel::kMaximumMessageSize 

For prefetch we just stop adding to the RETURN set after we're over this limit.
So with prefetch we always return at least one value (which logically should be
under the IPC limit, even if it's > 10MB).

https://codereview.chromium.org/1074493002/diff/20001/content/browser/indexed...
content/browser/indexed_db/indexed_db_database.cc:788:
found_values.push_back(return_value);
So here if the first value found is > 10MB there will be nothing returned.
Consider matching prefetch so we add the current value, but break the loop if >
the local limit.

(or make the limit based on IPC, in which case this logic is correct. or error
if we go past the limit, which makes this moot)

https://codereview.chromium.org/1074493002/diff/20001/content/browser/indexed...
File content/browser/indexed_db/indexed_db_database.h (right):

https://codereview.chromium.org/1074493002/diff/20001/content/browser/indexed...
content/browser/indexed_db/indexed_db_database.h:126: int64 object_store_id,
Can we add getAll() for indexes later w/o too much churn across the blink API
boundary?

Similarly, do we want to make GetAllKeys the same method with a different flag,
or a different method?

[jsbell, 2015-04-28 00:15:50]

On 2015/04/17 23:01:14, cmumford wrote:
> I added the size check & TODO for the next patch, but would just prefer to go
> ahead and implement a solution to that problem in this CL. I guess we could do
> one of the following:

Added some notes on the difference between this and prefetch. I'd forgotten we
had different limits for writes (based on IPC) and reads (arbitrary limit, for
no good reason, but always at least 1)
 
> 1. Override the success event handler to provide that error data.

It ends up being a value on IDBRequest.result - which is always simple e.g.
(value or undefined, cursor or null); not sure what we'd do except return an
object with more fields on it.

> 2. Add a property to the response (Array).

Bleah :)

> 3. Silently fail (undesired).

Bleah. :)
> 4. Fill in the truncated response values with undefined.

Interesting, but still requires iterating the entire DB to count them. Still,
interesting.

> 5. throw a DOMException of type (DataError???) New exception?:
>    a. Num values we have space to transport.
>    b. Deliver the values to the error handler.

This would show up on IDBRequest.error which is of type DOMError, which has a
name and message (both strings). We want to keep DOMError close to DOMException
in design, and aren't subclassing DOMException.

So, I guess my vote for now would be:

* implement it as an IPC-based limit
* make it an error of type DataError

That would make the transaction abort, but... I think this is the safest. We can
continue discussions w/ Moz to refine this.

> I'm sure there are other alternatives I'm missing. My preference is 5b.

[cmumford, 2015-04-29 23:17:46]

https://codereview.chromium.org/1074493002/diff/20001/content/browser/indexed...
File content/browser/indexed_db/indexed_db_callbacks.cc (right):

https://codereview.chromium.org/1074493002/diff/20001/content/browser/indexed...
content/browser/indexed_db/indexed_db_callbacks.cc:297:
&params->values.at(i).blob_or_file_info,
On 2015/04/28 at 00:06:53, jsbell wrote:
> Why at() rather than [] ?

I usually only use at() when it's a pointer. I prefer values->at(i) instead
(*values)[i] - it's just easier for me to read. However, in this case, I'm not
sure that member was ever a pointer.

https://codereview.chromium.org/1074493002/diff/20001/content/browser/indexed...
File content/browser/indexed_db/indexed_db_database.cc (right):

https://codereview.chromium.org/1074493002/diff/20001/content/browser/indexed...
content/browser/indexed_db/indexed_db_database.cc:749: const size_t
max_size_estimate = 10 * 1024 * 1024;
On 2015/04/28 at 00:06:53, jsbell wrote:
> Can you add a TODO here to replace this with IPC-based limit?
> 
> In indexed_db_dispatcher we try and cap a single value being WRITTEN using
IPC::Channel::kMaximumMessageSize 
> 
> For prefetch we just stop adding to the RETURN set after we're over this
limit. So with prefetch we always return at least one value (which logically
should be under the IPC limit, even if it's > 10MB).

Any reason why I shouldn't just use IPC::Channel::kMaximumMessageSize in lieu of
the TODO?

https://codereview.chromium.org/1074493002/diff/20001/content/browser/indexed...
content/browser/indexed_db/indexed_db_database.cc:788:
found_values.push_back(return_value);
On 2015/04/28 at 00:06:54, jsbell wrote:
> So here if the first value found is > 10MB there will be nothing returned.
Consider matching prefetch so we add the current value, but break the loop if >
the local limit.
> 
> (or make the limit based on IPC, in which case this logic is correct. or error
if we go past the limit, which makes this moot)

Yes, if I use IPC then we should be good (right), cause nothing could be here
greater than that value.

https://codereview.chromium.org/1074493002/diff/20001/content/browser/indexed...
File content/browser/indexed_db/indexed_db_database.h (right):

https://codereview.chromium.org/1074493002/diff/20001/content/browser/indexed...
content/browser/indexed_db/indexed_db_database.h:126: int64 object_store_id,
On 2015/04/28 at 00:06:54, jsbell wrote:
> Can we add getAll() for indexes later w/o too much churn across the blink API
boundary?
> 
> Similarly, do we want to make GetAllKeys the same method with a different
flag, or a different method?

Good question. I believe it would only be one additional parameter in the
Blink->Chrome direction, much like it is for GetOperation. Personally I think
that GetOperation doesn't benefit by being able to support getting both keys and
values, and my preference would be to split it into two different functions. But
I'm willing to go with what the team considers most readable there (and for
GetAllOperation).

[jsbell, 2015-04-30 00:01:15]

https://codereview.chromium.org/1074493002/diff/20001/content/browser/indexed...
File content/browser/indexed_db/indexed_db_database.cc (right):

https://codereview.chromium.org/1074493002/diff/20001/content/browser/indexed...
content/browser/indexed_db/indexed_db_database.cc:749: const size_t
max_size_estimate = 10 * 1024 * 1024;
On 2015/04/29 23:17:46, cmumford wrote:
> On 2015/04/28 at 00:06:53, jsbell wrote:
> > Can you add a TODO here to replace this with IPC-based limit?
> > 
> > In indexed_db_dispatcher we try and cap a single value being WRITTEN using
> IPC::Channel::kMaximumMessageSize 
> > 
> > For prefetch we just stop adding to the RETURN set after we're over this
> limit. So with prefetch we always return at least one value (which logically
> should be under the IPC limit, even if it's > 10MB).
> 
> Any reason why I shouldn't just use IPC::Channel::kMaximumMessageSize in lieu
of
> the TODO?

Doing the full-blown IPC-based fix is fine, too!

In that case, add a TODO for prefetch?

https://codereview.chromium.org/1074493002/diff/20001/content/browser/indexed...
content/browser/indexed_db/indexed_db_database.cc:788:
found_values.push_back(return_value);
On 2015/04/29 23:17:46, cmumford wrote:
> On 2015/04/28 at 00:06:54, jsbell wrote:
> > So here if the first value found is > 10MB there will be nothing returned.
> Consider matching prefetch so we add the current value, but break the loop if
>
> the local limit.
> > 
> > (or make the limit based on IPC, in which case this logic is correct. or
error
> if we go past the limit, which makes this moot)
> 
> Yes, if I use IPC then we should be good (right), cause nothing could be here
> greater than that value.

Acknowledged.

https://codereview.chromium.org/1074493002/diff/20001/content/browser/indexed...
File content/browser/indexed_db/indexed_db_database.h (right):

https://codereview.chromium.org/1074493002/diff/20001/content/browser/indexed...
content/browser/indexed_db/indexed_db_database.h:126: int64 object_store_id,
Split is fine, just wanted it thought through.

[jsbell, 2015-04-30 17:28:19]

looking pretty good!

https://codereview.chromium.org/1074493002/diff/60001/content/browser/indexed...
File content/browser/indexed_db/indexed_db_database.cc (right):

https://codereview.chromium.org/1074493002/diff/60001/content/browser/indexed...
content/browser/indexed_db/indexed_db_database.cc:758:
scoped_ptr<IndexedDBBackingStore::Cursor> backing_store_cursor;
Could just call this `cursor` to reduce clutter. Not a big deal, though.

https://codereview.chromium.org/1074493002/diff/60001/content/browser/indexed...
content/browser/indexed_db/indexed_db_database.cc:764: backing_store_cursor =
backing_store_->OpenObjectStoreCursor(
The `else` block looks identical to the `if` block?

https://codereview.chromium.org/1074493002/diff/60001/content/browser/indexed...
content/browser/indexed_db/indexed_db_database.cc:771: IndexedDBDatabaseError
error(blink::WebIDBDatabaseExceptionUnknownError,
Call callbacks->OnError() here?

https://codereview.chromium.org/1074493002/diff/60001/content/browser/indexed...
content/browser/indexed_db/indexed_db_database.cc:776: }
Should return at end of !s.ok() block?

https://codereview.chromium.org/1074493002/diff/60001/content/browser/indexed...
content/browser/indexed_db/indexed_db_database.cc:823:
IPC::Channel::kMaximumMessageSize) {
Since the estimate is approximate, can you add in the overhead from
indexed_db_dispatcher.cc ?

Maybe move it to content/common/indexed_db/indexed_db_constants.h ?

[cmumford, 2015-04-30 22:30:13]

https://codereview.chromium.org/1074493002/diff/60001/content/browser/indexed...
File content/browser/indexed_db/indexed_db_database.cc (right):

https://codereview.chromium.org/1074493002/diff/60001/content/browser/indexed...
content/browser/indexed_db/indexed_db_database.cc:758:
scoped_ptr<IndexedDBBackingStore::Cursor> backing_store_cursor;
On 2015/04/30 at 17:28:18, jsbell wrote:
> Could just call this `cursor` to reduce clutter. Not a big deal, though.

Done.

https://codereview.chromium.org/1074493002/diff/60001/content/browser/indexed...
content/browser/indexed_db/indexed_db_database.cc:764: backing_store_cursor =
backing_store_->OpenObjectStoreCursor(
On 2015/04/30 at 17:28:18, jsbell wrote:
> The `else` block looks identical to the `if` block?

True that!  :$

https://codereview.chromium.org/1074493002/diff/60001/content/browser/indexed...
content/browser/indexed_db/indexed_db_database.cc:776: }
On 2015/04/30 at 17:28:18, jsbell wrote:
> Should return at end of !s.ok() block?

Yes, you are correct (on both). Pointing this out also highlighted my omission
of a corruption test for getAll - which would have caught this. I wrote one, but
it didn't pass - which turned out to be an IDB bug (missing error handling). I
decided to put that in a separate CL (http://crrev.com/1116253003).

[jsbell, 2015-05-04 22:29:38]

lgtm

Please update the CL description with more context before landing.

[cmumford, 2015-05-13 18:52:35]

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

[cmumford, 2015-05-13 18:52:36]

Adding tsepez@ for security review of
content/common/indexed_db/indexed_db_messages.h

[Tom Sepez, 2015-05-13 21:09:05]

Messages LGTM.

[cmumford, 2015-05-14 01:48:10]

The CQ bit was checked by cmumford@chromium.org

[cmumford, 2015-05-14 01:48:10]

The patchset sent to the CQ was uploaded after l-g-t-m from jsbell@chromium.org
Link to the patchset: https://codereview.chromium.org/1074493002/#ps120001
(title: "Added unused indexId/keyOnly parameters to getAll.")

[commit-bot: I haz the power, 2015-05-14 01:49:27]

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

[commit-bot: I haz the power, 2015-05-14 02:00:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-05-14 02:00:58]

Try jobs failed on following builders:
  linux_chromium_gn_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[cmumford, 2015-05-14 13:37:36]

The CQ bit was checked by cmumford@chromium.org

[commit-bot: I haz the power, 2015-05-14 13:37:56]

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

[commit-bot: I haz the power, 2015-05-14 14:50:49]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-05-14 14:51:32]

Patchset 7 (id:??) landed as
https://crrev.com/b86405eb7d5712e42f96012724b975fbad6a74bb
Cr-Commit-Position: refs/heads/master@{#329848}