Update BrowsingDataRemover with the asynchronous CookieMonster API.

Update BrowsingDataRemover with the asynchronous CookieMonster API.
BUG=XXXX
TEST=XXXX

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

[ycxiao, 2011-06-30 20:12:51]

PTAL.

Thanks!

[erikwright (departed), 2011-07-04 16:47:33]

Looks correct to me.

http://codereview.chromium.org/7210034/diff/5001/chrome/browser/browsing_data...
File chrome/browser/browsing_data_remover.cc (right):

http://codereview.chromium.org/7210034/diff/5001/chrome/browser/browsing_data...
chrome/browser/browsing_data_remover.cc:657: if
(!BrowserThread::CurrentlyOn(BrowserThread::UI)) {
The CookieStore guarantees invocation of callbacks on the calling thread. I
assume that BrowsingDataRemover::Remove is expected to be invoked on the UI
thread and that the intention of the existing checks for UI thread is to keep
all activity on that thread. Can you confirm this by checking callers of Remove?

If so, perhaps add a comment in browsing_data_remover.h that this class expects
to be called on the UI thread, and a DCHECK(CurrentlyOn(UI)) at the top of
Remove(). Then you can remove this check and Post here (since you will be
guaranteed to be on the UI thread.

[ycxiao, 2011-07-12 20:06:13]

Could you please run the try server.

Thanks,
Yancheng

[ycxiao, 2011-07-13 21:01:23]

The try server passed.

Will jochen@chromium.org, michaeln@chromium.org be the right for the code
review?

Thanks,
Yancheng

[ycxiao, 2011-07-14 18:14:53]

Hi Jochen,

PTAL.

Thanks,
Yancheng

[jochen (gone - plz use gerrit), 2011-07-15 07:01:20]

looks good so far

please add a unit test

[ycxiao, 2011-07-15 20:43:39]

PTAL.

Added an unit test for remove cookie, and apply a change to the
BrowsingDataRemover::OnCookiesDeleted.

Yancheng

http://codereview.chromium.org/7210034/diff/20001/chrome/browser/browsing_dat...
File chrome/browser/browsing_data_remover.cc (right):

http://codereview.chromium.org/7210034/diff/20001/chrome/browser/browsing_dat...
chrome/browser/browsing_data_remover.cc:648: MessageLoop::current()->PostTask(
Post a Task here, because this callback make be invoked even before finish the
executing of BrowsingDataRemover::remover.

[erikwright (departed), 2011-07-17 17:21:55]

http://codereview.chromium.org/7210034/diff/20001/chrome/browser/browsing_dat...
File chrome/browser/browsing_data_remover.cc (right):

http://codereview.chromium.org/7210034/diff/20001/chrome/browser/browsing_dat...
chrome/browser/browsing_data_remover.cc:648: MessageLoop::current()->PostTask(
Add a comment in the code to this effect.

On 2011/07/15 20:43:39, ycxiao wrote:
> Post a Task here, because this callback make be invoked even before finish the
> executing of BrowsingDataRemover::remover.

[ycxiao, 2011-07-19 22:25:35]

Hi Jochen,

PTAL. Thanks!

Yancheng

[jochen (gone - plz use gerrit), 2011-07-20 12:24:12]

http://codereview.chromium.org/7210034/diff/19006/chrome/browser/browsing_dat...
File chrome/browser/browsing_data_remover.cc (right):

http://codereview.chromium.org/7210034/diff/19006/chrome/browser/browsing_dat...
chrome/browser/browsing_data_remover.cc:649: // finishing the execution of
BrowsingDataRemover::remover.
uh, how can this happen?

The function you invoke is ...Async, so it should complete async

If it's possible that it completes sync, then it should return a net_error code
(either net::ERR_IO_PENDING if it'll complete asynchronously, or net::OK if it
has completed synchronously)

[jochen (gone - plz use gerrit), 2011-07-22 18:40:35]

we discussed this offline with the conclusion that right now this pattern makes
sense

LGTM, thx for working on this!

[ycxiao, 2011-07-22 18:56:17]

On 2011/07/22 18:40:35, jochen wrote:
> we discussed this offline with the conclusion that right now this pattern
makes
> sense
> 
> LGTM, thx for working on this!

Thanks for all your discussions and given ideas related to this CL.

[ycxiao, 2011-07-22 21:45:58]

Hi Erik,

The delete cookie now is moving to IO thread. And I am running the try server
again.

PTAL.

Thanks!
Yancheng

[erikwright (departed), 2011-07-25 16:27:16]

LGTM with nits. Please fix nits, 'try', and commit.

FWIW, I regret that the unit tests are not sufficient to catch the thread error
that was noted (and fixed) because the TestUrlRequestContextGetter does not
enforce the thread constraint placed by ChromeUrlRequestContextGetter.
Furthermore, I regret that this constraint is mentioned only in
chrome_url_request_context and not in url_request_context_getter. An area for
some improvement later, but not within the scope of these changes I think.

http://codereview.chromium.org/7210034/diff/30007/chrome/browser/browsing_dat...
File chrome/browser/browsing_data_remover.cc (right):

http://codereview.chromium.org/7210034/diff/30007/chrome/browser/browsing_dat...
chrome/browser/browsing_data_remover.cc:641: void
BrowsingDataRemover::OnClearedCookie(int num_deleted) {
nit: OnClearedCookie->OnClearedCookies

http://codereview.chromium.org/7210034/diff/30007/chrome/browser/browsing_dat...
chrome/browser/browsing_data_remover.cc:654: void
BrowsingDataRemover::ClearCookieOnIOThread(
nit: ClearCookie->ClearCookies

http://codereview.chromium.org/7210034/diff/30007/chrome/browser/browsing_dat...
File chrome/browser/browsing_data_remover_unittest.cc (right):

http://codereview.chromium.org/7210034/diff/30007/chrome/browser/browsing_dat...
chrome/browser/browsing_data_remover_unittest.cc:14: #include
"chrome/test/test_url_request_context_getter.h"
Do you need this, or just url_request_context_getter.h?

[ycxiao, 2011-08-04 22:53:25]

On 2011/07/22 18:40:35, jochen wrote:
> we discussed this offline with the conclusion that right now this pattern
makes
> sense
> 
> LGTM, thx for working on this!

Hello Jochen,

I have been stacked by this CL for a long time. And I can't find out where is
the problem. Erik is on vocation this week. Could you please help me?

The RemoveCookieForever unit test fails at Windows platform. Erik takes a look
at it shortly last week. He build it at his Windows machine, and find out that
the test 90% succeed while debugging, but 90% fails while running normally. It
seems there are maybe some race condition. But I can't find it out.

Could you please look at the code and give me some hint?

Thanks a lot!

Yancheng

[jochen (gone - plz use gerrit), 2011-08-09 12:38:11]

On 2011/08/04 22:53:25, ycxiao wrote:
> On 2011/07/22 18:40:35, jochen wrote:
> > we discussed this offline with the conclusion that right now this pattern
> makes
> > sense
> > 
> > LGTM, thx for working on this!
> 
> Hello Jochen,
> 
> I have been stacked by this CL for a long time. And I can't find out where is
> the problem. Erik is on vocation this week. Could you please help me?
> 
> The RemoveCookieForever unit test fails at Windows platform. Erik takes a look
> at it shortly last week. He build it at his Windows machine, and find out that
> the test 90% succeed while debugging, but 90% fails while running normally. It
> seems there are maybe some race condition. But I can't find it out.
> 
> Could you please look at the code and give me some hint?
> 
> Thanks a lot!
> 
> Yancheng

sorry for the long reply. The problem is that the browsingdataremover didn't
initialize all it's waiting for something variables in the constructor.

this issue is fixed meanwhile, so your unit tests should pass

[commit-bot: I haz the power, 2011-08-09 19:32:54]

Change committed as 96035