Add the IndexedDBActiveBlobRegistry, currently unused, to enable future blob

Add the IndexedDBActiveBlobRegistry, currently unused, to enable future blob
support.

BUG=108012
R=cmumford,jsbell

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

[ericu, 2014-03-20 15:06:14]

This one's quite a bit messier than the previous ones; we're running out of
simple, clean, separable parts.  But I think it's worthwhile to review this bit
separately and get it in--it'll simplify the final review quite a bit.

[jsbell, 2014-03-20 18:04:32]

Some initial nits

https://codereview.chromium.org/203833003/diff/90001/content/browser/indexed_...
File content/browser/indexed_db/indexed_db_active_blob_registry.cc (right):

https://codereview.chromium.org/203833003/diff/90001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_active_blob_registry.cc:28:
DCHECK(deleted_dbs_.end() == deleted_dbs_.find(database_id));
Could use stl_util's ContainsKey()

https://codereview.chromium.org/203833003/diff/90001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_active_blob_registry.cc:138: void
IndexedDBActiveBlobRegistry::ForceShutdown() {
This doesn't actually "drop its reference to the backing store" as the comment
says; since it's a raw ptr, that's not strictly necessary, but should it null it
out and other methods can DCHECK that it's non-null?

https://codereview.chromium.org/203833003/diff/90001/content/browser/indexed_...
File content/browser/indexed_db/indexed_db_active_blob_registry.h (right):

https://codereview.chromium.org/203833003/diff/90001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_active_blob_registry.h:1: // Copyright (c)
2013 The Chromium Authors. All rights reserved.
2014 - here and in other new files

https://codereview.chromium.org/203833003/diff/90001/content/browser/indexed_...
File content/browser/indexed_db/indexed_db_active_blob_registry_unittest.cc
(right):

https://codereview.chromium.org/203833003/diff/90001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_active_blob_registry_unittest.cc:98:
static const int64 DATABASE_ID_0 = 7;
Naming style is kDatabaseId0

https://codereview.chromium.org/203833003/diff/90001/content/browser/indexed_...
File content/browser/indexed_db/indexed_db_backing_store.h (right):

https://codereview.chromium.org/203833003/diff/90001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_backing_store.h:372: // The
active_blob_registry_ will hold a refcount to this backing store
Does it?

[cmumford, 2014-03-24 23:14:34]

https://codereview.chromium.org/203833003/diff/90001/content/browser/indexed_...
File content/browser/indexed_db/indexed_db_active_blob_registry.cc (right):

https://codereview.chromium.org/203833003/diff/90001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_active_blob_registry.cc:89: return false;
Should this be a failed DCHECK, or is it a legitimate situation to be in?

https://codereview.chromium.org/203833003/diff/90001/content/browser/indexed_...
File content/browser/indexed_db/indexed_db_active_blob_registry.h (right):

https://codereview.chromium.org/203833003/diff/90001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_active_blob_registry.h:35:
GetFinalReleaseCallback(int64 database_id, int64 blob_key);
These two functions have the word "get" in their name - can the functions be
const?

https://codereview.chromium.org/203833003/diff/90001/content/browser/indexed_...
File content/browser/indexed_db/indexed_db_factory.cc (right):

https://codereview.chromium.org/203833003/diff/90001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_factory.cc:159: DCHECK(false);
I'm assuming this is OK for an official build? I can't tell what DCHECK
pre-processes down to for that build.

https://codereview.chromium.org/203833003/diff/90001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_factory.cc:165:
ReleaseBackingStore(origin_url, false);
/*immediate=*/false

[ericu, 2014-03-25 01:22:13]

https://codereview.chromium.org/203833003/diff/90001/content/browser/indexed_...
File content/browser/indexed_db/indexed_db_active_blob_registry.cc (right):

https://codereview.chromium.org/203833003/diff/90001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_active_blob_registry.cc:28:
DCHECK(deleted_dbs_.end() == deleted_dbs_.find(database_id));
On 2014/03/20 18:04:33, jsbell wrote:
> Could use stl_util's ContainsKey()

Done.

https://codereview.chromium.org/203833003/diff/90001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_active_blob_registry.cc:89: return false;
On 2014/03/24 23:14:35, cmumford wrote:
> Should this be a failed DCHECK, or is it a legitimate situation to be in?

It's legit; the tracker only contains entries for blobs that are in use, and
their databases, not for all existing blobs.

https://codereview.chromium.org/203833003/diff/90001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_active_blob_registry.cc:138: void
IndexedDBActiveBlobRegistry::ForceShutdown() {
On 2014/03/20 18:04:33, jsbell wrote:
> This doesn't actually "drop its reference to the backing store" as the comment
> says; since it's a raw ptr, that's not strictly necessary, but should it null
it
> out and other methods can DCHECK that it's non-null?

Comment: fixed; it dated back from an older way of doing things.
NULL/DCHECK: Good idea; done.

https://codereview.chromium.org/203833003/diff/90001/content/browser/indexed_...
File content/browser/indexed_db/indexed_db_active_blob_registry.h (right):

https://codereview.chromium.org/203833003/diff/90001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_active_blob_registry.h:1: // Copyright (c)
2013 The Chromium Authors. All rights reserved.
On 2014/03/20 18:04:33, jsbell wrote:
> 2014 - here and in other new files

Done.

https://codereview.chromium.org/203833003/diff/90001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_active_blob_registry.h:35:
GetFinalReleaseCallback(int64 database_id, int64 blob_key);
On 2014/03/24 23:14:35, cmumford wrote:
> These two functions have the word "get" in their name - can the functions be
> const?

No, they grab weak pointers from a WeakPointerFactory; I think that has to be
non-const due to the registration.

https://codereview.chromium.org/203833003/diff/90001/content/browser/indexed_...
File content/browser/indexed_db/indexed_db_active_blob_registry_unittest.cc
(right):

https://codereview.chromium.org/203833003/diff/90001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_active_blob_registry_unittest.cc:98:
static const int64 DATABASE_ID_0 = 7;
On 2014/03/20 18:04:33, jsbell wrote:
> Naming style is kDatabaseId0

Done.

https://codereview.chromium.org/203833003/diff/90001/content/browser/indexed_...
File content/browser/indexed_db/indexed_db_backing_store.h (right):

https://codereview.chromium.org/203833003/diff/90001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_backing_store.h:372: // The
active_blob_registry_ will hold a refcount to this backing store
On 2014/03/20 18:04:33, jsbell wrote:
> Does it?

Fixed.

https://codereview.chromium.org/203833003/diff/90001/content/browser/indexed_...
File content/browser/indexed_db/indexed_db_factory.cc (right):

https://codereview.chromium.org/203833003/diff/90001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_factory.cc:159: DCHECK(false);
On 2014/03/24 23:14:35, cmumford wrote:
> I'm assuming this is OK for an official build? I can't tell what DCHECK
> pre-processes down to for that build.

Yeah, it'll compile.  It turns into something like (void)0.

https://codereview.chromium.org/203833003/diff/90001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_factory.cc:165:
ReleaseBackingStore(origin_url, false);
On 2014/03/24 23:14:35, cmumford wrote:
> /*immediate=*/false

Done.

[jsbell, 2014-03-25 17:01:16]

lgtm but with a question that might simplify the patch...

https://codereview.chromium.org/203833003/diff/110001/content/browser/indexed...
File content/browser/indexed_db/indexed_db_factory.h (right):

https://codereview.chromium.org/203833003/diff/110001/content/browser/indexed...
content/browser/indexed_db/indexed_db_factory.h:91: base::TaskRunner*
task_runner);
Since the factory has a context reference, do we need to plumb the TaskRunner
through the various methods or can factory (or things with a factory reference)
grab it from the context?

[ericu, 2014-04-02 22:27:23]

https://codereview.chromium.org/203833003/diff/110001/content/browser/indexed...
File content/browser/indexed_db/indexed_db_factory.h (right):

https://codereview.chromium.org/203833003/diff/110001/content/browser/indexed...
content/browser/indexed_db/indexed_db_factory.h:91: base::TaskRunner*
task_runner);
On 2014/03/25 17:01:17, jsbell wrote:
> Since the factory has a context reference, do we need to plumb the TaskRunner
> through the various methods or can factory (or things with a factory
reference)
> grab it from the context?

I pulled it out of the factory param lists.  I did have to do a bit of test
fixup, since now it's pulling the task_runner from further away, but I think
that's not too bad.  However, I haven't exposed the task_runner [or context] on
IDBFactory to remove the parameter to e.g. the backing store, as it would mean
that the backing store tests would be pulling their task_runner from 3 classes
away, and injection for testing would then be a big mess.

[jsbell, 2014-04-02 22:32:42]

https://codereview.chromium.org/203833003/diff/110001/content/browser/indexed...
File content/browser/indexed_db/indexed_db_factory.h (right):

https://codereview.chromium.org/203833003/diff/110001/content/browser/indexed...
content/browser/indexed_db/indexed_db_factory.h:91: base::TaskRunner*
task_runner);
On 2014/04/02 22:27:23, ericu wrote:
> On 2014/03/25 17:01:17, jsbell wrote:
> > Since the factory has a context reference, do we need to plumb the
TaskRunner
> > through the various methods or can factory (or things with a factory
> reference)
> > grab it from the context?
> 
> I pulled it out of the factory param lists.  I did have to do a bit of test
> fixup, since now it's pulling the task_runner from further away, but I think
> that's not too bad.  However, I haven't exposed the task_runner [or context]
on
> IDBFactory to remove the parameter to e.g. the backing store, as it would mean
> that the backing store tests would be pulling their task_runner from 3 classes
> away, and injection for testing would then be a big mess.

That's cool. I hadn't considered injection/testability.

[cmumford, 2014-04-02 22:42:38]

On 2014/04/02 22:32:42, jsbell wrote:
>
https://codereview.chromium.org/203833003/diff/110001/content/browser/indexed...
> File content/browser/indexed_db/indexed_db_factory.h (right):
> 
>
https://codereview.chromium.org/203833003/diff/110001/content/browser/indexed...
> content/browser/indexed_db/indexed_db_factory.h:91: base::TaskRunner*
> task_runner);
> On 2014/04/02 22:27:23, ericu wrote:
> > On 2014/03/25 17:01:17, jsbell wrote:
> > > Since the factory has a context reference, do we need to plumb the
> TaskRunner
> > > through the various methods or can factory (or things with a factory
> > reference)
> > > grab it from the context?
> > 
> > I pulled it out of the factory param lists.  I did have to do a bit of test
> > fixup, since now it's pulling the task_runner from further away, but I think
> > that's not too bad.  However, I haven't exposed the task_runner [or context]
> on
> > IDBFactory to remove the parameter to e.g. the backing store, as it would
mean
> > that the backing store tests would be pulling their task_runner from 3
classes
> > away, and injection for testing would then be a big mess.
> 
> That's cool. I hadn't considered injection/testability.

lgtm

[ericu, 2014-04-02 23:28:51]

The CQ bit was checked by ericu@chromium.org

[commit-bot: I haz the power, 2014-04-02 23:34:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ericu@chromium.org/203833003/190001

[commit-bot: I haz the power, 2014-04-03 00:17:51]

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

[commit-bot: I haz the power, 2014-04-03 00:17:52]

Retried try job too often on win_rel for step(s) app_list_unittests,
ash_unittests, aura_unittests, cacheinvalidation_unittests, cc_unittests,
check_deps, check_deps2git, chrome_elf_unittests, chromedriver_unittests,
components_unittests, compositor_unittests, content_browsertests,
content_unittests, crypto_unittests, events_unittests, gfx_unittests,
google_apis_unittests, gpu_unittests, installer_util_unittests, ipc_tests,
jingle_unittests, media_unittests, mini_installer_test, nacl_integration,
ppapi_unittests, printing_unittests, remoting_unittests, sql_unittests,
sync_integration_tests, sync_unit_tests, telemetry_perf_unittests,
telemetry_unittests, views_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[ericu, 2014-04-03 01:21:09]

The CQ bit was checked by ericu@chromium.org

[commit-bot: I haz the power, 2014-04-03 01:23:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ericu@chromium.org/203833003/210001

[Sergey Berezin, 2014-04-03 15:48:21]

The CQ bit was unchecked by sergeyberezin@chromium.org

[Sergey Berezin, 2014-04-03 15:50:30]

The CQ bit was checked by sergeyberezin@chromium.org

[commit-bot: I haz the power, 2014-04-03 15:51:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ericu@chromium.org/203833003/210001

[commit-bot: I haz the power, 2014-04-03 16:26:21]

Change committed as 261440