IndexedDB: chromium side of IDBCursor.advance().

IndexedDB: chromium side of IDBCursor.advance().

This just implements the proxy and IPC code for advance(), modeled after 
IDBCursor.continue().

BUG=90647
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=133757

[alecflett, 2012-04-23 22:53:47]

a review of boilerplate.. jsbell@ ?

[jsbell, 2012-04-24 15:41:08]

http://codereview.chromium.org/10197001/diff/2001/content/browser/in_process_...
File content/browser/in_process_webkit/indexed_db_dispatcher_host.h (right):

http://codereview.chromium.org/10197001/diff/2001/content/browser/in_process_...
content/browser/in_process_webkit/indexed_db_dispatcher_host.h:268: unsigned
long,
Looks like the parameter should be named.

http://codereview.chromium.org/10197001/diff/2001/content/common/indexed_db/i...
File content/common/indexed_db/indexed_db_dispatcher.cc (right):

http://codereview.chromium.org/10197001/diff/2001/content/common/indexed_db/i...
content/common/indexed_db/indexed_db_dispatcher.cc:649: int32 response_id =
p.response_id;
This method is much simpler than OnSuccessCursorContinue - it doesn't have the
SetKeyAndValue() logic. Is that coming in a future patch or is it not needed
here?

http://codereview.chromium.org/10197001/diff/2001/content/common/indexed_db/i...
File content/common/indexed_db/indexed_db_messages.h (right):

http://codereview.chromium.org/10197001/diff/2001/content/common/indexed_db/i...
content/common/indexed_db/indexed_db_messages.h:172:
IPC_STRUCT_BEGIN(IndexedDBMsg_CallbacksSuccessCursorAdvance_Params)
It looks like the params are identical between
CallbackSuccess{IDBCursor|CursorAdvance|CursorContinue} - while I would keep
them separate for consistency for this patch, is there any reason they couldn't
be merged in the future?

[alecflett, 2012-04-24 16:34:22]

I'm going to see if I can write up a test that will fail without that
SetKeyAndValue logic, but that's on the webkit side, so this can still go in
after the next review.

http://chromiumcodereview.appspot.com/10197001/diff/2001/content/browser/in_p...
File content/browser/in_process_webkit/indexed_db_dispatcher_host.h (right):

http://chromiumcodereview.appspot.com/10197001/diff/2001/content/browser/in_p...
content/browser/in_process_webkit/indexed_db_dispatcher_host.h:268: unsigned
long,
On 2012/04/24 15:41:08, jsbell wrote:
> Looks like the parameter should be named.

Done.

http://chromiumcodereview.appspot.com/10197001/diff/2001/content/common/index...
File content/common/indexed_db/indexed_db_dispatcher.cc (right):

http://chromiumcodereview.appspot.com/10197001/diff/2001/content/common/index...
content/common/indexed_db/indexed_db_dispatcher.cc:649: int32 response_id =
p.response_id;
On 2012/04/24 15:41:08, jsbell wrote:
> This method is much simpler than OnSuccessCursorContinue - it doesn't have the
> SetKeyAndValue() logic. Is that coming in a future patch or is it not needed
> here?

Oops, I removed this as an experiment, and thought I added it back. Now to make
a test that forces this to fail...

http://chromiumcodereview.appspot.com/10197001/diff/2001/content/common/index...
File content/common/indexed_db/indexed_db_messages.h (right):

http://chromiumcodereview.appspot.com/10197001/diff/2001/content/common/index...
content/common/indexed_db/indexed_db_messages.h:172:
IPC_STRUCT_BEGIN(IndexedDBMsg_CallbacksSuccessCursorAdvance_Params)
On 2012/04/24 15:41:08, jsbell wrote:
> It looks like the params are identical between
> CallbackSuccess{IDBCursor|CursorAdvance|CursorContinue} - while I would keep
> them separate for consistency for this patch, is there any reason they
couldn't
> be merged in the future?

Its very possible - this was one of the areas that confused me a bit - exactly
how these IPC messages are filled in...

[alecflett, 2012-04-24 17:23:12]

ok, patch has been updated - comments addressed

[jsbell, 2012-04-24 17:36:21]

lgtm

[jsbell, 2012-04-24 17:38:20]

Aside: it's possible that we're advancing within the range that has been
prefetched, so as a future optimization we could check for that and not
invalidate the cache. Either way, there should be a test (on the WK side is
okay) that does continue() enough times to build up the cache, then does an
advance() (A) within the range and (b) beyond the range.

[commit-bot: I haz the power, 2012-04-24 17:38:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alecflett@chromium.org/10197001/10001

[commit-bot: I haz the power, 2012-04-24 20:34:49]

Change committed as 133757