Manage WebSQL callbacks with OwnPtr instead of refcounting

Manage WebSQL callbacks with OwnPtr instead of refcounting

The only tricky part of this change is in Database::runTransaction, where
a callback's refcount was temporarily 2 in order to queue an error callback.
I've reorganized the code a bit to avoid sharing ownership in that case, but
I'm not terribly familiar with Blink's WebSQL implementation so I'd appreciate
a careful look at what I've done there.

R=abarth@chromium.org
BUG=323681
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=163191

[abarth-chromium, 2013-12-04 01:55:49]

LGTM, but I'm not really an expert here either.

https://codereview.chromium.org/103473002/diff/1/Source/core/inspector/Inspec...
File Source/core/inspector/InspectorDatabaseAgent.cpp (right):

https://codereview.chromium.org/103473002/diff/1/Source/core/inspector/Inspec...
Source/core/inspector/InspectorDatabaseAgent.cpp:301: OwnPtr<SQLVoidCallback>
successCallback(TransactionSuccessCallback::create());
Do you plan to remove SQLVoidCallback in a subsequent CL?

https://codereview.chromium.org/103473002/diff/1/Source/modules/webdatabase/D...
File Source/modules/webdatabase/Database.cpp (right):

https://codereview.chromium.org/103473002/diff/1/Source/modules/webdatabase/D...
Source/modules/webdatabase/Database.cpp:169: transactionBackend =
backend()->runTransaction(transaction, readOnly, changeVersionData);
Combine this line with the previous one?

https://codereview.chromium.org/103473002/diff/1/Source/modules/webdatabase/D...
Source/modules/webdatabase/Database.cpp:171: if
(OwnPtr<SQLTransactionErrorCallback> callback =
transaction->releaseErrorCallback()) {
Will this always be non-zero if errorCallback was non-zero to start with?  That
was true in the old world.  Can we add an ASSERT to that effect?

https://codereview.chromium.org/103473002/diff/1/Source/modules/webdatabase/S...
File Source/modules/webdatabase/SQLCallbackWrapper.h (right):

https://codereview.chromium.org/103473002/diff/1/Source/modules/webdatabase/S...
Source/modules/webdatabase/SQLCallbackWrapper.h:78: PassOwnPtr<T> unwrap()
release() ?

https://codereview.chromium.org/103473002/diff/1/Source/modules/webdatabase/S...
Source/modules/webdatabase/SQLCallbackWrapper.h:87: bool hasCallback() const {
return m_callback; }
No mutex?  This function is kind of scary!

[adamk, 2013-12-04 02:08:26]

https://codereview.chromium.org/103473002/diff/1/Source/core/inspector/Inspec...
File Source/core/inspector/InspectorDatabaseAgent.cpp (right):

https://codereview.chromium.org/103473002/diff/1/Source/core/inspector/Inspec...
Source/core/inspector/InspectorDatabaseAgent.cpp:301: OwnPtr<SQLVoidCallback>
successCallback(TransactionSuccessCallback::create());
On 2013/12/04 01:55:50, abarth wrote:
> Do you plan to remove SQLVoidCallback in a subsequent CL?

Yes, added a FIXME in SQLVoidCallback.h

https://codereview.chromium.org/103473002/diff/1/Source/modules/webdatabase/D...
File Source/modules/webdatabase/Database.cpp (right):

https://codereview.chromium.org/103473002/diff/1/Source/modules/webdatabase/D...
Source/modules/webdatabase/Database.cpp:169: transactionBackend =
backend()->runTransaction(transaction, readOnly, changeVersionData);
On 2013/12/04 01:55:50, abarth wrote:
> Combine this line with the previous one?

Done.

https://codereview.chromium.org/103473002/diff/1/Source/modules/webdatabase/D...
Source/modules/webdatabase/Database.cpp:171: if
(OwnPtr<SQLTransactionErrorCallback> callback =
transaction->releaseErrorCallback()) {
On 2013/12/04 01:55:50, abarth wrote:
> Will this always be non-zero if errorCallback was non-zero to start with? 
That
> was true in the old world.  Can we add an ASSERT to that effect?

Added an ASSERT, but it's ugly. Also added a FIXME; I think this whole function
should probably live in DatabaseBackend, but I can't figure out what that class
is even for (it's actually |this|).

https://codereview.chromium.org/103473002/diff/1/Source/modules/webdatabase/S...
File Source/modules/webdatabase/SQLCallbackWrapper.h (right):

https://codereview.chromium.org/103473002/diff/1/Source/modules/webdatabase/S...
Source/modules/webdatabase/SQLCallbackWrapper.h:78: PassOwnPtr<T> unwrap()
On 2013/12/04 01:55:50, abarth wrote:
> release() ?

Don't want to touch this in this change; unwrap() is mentioned in comments.

https://codereview.chromium.org/103473002/diff/1/Source/modules/webdatabase/S...
Source/modules/webdatabase/SQLCallbackWrapper.h:87: bool hasCallback() const {
return m_callback; }
On 2013/12/04 01:55:50, abarth wrote:
> No mutex?  This function is kind of scary!

Yeah, don't quite get what this is for, especially considering the comment.
Anyway, I didn't make it any worse!

[commit-bot: I haz the power, 2013-12-04 02:08:46]

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

[commit-bot: I haz the power, 2013-12-04 02:52:34]

Retried try job too often on linux_blink for step(s) blink_platform_unittests,
webkit_lint, webkit_python_tests, webkit_tests, webkit_unit_tests, wtf_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...

[commit-bot: I haz the power, 2013-12-04 20:03:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/adamk@chromium.org/103473002/30001

[adamk, 2013-12-04 22:05:34]

Committed patchset #3 manually as r163191 (presubmit successful).