Only invoke WebKit methods in browsing data helpers on the WEBKIT thread.

Only invoke WebKit methods in browsing data helpers on the WEBKIT thread.

BUG=71786
TEST=browser & unit tests

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=74433

[jochen (gone - plz use gerrit), 2011-02-04 15:18:00]

please review

[Paweł Hajdan Jr., 2011-02-05 12:44:51]

Drive-by with testing comments. Please fix the issues and let me take another
look before committing.

While this kind of loop doesn't necessarily induce flakiness, converting the
code to only run the message loop once and quit it once makes it easier to
reason about, debug, and generally makes it cleaner.

http://codereview.chromium.org/6246105/diff/1/chrome/browser/browsing_data_da...
File chrome/browser/browsing_data_database_helper_browsertest.cc (right):

http://codereview.chromium.org/6246105/diff/1/chrome/browser/browsing_data_da...
chrome/browser/browsing_data_database_helper_browsertest.cc:41: while
(!have_result_) {
Is this loop needed? Generally we should only run the loop once, and quit it
once - when the result is ready.

http://codereview.chromium.org/6246105/diff/1/chrome/browser/browsing_data_in...
File chrome/browser/browsing_data_indexed_db_helper_browsertest.cc (right):

http://codereview.chromium.org/6246105/diff/1/chrome/browser/browsing_data_in...
chrome/browser/browsing_data_indexed_db_helper_browsertest.cc:42: while
(!have_result_) {
Is this loop needed? Generally we should only run the loop once, and quit it
once - when the result is ready.

http://codereview.chromium.org/6246105/diff/1/chrome/browser/browsing_data_lo...
File chrome/browser/browsing_data_local_storage_helper_browsertest.cc (right):

http://codereview.chromium.org/6246105/diff/1/chrome/browser/browsing_data_lo...
chrome/browser/browsing_data_local_storage_helper_browsertest.cc:45: while
(!have_result_) {
Is this loop needed? Generally we should only run the loop once, and quit it
once - when the result is ready.

[jochen (gone - plz use gerrit), 2011-02-05 12:57:02]

On 2011/02/05 12:44:51, Paweł Hajdan Jr. wrote:
> Drive-by with testing comments. Please fix the issues and let me take another
> look before committing.
> 
> While this kind of loop doesn't necessarily induce flakiness, converting the
> code to only run the message loop once and quit it once makes it easier to
> reason about, debug, and generally makes it cleaner.

Pawel, this is the pattern from net/base/testing_completion_callback.*

I'd assume that there is a reason, but I do not know it.

[Paweł Hajdan Jr., 2011-02-07 07:59:57]

On 2011/02/05 12:57:02, jochen wrote:
> Pawel, this is the pattern from net/base/testing_completion_callback.*

Are you sure it is applicable here? (i.e. have you tried without the loop and it
was broken? If so, what did the breakage look like?).

> I'd assume that there is a reason, but I do not know it.

src/net code often quits the message loop multiple times, so it's non-trivial to
find a better way. However, in many other applications it's not really necessary
(for example, it looks like the loop was not there in the previous version of
the code).

[jochen (gone - plz use gerrit), 2011-02-07 08:08:47]

On 2011/02/07 07:59:57, Paweł Hajdan Jr. wrote:
> On 2011/02/05 12:57:02, jochen wrote:
> > Pawel, this is the pattern from net/base/testing_completion_callback.*
> 
> Are you sure it is applicable here? (i.e. have you tried without the loop and
it
> was broken? If so, what did the breakage look like?).
> 
> > I'd assume that there is a reason, but I do not know it.
> 
> src/net code often quits the message loop multiple times, so it's non-trivial
to
> find a better way. However, in many other applications it's not really
necessary
> (for example, it looks like the loop was not there in the previous version of
> the code).

The previous version of the code didn't use several threads, so it didn't mess
with message loop at all.

But I guess if nothing quits the message loop, it should be fine, so I'll try it

[jochen (gone - plz use gerrit), 2011-02-08 10:21:24]

Bernhard, can you take a look?

[Bernhard Bauer, 2011-02-08 10:49:08]

Apart from one nit, I only really looked at the test changes. 

The comments in the test files apply to all three tests.

http://codereview.chromium.org/6246105/diff/5002/chrome/browser/browsing_data...
File chrome/browser/browsing_data_database_helper.cc (right):

http://codereview.chromium.org/6246105/diff/5002/chrome/browser/browsing_data...
chrome/browser/browsing_data_database_helper.cc:214:
web_security_origin.host().utf8(),
Nit: Indent by two spaces less?

http://codereview.chromium.org/6246105/diff/5002/chrome/browser/browsing_data...
File chrome/browser/browsing_data_database_helper_browsertest.cc (right):

http://codereview.chromium.org/6246105/diff/5002/chrome/browser/browsing_data...
chrome/browser/browsing_data_database_helper_browsertest.cc:19: class
TestCompletionCallback {
Do you think it would be complicated to pull this callback out into a class
(template) you could use for all three browser tests?

http://codereview.chromium.org/6246105/diff/5002/chrome/browser/browsing_data...
chrome/browser/browsing_data_database_helper_browsertest.cc:35: if
(waiting_for_result_)
I think this is unnecessary. |MessageLoop::Quit| will DCHECK that it's never
called outside of |MessageLoop::Run|, so we can be sure that |callback| isn't
either even if we unconditionally quit the MessageLoop here.

http://codereview.chromium.org/6246105/diff/5002/chrome/browser/browsing_data...
chrome/browser/browsing_data_database_helper_browsertest.cc:41: if
(!have_result_) {
That doesn't make sense now ;-) You should DCHECK |have_result_| after
|WaitForResult| (or after |MessageLoop::Run|).

[jochen (gone - plz use gerrit), 2011-02-10 12:34:59]

addressed all comments

[Bernhard Bauer, 2011-02-10 12:51:09]

Tests LGTM with lint errors fixed
(http://codereview.chromium.org/lint_patch/issue6246105_10001_11005) and a
suggestion below:

http://codereview.chromium.org/6246105/diff/10001/chrome/browser/browsing_dat...
File chrome/browser/browsing_data_local_storage_helper_browsertest.cc (right):

http://codereview.chromium.org/6246105/diff/10001/chrome/browser/browsing_dat...
chrome/browser/browsing_data_local_storage_helper_browsertest.cc:24:
BrowsingDataHelperCallback<BrowsingDataLocalStorageHelper::LocalStorageInfo>
Nit: Maybe |using BrowsingDataLocalStorageHelper::LocalStorageInfo;| would
improve things here :)

[Bernhard Bauer, 2011-02-10 12:52:19]

Forgot one thing:

http://codereview.chromium.org/6246105/diff/10001/chrome/browser/browsing_dat...
File chrome/browser/browsing_data_helper_browsertest.h (right):

http://codereview.chromium.org/6246105/diff/10001/chrome/browser/browsing_dat...
chrome/browser/browsing_data_helper_browsertest.h:26:
MessageLoop::current()->Run();
Maybe you could DCHECK that |result_| was actually set (and nothing quit the
MessageLoop before that)?

[Paweł Hajdan Jr., 2011-02-10 12:54:24]

Code I commented in the drive-by LGTM, thanks!

[jochen (gone - plz use gerrit), 2011-02-10 13:46:34]

On 2011/02/10 12:51:09, Bernhard Bauer wrote:
> Tests LGTM with lint errors fixed
> (http://codereview.chromium.org/lint_patch/issue6246105_10001_11005) and a
> suggestion below:
> 
>
http://codereview.chromium.org/6246105/diff/10001/chrome/browser/browsing_dat...
> File chrome/browser/browsing_data_local_storage_helper_browsertest.cc (right):
> 
>
http://codereview.chromium.org/6246105/diff/10001/chrome/browser/browsing_dat...
> chrome/browser/browsing_data_local_storage_helper_browsertest.cc:24:
> BrowsingDataHelperCallback<BrowsingDataLocalStorageHelper::LocalStorageInfo>
> Nit: Maybe |using BrowsingDataLocalStorageHelper::LocalStorageInfo;| would
> improve things here :)

not sure, don't like adding using here.

[bulach, 2011-02-22 22:34:01]

LGTM

i have just some nits and probably unrelated suggestions below, let me know if
you agree... also, I know I wrote this a while ago :) I think I can do some
refactoring... all the "helpers" basically follow the same pattern, we could
have a base class for all of them and just have the actual "FetchInWebKitThread"
for each one of them... how does it sound? I can try to look at it at some point
if you agree...

http://codereview.chromium.org/6246105/diff/14003/chrome/browser/browsing_dat...
File chrome/browser/browsing_data_database_helper.cc (right):

http://codereview.chromium.org/6246105/diff/14003/chrome/browser/browsing_dat...
chrome/browser/browsing_data_database_helper.cc:178: return
database_info_.empty() && pending_database_info_.empty();
nit: odd indent (sorry!)

http://codereview.chromium.org/6246105/diff/14003/chrome/browser/browsing_dat...
chrome/browser/browsing_data_database_helper.cc:193: base::AutoLock
auto_lock(lock_);
nit: you may want to put the this lock and the following block up to 224 into
its own scope.
alternatively, perhaps rename this method to "FetchInWebKitThread", (which would
be called by "StartFetching"), and then split 193-224 to perhaps
"FetchInternal"?
convert seems it's a simple datatype translation, but there's a bit more to it..

http://codereview.chromium.org/6246105/diff/14003/chrome/browser/browsing_dat...
File chrome/browser/browsing_data_database_helper_browsertest.cc (right):

http://codereview.chromium.org/6246105/diff/14003/chrome/browser/browsing_dat...
chrome/browser/browsing_data_database_helper_browsertest.cc:14: namespace {
I think there's a \n here and when closing the namespace

http://codereview.chromium.org/6246105/diff/14003/chrome/browser/browsing_dat...
chrome/browser/browsing_data_database_helper_browsertest.cc:141: }
ditto

http://codereview.chromium.org/6246105/diff/14003/chrome/browser/browsing_dat...
File chrome/browser/browsing_data_database_helper_unittest.cc (right):

http://codereview.chromium.org/6246105/diff/14003/chrome/browser/browsing_dat...
chrome/browser/browsing_data_database_helper_unittest.cc:11: namespace {
ditto

http://codereview.chromium.org/6246105/diff/14003/chrome/browser/browsing_dat...
chrome/browser/browsing_data_database_helper_unittest.cc:26: }
ditto

http://codereview.chromium.org/6246105/diff/14003/chrome/browser/browsing_dat...
File chrome/browser/browsing_data_indexed_db_helper.cc (right):

http://codereview.chromium.org/6246105/diff/14003/chrome/browser/browsing_dat...
chrome/browser/browsing_data_indexed_db_helper.cc:288: if (completion_callback_
!= NULL) {
oh, I think we can change both here (and the original code too).
a DCHECK(is_fetching_) and DCHECK(completion_callback_ != NULL) should be
enough, no need for 286-288 and 291.

http://codereview.chromium.org/6246105/diff/14003/chrome/browser/browsing_dat...
File chrome/browser/browsing_data_indexed_db_helper.h (right):

http://codereview.chromium.org/6246105/diff/14003/chrome/browser/browsing_dat...
chrome/browser/browsing_data_indexed_db_helper.h:118: void
ConvertPendingInfoInWebKitThread();
as above