Chrome side changes for integer versions

Chrome side changes for integer versions

BUG=108223
TEST=


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

[dgrogan, 2012-07-25 20:26:10]

Can't land until https://bugs.webkit.org/show_bug.cgi?id=92203 does.

https://chromiumcodereview.appspot.com/10829013/diff/7013/content/browser/in_...
File content/browser/in_process_webkit/indexed_db_callbacks.h (right):

https://chromiumcodereview.appspot.com/10829013/diff/7013/content/browser/in_...
content/browser/in_process_webkit/indexed_db_callbacks.h:84: delete idb_object;
A better fix might be to add a map to IDBCallbacksProxy.cpp that would prevent a
new WebIDBDatabaseImpl from being instantiated.  See
https://bugs.webkit.org/attachment.cgi?id=153651&action=review#line1383

[dgrogan, 2012-07-25 20:27:09]

https://chromiumcodereview.appspot.com/10829013/diff/7013/content/browser/in_...
File content/browser/in_process_webkit/indexed_db_callbacks.h (right):

https://chromiumcodereview.appspot.com/10829013/diff/7013/content/browser/in_...
content/browser/in_process_webkit/indexed_db_callbacks.h:84: delete idb_object;
That link to IDBCallbacksProxy doesn't actually implement the map, it's just for
context.

[dgrogan, 2012-07-25 20:28:26]

Josh, could you review this?

It can't land until the WebKit API changes
(https://bugs.webkit.org/show_bug.cgi?id=92203) are rolled into chromium.

[jsbell, 2012-07-25 22:12:16]

lgtm with nits/questions

https://chromiumcodereview.appspot.com/10829013/diff/7013/content/browser/in_...
File content/browser/in_process_webkit/indexed_db_dispatcher_host.cc (right):

https://chromiumcodereview.appspot.com/10829013/diff/7013/content/browser/in_...
content/browser/in_process_webkit/indexed_db_dispatcher_host.cc:1149: //
transactions.
Maybe clarify that it isn't new with this patch, for future investigators.

https://chromiumcodereview.appspot.com/10829013/diff/7013/content/common/inde...
File content/common/indexed_db/indexed_db_dispatcher.cc (right):

https://chromiumcodereview.appspot.com/10829013/diff/7013/content/common/inde...
content/common/indexed_db/indexed_db_dispatcher.cc:111:
IPC_MESSAGE_HANDLER(IndexedDBMsg_CallbacksUpgradeNeeded, OnUpgradeNeeded)
Can you put this with the other _Callbacks messages?

https://chromiumcodereview.appspot.com/10829013/diff/7013/content/common/inde...
content/common/indexed_db/indexed_db_dispatcher.cc:571: if
(!databases_.count(object_id))
Add comment here that count will be non-zero if an upgrade was performed.

(I wonder what happens if you retain the IDBDatabase object in script after the
upgradeneeded transaction completes and call close() on it before you get an
onsuccess?)

https://chromiumcodereview.appspot.com/10829013/diff/7013/content/common/inde...
content/common/indexed_db/indexed_db_dispatcher.cc:692:
callbacks->onBlocked(existing_version);
Will the non-int OnBlocked plumbing be removed before the setVersion API itself
is removed, or will we have these both in parallel for a while? (Wondering if we
should combine these into one IPC message and using a sentinel value for
distinguishing here.)

https://chromiumcodereview.appspot.com/10829013/diff/7013/content/common/inde...
File content/common/indexed_db/indexed_db_dispatcher.h (right):

https://chromiumcodereview.appspot.com/10829013/diff/7013/content/common/inde...
content/common/indexed_db/indexed_db_dispatcher.h:263: void
OnUpgradeNeeded(int32 thread_id,
Should group with other events headed back to an an IDBRequest (i.e. those being
routed with a response_id)

[dgrogan, 2012-07-25 23:59:58]

https://chromiumcodereview.appspot.com/10829013/diff/7013/content/browser/in_...
File content/browser/in_process_webkit/indexed_db_dispatcher_host.cc (right):

https://chromiumcodereview.appspot.com/10829013/diff/7013/content/browser/in_...
content/browser/in_process_webkit/indexed_db_dispatcher_host.cc:1149: //
transactions.
On 2012/07/25 22:12:16, jsbell wrote:
> Maybe clarify that it isn't new with this patch, for future investigators.

It could be new with this patch, I'm not sure, but clarified.

https://chromiumcodereview.appspot.com/10829013/diff/7013/content/common/inde...
File content/common/indexed_db/indexed_db_dispatcher.cc (right):

https://chromiumcodereview.appspot.com/10829013/diff/7013/content/common/inde...
content/common/indexed_db/indexed_db_dispatcher.cc:111:
IPC_MESSAGE_HANDLER(IndexedDBMsg_CallbacksUpgradeNeeded, OnUpgradeNeeded)
On 2012/07/25 22:12:16, jsbell wrote:
> Can you put this with the other _Callbacks messages?

Done.

https://chromiumcodereview.appspot.com/10829013/diff/7013/content/common/inde...
content/common/indexed_db/indexed_db_dispatcher.cc:571: if
(!databases_.count(object_id))
On 2012/07/25 22:12:16, jsbell wrote:
> Add comment here that count will be non-zero if an upgrade was performed.

Done.

> (I wonder what happens if you retain the IDBDatabase object in script after
the
> upgradeneeded transaction completes and call close() on it before you get an
> onsuccess?)

Ugh, good question.  In trying to test this, I put setTimeout(f(){db.close()},
0) at the end of oncomplete but the success event is fired before the function
in setTimeout.

https://chromiumcodereview.appspot.com/10829013/diff/7013/content/common/inde...
content/common/indexed_db/indexed_db_dispatcher.cc:692:
callbacks->onBlocked(existing_version);
On 2012/07/25 22:12:16, jsbell wrote:
> Will the non-int OnBlocked plumbing be removed before the setVersion API
itself
> is removed, or will we have these both in parallel for a while?

Probably both in parallel for a while.

(Wondering if we
> should combine these into one IPC message and using a sentinel value for
> distinguishing here.)

Yeah, the messages and handlers are fairly redundant. But I think I'd rather
have the redundancy than logic (if (sentinel)...) that could be suspect if
there's a bug.

https://chromiumcodereview.appspot.com/10829013/diff/7013/content/common/inde...
File content/common/indexed_db/indexed_db_dispatcher.h (right):

https://chromiumcodereview.appspot.com/10829013/diff/7013/content/common/inde...
content/common/indexed_db/indexed_db_dispatcher.h:263: void
OnUpgradeNeeded(int32 thread_id,
On 2012/07/25 22:12:16, jsbell wrote:
> Should group with other events headed back to an an IDBRequest (i.e. those
being
> routed with a response_id)

Done.

[jsbell, 2012-07-26 00:11:59]

On 2012/07/25 23:59:58, dgrogan wrote:

> > (I wonder what happens if you retain the IDBDatabase object in script after
> the
> > upgradeneeded transaction completes and call close() on it before you get an
> > onsuccess?)
> 
> Ugh, good question.  In trying to test this, I put setTimeout(f(){db.close()},
> 0) at the end of oncomplete but the success event is fired before the function
> in setTimeout.

How about doing it in the oncomplete itself (not a timeout)? If it behaves
sensibly in both of those I think we're okay.

[dgrogan, 2012-07-26 01:19:25]

Brett W, could you review webkit/support/test_webkit_platform_support.cc as an
owner of webkit/ ?  Feel free to look at anything else too, of course.

[brettw, 2012-07-26 20:51:23]

webkit lgtm rubberstamp

[commit-bot: I haz the power, 2012-07-26 20:54:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dgrogan@chromium.org/10829013/11003

[commit-bot: I haz the power, 2012-07-26 21:41:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dgrogan@chromium.org/10829013/5007

[commit-bot: I haz the power, 2012-07-27 00:21:03]

Change committed as 148673