Convert WebIDBDatabaseImpl to IndexedDBConnection

Convert WebIDBDatabaseImpl to IndexedDBConnection

Simplify what was a proxy class with a little bit of state to just the state, and reveal its true nature as the object that binds a db callback to a database - i.e. the actual connection object. Track this, rather than the db callback, in the database itself so that the a database can operate on connections (e.g. force close them all in case of backing store error)

BUG=234278
R=dgrogan@chromium.org
TBR=jam@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=209886

[jsbell, 2013-07-02 17:38:14]

https://codereview.chromium.org/18221003/diff/7001/content/browser/indexed_db...
File content/browser/indexed_db/indexed_db_callbacks.h (left):

https://codereview.chromium.org/18221003/diff/7001/content/browser/indexed_db...
content/browser/indexed_db/indexed_db_callbacks.h:147:
scoped_ptr<WebIDBDatabaseImpl> web_database_impl_;
Unused even before this patch.

https://codereview.chromium.org/18221003/diff/7001/content/browser/indexed_db...
content/browser/indexed_db/indexed_db_callbacks.h:148:
scoped_refptr<IndexedDBDatabaseCallbacks> database_callbacks_;
The db callbacks and connection are now associated during the creation of the
connection, so this storage is not necessary.

https://codereview.chromium.org/18221003/diff/7001/content/browser/indexed_db...
content/browser/indexed_db/indexed_db_callbacks.h:149: bool did_complete_;
Unused even before this patch.

https://codereview.chromium.org/18221003/diff/7001/content/browser/indexed_db...
content/browser/indexed_db/indexed_db_callbacks.h:150: bool did_create_proxy_;
Redundant with ipc_database_id_

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

https://codereview.chromium.org/18221003/diff/7001/content/browser/indexed_db...
content/browser/indexed_db/indexed_db_database.cc:395: class
IndexedDBDatabase::PendingUpgradeCall {
This addition of two new PendingXXXCall types is unfortunate. We may be able to
replace these with closures c/o base::Bind in some cases.

[jsbell, 2013-07-02 18:10:03]

Forgot to mention my process here:

1. Rip the trampoline methods out of WebIDBDatabaseImpl (so callers use
impl->database()->foo() or impl->callbacks()->bar())
2. Rename WebIDBDatabaseImpl to IndexedDBConnection
3. Have IndexedDBDatabase track IndexedDBConnection objects instead of
IndexedDBDatabaseCallbacks objects

#3 proved to be the trickiest since that's where the connection objects get
minted and passed around during the connection phase (including blocked/pending
upgrade/running upgrade/upgrade success). But its just the payload - the calls
sequence didn't change at all, so the diff isn't too bad.

[dgrogan, 2013-07-02 22:01:57]

lgtm

https://codereview.chromium.org/18221003/diff/7001/content/browser/indexed_db...
File content/browser/indexed_db/indexed_db_connection.h (right):

https://codereview.chromium.org/18221003/diff/7001/content/browser/indexed_db...
content/browser/indexed_db/indexed_db_connection.h:26:
IndexedDBDatabaseCallbacks* callbacks() { return callbacks_; }
These should be consistent in whether or not they call get().

https://codereview.chromium.org/18221003/diff/7001/content/browser/indexed_db...
content/browser/indexed_db/indexed_db_connection.h:32: // The
database_callbacks_ member is cleared when the connection
s/database_//

https://codereview.chromium.org/18221003/diff/7001/content/browser/indexed_db...
File content/browser/indexed_db/indexed_db_database.cc (left):

https://codereview.chromium.org/18221003/diff/7001/content/browser/indexed_db...
content/browser/indexed_db/indexed_db_database.cc:1453:
DCHECK_EQ(pending_second_half_open_->Version(), metadata_.int_version);
What happened here? This block was just never entered? Was it left over from the
string->int version migration?

[jsbell, 2013-07-02 22:19:24]

https://codereview.chromium.org/18221003/diff/7001/content/browser/indexed_db...
File content/browser/indexed_db/indexed_db_connection.h (right):

https://codereview.chromium.org/18221003/diff/7001/content/browser/indexed_db...
content/browser/indexed_db/indexed_db_connection.h:26:
IndexedDBDatabaseCallbacks* callbacks() { return callbacks_; }
On 2013/07/02 22:01:58, dgrogan wrote:
> These should be consistent in whether or not they call get().

Done. Also, see follow-up patch https://codereview.chromium.org/18147009/

https://codereview.chromium.org/18221003/diff/7001/content/browser/indexed_db...
content/browser/indexed_db/indexed_db_connection.h:32: // The
database_callbacks_ member is cleared when the connection
On 2013/07/02 22:01:58, dgrogan wrote:
> s/database_//

Done.

https://codereview.chromium.org/18221003/diff/7001/content/browser/indexed_db...
File content/browser/indexed_db/indexed_db_database.cc (left):

https://codereview.chromium.org/18221003/diff/7001/content/browser/indexed_db...
content/browser/indexed_db/indexed_db_database.cc:1453:
DCHECK_EQ(pending_second_half_open_->Version(), metadata_.int_version);
On 2013/07/02 22:01:58, dgrogan wrote:
> What happened here? This block was just never entered? Was it left over from
the
> string->int version migration?

Oh yeah, meant to comment on this. Thanks. :)

During the Blink migration I moved this clause from ProcessPendingCalls up to
TransactionFinishedAndCompleteFired which was the only path that would lead to
it; this made TransactionFinishedAndXXXFired symmetrical and ProcessPendingCalls
simpler.

When doing this refactor I was quite surprised to see it duplicated here again
in ProcessPendingCalls! It must have come back in during a merge at some point,
and was harmless dead code since the condition could never be true due to the
processing in TransactionFinishedAndCompleteFired which happens first.

[commit-bot: I haz the power, 2013-07-02 22:20:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jsbell@chromium.org/18221003/20001

[commit-bot: I haz the power, 2013-07-02 22:31:07]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[commit-bot: I haz the power, 2013-07-02 22:42:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jsbell@chromium.org/18221003/20001

[jsbell, 2013-07-02 22:43:48]

TBR=jam for OWNERS review of the content_browser.gypi changes

[jam, 2013-07-02 22:59:12]

gypi lgtm

[commit-bot: I haz the power, 2013-07-03 04:33:50]

Change committed as 209886