IndexedDB: Implement explicit queue for IDBOpenDBRequests

IndexedDB: Implement explicit queue for IDBOpenDBRequests

Align Chrome's processing of IDB open() and deleteDatabase() requests
with other browsers: such requests should be processed in FIFO order.
Previously, Chrome let delete requests jump ahead of blocked opens, as
hinted at in the spec but not supported by other browsers.

R=cmumford@chromium.org
BUG=479388
Committed: https://crrev.com/196442092e4c89aa9fe2e9c534374aaf2ebdb92b
Cr-Commit-Position: refs/heads/master@{#405880}

[jsbell, 2016-07-14 18:39:24]

The CQ bit was checked by jsbell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-14 18:40:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-14 19:55:37]

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

[commit-bot: I haz the power, 2016-07-14 19:55:38]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[jsbell, 2016-07-14 20:14:40]

The CQ bit was checked by jsbell@chromium.org to run a CQ dry run

[jsbell, 2016-07-14 20:14:44]

D'oh, failures are from an incorrect change I slipped in at the last minute
while minimizing diff, w/o rerunning tests.

Trying again.

[commit-bot: I haz the power, 2016-07-14 20:15:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-14 21:52:34]

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

[commit-bot: I haz the power, 2016-07-14 21:52:34]

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

[jsbell, 2016-07-14 22:18:13]

The CQ bit was checked by jsbell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-14 22:19:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jsbell, 2016-07-14 23:38:47]

Yay, okay, actually happy now. please take a look?

[commit-bot: I haz the power, 2016-07-14 23:39:24]

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

[commit-bot: I haz the power, 2016-07-14 23:39:25]

Dry run: This issue passed the CQ dry run.

[cmumford, 2016-07-15 14:55:37]

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

https://codereview.chromium.org/2148253003/diff/40001/content/browser/indexed...
content/browser/indexed_db/indexed_db_database.cc:130: if
(!db_->OpenInternal().ok()) {
We're throwing away the message in leveldb::Status returned by OpenInternal().
Do you think that might contain valuable info?

https://codereview.chromium.org/2148253003/diff/40001/content/browser/indexed...
content/browser/indexed_db/indexed_db_database.cc:165: // Database already
exists and either requested version is equal to
The comment is a restatement of the if (...) condition - which is pretty easy to
read. Not sure it adds much to the code.

https://codereview.chromium.org/2148253003/diff/40001/content/browser/indexed...
content/browser/indexed_db/indexed_db_database.cc:259: // Ownership of
connection was already pased along in OnUpgradeNeeded.
s/pased/passed/

https://codereview.chromium.org/2148253003/diff/40001/content/browser/indexed...
content/browser/indexed_db/indexed_db_database.cc:277: const int
child_process_id_;
This class is passed (in the constructor) a IndexedDBPendingConnection and
duplicates all five members of it's members transaction_id_, etc. Wouldn't it be
simpler to just have a IndexedDBPendingConnection member?

https://codereview.chromium.org/2148253003/diff/40001/content/browser/indexed...
content/browser/indexed_db/indexed_db_database.cc:328: "Internal error deleting
database.");
Can we sanitize and use message from |s|?

https://codereview.chromium.org/2148253003/diff/40001/content/browser/indexed...
content/browser/indexed_db/indexed_db_database.cc:332: db_->backing_store_ =
NULL;
nullptr

https://codereview.chromium.org/2148253003/diff/40001/content/browser/indexed...
content/browser/indexed_db/indexed_db_database.cc:342:
db_->metadata_.object_stores.clear();
Reset max_object_store_id?

https://codereview.chromium.org/2148253003/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/storage/indexeddb/resources/deletedatabase-delayed-by-versionchange.js
(right):

https://codereview.chromium.org/2148253003/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/storage/indexeddb/resources/deletedatabase-delayed-by-versionchange.js:17:

delete extra line.

[jsbell, 2016-07-15 17:21:44]

The CQ bit was checked by jsbell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-15 17:22:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jsbell, 2016-07-15 17:22:22]

Thanks! One more look?

I'm not sold on |pending_| as a member name but I wanted to keep it short.
Alternatives welcome.

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

https://codereview.chromium.org/2148253003/diff/40001/content/browser/indexed...
content/browser/indexed_db/indexed_db_database.cc:130: if
(!db_->OpenInternal().ok()) {
On 2016/07/15 14:55:36, cmumford wrote:
> We're throwing away the message in leveldb::Status returned by OpenInternal().
> Do you think that might contain valuable info?

Behavior change, making that a TODO.

https://codereview.chromium.org/2148253003/diff/40001/content/browser/indexed...
content/browser/indexed_db/indexed_db_database.cc:165: // Database already
exists and either requested version is equal to
On 2016/07/15 14:55:36, cmumford wrote:
> The comment is a restatement of the if (...) condition - which is pretty easy
to
> read. Not sure it adds much to the code.

Removed.

https://codereview.chromium.org/2148253003/diff/40001/content/browser/indexed...
content/browser/indexed_db/indexed_db_database.cc:259: // Ownership of
connection was already pased along in OnUpgradeNeeded.
On 2016/07/15 14:55:36, cmumford wrote:
> s/pased/passed/

Done.

https://codereview.chromium.org/2148253003/diff/40001/content/browser/indexed...
content/browser/indexed_db/indexed_db_database.cc:277: const int
child_process_id_;
On 2016/07/15 14:55:36, cmumford wrote:
> This class is passed (in the constructor) a IndexedDBPendingConnection and
> duplicates all five members of it's members transaction_id_, etc. Wouldn't it
be
> simpler to just have a IndexedDBPendingConnection member?

Done.

https://codereview.chromium.org/2148253003/diff/40001/content/browser/indexed...
content/browser/indexed_db/indexed_db_database.cc:328: "Internal error deleting
database.");
On 2016/07/15 14:55:36, cmumford wrote:
> Can we sanitize and use message from |s|?

Added a TODO.

https://codereview.chromium.org/2148253003/diff/40001/content/browser/indexed...
content/browser/indexed_db/indexed_db_database.cc:332: db_->backing_store_ =
NULL;
On 2016/07/15 14:55:36, cmumford wrote:
> nullptr

Done.

https://codereview.chromium.org/2148253003/diff/40001/content/browser/indexed...
content/browser/indexed_db/indexed_db_database.cc:342:
db_->metadata_.object_stores.clear();
On 2016/07/15 14:55:36, cmumford wrote:
> Reset max_object_store_id?

Interesting that this wasn't done before. Done!

https://codereview.chromium.org/2148253003/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/storage/indexeddb/resources/deletedatabase-delayed-by-versionchange.js
(right):

https://codereview.chromium.org/2148253003/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/storage/indexeddb/resources/deletedatabase-delayed-by-versionchange.js:17:

On 2016/07/15 14:55:37, cmumford wrote:
> delete extra line.

Done.

[cmumford, 2016-07-15 19:19:28]

lgtm

[commit-bot: I haz the power, 2016-07-15 20:01:10]

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

[commit-bot: I haz the power, 2016-07-15 20:01:11]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[jsbell, 2016-07-15 21:26:22]

The CQ bit was checked by jsbell@chromium.org

[commit-bot: I haz the power, 2016-07-15 21:26:57]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-15 22:27:24]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-07-15 22:27:28]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-15 22:29:59]

Description was changed from

==========
IndexedDB: Implement explicit queue for IDBOpenDBRequests

Align Chrome's processing of IDB open() and deleteDatabase() requests
with other browsers: such requests should be processed in FIFO order.
Previously, Chrome let delete requests jump ahead of blocked opens, as
hinted at in the spec but not supported by other browsers.

R=cmumford@chromium.org
BUG=479388
==========

to

==========
IndexedDB: Implement explicit queue for IDBOpenDBRequests

Align Chrome's processing of IDB open() and deleteDatabase() requests
with other browsers: such requests should be processed in FIFO order.
Previously, Chrome let delete requests jump ahead of blocked opens, as
hinted at in the spec but not supported by other browsers.

R=cmumford@chromium.org
BUG=479388

Committed: https://crrev.com/196442092e4c89aa9fe2e9c534374aaf2ebdb92b
Cr-Commit-Position: refs/heads/master@{#405880}
==========

[commit-bot: I haz the power, 2016-07-15 22:30:00]

Patchset 4 (id:??) landed as
https://crrev.com/196442092e4c89aa9fe2e9c534374aaf2ebdb92b
Cr-Commit-Position: refs/heads/master@{#405880}