Added completion notification to Profile's ClearNetworkingHistorySince.

Added completion notification to the call chain from Profile's ClearNetworkingHistorySince() to HttpServerPropertiesManager's Clear(), so now the "Clear Browsing Data" dialog will not close before clearing for this particular component has been completed.

Updated the unit test HttpServerPropertiesManagerTest.Clear() to expect a callback, and used existing browser tests to make sure that the delegation of the callback all the way from ClearNetworkingHistorySince() works properly.

TBR-ing Jay because of a minor change to TestingProfile, which was required because of a minor change to the Profile interface, which was in turn reviewed by one of the Profile owners.

BUG=151615
TBR=jcivelli@chromium.org

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

[engedy, 2012-10-22 19:46:34]

Hey Raman, Will,

same issue, new CL (due to Rietveld bug) for adding the callback to
HttpServerProperitesManager's Clear function. Plus updated unit tests.

I expect heavy criticism regarding how I augmented the TestingProfile with a
skeleton Profile[Impl]IOData. My reasoning was:
1.) The delegation path seems long and important enough to warrant some form of
testing (if the callback is not fired, the Clear Data Dialog will hang).
2.) BrowserTests are very expensive, only wanted to do that as a last resort.
3.) The classes along the call chain do not really lend themselves for mocking,
way too many interfaces and virtual functions would need to be created if we
really wanted to test the real thing.

However, if you really don't like the skeletons, I can be persuaded to write a
browsertest instead.

[battre, 2012-10-23 08:38:32]

I think I'll leave the decision on the ProfileImplIOData changes to the OWNERs.

http://codereview.chromium.org/11238034/diff/1019/chrome/browser/net/http_ser...
File chrome/browser/net/http_server_properties_manager.cc (right):

http://codereview.chromium.org/11238034/diff/1019/chrome/browser/net/http_ser...
chrome/browser/net/http_server_properties_manager.cc:657: }
nit: no {}

http://codereview.chromium.org/11238034/diff/1019/chrome/browser/net/http_ser...
File chrome/browser/net/http_server_properties_manager_unittest.cc (right):

http://codereview.chromium.org/11238034/diff/1019/chrome/browser/net/http_ser...
chrome/browser/net/http_server_properties_manager_unittest.cc:79: class
MockClosure : public base::RefCountedThreadSafe<MockClosure> {
I think this is quite some complexity given that this is used only once.

What do you (and the other reviewers) think of:

- Moving this to chrome/test/base? (my preferred solution), or

- Adding a
    MOCK_METHOD0(MockCallback, void())
  to the test fixture,
  and then adding
    EXPECT_CALL(*this, MockCallback());
    http_server_props_manager_->Clear(
        base::Bind(HttpServerPropertiesManagerTest::MockCallback,
Unretained(this)));

http://codereview.chromium.org/11238034/diff/1019/chrome/test/base/testing_pr...
File chrome/test/base/testing_profile.h (right):

http://codereview.chromium.org/11238034/diff/1019/chrome/test/base/testing_pr...
chrome/test/base/testing_profile.h:364: // Initialized on-demand by
callingCreateSkeletonIODataForTesting().
nit: space after calling.

[engedy, 2012-10-23 10:40:09]

Will, Raman, could you please also take a look?

http://codereview.chromium.org/11238034/diff/1019/chrome/browser/net/http_ser...
File chrome/browser/net/http_server_properties_manager.cc (right):

http://codereview.chromium.org/11238034/diff/1019/chrome/browser/net/http_ser...
chrome/browser/net/http_server_properties_manager.cc:657: }
On 2012/10/23 08:38:33, battre wrote:
> nit: no {}

Done.

http://codereview.chromium.org/11238034/diff/1019/chrome/browser/net/http_ser...
File chrome/browser/net/http_server_properties_manager_unittest.cc (right):

http://codereview.chromium.org/11238034/diff/1019/chrome/browser/net/http_ser...
chrome/browser/net/http_server_properties_manager_unittest.cc:79: class
MockClosure : public base::RefCountedThreadSafe<MockClosure> {
Will, Raman, what do you think?

On 2012/10/23 08:38:33, battre wrote:
> I think this is quite some complexity given that this is used only once.
> 
> What do you (and the other reviewers) think of:
> 
> - Moving this to chrome/test/base? (my preferred solution), or
> 
> - Adding a
>     MOCK_METHOD0(MockCallback, void())
>   to the test fixture,
>   and then adding
>     EXPECT_CALL(*this, MockCallback());
>     http_server_props_manager_->Clear(
>         base::Bind(HttpServerPropertiesManagerTest::MockCallback,
> Unretained(this)));

http://codereview.chromium.org/11238034/diff/1019/chrome/test/base/testing_pr...
File chrome/test/base/testing_profile.h (right):

http://codereview.chromium.org/11238034/diff/1019/chrome/test/base/testing_pr...
chrome/test/base/testing_profile.h:364: // Initialized on-demand by
callingCreateSkeletonIODataForTesting().
On 2012/10/23 08:38:33, battre wrote:
> nit: space after calling.

Done.

[ramant (doing other things), 2012-10-23 23:51:47]

http://codereview.chromium.org/11238034/diff/1019/chrome/browser/net/http_ser...
File chrome/browser/net/http_server_properties_manager_unittest.cc (right):

http://codereview.chromium.org/11238034/diff/1019/chrome/browser/net/http_ser...
chrome/browser/net/http_server_properties_manager_unittest.cc:79: class
MockClosure : public base::RefCountedThreadSafe<MockClosure> {
+1 to moving it to chrome/test/base.

In this file, we had used approach#2 (IMO, it is easy slightly easier to
understand). Don't have objections with either approach.

thanks for doing this.

On 2012/10/23 08:38:33, battre wrote:
> I think this is quite some complexity given that this is used only once.
> 
> What do you (and the other reviewers) think of:
> 
> - Moving this to chrome/test/base? (my preferred solution), or
> 
> - Adding a
>     MOCK_METHOD0(MockCallback, void())
>   to the test fixture,
>   and then adding
>     EXPECT_CALL(*this, MockCallback());
>     http_server_props_manager_->Clear(
>         base::Bind(HttpServerPropertiesManagerTest::MockCallback,
> Unretained(this)));

[willchan no longer on Chromium, 2012-10-24 23:44:52]

I'm transferring this review to mmenke because I'm overloaded.

[mmenke, 2012-10-25 16:01:15]

A couple high level comments:

- I'm a little surprised this issue surfaced.  I'd have thought thought that
clearing the history would take longer than the UI->IO->ClearData->UI that this
does.  We do flush preferences on quit, right?  And you have verified this seems
to fix this issue?

- I don't see any need at all for Profile::CreateSkeletonIODataForTesting. 
There are already browser tests for the BrowsingDataRemover, and they seem to
already seem to exercise everything that the skeleton io data tests for. 
Unfortunately, there's no test that we clearing the server properties before the
BrowsingDataRemover returns, but think that due to complexity, not having one
may well be the right choice.

- I think carrying the callback through the IO thread and back to the UI thread,
through 3 different classes, is rather non-intuitive, particularly since the
profile classes take it to the IO thread, and then a completely unrelated
browser/net class takes it back.  I don't see a good alternative.  A
notification might be a little cleaner, but I'm not a big fan of notifications
in general.  ProfileImplIOData::ClearNetworkingHistorySince should at least have
this behavior documented.  Seems like there should be a better way to do this.

https://codereview.chromium.org/11238034/diff/11004/chrome/browser/browsing_d...
File chrome/browser/browsing_data/browsing_data_remover.cc (right):

https://codereview.chromium.org/11238034/diff/11004/chrome/browser/browsing_d...
chrome/browser/browsing_data/browsing_data_remover.cc:500:
base::Unretained(this)));
This strikes me as very regression-prone.  Since OffTheRecordProfileImpl will
cause synchronous deletion, if all the other waiting_for_* flags are false,
anything else put below this line will cause a crash for incognito profiles in
that case, but not for ProfileImpls.

My suggestion is to either make the callback async for the incognito profile
(And update Profile API docs to mention this).

Could also have it return some indication whether it ran synchronously or not,
but think that's a bit counter-intuitive.

[engedy, 2012-10-25 19:41:31]

https://codereview.chromium.org/11238034/diff/11004/chrome/browser/browsing_d...
File chrome/browser/browsing_data/browsing_data_remover.cc (right):

https://codereview.chromium.org/11238034/diff/11004/chrome/browser/browsing_d...
chrome/browser/browsing_data/browsing_data_remover.cc:500:
base::Unretained(this)));
On 2012/10/25 16:01:15, Matt Menke wrote:
> This strikes me as very regression-prone.  Since OffTheRecordProfileImpl will
> cause synchronous deletion, if all the other waiting_for_* flags are false,
> anything else put below this line will cause a crash for incognito profiles in
> that case, but not for ProfileImpls.
> 
> My suggestion is to either make the callback async for the incognito profile
> (And update Profile API docs to mention this).
> 
> Could also have it return some indication whether it ran synchronously or not,
> but think that's a bit counter-intuitive.

Done.

[engedy, 2012-10-25 19:56:13]

> - I'm a little surprised this issue surfaced.  I'd have thought thought that
> clearing the history would take longer than the UI->IO->ClearData->UI that
this
> does.  We do flush preferences on quit, right?  And you have verified this
seems
> to fix this issue?

Currently the only requirement is that [[everything should be properly cleared
if the user exits Chrome quickly after the "Clear Browsing Data" dialog
closes]]. We do not guarantee anything if you close Chrome while the dialog is
still open (although we are planning to address this and ensure not getting into
an inconsistent state, but we don't want to block shut-down), and also nothing
is guaranteed if Chrome crashes at any point.

Requirement [[...]] is currently satisfied (except for the
HttpServerPropertiesManager), even though writing stuff to disk indeed takes
quite some time after the dialog closes -- because those writes always come with
the guarantee that they will happen at latest during shutdown.

With this patch the dialog won't close until we get the callback, which won't
happen until HttpServerPropertiesManager has safely written everything to the
PrefStore. This latter indeed makes the guarantee to write anything to disk at
one point.
 
> - I don't see any need at all for Profile::CreateSkeletonIODataForTesting. 
> There are already browser tests for the BrowsingDataRemover, and they seem to
> already seem to exercise everything that the skeleton io data tests for. 
> Unfortunately, there's no test that we clearing the server properties before
the
> BrowsingDataRemover returns, but think that due to complexity, not having one
> may well be the right choice.

You are absolutely right, I was just so categorically against creating new
browser tests that I totally missed the fact we already have not one, but two of
them that exercises this code path. Removed now-unnecessary code parts.
 
> - I think carrying the callback through the IO thread and back to the UI
thread,
> through 3 different classes, is rather non-intuitive, particularly since the
> profile classes take it to the IO thread, and then a completely unrelated
> browser/net class takes it back.  I don't see a good alternative.  A
> notification might be a little cleaner, but I'm not a big fan of notifications
> in general.  ProfileImplIOData::ClearNetworkingHistorySince should at least
have
> this behavior documented.  Seems like there should be a better way to do this.

I agree it is not the nicest solution. But neither Raman, Will or apparently we
can come up with anything better. :-(

[engedy, 2012-10-25 20:01:03]

Matt, can you please take an other look?

[mmenke, 2012-10-25 20:01:28]

On 2012/10/25 19:56:13, engedy wrote:
> > - I'm a little surprised this issue surfaced.  I'd have thought thought that
> > clearing the history would take longer than the UI->IO->ClearData->UI that
> this
> > does.  We do flush preferences on quit, right?  And you have verified this
> seems
> > to fix this issue?
> 
> Currently the only requirement is that [[everything should be properly cleared
> if the user exits Chrome quickly after the "Clear Browsing Data" dialog
> closes]]. We do not guarantee anything if you close Chrome while the dialog is
> still open (although we are planning to address this and ensure not getting
into
> an inconsistent state, but we don't want to block shut-down), and also nothing
> is guaranteed if Chrome crashes at any point.
> 
> Requirement [[...]] is currently satisfied (except for the
> HttpServerPropertiesManager), even though writing stuff to disk indeed takes
> quite some time after the dialog closes -- because those writes always come
with
> the guarantee that they will happen at latest during shutdown.
> 
> With this patch the dialog won't close until we get the callback, which won't
> happen until HttpServerPropertiesManager has safely written everything to the
> PrefStore. This latter indeed makes the guarantee to write anything to disk at
> one point.

I was just surprised that we actually "lost" ("won"?) the race with respect to
clearing the history.  Not arguing this wouldn't be a bug if it never happened
in practice, but I would have thought that when clearing the history, it would
take longer to clear the history and then close the dialog than it would to
clear this data.

Anyhow, looking at it now.

[mmenke, 2012-10-25 20:15:52]

Only one significant issue.

https://codereview.chromium.org/11238034/diff/26002/chrome/browser/net/http_s...
File chrome/browser/net/http_server_properties_manager_unittest.cc (right):

https://codereview.chromium.org/11238034/diff/26002/chrome/browser/net/http_s...
chrome/browser/net/http_server_properties_manager_unittest.cc:91: virtual
~MockClosure() {}
nit:  friend classes are generally put in the private section.  Also, friends
can call private members, so the destructor can be private, too.

https://codereview.chromium.org/11238034/diff/26002/chrome/browser/net/http_s...
chrome/browser/net/http_server_properties_manager_unittest.cc:94: MockClosure()
{}
nit:  Suggest a blank line here.

https://codereview.chromium.org/11238034/diff/26002/chrome/browser/profiles/o...
File chrome/browser/profiles/off_the_record_profile_impl.cc (right):

https://codereview.chromium.org/11238034/diff/26002/chrome/browser/profiles/o...
chrome/browser/profiles/off_the_record_profile_impl.cc:459: // Fire the callback
though, to make sure we do not cause a hang in any case.
nit:  It's actually a leak we're preventing.

I'd suggest mentioning it's async specifically.  Maybe "Run callback
asynchronously to avoid reetrancy issues."

https://codereview.chromium.org/11238034/diff/26002/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_impl_io_data.h (right):

https://codereview.chromium.org/11238034/diff/26002/chrome/browser/profiles/p...
chrome/browser/profiles/profile_impl_io_data.h:75: const base::Closure&
completion);
Think you should mention that |completion| will be called on the UI thread here.
 This behavior is non-obvious from looking at this class.  Admittedly, it's
mentioned in the other two classes, and not a whole lot of classes actually have
access to this class, but I'm paranoid.

https://codereview.chromium.org/11238034/diff/26002/chrome/test/base/testing_...
File chrome/test/base/testing_profile.cc (right):

https://codereview.chromium.org/11238034/diff/26002/chrome/test/base/testing_...
chrome/test/base/testing_profile.cc:791:
BrowserThread::PostTask(BrowserThread::IO, FROM_HERE, completion);
IMPORTANT:  The completion should be run on the UI thread.

[engedy, 2012-10-25 20:18:56]

> I was just surprised that we actually "lost" ("won"?) the race with respect to
> clearing the history.  Not arguing this wouldn't be a bug if it never happened
> in practice, but I would have thought that when clearing the history, it would
> take longer to clear the history and then close the dialog than it would to
> clear this data.

Oh, sorry, I have misunderstood what you meant earlier. The issue surfaced
because previously clearing data in the HttpServerPropertiesManager used the
ScheduleUpdatePrefsOnIO() function to clear the Prefs, which has a built-in
delay of 10 seconds before touching the Prefs to allow coalescing writes in
usual cases. A timer was used to achieve this, which, of course, did not fire at
all if the user made a hasty shutdown.

Clearing the HttpServerPropertiesManager itself should be very fast indeed.
Still, I would not be comfortable without the callback, because that would still
leave a theoretical chance of closing the dialog and quitting too early (by the
time the IO thread posts the task that writes the Prefs to the UI thread, this
thread might have already exited).

What do you think?

[mmenke, 2012-10-25 20:20:56]

On 2012/10/25 20:18:56, engedy wrote:
> > I was just surprised that we actually "lost" ("won"?) the race with respect
to
> > clearing the history.  Not arguing this wouldn't be a bug if it never
happened
> > in practice, but I would have thought that when clearing the history, it
would
> > take longer to clear the history and then close the dialog than it would to
> > clear this data.
> 
> Oh, sorry, I have misunderstood what you meant earlier. The issue surfaced
> because previously clearing data in the HttpServerPropertiesManager used the
> ScheduleUpdatePrefsOnIO() function to clear the Prefs, which has a built-in
> delay of 10 seconds before touching the Prefs to allow coalescing writes in
> usual cases. A timer was used to achieve this, which, of course, did not fire
at
> all if the user made a hasty shutdown.
> 
> Clearing the HttpServerPropertiesManager itself should be very fast indeed.
> Still, I would not be comfortable without the callback, because that would
still
> leave a theoretical chance of closing the dialog and quitting too early (by
the
> time the IO thread posts the task that writes the Prefs to the UI thread, this
> thread might have already exited).
> 
> What do you think?

I agree.  Was just worried that we could be missing something, and this wouldn't
be a full fix.  Your explanation is completely reasonable, and I agree that this
is needed for correctness.

[engedy, 2012-10-25 21:47:43]

All done, one more question:

Dominic also suggested moving the MockClosure to "chrome/test/base", although I
believe it is not Chromium-browser-specific, so I would prefer "testing/base"
instead. What do you think?

Moreover, I guess the actual MockClosure class should not be exposed as part of
the interface, because it seems totally uninteresting for our users. 

So our interface would basically consist of a single static factory method that
returns a base::Closure. What is our preferred way of doing that? How about a
non-member function inside a namespace like MockClosure::ExpectOnce()?

http://codereview.chromium.org/11238034/diff/26002/chrome/browser/net/http_se...
File chrome/browser/net/http_server_properties_manager_unittest.cc (right):

http://codereview.chromium.org/11238034/diff/26002/chrome/browser/net/http_se...
chrome/browser/net/http_server_properties_manager_unittest.cc:91: virtual
~MockClosure() {}
On 2012/10/25 20:15:52, Matt Menke wrote:
> nit:  friend classes are generally put in the private section.  Also, friends
> can call private members, so the destructor can be private, too.

Done.

http://codereview.chromium.org/11238034/diff/26002/chrome/browser/net/http_se...
chrome/browser/net/http_server_properties_manager_unittest.cc:94: MockClosure()
{}
On 2012/10/25 20:15:52, Matt Menke wrote:
> nit:  Suggest a blank line here.

Done.

http://codereview.chromium.org/11238034/diff/26002/chrome/browser/profiles/of...
File chrome/browser/profiles/off_the_record_profile_impl.cc (right):

http://codereview.chromium.org/11238034/diff/26002/chrome/browser/profiles/of...
chrome/browser/profiles/off_the_record_profile_impl.cc:459: // Fire the callback
though, to make sure we do not cause a hang in any case.
On 2012/10/25 20:15:52, Matt Menke wrote:
> nit:  It's actually a leak we're preventing.
> 
> I'd suggest mentioning it's async specifically.  Maybe "Run callback
> asynchronously to avoid reetrancy issues."

Done.

http://codereview.chromium.org/11238034/diff/26002/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_impl_io_data.h (right):

http://codereview.chromium.org/11238034/diff/26002/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_impl_io_data.h:75: const base::Closure&
completion);
On 2012/10/25 20:15:52, Matt Menke wrote:
> Think you should mention that |completion| will be called on the UI thread
here.
>  This behavior is non-obvious from looking at this class.  Admittedly, it's
> mentioned in the other two classes, and not a whole lot of classes actually
have
> access to this class, but I'm paranoid.

I agree. I copied the comment from profile.h and also added it on line 171.

http://codereview.chromium.org/11238034/diff/26002/chrome/test/base/testing_p...
File chrome/test/base/testing_profile.cc (right):

http://codereview.chromium.org/11238034/diff/26002/chrome/test/base/testing_p...
chrome/test/base/testing_profile.cc:791:
BrowserThread::PostTask(BrowserThread::IO, FROM_HERE, completion);
On 2012/10/25 20:15:52, Matt Menke wrote:
> IMPORTANT:  The completion should be run on the UI thread.

Duh! Thanks for spotting that.

[engedy, 2012-10-26 16:27:46]

Refactored MockClosure into a NewExpectedClosure() function in base/test.

Matt, could you please take a final look?

[mmenke, 2012-10-26 16:29:04]

On 2012/10/26 16:27:46, engedy wrote:
> Refactored MockClosure into a NewExpectedClosure() function in base/test.
> 
> Matt, could you please take a final look?

When I have a chance, later today.  I have 4 or 5 reviews pending, and appengine
has been down...

[mmenke, 2012-10-26 19:06:20]

LGTM.

I don't really have any strong opinion on where MockClosure lives - Though I
agree that base/test is probably better than chrome/test, I'd be just as happy
with it in the individual unittest file.

http://codereview.chromium.org/11238034/diff/39001/base/test/mock_callback.cc
File base/test/mock_callback.cc (right):

http://codereview.chromium.org/11238034/diff/39001/base/test/mock_callback.cc...
base/test/mock_callback.cc:23: DISALLOW_COPY_AND_ASSIGN(MockClosure);
You should include basictypes.h for this.

http://codereview.chromium.org/11238034/diff/39001/chrome/browser/profiles/of...
File chrome/browser/profiles/off_the_record_profile_impl.cc (right):

http://codereview.chromium.org/11238034/diff/39001/chrome/browser/profiles/of...
chrome/browser/profiles/off_the_record_profile_impl.cc:460: // Remover will
never be destroyed and the dialog will never be closed. Also,
nit:  Don't think I've ever seen words split like this in the Chrome code base. 
Just use BrowserDataRemover, and wrap the line.

http://codereview.chromium.org/11238034/diff/39001/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile.h (right):

http://codereview.chromium.org/11238034/diff/39001/chrome/browser/profiles/pr...
chrome/browser/profiles/profile.h:328: // will be posted on the UI thread once
the removal process completes.
May want to mention that completion may theoretically be called after the
deletion of the profile (In some similar cases with other classes, callbacks
would be cancelled).

[engedy, 2012-10-29 14:37:56]

Mike: Please review the changes in BrowsingDataRemover for owner's approval.

Will: Please review the changes concerning HttpServerPropertiesManager and in
/base/test for owner's approval.

https://codereview.chromium.org/11238034/diff/39001/base/test/mock_callback.cc
File base/test/mock_callback.cc (right):

https://codereview.chromium.org/11238034/diff/39001/base/test/mock_callback.c...
base/test/mock_callback.cc:23: DISALLOW_COPY_AND_ASSIGN(MockClosure);
On 2012/10/26 19:06:20, Matt Menke wrote:
> You should include basictypes.h for this.

Done.

https://codereview.chromium.org/11238034/diff/39001/chrome/browser/profiles/o...
File chrome/browser/profiles/off_the_record_profile_impl.cc (right):

https://codereview.chromium.org/11238034/diff/39001/chrome/browser/profiles/o...
chrome/browser/profiles/off_the_record_profile_impl.cc:460: // Remover will
never be destroyed and the dialog will never be closed. Also,
On 2012/10/26 19:06:20, Matt Menke wrote:
> nit:  Don't think I've ever seen words split like this in the Chrome code
base. 
> Just use BrowserDataRemover, and wrap the line.

Done.

https://codereview.chromium.org/11238034/diff/39001/chrome/browser/profiles/p...
File chrome/browser/profiles/profile.h (right):

https://codereview.chromium.org/11238034/diff/39001/chrome/browser/profiles/p...
chrome/browser/profiles/profile.h:328: // will be posted on the UI thread once
the removal process completes.
On 2012/10/26 19:06:20, Matt Menke wrote:
> May want to mention that completion may theoretically be called after the
> deletion of the profile (In some similar cases with other classes, callbacks
> would be cancelled).

Done.

[willchan no longer on Chromium, 2012-10-29 17:17:53]

Matt's got the HttpServerPropertiesManager change covered. I'll look at
base/test/.


On Mon, Oct 29, 2012 at 7:37 AM, <engedy@chromium.org> wrote:

> Mike: Please review the changes in BrowsingDataRemover for owner's
> approval.
>
> Will: Please review the changes concerning HttpServerPropertiesManager and
> in
> /base/test for owner's approval.
>
>
> https://codereview.chromium.**org/11238034/diff/39001/base/**
>
test/mock_callback.cc<https://codereview.chromium.org/11238034/diff/39001/base/test/mock_callback.cc>
> File base/test/mock_callback.cc (right):
>
> https://codereview.chromium.**org/11238034/diff/39001/base/**
>
test/mock_callback.cc#**newcode23<https://codereview.chromium.org/11238034/diff/39001/base/test/mock_callback.cc#newcode23>
> base/test/mock_callback.cc:23: DISALLOW_COPY_AND_ASSIGN(**MockClosure);
>
> On 2012/10/26 19:06:20, Matt Menke wrote:
>
>> You should include basictypes.h for this.
>>
>
> Done.
>
> https://codereview.chromium.**org/11238034/diff/39001/**
>
chrome/browser/profiles/off_**the_record_profile_impl.cc<https://codereview.chromium.org/11238034/diff/39001/chrome/browser/profiles/off_the_record_profile_impl.cc>
> File chrome/browser/profiles/off_**the_record_profile_impl.cc (right):
>
> https://codereview.chromium.**org/11238034/diff/39001/**
>
chrome/browser/profiles/off_**the_record_profile_impl.cc#**newcode460<https://codereview.chromium.org/11238034/diff/39001/chrome/browser/profiles/off_the_record_profile_impl.cc#newcode460>
>
> chrome/browser/profiles/off_**the_record_profile_impl.cc:**460: // Remover
> will never be destroyed and the dialog will never be closed. Also,
> On 2012/10/26 19:06:20, Matt Menke wrote:
>
>> nit:  Don't think I've ever seen words split like this in the Chrome
>>
> code base.
>
>> Just use BrowserDataRemover, and wrap the line.
>>
>
> Done.
>
> https://codereview.chromium.**org/11238034/diff/39001/**
>
chrome/browser/profiles/**profile.h<https://codereview.chromium.org/11238034/diff/39001/chrome/browser/profiles/profile.h>
> File chrome/browser/profiles/**profile.h (right):
>
> https://codereview.chromium.**org/11238034/diff/39001/**
>
chrome/browser/profiles/**profile.h#newcode328<https://codereview.chromium.org/11238034/diff/39001/chrome/browser/profiles/profile.h#newcode328>
>
> chrome/browser/profiles/**profile.h:328: // will be posted on the UI
> thread once the removal process completes.
> On 2012/10/26 19:06:20, Matt Menke wrote:
>
>> May want to mention that completion may theoretically be called after
>>
> the
>
>> deletion of the profile (In some similar cases with other classes,
>>
> callbacks
>
>> would be cancelled).
>>
>
> Done.
>
>
https://codereview.chromium.**org/11238034/<https://codereview.chromium.org/1...
>

[Mike West, 2012-10-30 10:28:53]

On 2012/10/29 14:37:56, engedy wrote:
> Mike: Please review the changes in BrowsingDataRemover for owner's approval.

BrowsingDataRemove LGTM with one nit. Thanks for making the naming changes we
discussed.

[Mike West, 2012-10-30 10:33:22]

http://codereview.chromium.org/11238034/diff/57020/chrome/browser/browsing_da...
File chrome/browser/browsing_data/browsing_data_remover.cc (right):

http://codereview.chromium.org/11238034/diff/57020/chrome/browser/browsing_da...
chrome/browser/browsing_data/browsing_data_remover.cc:573:
!waiting_for_clear_network_predictor_;
Nit: Since you're already touching this, can you sort it?

[engedy, 2012-10-30 11:01:14]

https://codereview.chromium.org/11238034/diff/57020/chrome/browser/browsing_d...
File chrome/browser/browsing_data/browsing_data_remover.cc (right):

https://codereview.chromium.org/11238034/diff/57020/chrome/browser/browsing_d...
chrome/browser/browsing_data/browsing_data_remover.cc:573:
!waiting_for_clear_network_predictor_;
On 2012/10/30 10:33:22, Mike West (chromium) wrote:
> Nit: Since you're already touching this, can you sort it?

Done.

[awong, 2012-10-30 19:03:42]

Drive-by disagreement with MockClosure...

I actually do have a rather strong opinion on MockClosure... In general, mocking
the Closure itself is the wrong level of abstractions.  I know media has this
code, but I think it's left-over from NewRunnableMethod when creating a test
Task required more boilerplate. Talking with Ami, I think we're actually going
to try to remove it from media as well.

Since a Closure is really just a conduit for executing a target function, a
better place to mock stuff is actually at the target. My formulas are:

(1) If your test doesn't need to strongly assert that the Callback is run, use
Bind(&DoNothing).  This is probably the most common case.

(2) If you need to synchronize your test to another thread, use WaitableEvent e;
Bind(&WaitableEvent::Signal, &e); e.Wait();

(3) If you're on the same thread, start a MessageLoop and do something similar
using MessageLoop::QuitClosure() and MessageLoop::Run().

(4) If you really need to count the number of times run (like you're asserting
that 5 queued tasks result in 5 callbacks), then do something like void
Increment(int* n) { *n += 1; } and in the TEST_F, do int cb_count = 0;
Bind(&Increment, &cb_count); /* Execute Your test */ EXPECT_EQ(kExpectedCalls,
cb_count);

Looking at your test case, I think (1) is most appropriate. I could see an
argument for (4), but since we can't guard against a path where the cb isn't
actually run, I'm not sure it actually buys us any real invariant checking.

-Albert



On 2012/10/30 11:01:14, engedy wrote:
>
https://codereview.chromium.org/11238034/diff/57020/chrome/browser/browsing_d...
> File chrome/browser/browsing_data/browsing_data_remover.cc (right):
> 
>
https://codereview.chromium.org/11238034/diff/57020/chrome/browser/browsing_d...
> chrome/browser/browsing_data/browsing_data_remover.cc:573:
> !waiting_for_clear_network_predictor_;
> On 2012/10/30 10:33:22, Mike West (chromium) wrote:
> > Nit: Since you're already touching this, can you sort it?
> 
> Done.

[engedy, 2012-10-30 20:50:39]

Philosophically, I tend to disagree with the reasoning about abstraction levels.
We are not mocking the Closure itself, we are mocking a target function. We are
just also providing much more; namely a single-function-shortcut that does all
the following steps:
 i.) Defines a target function that can be used as a callback.
 ii.) Sets up an expectation (using EXPECT_CALL) that this target function will
be called.
 iii.) Returns a Closure that points to and will execute this "armed" target
function.

Nevertheless, given that no one else has a strong preference in this question,
and that gMock has fallen from grace in general anyway, I went with what you
have suggested as option (3).

Could you please take a look?

PS.: Also, I believe I do not understand exactly what you mean in the last
paragraph. Are you referring to the previous testing approach not working, or
are you highlighting that checking one code path does not mean it will work OK
in all cases?

On 2012/10/30 19:03:42, awong wrote:
> Drive-by disagreement with MockClosure...
> 
> I actually do have a rather strong opinion on MockClosure... In general,
mocking
> the Closure itself is the wrong level of abstractions.  I know media has this
> code, but I think it's left-over from NewRunnableMethod when creating a test
> Task required more boilerplate. Talking with Ami, I think we're actually going
> to try to remove it from media as well.
> 
> Since a Closure is really just a conduit for executing a target function, a
> better place to mock stuff is actually at the target. My formulas are:
> 
> (1) If your test doesn't need to strongly assert that the Callback is run, use
> Bind(&DoNothing).  This is probably the most common case.
> 
> (2) If you need to synchronize your test to another thread, use WaitableEvent
e;
> Bind(&WaitableEvent::Signal, &e); e.Wait();
> 
> (3) If you're on the same thread, start a MessageLoop and do something similar
> using MessageLoop::QuitClosure() and MessageLoop::Run().
> 
> (4) If you really need to count the number of times run (like you're asserting
> that 5 queued tasks result in 5 callbacks), then do something like void
> Increment(int* n) { *n += 1; } and in the TEST_F, do int cb_count = 0;
> Bind(&Increment, &cb_count); /* Execute Your test */ EXPECT_EQ(kExpectedCalls,
> cb_count);
> 
> Looking at your test case, I think (1) is most appropriate. I could see an
> argument for (4), but since we can't guard against a path where the cb isn't
> actually run, I'm not sure it actually buys us any real invariant checking.
> 
> -Albert
> 
> 
> 
> On 2012/10/30 11:01:14, engedy wrote:
> >
>
https://codereview.chromium.org/11238034/diff/57020/chrome/browser/browsing_d...
> > File chrome/browser/browsing_data/browsing_data_remover.cc (right):
> > 
> >
>
https://codereview.chromium.org/11238034/diff/57020/chrome/browser/browsing_d...
> > chrome/browser/browsing_data/browsing_data_remover.cc:573:
> > !waiting_for_clear_network_predictor_;
> > On 2012/10/30 10:33:22, Mike West (chromium) wrote:
> > > Nit: Since you're already touching this, can you sort it?
> > 
> > Done.

[awong, 2012-11-01 19:43:37]

Regarding my last paragraph, yeah, I was just highlighting that checking that
the callback was invoked is incremental beneficial because the expectation is
highly correlated with other parts of the test.

It'd be hard to have a fail for expected calls, but a success everywhere else. 
However, it's also quite easy to add a dynamic code path that doesn't execute
the callback, not cover it in the expectation, and then get a "success" for the
check. So I don't see what the assertion that the CB is run is really proving.

But, like you mention, this is getting into philosophy.  LGTM and thanks for
listening to me rant/digress. :)


On 2012/10/30 20:50:39, engedy wrote:
> Philosophically, I tend to disagree with the reasoning about abstraction
levels.
> We are not mocking the Closure itself, we are mocking a target function. We
are
> just also providing much more; namely a single-function-shortcut that does all
> the following steps:
>  i.) Defines a target function that can be used as a callback.
>  ii.) Sets up an expectation (using EXPECT_CALL) that this target function
will
> be called.
>  iii.) Returns a Closure that points to and will execute this "armed" target
> function.
> 
> Nevertheless, given that no one else has a strong preference in this question,
> and that gMock has fallen from grace in general anyway, I went with what you
> have suggested as option (3).
> 
> Could you please take a look?
> 
> PS.: Also, I believe I do not understand exactly what you mean in the last
> paragraph. Are you referring to the previous testing approach not working, or
> are you highlighting that checking one code path does not mean it will work OK
> in all cases?
> 
> On 2012/10/30 19:03:42, awong wrote:
> > Drive-by disagreement with MockClosure...
> > 
> > I actually do have a rather strong opinion on MockClosure... In general,
> mocking
> > the Closure itself is the wrong level of abstractions.  I know media has
this
> > code, but I think it's left-over from NewRunnableMethod when creating a test
> > Task required more boilerplate. Talking with Ami, I think we're actually
going
> > to try to remove it from media as well.
> > 
> > Since a Closure is really just a conduit for executing a target function, a
> > better place to mock stuff is actually at the target. My formulas are:
> > 
> > (1) If your test doesn't need to strongly assert that the Callback is run,
use
> > Bind(&DoNothing).  This is probably the most common case.
> > 
> > (2) If you need to synchronize your test to another thread, use
WaitableEvent
> e;
> > Bind(&WaitableEvent::Signal, &e); e.Wait();
> > 
> > (3) If you're on the same thread, start a MessageLoop and do something
similar
> > using MessageLoop::QuitClosure() and MessageLoop::Run().
> > 
> > (4) If you really need to count the number of times run (like you're
asserting
> > that 5 queued tasks result in 5 callbacks), then do something like void
> > Increment(int* n) { *n += 1; } and in the TEST_F, do int cb_count = 0;
> > Bind(&Increment, &cb_count); /* Execute Your test */
EXPECT_EQ(kExpectedCalls,
> > cb_count);
> > 
> > Looking at your test case, I think (1) is most appropriate. I could see an
> > argument for (4), but since we can't guard against a path where the cb isn't
> > actually run, I'm not sure it actually buys us any real invariant checking.
> > 
> > -Albert
> > 
> > 
> > 
> > On 2012/10/30 11:01:14, engedy wrote:
> > >
> >
>
https://codereview.chromium.org/11238034/diff/57020/chrome/browser/browsing_d...
> > > File chrome/browser/browsing_data/browsing_data_remover.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/11238034/diff/57020/chrome/browser/browsing_d...
> > > chrome/browser/browsing_data/browsing_data_remover.cc:573:
> > > !waiting_for_clear_network_predictor_;
> > > On 2012/10/30 10:33:22, Mike West (chromium) wrote:
> > > > Nit: Since you're already touching this, can you sort it?
> > > 
> > > Done.

[engedy, 2012-11-02 10:42:14]

Yes, I completely agree that adding such an expectation does not help too much.

However, given that adding this callback was the only change I made to the code,
the alternative would have been not adding any new tests, something I would have
felt really uneasy about. Should I have screwed up something in the new code
part, I could easily have ended up not calling the callback -- and I would not
have known, because the existing tests would still have succeeded. So in this
sense, I believe this is one of the few cases where checking that a callback is
actually invoked has some value, because this "new" expectation is not so highly
correlated with the "old", existing ones.

On the other hand, yes, it does not prevent somebody from adding an other
dynamic path that does not invoke the callback. But if at least that somebody
takes the time to look at the tests, he might realize that wow, strange, we are
waiting on a callback here... perhaps I should invoke one myself.

All in all, I am not sure that the above makes sense, but these were the reasons
why I decided to add this piece of test: to formally make sure the code I have
added works at this very moment, and to document and informally remind future
adventurers that this callback really has to be invoked.

On 2012/11/01 19:43:37, awong wrote:
> Regarding my last paragraph, yeah, I was just highlighting that checking that
> the callback was invoked is incremental beneficial because the expectation is
> highly correlated with other parts of the test.
> 
> It'd be hard to have a fail for expected calls, but a success everywhere else.

> However, it's also quite easy to add a dynamic code path that doesn't execute
> the callback, not cover it in the expectation, and then get a "success" for
the
> check. So I don't see what the assertion that the CB is run is really proving.

[engedy, 2012-11-05 13:05:21]

@Will: given that Matt already reviewed everything except the base/test part,
which in turn has been removed already, can I get a rubberstamp LGTM, so I can
land this? Thanks!

On 2012/11/02 10:42:14, engedy wrote:
> Yes, I completely agree that adding such an expectation does not help too
much.
> 
> However, given that adding this callback was the only change I made to the
code,
> the alternative would have been not adding any new tests, something I would
have
> felt really uneasy about. Should I have screwed up something in the new code
> part, I could easily have ended up not calling the callback -- and I would not
> have known, because the existing tests would still have succeeded. So in this
> sense, I believe this is one of the few cases where checking that a callback
is
> actually invoked has some value, because this "new" expectation is not so
highly
> correlated with the "old", existing ones.
> 
> On the other hand, yes, it does not prevent somebody from adding an other
> dynamic path that does not invoke the callback. But if at least that somebody
> takes the time to look at the tests, he might realize that wow, strange, we
are
> waiting on a callback here... perhaps I should invoke one myself.
> 
> All in all, I am not sure that the above makes sense, but these were the
reasons
> why I decided to add this piece of test: to formally make sure the code I have
> added works at this very moment, and to document and informally remind future
> adventurers that this callback really has to be invoked.
> 
> On 2012/11/01 19:43:37, awong wrote:
> > Regarding my last paragraph, yeah, I was just highlighting that checking
that
> > the callback was invoked is incremental beneficial because the expectation
is
> > highly correlated with other parts of the test.
> > 
> > It'd be hard to have a fail for expected calls, but a success everywhere
else.
> 
> > However, it's also quite easy to add a dynamic code path that doesn't
execute
> > the callback, not cover it in the expectation, and then get a "success" for
> the
> > check. So I don't see what the assertion that the CB is run is really
proving.

[willchan no longer on Chromium, 2012-11-05 15:43:17]

What am I reviewing that Matt hasn't reviewed? He's in all the same OWNERS
files as me except base/.


On Mon, Nov 5, 2012 at 5:05 AM, <engedy@chromium.org> wrote:

> @Will: given that Matt already reviewed everything except the base/test
> part,
> which in turn has been removed already, can I get a rubberstamp LGTM, so I
> can
> land this? Thanks!
>
>
> On 2012/11/02 10:42:14, engedy wrote:
>
>> Yes, I completely agree that adding such an expectation does not help too
>>
> much.
>
>  However, given that adding this callback was the only change I made to the
>>
> code,
>
>> the alternative would have been not adding any new tests, something I
>> would
>>
> have
>
>> felt really uneasy about. Should I have screwed up something in the new
>> code
>> part, I could easily have ended up not calling the callback -- and I
>> would not
>> have known, because the existing tests would still have succeeded. So in
>> this
>> sense, I believe this is one of the few cases where checking that a
>> callback
>>
> is
>
>> actually invoked has some value, because this "new" expectation is not so
>>
> highly
>
>> correlated with the "old", existing ones.
>>
>
>  On the other hand, yes, it does not prevent somebody from adding an other
>> dynamic path that does not invoke the callback. But if at least that
>> somebody
>> takes the time to look at the tests, he might realize that wow, strange,
>> we
>>
> are
>
>> waiting on a callback here... perhaps I should invoke one myself.
>>
>
>  All in all, I am not sure that the above makes sense, but these were the
>>
> reasons
>
>> why I decided to add this piece of test: to formally make sure the code I
>> have
>> added works at this very moment, and to document and informally remind
>> future
>> adventurers that this callback really has to be invoked.
>>
>
>  On 2012/11/01 19:43:37, awong wrote:
>> > Regarding my last paragraph, yeah, I was just highlighting that checking
>>
> that
>
>> > the callback was invoked is incremental beneficial because the
>> expectation
>>
> is
>
>> > highly correlated with other parts of the test.
>> >
>> > It'd be hard to have a fail for expected calls, but a success everywhere
>>
> else.
>
>  > However, it's also quite easy to add a dynamic code path that doesn't
>>
> execute
>
>> > the callback, not cover it in the expectation, and then get a "success"
>> for
>> the
>> > check. So I don't see what the assertion that the CB is run is really
>>
> proving.
>
>
>
>
http://codereview.chromium.**org/11238034/<http://codereview.chromium.org/112...
>

[engedy, 2012-11-05 16:16:29]

You are completely right, of course. Apparently, I can't read. :-(

On 2012/11/05 15:43:17, willchan wrote:
> What am I reviewing that Matt hasn't reviewed? He's in all the same OWNERS
> files as me except base/.
> 
> 
> On Mon, Nov 5, 2012 at 5:05 AM, <mailto:engedy@chromium.org> wrote:
> 
> > @Will: given that Matt already reviewed everything except the base/test
> > part,
> > which in turn has been removed already, can I get a rubberstamp LGTM, so I
> > can
> > land this? Thanks!
> >
> >
> > On 2012/11/02 10:42:14, engedy wrote:
> >
> >> Yes, I completely agree that adding such an expectation does not help too
> >>
> > much.
> >
> >  However, given that adding this callback was the only change I made to the
> >>
> > code,
> >
> >> the alternative would have been not adding any new tests, something I
> >> would
> >>
> > have
> >
> >> felt really uneasy about. Should I have screwed up something in the new
> >> code
> >> part, I could easily have ended up not calling the callback -- and I
> >> would not
> >> have known, because the existing tests would still have succeeded. So in
> >> this
> >> sense, I believe this is one of the few cases where checking that a
> >> callback
> >>
> > is
> >
> >> actually invoked has some value, because this "new" expectation is not so
> >>
> > highly
> >
> >> correlated with the "old", existing ones.
> >>
> >
> >  On the other hand, yes, it does not prevent somebody from adding an other
> >> dynamic path that does not invoke the callback. But if at least that
> >> somebody
> >> takes the time to look at the tests, he might realize that wow, strange,
> >> we
> >>
> > are
> >
> >> waiting on a callback here... perhaps I should invoke one myself.
> >>
> >
> >  All in all, I am not sure that the above makes sense, but these were the
> >>
> > reasons
> >
> >> why I decided to add this piece of test: to formally make sure the code I
> >> have
> >> added works at this very moment, and to document and informally remind
> >> future
> >> adventurers that this callback really has to be invoked.
> >>
> >
> >  On 2012/11/01 19:43:37, awong wrote:
> >> > Regarding my last paragraph, yeah, I was just highlighting that checking
> >>
> > that
> >
> >> > the callback was invoked is incremental beneficial because the
> >> expectation
> >>
> > is
> >
> >> > highly correlated with other parts of the test.
> >> >
> >> > It'd be hard to have a fail for expected calls, but a success everywhere
> >>
> > else.
> >
> >  > However, it's also quite easy to add a dynamic code path that doesn't
> >>
> > execute
> >
> >> > the callback, not cover it in the expectation, and then get a "success"
> >> for
> >> the
> >> > check. So I don't see what the assertion that the CB is run is really
> >>
> > proving.
> >
> >
> >
> >
>
http://codereview.chromium.**org/11238034/%3Chttp://codereview.chromium.org/1...>
> >

[commit-bot: I haz the power, 2012-11-05 16:23:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/engedy@chromium.org/11238034/58001

[commit-bot: I haz the power, 2012-11-05 18:25:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/engedy@chromium.org/11238034/62004

[commit-bot: I haz the power, 2012-11-05 20:25:37]

Change committed as 166016