Move IndexedDB from WEBKIT_DEPRECATED to dedicated thread

Move IndexedDB from WEBKIT_DEPRECATED to dedicated thread

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

[jsbell, 2013-06-20 22:26:16]

Here's the latest iteration (as a separate CL to make comparing easier).

Most of the hideousness comes down to test refactoring to get a message loop set
up correctly for tests. When I got the Extension API tests I gave up and added
an explicit IndexedDBContext::SetMessageLoopForTesting but I'm not proud of it.
Using that method elsewhere could reduce the size of the patch.

I'm expecting the try bots to be sad about this one as well.

[jsbell, 2013-06-20 22:40:09]

Whoops, typo+autocomplete in the reviewers line. +darin@ for realz.

[jsbell, 2013-06-21 00:26:30]

Of course, a gazillion other tests failed since they also depend on
ExtensionServiceTestBase which needs to configure the IDB thread. Can't do it in
the constructor, though, since it sets up profile_ later. Will have to dork
around and see what the right place for that is.

Alternately I could make the StoragePartition say "if there's no IDB thread,
we're in a test, so initialize it with <something>" - I'll give that a whirl
tomorrow.

[jam, 2013-06-21 16:00:50]

https://codereview.chromium.org/17518004/diff/5001/content/browser/indexed_db...
File content/browser/indexed_db/indexed_db_context_impl.h (right):

https://codereview.chromium.org/17518004/diff/5001/content/browser/indexed_db...
content/browser/indexed_db/indexed_db_context_impl.h:45: base::MessageLoop*
message_loop);
question: why did you switch from MLP to ML? the former is generally safer.
also, why not just take a TaskRunner? ML/MLP pointers are deprecated in favor of
TR

https://codereview.chromium.org/17518004/diff/5001/content/browser/indexed_db...
content/browser/indexed_db/indexed_db_context_impl.h:85:
scoped_refptr<base::MessageLoopProxy> MessageLoopProxy();
nit: document

https://codereview.chromium.org/17518004/diff/5001/content/browser/indexed_db...
File content/browser/indexed_db/indexed_db_quota_client.h (right):

https://codereview.chromium.org/17518004/diff/5001/content/browser/indexed_db...
content/browser/indexed_db/indexed_db_quota_client.h:28: CONTENT_EXPORT
IndexedDBQuotaClient(IndexedDBContextImpl* indexed_db_context);
nit: explicit

https://codereview.chromium.org/17518004/diff/5001/content/browser/indexed_db...
File content/browser/indexed_db/indexed_db_thread.h (right):

https://codereview.chromium.org/17518004/diff/5001/content/browser/indexed_db...
content/browser/indexed_db/indexed_db_thread.h:16: class CONTENT_EXPORT
IndexedDBThread {
why do we need this wrapper class?

[jsbell, 2013-06-21 16:34:16]

Updated patch coming up

https://codereview.chromium.org/17518004/diff/5001/content/browser/indexed_db...
File content/browser/indexed_db/indexed_db_context_impl.h (right):

https://codereview.chromium.org/17518004/diff/5001/content/browser/indexed_db...
content/browser/indexed_db/indexed_db_context_impl.h:45: base::MessageLoop*
message_loop);
On 2013/06/21 16:00:50, jam wrote:
> question: why did you switch from MLP to ML? the former is generally safer.
> also, why not just take a TaskRunner? ML/MLP pointers are deprecated in favor
of
> TR

Purely for the base::MessageLoop::current() == message_loop_ check in
OnIndexedDBThread(). I didn't immediately see a way to do with an MLP.

As far as TaskRunner goes, tests were using base::ThreadTestHelper() which takes
an MLP. There's also an instance of DeleteSoon; TR itself doesn't appear to have
that.

Suggestions appreciated in either case.

https://codereview.chromium.org/17518004/diff/5001/content/browser/indexed_db...
content/browser/indexed_db/indexed_db_context_impl.h:85:
scoped_refptr<base::MessageLoopProxy> MessageLoopProxy();
On 2013/06/21 16:00:50, jam wrote:
> nit: document

Done.

https://codereview.chromium.org/17518004/diff/5001/content/browser/indexed_db...
File content/browser/indexed_db/indexed_db_quota_client.h (right):

https://codereview.chromium.org/17518004/diff/5001/content/browser/indexed_db...
content/browser/indexed_db/indexed_db_quota_client.h:28: CONTENT_EXPORT
IndexedDBQuotaClient(IndexedDBContextImpl* indexed_db_context);
On 2013/06/21 16:00:50, jam wrote:
> nit: explicit

Done.

https://codereview.chromium.org/17518004/diff/5001/content/browser/indexed_db...
File content/browser/indexed_db/indexed_db_thread.h (right):

https://codereview.chromium.org/17518004/diff/5001/content/browser/indexed_db...
content/browser/indexed_db/indexed_db_thread.h:16: class CONTENT_EXPORT
IndexedDBThread {
On 2013/06/21 16:00:50, jam wrote:
> why do we need this wrapper class?

At this point I don't - more logic lived here at one point. I'll remove.

[jsbell, 2013-06-21 16:49:47]

https://codereview.chromium.org/17518004/diff/5001/content/browser/indexed_db...
File content/browser/indexed_db/indexed_db_context_impl.h (right):

https://codereview.chromium.org/17518004/diff/5001/content/browser/indexed_db...
content/browser/indexed_db/indexed_db_context_impl.h:45: base::MessageLoop*
message_loop);
On 2013/06/21 16:34:16, jsbell wrote:
> On 2013/06/21 16:00:50, jam wrote:
> > question: why did you switch from MLP to ML? the former is generally safer.
> > also, why not just take a TaskRunner? ML/MLP pointers are deprecated in
favor
> of
> > TR
> 
> Purely for the base::MessageLoop::current() == message_loop_ check in
> OnIndexedDBThread(). I didn't immediately see a way to do with an MLP.
> 
> As far as TaskRunner goes, tests were using base::ThreadTestHelper() which
takes
> an MLP. There's also an instance of DeleteSoon; TR itself doesn't appear to
have
> that.
> 
> Suggestions appreciated in either case.

Oh, RunsTasksOnCurrentThread should do it for the former. Not sure why I didn't
clue into that. Will give that a whirl.

[jsbell, 2013-06-21 17:38:21]

https://codereview.chromium.org/17518004/diff/5001/content/browser/indexed_db...
File content/browser/indexed_db/indexed_db_context_impl.h (right):

https://codereview.chromium.org/17518004/diff/5001/content/browser/indexed_db...
content/browser/indexed_db/indexed_db_context_impl.h:45: base::MessageLoop*
message_loop);
On 2013/06/21 16:49:47, jsbell wrote:
> On 2013/06/21 16:34:16, jsbell wrote:
> > On 2013/06/21 16:00:50, jam wrote:
> > > question: why did you switch from MLP to ML? the former is generally
safer.
> > > also, why not just take a TaskRunner? ML/MLP pointers are deprecated in
> favor
> > of
> > > TR
> > 
> > Purely for the base::MessageLoop::current() == message_loop_ check in
> > OnIndexedDBThread(). I didn't immediately see a way to do with an MLP.
> > 
> > As far as TaskRunner goes, tests were using base::ThreadTestHelper() which
> takes
> > an MLP. There's also an instance of DeleteSoon; TR itself doesn't appear to
> have
> > that.
> > 
> > Suggestions appreciated in either case.
> 
> Oh, RunsTasksOnCurrentThread should do it for the former. Not sure why I
didn't
> clue into that. Will give that a whirl.

And the TestThreadHelper can replaced by ThreadSimpleTaskRunner or grabbing the
thread directly, so one more update coming...

[jsbell, 2013-06-21 18:00:47]

New patch. IndexedDBContext's API is now just [Sequenced]TaskRunner-based. Much
nicer, thanks for prompting!

There's one more helper method for browser tests to get the actual thread
directly to pump the loop; it seemed to be a choice between this and making
BrowserMainLoop CONTENT_EXPORT for tests, but other suggestions welcome.

You can blame clang-format for these:

DCHECK(
    parent_->indexed_db_context_->TaskRunner()->RunsTasksOnCurrentThread());

[michaeln, 2013-06-21 19:35:57]

generally lgtm

https://codereview.chromium.org/17518004/diff/5001/chrome/browser/browsing_da...
File chrome/browser/browsing_data/browsing_data_indexed_db_helper.cc (right):

https://codereview.chromium.org/17518004/diff/5001/chrome/browser/browsing_da...
chrome/browser/browsing_data/browsing_data_indexed_db_helper.cc:238:
IndexedDBInfo info(pending_info->origin, 0, base::Time(), base::FilePath());
Not sure there's a need for a dependency on a Profile or real idbcontext in this
canned mock class.

There is nothing about this loop that requires it to be run on some special
background thread (afaict). The setup required to make it such that you can run
this loop on the special background thread might be be more trouble than it is
worth.

I think StartFetching could populate the indexed_db_info_ collection and then
MessageLoop::current()->PostTask(&NotifyInUIThread) to deliver the callback
asyncly.

https://codereview.chromium.org/17518004/diff/5001/chrome/browser/content_set...
File chrome/browser/content_settings/local_shared_objects_container.cc (right):

https://codereview.chromium.org/17518004/diff/5001/chrome/browser/content_set...
chrome/browser/content_settings/local_shared_objects_container.cc:34:
LocalSharedObjectsContainer::LocalSharedObjectsContainer(Profile* profile)
i hope this class is test code but it's not labeled as so?

[jam, 2013-06-21 19:41:58]

https://codereview.chromium.org/17518004/diff/26001/content/browser/browser_c...
File content/browser/browser_context.cc (right):

https://codereview.chromium.org/17518004/diff/26001/content/browser/browser_c...
content/browser/browser_context.cc:244: // No task runner in unit tests.
same question as before, i thought the tests set this? or is it not all tests?

https://codereview.chromium.org/17518004/diff/26001/content/browser/indexed_d...
File content/browser/indexed_db/indexed_db_quota_client.cc (right):

https://codereview.chromium.org/17518004/diff/26001/content/browser/indexed_d...
content/browser/indexed_db/indexed_db_quota_client.cc:86: // No task runner
means unit test; no cleanup necessary.
hmm, just saw all these unit test lines in this file. can we avoid them? i'm not
sure i follow why they're needed, isn't a task runner set for unit tests?

https://codereview.chromium.org/17518004/diff/26001/content/browser/storage_p...
File content/browser/storage_partition_impl.cc (right):

https://codereview.chromium.org/17518004/diff/26001/content/browser/storage_p...
content/browser/storage_partition_impl.cc:232:
BrowserThread::CurrentlyOn(BrowserThread::UI) &&
what is the point of the "BrowserThread::CurrentlyOn(BrowserThread::UI)" check?
can we just drop that?

https://codereview.chromium.org/17518004/diff/26001/content/browser/storage_p...
content/browser/storage_partition_impl.cc:235: ->message_loop_proxy()
nit: this is weird formatting, usually the "->" never starts on a line

https://codereview.chromium.org/17518004/diff/26001/content/public/browser/in...
File content/public/browser/indexed_db_context.h (right):

https://codereview.chromium.org/17518004/diff/26001/content/public/browser/in...
content/public/browser/indexed_db_context.h:14: #include "base/task_runner.h"
nit: you don't need these two methods, right? you can just forward declare

[jsbell, 2013-06-21 20:37:14]

https://codereview.chromium.org/17518004/diff/26001/content/browser/indexed_d...
File content/browser/indexed_db/indexed_db_quota_client.cc (right):

https://codereview.chromium.org/17518004/diff/26001/content/browser/indexed_d...
content/browser/indexed_db/indexed_db_quota_client.cc:86: // No task runner
means unit test; no cleanup necessary.
On 2013/06/21 19:41:59, jam wrote:
> hmm, just saw all these unit test lines in this file. can we avoid them? i'm
not
> sure i follow why they're needed, isn't a task runner set for unit tests?

Not all tests. For example, all of the tests listed here
http://build.chromium.org/p/tryserver.chromium/builders/swarm_triggered/build...
create a storage partition but do nothing related to IndexedDB with it -
BackgroundApplicationListModelTest.RandomTest is one example. It initializes an
ExtensionServiceTestBase (which inits the various BrowserThreads but not an IDB
thread). I also considered making the ExtensionServiceTestBase set a thread in
some of the InitializeXXXX methods, after the profile is created, but that
seemed more fragile/intrusive.

https://codereview.chromium.org/17518004/diff/26001/content/browser/storage_p...
File content/browser/storage_partition_impl.cc (right):

https://codereview.chromium.org/17518004/diff/26001/content/browser/storage_p...
content/browser/storage_partition_impl.cc:232:
BrowserThread::CurrentlyOn(BrowserThread::UI) &&
On 2013/06/21 19:41:59, jam wrote:
> what is the point of the "BrowserThread::CurrentlyOn(BrowserThread::UI)"
check?
> can we just drop that?

BrowserMainLoop::GetInstance() DCHECK()s that it's on the UI thread, but a
StoragePartitionImpl is created in various tests that don't initialize a UI
thread.

(That wasn't necessary before the GetInstance() refactor.)

https://codereview.chromium.org/17518004/diff/26001/content/browser/storage_p...
content/browser/storage_partition_impl.cc:235: ->message_loop_proxy()
On 2013/06/21 19:41:59, jam wrote:
> nit: this is weird formatting, usually the "->" never starts on a line

clang-format - I'll file a bug.

https://codereview.chromium.org/17518004/diff/26001/content/public/browser/in...
File content/public/browser/indexed_db_context.h (right):

https://codereview.chromium.org/17518004/diff/26001/content/public/browser/in...
content/public/browser/indexed_db_context.h:14: #include "base/task_runner.h"
On 2013/06/21 19:41:59, jam wrote:
> nit: you don't need these two methods, right? you can just forward declare

Done.

[jsbell, 2013-06-21 20:39:37]

https://codereview.chromium.org/17518004/diff/5001/chrome/browser/browsing_da...
File chrome/browser/browsing_data/browsing_data_indexed_db_helper.cc (right):

https://codereview.chromium.org/17518004/diff/5001/chrome/browser/browsing_da...
chrome/browser/browsing_data/browsing_data_indexed_db_helper.cc:238:
IndexedDBInfo info(pending_info->origin, 0, base::Time(), base::FilePath());
On 2013/06/21 19:35:57, michaeln wrote:
> Not sure there's a need for a dependency on a Profile or real idbcontext in
this
> canned mock class.
> 
> There is nothing about this loop that requires it to be run on some special
> background thread (afaict). The setup required to make it such that you can
run
> this loop on the special background thread might be be more trouble than it is
> worth.
> 
> I think StartFetching could populate the indexed_db_info_ collection and then
> MessageLoop::current()->PostTask(&NotifyInUIThread) to deliver the callback
> asyncly.
> 
> 

Yeah, I remember your feedback from the previous CL. I'll take another look - I
was looking at adjacent files for the pattern to follow, but it can probably be
simplified.

[jam, 2013-06-21 20:46:09]

lgtm, thanks for all the patience with this change :)

[jsbell, 2013-06-24 17:50:43]

TL;DR - I'll post a separate CL which should simplify this one.

https://codereview.chromium.org/17518004/diff/5001/chrome/browser/browsing_da...
File chrome/browser/browsing_data/browsing_data_indexed_db_helper.cc (right):

https://codereview.chromium.org/17518004/diff/5001/chrome/browser/browsing_da...
chrome/browser/browsing_data/browsing_data_indexed_db_helper.cc:238:
IndexedDBInfo info(pending_info->origin, 0, base::Time(), base::FilePath());
On 2013/06/21 19:35:57, michaeln wrote:
> Not sure there's a need for a dependency on a Profile or real idbcontext in
this
> canned mock class.
> 
> There is nothing about this loop that requires it to be run on some special
> background thread (afaict). The setup required to make it such that you can
run
> this loop on the special background thread might be be more trouble than it is
> worth.
> 
> I think StartFetching could populate the indexed_db_info_ collection and then
> MessageLoop::current()->PostTask(&NotifyInUIThread) to deliver the callback
> asyncly.
> 
> 

Digging through history, it looks like this plumbing was put in in
crrev.com/6246105 due to unsafe access to WebSecurityOrigin from the UI thread.
The guts of the IndexedDBInfo copying therefore was moved to the WEBKIT thread.
Since then, IndexedDBInfo has dropped dependency on WebSecurityOrigin so this
can be dramatically simplified.

I'll post a separate CL which does that and land it first.

https://codereview.chromium.org/17518004/diff/5001/chrome/browser/content_set...
File chrome/browser/content_settings/local_shared_objects_container.cc (right):

https://codereview.chromium.org/17518004/diff/5001/chrome/browser/content_set...
chrome/browser/content_settings/local_shared_objects_container.cc:34:
LocalSharedObjectsContainer::LocalSharedObjectsContainer(Profile* profile)
On 2013/06/21 19:35:57, michaeln wrote:
> i hope this class is test code but it's not labeled as so?

It's not - instances of this class apparently hold the results of the butter
bars that ask the user "Can origin FOO access resource BAR?"

Seems a little roundabout.

[jsbell, 2013-06-24 18:33:10]

On 2013/06/24 17:50:43, jsbell wrote:
> TL;DR - I'll post a separate CL which should simplify this one.

Scratch that - I'd started to over-simplify, but I was making too many
assumptions. I can basically do just what michaeln@ suggested which will
(surprise!) closely match how the LocalStorage variant works, so I'll keep it in
this CL.

[commit-bot: I haz the power, 2013-06-25 18:38:06]

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

[commit-bot: I haz the power, 2013-06-25 20:52:52]

Change committed as 208544