Allow closing IndexedDB database before deleting

Allow closing IndexedDB database before deleting

DevTools wants to allow the users to delete individual IndexedDB
databases. Current API cannot delete database that is currently open.
This change introduces a new API parameter to force closing the
database.

BUG=682425
Review-Url: https://codereview.chromium.org/2642943002
Cr-Commit-Position: refs/heads/master@{#445486}
Committed: https://chromium.googlesource.com/chromium/src/+/1470b1f47385ebd166e6ca60a34a770c0f4c40f6

[eostroukhov, 2017-01-18 22:18:04]

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

[commit-bot: I haz the power, 2017-01-18 22:18:43]

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, 2017-01-18 22:34:29]

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

[commit-bot: I haz the power, 2017-01-18 22:34:30]

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

[eostroukhov, 2017-01-18 23:03:10]

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

[eostroukhov, 2017-01-18 23:03:33]

Description was changed from

==========
Allow closng IndexedDB database before deleting

DevTools wants to allow the users to delete individual IndexedDB
databases. Current API cannot delete database that is currently open.
This change introduces a new API parameter to force closing the
database.

BUG=682425
==========

to

==========
Allow closng IndexedDB database before deleting

DevTools wants to allow the users to delete individual IndexedDB
databases. Current API cannot delete database that is currently open.
This change introduces a new API parameter to force closing the
database.

BUG=682425
==========

[eostroukhov, 2017-01-18 23:04:04]

Description was changed from

==========
Allow closng IndexedDB database before deleting

DevTools wants to allow the users to delete individual IndexedDB
databases. Current API cannot delete database that is currently open.
This change introduces a new API parameter to force closing the
database.

BUG=682425
==========

to

==========
Allow closing IndexedDB database before deleting

DevTools wants to allow the users to delete individual IndexedDB
databases. Current API cannot delete database that is currently open.
This change introduces a new API parameter to force closing the
database.

BUG=682425
==========

[commit-bot: I haz the power, 2017-01-18 23:04:27]

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, 2017-01-19 00:49:23]

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

[commit-bot: I haz the power, 2017-01-19 00:49:24]

Dry run: This issue passed the CQ dry run.

[eostroukhov, 2017-01-19 00:50:26]

eostroukhov@chromium.org changed reviewers:
+ cmumford@chromium.org, jsbell@chromium.org

[eostroukhov, 2017-01-19 00:50:27]

Please take a look. I am not entirely sure this is what the API should look like
(e.g. should it be the boolean parameter or an entirely new call chain?) - I
tested this with the DevTools and looks like this API is enough. I will have a
DevTools CL once the API is finalized

Please note that DevTools are ok with objects not being deleted 100% of time
(e.g. if the database is recreated).

[cmumford, 2017-01-19 20:47:16]

Can you also 1) run git-cl-format, and 2) add a test for this?

https://codereview.chromium.org/2642943002/diff/20001/content/browser/indexed...
File content/browser/indexed_db/indexed_db_factory_impl.cc (right):

https://codereview.chromium.org/2642943002/diff/20001/content/browser/indexed...
content/browser/indexed_db/indexed_db_factory_impl.cc:223:
it->second->ForceClose();
Any reason why we can't pass force_close into DeleteDatabase? This would
preserve the symmetry of the function parameters, but also avoid closing and
reopening the database.

https://codereview.chromium.org/2642943002/diff/20001/content/browser/indexed...
File content/browser/indexed_db/indexed_db_factory_unittest.cc (right):

https://codereview.chromium.org/2642943002/diff/20001/content/browser/indexed...
content/browser/indexed_db/indexed_db_factory_unittest.cc:347: callbacks,
origin, temp_directory.GetPath(), false);
Add /* force_close */ to parameter.

https://codereview.chromium.org/2642943002/diff/20001/content/child/indexed_d...
File content/child/indexed_db/webidbfactory_impl.h (right):

https://codereview.chromium.org/2642943002/diff/20001/content/child/indexed_d...
content/child/indexed_db/webidbfactory_impl.h:44: bool) override;
Only omit parameter name when it's purpose is obvious from the type.

https://codereview.chromium.org/2642943002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/indexeddb/IDBFactory.h (right):

https://codereview.chromium.org/2642943002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/indexeddb/IDBFactory.h:60: IDBOpenDBRequest*
closeConnectionsAndDeleteDatabase(ScriptState*,
I don't know much about DevTools, but do you need to add an entry to
IDBFactory.idl, or DevTools bypass that?

https://codereview.chromium.org/2642943002/diff/20001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/modules/indexeddb/WebIDBFactory.h
(right):

https://codereview.chromium.org/2642943002/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/indexeddb/WebIDBFactory.h:55: bool) =
0;
add param name.

[eostroukhov, 2017-01-20 18:41:34]

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

[commit-bot: I haz the power, 2017-01-20 18:41:52]

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

[eostroukhov, 2017-01-20 18:43:19]

Thank you for the review. I addressed your comments (and added a simple test) -
please take another look.

https://codereview.chromium.org/2642943002/diff/20001/content/browser/indexed...
File content/browser/indexed_db/indexed_db_factory_impl.cc (right):

https://codereview.chromium.org/2642943002/diff/20001/content/browser/indexed...
content/browser/indexed_db/indexed_db_factory_impl.cc:223:
it->second->ForceClose();
On 2017/01/19 20:47:16, cmumford wrote:
> Any reason why we can't pass force_close into DeleteDatabase? This would
> preserve the symmetry of the function parameters, but also avoid closing and
> reopening the database.

Did that, also added a test.

https://codereview.chromium.org/2642943002/diff/20001/content/browser/indexed...
File content/browser/indexed_db/indexed_db_factory_unittest.cc (right):

https://codereview.chromium.org/2642943002/diff/20001/content/browser/indexed...
content/browser/indexed_db/indexed_db_factory_unittest.cc:347: callbacks,
origin, temp_directory.GetPath(), false);
On 2017/01/19 20:47:16, cmumford wrote:
> Add /* force_close */ to parameter.

Done.

https://codereview.chromium.org/2642943002/diff/20001/content/child/indexed_d...
File content/child/indexed_db/webidbfactory_impl.h (right):

https://codereview.chromium.org/2642943002/diff/20001/content/child/indexed_d...
content/child/indexed_db/webidbfactory_impl.h:44: bool) override;
On 2017/01/19 20:47:16, cmumford wrote:
> Only omit parameter name when it's purpose is obvious from the type.

Added a parameter name.

https://codereview.chromium.org/2642943002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/indexeddb/IDBFactory.h (right):

https://codereview.chromium.org/2642943002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/indexeddb/IDBFactory.h:60: IDBOpenDBRequest*
closeConnectionsAndDeleteDatabase(ScriptState*,
On 2017/01/19 20:47:16, cmumford wrote:
> I don't know much about DevTools, but do you need to add an entry to
> IDBFactory.idl, or DevTools bypass that?

Devtools are calling on C++ side (e.g.
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/indexe...
for the code that clears the IDB stores).

My understanding is that we don't need IDL for this scenario.

https://codereview.chromium.org/2642943002/diff/20001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/modules/indexeddb/WebIDBFactory.h
(right):

https://codereview.chromium.org/2642943002/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/indexeddb/WebIDBFactory.h:55: bool) =
0;
On 2017/01/19 20:47:16, cmumford wrote:
> add param name.

Done.

[jsbell, 2017-01-20 19:34:25]

https://codereview.chromium.org/2642943002/diff/40001/content/browser/indexed...
File content/browser/indexed_db/indexed_db_factory_impl.h (right):

https://codereview.chromium.org/2642943002/diff/40001/content/browser/indexed...
content/browser/indexed_db/indexed_db_factory_impl.h:52: bool forced_close)
override;
nit: it's 'force_close' (no 'd') elsewhere

https://codereview.chromium.org/2642943002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/indexeddb/IDBFactory.h (right):

https://codereview.chromium.org/2642943002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/indexeddb/IDBFactory.h:60: IDBOpenDBRequest*
closeConnectionsAndDeleteDatabase(ScriptState*,
Please add a comment that this is not Web-exposed, and is for use by devtools.

[eostroukhov, 2017-01-20 19:44:11]

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

[commit-bot: I haz the power, 2017-01-20 19:44:26]

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

[eostroukhov, 2017-01-20 19:45:26]

Thanks for the review. I updated the CL.

https://codereview.chromium.org/2642943002/diff/40001/content/browser/indexed...
File content/browser/indexed_db/indexed_db_factory_impl.h (right):

https://codereview.chromium.org/2642943002/diff/40001/content/browser/indexed...
content/browser/indexed_db/indexed_db_factory_impl.h:52: bool forced_close)
override;
On 2017/01/20 19:34:25, jsbell wrote:
> nit: it's 'force_close' (no 'd') elsewhere

Done.

https://codereview.chromium.org/2642943002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/indexeddb/IDBFactory.h (right):

https://codereview.chromium.org/2642943002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/indexeddb/IDBFactory.h:60: IDBOpenDBRequest*
closeConnectionsAndDeleteDatabase(ScriptState*,
On 2017/01/20 19:34:25, jsbell wrote:
> Please add a comment that this is not Web-exposed, and is for use by devtools.

Done.

[commit-bot: I haz the power, 2017-01-20 21:14:35]

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

[commit-bot: I haz the power, 2017-01-20 21:14:35]

Dry run: This issue passed the CQ dry run.

[jsbell, 2017-01-20 23:33:40]

lgtm with just one tidbit

https://codereview.chromium.org/2642943002/diff/60001/content/browser/indexed...
File content/browser/indexed_db/indexed_db_factory_impl.cc (right):

https://codereview.chromium.org/2642943002/diff/60001/content/browser/indexed...
content/browser/indexed_db/indexed_db_factory_impl.cc:286:
database->DeleteDatabase(callbacks, false /* force_close */);
Reading this CL it's a little weird to see false passed here when a seemingly
equivalent flag was passed in to this method. It makes sense with more context
(i.e. that if there were any connections we never would have made it this far,
so the flag becomes a no-op).

What do you think about either adding a comment, an assertion (e.g.
DCHECK_EQ(database->ConnectionCount(), 0), or just passing the force_close flag
(even though it's a no-op)?

[eostroukhov, 2017-01-23 19:22:01]

The CQ bit was checked by eostroukhov@chromium.org

[eostroukhov, 2017-01-23 19:22:02]

The patchset sent to the CQ was uploaded after l-g-t-m from jsbell@chromium.org
Link to the patchset: https://codereview.chromium.org/2642943002/#ps100001
(title: "Actually pass the flag")

[eostroukhov, 2017-01-23 19:22:22]

Thank you for the review, submitting now.

https://codereview.chromium.org/2642943002/diff/60001/content/browser/indexed...
File content/browser/indexed_db/indexed_db_factory_impl.cc (right):

https://codereview.chromium.org/2642943002/diff/60001/content/browser/indexed...
content/browser/indexed_db/indexed_db_factory_impl.cc:286:
database->DeleteDatabase(callbacks, false /* force_close */);
On 2017/01/20 23:33:40, jsbell wrote:
> Reading this CL it's a little weird to see false passed here when a seemingly
> equivalent flag was passed in to this method. It makes sense with more context
> (i.e. that if there were any connections we never would have made it this far,
> so the flag becomes a no-op).
> 
> What do you think about either adding a comment, an assertion (e.g.
> DCHECK_EQ(database->ConnectionCount(), 0), or just passing the force_close
flag
> (even though it's a no-op)?

I changed the code to pass the flag on.

[commit-bot: I haz the power, 2017-01-23 19:22:52]

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, 2017-01-23 21:13:57]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1485199321945150,
"parent_rev": "01f28a09b779a3081b4c485dae8e25e7fe8eba40", "commit_rev":
"1470b1f47385ebd166e6ca60a34a770c0f4c40f6"}

[commit-bot: I haz the power, 2017-01-23 21:14:27]

Description was changed from

==========
Allow closing IndexedDB database before deleting

DevTools wants to allow the users to delete individual IndexedDB
databases. Current API cannot delete database that is currently open.
This change introduces a new API parameter to force closing the
database.

BUG=682425
==========

to

==========
Allow closing IndexedDB database before deleting

DevTools wants to allow the users to delete individual IndexedDB
databases. Current API cannot delete database that is currently open.
This change introduces a new API parameter to force closing the
database.

BUG=682425

Review-Url: https://codereview.chromium.org/2642943002
Cr-Commit-Position: refs/heads/master@{#445486}
Committed:
https://chromium.googlesource.com/chromium/src/+/1470b1f47385ebd166e6ca60a34a...
==========

[commit-bot: I haz the power, 2017-01-23 21:14:29]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/1470b1f47385ebd166e6ca60a34a...

[Wez, 2017-02-08 01:03:58]

Description was changed from

==========
Allow closing IndexedDB database before deleting

DevTools wants to allow the users to delete individual IndexedDB
databases. Current API cannot delete database that is currently open.
This change introduces a new API parameter to force closing the
database.

BUG=682425

Review-Url: https://codereview.chromium.org/2642943002
Cr-Commit-Position: refs/heads/master@{#445486}
Committed:
https://chromium.googlesource.com/chromium/src/+/1470b1f47385ebd166e6ca60a34a...
==========

to

==========
Allow closing IndexedDB database before deleting

DevTools wants to allow the users to delete individual IndexedDB
databases. Current API cannot delete database that is currently open.
This change introduces a new API parameter to force closing the
database.

BUG=682425

Review-Url: https://codereview.chromium.org/2642943002
Cr-Commit-Position: refs/heads/master@{#445486}
Committed:
https://chromium.googlesource.com/chromium/src/+/1470b1f47385ebd166e6ca60a34a...
==========

[Wez, 2017-02-08 01:03:58]

wez@chromium.org changed reviewers:
+ wfh@chromium.org

[Wez, 2017-02-08 02:09:37]

Hallo wfh@chromium.org!
Due to a depot_tools patch which mistakenly removed the OWNERS check for
non-source files (see crbug.com/684270), the following files landed in this CL
and need a retrospective review from you:
	content/common/indexed_db/indexed_db.mojom
Thanks,
Wez

[Will Harris, 2017-02-13 18:33:01]

https://codereview.chromium.org/2642943002/diff/100001/content/common/indexed...
File content/common/indexed_db/indexed_db.mojom (right):

https://codereview.chromium.org/2642943002/diff/100001/content/common/indexed...
content/common/indexed_db/indexed_db.mojom:331: DeleteDatabase(associated
Callbacks callbacks, url.mojom.Origin origin,
who calls this DeleteDatabase function and where is it called from?

is it browser -> renderer or renderer -> browser?

[eostroukhov, 2017-02-13 18:52:57]

On 2017/02/13 18:33:01, Will Harris wrote:
>
https://codereview.chromium.org/2642943002/diff/100001/content/common/indexed...
> File content/common/indexed_db/indexed_db.mojom (right):
> 
>
https://codereview.chromium.org/2642943002/diff/100001/content/common/indexed...
> content/common/indexed_db/indexed_db.mojom:331: DeleteDatabase(associated
> Callbacks callbacks, url.mojom.Origin origin,
> who calls this DeleteDatabase function and where is it called from?
> 
> is it browser -> renderer or renderer -> browser?

It is renderer -> browser

[Will Harris, 2017-02-13 19:14:11]

lgtm