Apply the new asynchronous CookieMonster API to extension_service_unittest.cc.

Apply the new asynchronous CookieMonster API to extension_service_unittest.cc.
BUG=68657
TEST=ExtensionServiceTest.ClearExtensionData

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

[ycxiao, 2011-08-03 00:22:12]

PTAL.

Moving here form CL http://codereview.chromium.org/7537039/.

Thanks!
Yancheng

[Paweł Hajdan Jr., 2011-08-03 16:20:41]

LGTM

Please make sure you have all OWNERS and similar checks covered. I don't usually
work on extensions code, etc, etc.

[Paweł Hajdan Jr., 2011-08-03 16:20:41]

LGTM

Please make sure you have all OWNERS and similar checks covered. I don't usually
work on extensions code, etc, etc.

[ycxiao, 2011-08-03 23:39:18]

Hello Jone,

PTAL. It is a small change which apply the asynchronous CookieMonster API to the
test.

Thanks!

Yancheng

[ycxiao, 2011-08-03 23:40:06]

On 2011/08/03 16:20:41, Paweł Hajdan Jr. wrote:
> LGTM
> 
> Please make sure you have all OWNERS and similar checks covered. I don't
usually
> work on extensions code, etc, etc.

I will. Thanks!

[jstritar, 2011-08-05 16:05:06]

http://codereview.chromium.org/7550020/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_service_unittest.cc (right):

http://codereview.chromium.org/7550020/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_service_unittest.cc:2725:
callback.message_loop_factory_.RevokeAll();
I'm not that familiar with the message loop code... why do you need the
RevokeAlls?

[jstritar, 2011-08-05 16:09:29]

http://codereview.chromium.org/7550020/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_service_unittest.cc (right):

http://codereview.chromium.org/7550020/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_service_unittest.cc:2667: class Callback {
Could you rename this something more specific? Like ExtensionCookieCallback?

[ycxiao, 2011-08-05 17:19:40]

Hi Jone,

Please take a look at comment.

Thanks!
Yancheng

http://codereview.chromium.org/7550020/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_service_unittest.cc (right):

http://codereview.chromium.org/7550020/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_service_unittest.cc:2667: class Callback {
On 2011/08/05 16:09:30, jstritar wrote:
> Could you rename this something more specific? Like ExtensionCookieCallback?

Sure, I will rename it when I back to office(I am travelling now).

http://codereview.chromium.org/7550020/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_service_unittest.cc:2725:
callback.message_loop_factory_.RevokeAll();
On 2011/08/05 16:05:06, jstritar wrote:
> I'm not that familiar with the message loop code... why do you need the
> RevokeAlls?

This RevokeAlls is a ScopedRunnableMethodFactory method which revoke any
outstanding runnable methods scheduled. It makes sure that the previous posted
tasks such as Quit() won't stay at the MessageLoop and have some further
effects.

[jstritar, 2011-08-05 17:46:55]

Can you also add the test this affects on the TEST= line of the description?

TEST=ExtensionServiceTest.ClearExtensionData

http://codereview.chromium.org/7550020/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_service_unittest.cc (right):

http://codereview.chromium.org/7550020/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_service_unittest.cc:2667: class Callback {
On 2011/08/05 17:19:40, ycxiao wrote:
> On 2011/08/05 16:09:30, jstritar wrote:
> > Could you rename this something more specific? Like ExtensionCookieCallback?
> 
> Sure, I will rename it when I back to office(I am travelling now).

Thanks!

http://codereview.chromium.org/7550020/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_service_unittest.cc:2725:
callback.message_loop_factory_.RevokeAll();
On 2011/08/05 17:19:40, ycxiao wrote:
> This RevokeAlls is a ScopedRunnableMethodFactory method which revoke any
> outstanding runnable methods scheduled. It makes sure that the previous posted
> tasks such as Quit() won't stay at the MessageLoop and have some further
> effects.  
> 

Are they actually needed since you call loop_.RunAllPending()?

[ycxiao1, 2011-08-05 18:01:33]

On Fri, Aug 5, 2011 at 1:46 PM, <jstritar@chromium.org> wrote:

> Can you also add the test this affects on the TEST= line of the
> description?
>
> TEST=ExtensionServiceTest.**ClearExtensionData
>
>
>
> http://codereview.chromium.**org/7550020/diff/1/chrome/**
>
browser/extensions/extension_**service_unittest.cc<http://codereview.chromium.org/7550020/diff/1/chrome/browser/extensions/extension_service_unittest.cc>
> File chrome/browser/extensions/**extension_service_unittest.cc (right):
>
> http://codereview.chromium.**org/7550020/diff/1/chrome/**
>
browser/extensions/extension_**service_unittest.cc#**newcode2667<http://codereview.chromium.org/7550020/diff/1/chrome/browser/extensions/extension_service_unittest.cc#newcode2667>
> chrome/browser/extensions/**extension_service_unittest.cc:**2667: class
> Callback {
> On 2011/08/05 17:19:40, ycxiao wrote:
>
>> On 2011/08/05 16:09:30, jstritar wrote:
>> > Could you rename this something more specific? Like
>>
> ExtensionCookieCallback?
>
>  Sure, I will rename it when I back to office(I am travelling now).
>>
>
> Thanks!
>
>
> http://codereview.chromium.**org/7550020/diff/1/chrome/**
>
browser/extensions/extension_**service_unittest.cc#**newcode2725<http://codereview.chromium.org/7550020/diff/1/chrome/browser/extensions/extension_service_unittest.cc#newcode2725>
> chrome/browser/extensions/**extension_service_unittest.cc:**2725:
> callback.message_loop_factory_**.RevokeAll();
> On 2011/08/05 17:19:40, ycxiao wrote:
>
>> This RevokeAlls is a ScopedRunnableMethodFactory method which revoke
>>
> any
>
>> outstanding runnable methods scheduled. It makes sure that the
>>
> previous posted
>
>> tasks such as Quit() won't stay at the MessageLoop and have some
>>
> further
>
>> effects.
>>
>
>
> Are they actually needed since you call loop_.RunAllPending()?


You might be right. It seems that loop_.RunAllPending() is enough. I will
remove those RevokeAll() if the unit test all passed.

>
>
>
http://codereview.chromium.**org/7550020/<http://codereview.chromium.org/7550...
>

[ycxiao, 2011-08-06 18:43:27]

http://codereview.chromium.org/7550020/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_service_unittest.cc (right):

http://codereview.chromium.org/7550020/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_service_unittest.cc:2667: class Callback {
On 2011/08/05 17:46:55, jstritar wrote:
> On 2011/08/05 17:19:40, ycxiao wrote:
> > On 2011/08/05 16:09:30, jstritar wrote:
> > > Could you rename this something more specific? Like
ExtensionCookieCallback?
> > 
> > Sure, I will rename it when I back to office(I am travelling now).
> 
> Thanks!

Done.

http://codereview.chromium.org/7550020/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_service_unittest.cc:2725:
callback.message_loop_factory_.RevokeAll();
On 2011/08/05 17:46:55, jstritar wrote:
> On 2011/08/05 17:19:40, ycxiao wrote:
> > This RevokeAlls is a ScopedRunnableMethodFactory method which revoke any
> > outstanding runnable methods scheduled. It makes sure that the previous
posted
> > tasks such as Quit() won't stay at the MessageLoop and have some further
> > effects.  
> > 
> 
> Are they actually needed since you call loop_.RunAllPending()?

I think you are right. I removed the RevokeAll(). And resend a try job.

Thanks for notify!

[ycxiao, 2011-08-09 13:29:18]

Hello Jone,

I have deal with the issues we discuss before. PTAL.

Thanks!

Yancheng

[jstritar, 2011-08-09 18:27:42]

LGTM

[commit-bot: I haz the power, 2011-08-09 23:04:25]

Change committed as 96092