Move BrowsingDataRemoverImpl:: CompletionInhibitor to the public interface

Move BrowsingDataRemoverImpl:: CompletionInhibitor to the public interface

The CompletionInhibitor interface is removed in favor of a simpler
SetWouldCompleteCallbackForTesting() method defined in BrowsingDataRemover.
Its implementation BrowsingDataRemoverCompletionInhibitor is updated to use
the new interface.

This is necessary due to the fact that CompletionInhibitor was used both
in chrome/ and in BrowsingDataRemoverImplTest which will move to content/,
and thus its definition in BrowsingDataRemoverImpl (which will move to
non-public content/) would not be visible in chrome/.

On the way, this CL also resolves a TODO to make CompletionInhibitor non-static.

TBR=dbeam@chromium.org
BUG=483528, 668114
Review-Url: https://codereview.chromium.org/2802013002
Cr-Commit-Position: refs/heads/master@{#464199}
Committed: https://chromium.googlesource.com/chromium/src/+/bb8dc5c563daf902f09b6f0ab6079d3ec66a089f

[msramek, 2017-04-06 12:56:17]

msramek@chromium.org changed reviewers:
+ bauerb@chromium.org

[msramek, 2017-04-06 12:56:18]

Hi Bernhard!

Can you please have a look at this as well? Sorry for sending you two CLs at the
same time, but I'm addressing your TODO here, so I thought you might be again
the right person :)

Thanks!
Martin

[Bernhard Bauer, 2017-04-06 15:13:07]

https://codereview.chromium.org/2802013002/diff/1/chrome/browser/browsing_dat...
File chrome/browser/browsing_data/browsing_data_remover.h (right):

https://codereview.chromium.org/2802013002/diff/1/chrome/browser/browsing_dat...
chrome/browser/browsing_data/browsing_data_remover.h:138: virtual void
OnBrowsingDataRemoverWouldComplete(
You could consider replacing this with a Callback.

Also, IIRC at some point we were thinking about replacing this with a SubTask –
would that make sense now? If it's just additional work, don't worry about it,
but if it reduces churn to do this in one go, I would consider it.

[msramek, 2017-04-07 10:58:42]

https://codereview.chromium.org/2802013002/diff/1/chrome/browser/browsing_dat...
File chrome/browser/browsing_data/browsing_data_remover.h (right):

https://codereview.chromium.org/2802013002/diff/1/chrome/browser/browsing_dat...
chrome/browser/browsing_data/browsing_data_remover.h:138: virtual void
OnBrowsingDataRemoverWouldComplete(
On 2017/04/06 15:13:07, Bernhard Bauer wrote:
> You could consider replacing this with a Callback.

Note that we're in content/public, so everything needs to be pure virtual.

BrowsingDataRemoverImpl currently needs to call
inhibitor->OnBrowsingDataRemoverWouldComplete(). If we replace this with a
Callback, it will have to be exposed through something like GetCallback(), and
we will call inhibitor->GetCallback().Run(). I think that's a bit of an unusual
interface.

> 
> Also, IIRC at some point we were thinking about replacing this with a SubTask
–
> would that make sense now? If it's just additional work, don't worry about it,
> but if it reduces churn to do this in one go, I would consider it.

Yes. I've been looking into this back then, but it doesn't seem to be useful.

We need to call the inhibitor callback when all *other* SubTasks are completed.
Therefore, AllDone() needs to look at all tasks without inhibitor. But we can
only Notify() if all SubTasks, including inhibitor, are completed. And this is
the logic that we already have in NotifyIfDone(). Making inhibitor a SubTask
will not simplify it.

As for simplifying the public interface, that's not possible. The fact that the
deletion is organized into SubTasks is an implementation detail of
BrowsingDataRemoverImpl; it shouldn't be known to BrowsingDataRemover. However,
Inhibitor needs to be there.

[Bernhard Bauer, 2017-04-07 15:14:18]

https://codereview.chromium.org/2802013002/diff/1/chrome/browser/browsing_dat...
File chrome/browser/browsing_data/browsing_data_remover.h (right):

https://codereview.chromium.org/2802013002/diff/1/chrome/browser/browsing_dat...
chrome/browser/browsing_data/browsing_data_remover.h:138: virtual void
OnBrowsingDataRemoverWouldComplete(
On 2017/04/07 10:58:42, msramek wrote:
> On 2017/04/06 15:13:07, Bernhard Bauer wrote:
> > You could consider replacing this with a Callback.
> 
> Note that we're in content/public, so everything needs to be pure virtual.
> 
> BrowsingDataRemoverImpl currently needs to call
> inhibitor->OnBrowsingDataRemoverWouldComplete(). If we replace this with a
> Callback, it will have to be exposed through something like GetCallback(), and
> we will call inhibitor->GetCallback().Run(). I think that's a bit of an
unusual
> interface.

No, what I meant was replacing the whole CompletionInhibitor with a
base::Callback<void(BrowsingDataRemover*, const base::Closure&>. Replacing
one-method interfaces with callbacks is a pretty common pattern in the content
API.

[msramek, 2017-04-10 10:02:10]

The CQ bit was checked by msramek@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-10 10:02:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-10 10:26:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-10 10:26:40]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[msramek, 2017-04-10 13:03:43]

Patchset #3 (id:40001) has been deleted

[msramek, 2017-04-10 13:03:54]

The CQ bit was checked by msramek@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-10 13:04:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-10 13:08:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-10 13:08:41]

Dry run: Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[msramek, 2017-04-10 13:20:08]

I addressed your comment, and did a number of other changes, since a recent CL
has introduced conflicting changes. PTAL!

(note that the red iOS bots are from the tree, not mine)

https://codereview.chromium.org/2802013002/diff/1/chrome/browser/browsing_dat...
File chrome/browser/browsing_data/browsing_data_remover.h (right):

https://codereview.chromium.org/2802013002/diff/1/chrome/browser/browsing_dat...
chrome/browser/browsing_data/browsing_data_remover.h:138: virtual void
OnBrowsingDataRemoverWouldComplete(
On 2017/04/07 15:14:17, Bernhard Bauer wrote:
> On 2017/04/07 10:58:42, msramek wrote:
> > On 2017/04/06 15:13:07, Bernhard Bauer wrote:
> > > You could consider replacing this with a Callback.
> > 
> > Note that we're in content/public, so everything needs to be pure virtual.
> > 
> > BrowsingDataRemoverImpl currently needs to call
> > inhibitor->OnBrowsingDataRemoverWouldComplete(). If we replace this with a
> > Callback, it will have to be exposed through something like GetCallback(),
and
> > we will call inhibitor->GetCallback().Run(). I think that's a bit of an
> unusual
> > interface.
> 
> No, what I meant was replacing the whole CompletionInhibitor with a
> base::Callback<void(BrowsingDataRemover*, const base::Closure&>. Replacing
> one-method interfaces with callbacks is a pretty common pattern in the content
> API.

Got it. Done. I changed the naming to WouldCompleteCallback, since that's how
the event called, and the naming pattern "SomethingDoer" generally implies a
class.

I left the BrowsingDataRemoverCompletionInhibitor class in tests utils though,
which serves as an adapter. There are two tests which depend on this class
outliving BrowsingDataRemover, so I had to make it use a weak pointer, and thus
expose GetWeakPtr() publicly from BrowsingDataRemover.

https://codereview.chromium.org/2802013002/diff/60001/chrome/browser/browsing...
File chrome/browser/browsing_data/browsing_data_remover_test_util.h (left):

https://codereview.chromium.org/2802013002/diff/60001/chrome/browser/browsing...
chrome/browser/browsing_data/browsing_data_remover_test_util.h:48:
BrowsingDataRemoverImpl* remover,
I removed this parameter. Since the inhibitor is no longer static, the caller
should know which remover has which callback. And if not, it can bind it back
in.

https://codereview.chromium.org/2802013002/diff/60001/chrome/browser/profiles...
File chrome/browser/profiles/profile_manager_browsertest.cc (right):

https://codereview.chromium.org/2802013002/diff/60001/chrome/browser/profiles...
chrome/browser/profiles/profile_manager_browsertest.cc:85: for (Profile* profile
: profile_manager->GetLoadedProfiles()) {
I rewrote this test to use BrowsingDataRemoverCompletionInhibitor as well. Since
the inhibition is no longer static, we need to call the appropriate method on
every profile.

I verified that meddling with the int counters above breaks the test, so doing
this for all GetLoadedProfiles() seems to be the right thing to do.

https://codereview.chromium.org/2802013002/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/profile_helper_browsertest.cc (right):

https://codereview.chromium.org/2802013002/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/profile_helper_browsertest.cc:191:
inhibitor.BlockUntilNearCompletion();
This test was just reinventing BlockUntilNearCompletion() and
ContinueToCompletion(), so I rewrote it.

[msramek, 2017-04-10 13:58:56]

The CQ bit was checked by msramek@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-10 13:59:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-10 14:09:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-10 14:09:04]

Dry run: This issue passed the CQ dry run.

[Bernhard Bauer, 2017-04-10 23:29:37]

https://codereview.chromium.org/2802013002/diff/60001/chrome/browser/browsing...
File chrome/browser/browsing_data/browsing_data_remover_test_util.cc (right):

https://codereview.chromium.org/2802013002/diff/60001/chrome/browser/browsing...
chrome/browser/browsing_data/browsing_data_remover_test_util.cc:39: // in some
tests.
So sometimes this class outlives the BrowsingDataRemover and sometimes the way
around? That's… really not ideal. What are those cases?

(If it _always_ outlived the BrowsingDataRemover, things would be easy again,
because we wouldn't have to reset the callback at all.)

https://codereview.chromium.org/2802013002/diff/60001/chrome/browser/profiles...
File chrome/browser/profiles/profile_manager_browsertest.cc (right):

https://codereview.chromium.org/2802013002/diff/60001/chrome/browser/profiles...
chrome/browser/profiles/profile_manager_browsertest.cc:88:
base::Bind(&MultipleProfileDeletionObserver::
Hm, this callback is always identical, right? Couldn't we just directly call
HandleBrowsingDataRemoverWouldComplete() from
CompletionInhibitor::OnBrowsingDataRemoverWouldComplete? And then do we still
need a list of inhibitors? IOW, couldn't we just iterate over the
BrowsingDataRemovers and set the WouldCompleteCallbackForTesting to the same
callback for each of them?

[msramek, 2017-04-11 13:06:02]

The CQ bit was checked by msramek@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-11 13:06:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[msramek, 2017-04-11 13:09:19]

Patchset #4 (id:80001) has been deleted

[commit-bot: I haz the power, 2017-04-11 13:10:02]

Dry run: Try jobs failed on following builders:
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[msramek, 2017-04-11 13:10:10]

The CQ bit was checked by msramek@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-11 13:10:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[msramek, 2017-04-11 13:10:55]

The CQ bit was checked by msramek@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-11 13:11:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[msramek, 2017-04-11 13:13:04]

https://codereview.chromium.org/2802013002/diff/60001/chrome/browser/browsing...
File chrome/browser/browsing_data/browsing_data_remover_test_util.cc (right):

https://codereview.chromium.org/2802013002/diff/60001/chrome/browser/browsing...
chrome/browser/browsing_data/browsing_data_remover_test_util.cc:39: // in some
tests.
On 2017/04/10 23:29:37, Bernhard Bauer wrote:
> So sometimes this class outlives the BrowsingDataRemover and sometimes the way
> around? That's… really not ideal. What are those cases?
> 
> (If it _always_ outlived the BrowsingDataRemover, things would be easy again,
> because we wouldn't have to reset the callback at all.)

The unittest contains a testcase specifically about the fact that nothing
crashes if Inhibitor outlives BrowsingDataRemover. My suspicion is that this is
also the reason why Inhibitor used to be static - so we wouldn't have to deal
with the weak pointer here. So this part looks intentional.

I was going to question whether we need that guarantee, and now someone started
using Inhibitor in profile deletion tests. So I guess we do need it.

However, I don't think we should *require* it. That would mean that someone who
does not destroy BrowsingDataRemover will now have to artificially maintain
Inhibitor alive after the test finishes, because BrowsingDataRemover is
long-lived (it's tied to a Profile). It should be fine to let Inhibitor go out
of scope at the end of a test, or destroy it even earlier, if you want
subsequent calls in your test to be un-inhibited.

Note also that this class is just an adapter over
SetWouldCompleteCallbackForTesting() to provide convenience in tests, so... it
should be convenient.

https://codereview.chromium.org/2802013002/diff/60001/chrome/browser/profiles...
File chrome/browser/profiles/profile_manager_browsertest.cc (right):

https://codereview.chromium.org/2802013002/diff/60001/chrome/browser/profiles...
chrome/browser/profiles/profile_manager_browsertest.cc:88:
base::Bind(&MultipleProfileDeletionObserver::
On 2017/04/10 23:29:37, Bernhard Bauer wrote:
> Hm, this callback is always identical, right? Couldn't we just directly call
> HandleBrowsingDataRemoverWouldComplete() from
> CompletionInhibitor::OnBrowsingDataRemoverWouldComplete? And then do we still
> need a list of inhibitors? IOW, couldn't we just iterate over the
> BrowsingDataRemovers and set the WouldCompleteCallbackForTesting to the same
> callback for each of them?

Done.

Yes, that's a good idea. I partially reverted this back to the original. I'm
setting the callback directly now (and not unsetting it later, as it doesn't
matter).

[commit-bot: I haz the power, 2017-04-11 13:51:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-11 13:51:50]

Dry run: This issue passed the CQ dry run.

[Bernhard Bauer, 2017-04-11 15:58:42]

https://codereview.chromium.org/2802013002/diff/60001/chrome/browser/browsing...
File chrome/browser/browsing_data/browsing_data_remover_test_util.cc (right):

https://codereview.chromium.org/2802013002/diff/60001/chrome/browser/browsing...
chrome/browser/browsing_data/browsing_data_remover_test_util.cc:39: // in some
tests.
On 2017/04/11 13:13:03, msramek wrote:
> On 2017/04/10 23:29:37, Bernhard Bauer wrote:
> > So sometimes this class outlives the BrowsingDataRemover and sometimes the
way
> > around? That's… really not ideal. What are those cases?
> > 
> > (If it _always_ outlived the BrowsingDataRemover, things would be easy
again,
> > because we wouldn't have to reset the callback at all.)
> 
> The unittest contains a testcase specifically about the fact that nothing
> crashes if Inhibitor outlives BrowsingDataRemover. My suspicion is that this
is
> also the reason why Inhibitor used to be static - so we wouldn't have to deal
> with the weak pointer here. So this part looks intentional.
> 
> I was going to question whether we need that guarantee, and now someone
started
> using Inhibitor in profile deletion tests. 

(Which BTW was just a workaround for the fact that it's not possible to observe
arbitrary browsing data deletions. I did CC you on that CL 😃)

> So I guess we do need it.
> 
> However, I don't think we should *require* it. That would mean that someone
who
> does not destroy BrowsingDataRemover will now have to artificially maintain
> Inhibitor alive after the test finishes, because BrowsingDataRemover is
> long-lived (it's tied to a Profile). It should be fine to let Inhibitor go out
> of scope at the end of a test, or destroy it even earlier, if you want
> subsequent calls in your test to be un-inhibited.

There is an easier way to achieve this: Store a regular pointer instead of weak
and add a Reset() method that clears the pointer – you know the exact
circumstances under which the BrowsingDataRemoverCompletionInhibitor outlives
the BrowsingDataRemover it's attached to.

[msramek, 2017-04-11 19:46:19]

https://codereview.chromium.org/2802013002/diff/60001/chrome/browser/browsing...
File chrome/browser/browsing_data/browsing_data_remover_test_util.cc (right):

https://codereview.chromium.org/2802013002/diff/60001/chrome/browser/browsing...
chrome/browser/browsing_data/browsing_data_remover_test_util.cc:39: // in some
tests.
On 2017/04/11 15:58:41, Bernhard Bauer wrote:
> On 2017/04/11 13:13:03, msramek wrote:
> > On 2017/04/10 23:29:37, Bernhard Bauer wrote:
> > > So sometimes this class outlives the BrowsingDataRemover and sometimes the
> way
> > > around? That's… really not ideal. What are those cases?
> > > 
> > > (If it _always_ outlived the BrowsingDataRemover, things would be easy
> again,
> > > because we wouldn't have to reset the callback at all.)
> > 
> > The unittest contains a testcase specifically about the fact that nothing
> > crashes if Inhibitor outlives BrowsingDataRemover. My suspicion is that this
> is
> > also the reason why Inhibitor used to be static - so we wouldn't have to
deal
> > with the weak pointer here. So this part looks intentional.
> > 
> > I was going to question whether we need that guarantee, and now someone
> started
> > using Inhibitor in profile deletion tests. 
> 
> (Which BTW was just a workaround for the fact that it's not possible to
observe
> arbitrary browsing data deletions. I did CC you on that CL 😃)

Huh. I remember seeing it now, but I guess I wasn't paying attention... :(

> 
> > So I guess we do need it.
> > 
> > However, I don't think we should *require* it. That would mean that someone
> who
> > does not destroy BrowsingDataRemover will now have to artificially maintain
> > Inhibitor alive after the test finishes, because BrowsingDataRemover is
> > long-lived (it's tied to a Profile). It should be fine to let Inhibitor go
out
> > of scope at the end of a test, or destroy it even earlier, if you want
> > subsequent calls in your test to be un-inhibited.
> 
> There is an easier way to achieve this: Store a regular pointer instead of
weak
> and add a Reset() method that clears the pointer – you know the exact
> circumstances under which the BrowsingDataRemoverCompletionInhibitor outlives
> the BrowsingDataRemover it's attached to.

It is less code in BrowsingDataRemoverImpl, but I think it's a messier interface
for tests.

If this class hypothetically became popular, I would expect that the majority of
tests would not destroy BrowsingDataRemover. But they would have to call Reset()
because of a minority that do. And the bigger problem is that if they forget,
they will segfault. And we can't DCHECK them:

~Inhibitor() {
  DCHECK(Reset() was called or remover is destroyed);
}

because we don't know if remover is destroyed without that weak pointer.

I know that the current approach adds yet another method to the public
interface, but I think GetWeakPtr() is a relatively reasonable thing to have
there.

[Bernhard Bauer, 2017-04-12 00:12:55]

https://codereview.chromium.org/2802013002/diff/60001/chrome/browser/browsing...
File chrome/browser/browsing_data/browsing_data_remover_test_util.cc (right):

https://codereview.chromium.org/2802013002/diff/60001/chrome/browser/browsing...
chrome/browser/browsing_data/browsing_data_remover_test_util.cc:39: // in some
tests.
On 2017/04/11 19:46:19, msramek wrote:
> On 2017/04/11 15:58:41, Bernhard Bauer wrote:
> > On 2017/04/11 13:13:03, msramek wrote:
> > > On 2017/04/10 23:29:37, Bernhard Bauer wrote:
> > > > So sometimes this class outlives the BrowsingDataRemover and sometimes
the
> > way
> > > > around? That's… really not ideal. What are those cases?
> > > > 
> > > > (If it _always_ outlived the BrowsingDataRemover, things would be easy
> > again,
> > > > because we wouldn't have to reset the callback at all.)
> > > 
> > > The unittest contains a testcase specifically about the fact that nothing
> > > crashes if Inhibitor outlives BrowsingDataRemover. My suspicion is that
this
> > is
> > > also the reason why Inhibitor used to be static - so we wouldn't have to
> deal
> > > with the weak pointer here. So this part looks intentional.
> > > 
> > > I was going to question whether we need that guarantee, and now someone
> > started
> > > using Inhibitor in profile deletion tests. 
> > 
> > (Which BTW was just a workaround for the fact that it's not possible to
> observe
> > arbitrary browsing data deletions. I did CC you on that CL 😃)
> 
> Huh. I remember seeing it now, but I guess I wasn't paying attention... :(
> 
> > 
> > > So I guess we do need it.
> > > 
> > > However, I don't think we should *require* it. That would mean that
someone
> > who
> > > does not destroy BrowsingDataRemover will now have to artificially
maintain
> > > Inhibitor alive after the test finishes, because BrowsingDataRemover is
> > > long-lived (it's tied to a Profile). It should be fine to let Inhibitor go
> out
> > > of scope at the end of a test, or destroy it even earlier, if you want
> > > subsequent calls in your test to be un-inhibited.
> > 
> > There is an easier way to achieve this: Store a regular pointer instead of
> weak
> > and add a Reset() method that clears the pointer – you know the exact
> > circumstances under which the BrowsingDataRemoverCompletionInhibitor
outlives
> > the BrowsingDataRemover it's attached to.
> 
> It is less code in BrowsingDataRemoverImpl, but I think it's a messier
interface
> for tests.
> 
> If this class hypothetically became popular, I would expect that the majority
of
> tests would not destroy BrowsingDataRemover. But they would have to call
Reset()
> because of a minority that do. 

No, only in the instances where the BrowsingDataRemover is destroyed before the
inhibitor. By default the BrowsingDataRemover would outlive the inhibitor, and
if it doesn't, whoever destroys it needs to Reset() the inhibitor.

> And the bigger problem is that if they forget,
> they will segfault. And we can't DCHECK them:
> 
> ~Inhibitor() {
>   DCHECK(Reset() was called or remover is destroyed);
> }
> 
> because we don't know if remover is destroyed without that weak pointer.

Sure, but that's generally the deal with pointer ownership. Lifetime semantics
are a very important part of the contract of a class, and it's reasonable to
expect clients to respect them, and think about them whenever handling raw
pointers.

> I know that the current approach adds yet another method to the public
> interface, but I think GetWeakPtr() is a relatively reasonable thing to have
> there.

I disagree. Exposing a weak pointer to an object outside of the class is
actually discouraged, precisely because it's usually a smell for sloppy lifetime
management.

[msramek, 2017-04-12 12:01:08]

The CQ bit was checked by msramek@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-12 12:01:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[msramek, 2017-04-12 12:16:15]

The CQ bit was checked by msramek@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-12 12:16:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[msramek, 2017-04-12 12:16:33]

Patchset #5 (id:120001) has been deleted

[msramek, 2017-04-12 12:18:35]

https://codereview.chromium.org/2802013002/diff/60001/chrome/browser/browsing...
File chrome/browser/browsing_data/browsing_data_remover_test_util.cc (right):

https://codereview.chromium.org/2802013002/diff/60001/chrome/browser/browsing...
chrome/browser/browsing_data/browsing_data_remover_test_util.cc:39: // in some
tests.
On 2017/04/12 00:12:55, Bernhard Bauer wrote:
> On 2017/04/11 19:46:19, msramek wrote:
> > On 2017/04/11 15:58:41, Bernhard Bauer wrote:
> > > On 2017/04/11 13:13:03, msramek wrote:
> > > > On 2017/04/10 23:29:37, Bernhard Bauer wrote:
> > > > > So sometimes this class outlives the BrowsingDataRemover and sometimes
> the
> > > way
> > > > > around? That's… really not ideal. What are those cases?
> > > > > 
> > > > > (If it _always_ outlived the BrowsingDataRemover, things would be easy
> > > again,
> > > > > because we wouldn't have to reset the callback at all.)
> > > > 
> > > > The unittest contains a testcase specifically about the fact that
nothing
> > > > crashes if Inhibitor outlives BrowsingDataRemover. My suspicion is that
> this
> > > is
> > > > also the reason why Inhibitor used to be static - so we wouldn't have to
> > deal
> > > > with the weak pointer here. So this part looks intentional.
> > > > 
> > > > I was going to question whether we need that guarantee, and now someone
> > > started
> > > > using Inhibitor in profile deletion tests. 
> > > 
> > > (Which BTW was just a workaround for the fact that it's not possible to
> > observe
> > > arbitrary browsing data deletions. I did CC you on that CL 😃)
> > 
> > Huh. I remember seeing it now, but I guess I wasn't paying attention... :(
> > 
> > > 
> > > > So I guess we do need it.
> > > > 
> > > > However, I don't think we should *require* it. That would mean that
> someone
> > > who
> > > > does not destroy BrowsingDataRemover will now have to artificially
> maintain
> > > > Inhibitor alive after the test finishes, because BrowsingDataRemover is
> > > > long-lived (it's tied to a Profile). It should be fine to let Inhibitor
go
> > out
> > > > of scope at the end of a test, or destroy it even earlier, if you want
> > > > subsequent calls in your test to be un-inhibited.
> > > 
> > > There is an easier way to achieve this: Store a regular pointer instead of
> > weak
> > > and add a Reset() method that clears the pointer – you know the exact
> > > circumstances under which the BrowsingDataRemoverCompletionInhibitor
> outlives
> > > the BrowsingDataRemover it's attached to.
> > 
> > It is less code in BrowsingDataRemoverImpl, but I think it's a messier
> interface
> > for tests.
> > 
> > If this class hypothetically became popular, I would expect that the
majority
> of
> > tests would not destroy BrowsingDataRemover. But they would have to call
> Reset()
> > because of a minority that do. 
> 
> No, only in the instances where the BrowsingDataRemover is destroyed before
the
> inhibitor. By default the BrowsingDataRemover would outlive the inhibitor, and
> if it doesn't, whoever destroys it needs to Reset() the inhibitor.

I see. I though you meant the other way around, because we had the "Finishing
after shutdown shouldn't break anything." part in the test. Which we now change
to "Don't use inhibitor after the shutdown."

> 
> > And the bigger problem is that if they forget,
> > they will segfault. And we can't DCHECK them:
> > 
> > ~Inhibitor() {
> >   DCHECK(Reset() was called or remover is destroyed);
> > }
> > 
> > because we don't know if remover is destroyed without that weak pointer.
> 
> Sure, but that's generally the deal with pointer ownership. Lifetime semantics
> are a very important part of the contract of a class, and it's reasonable to
> expect clients to respect them, and think about them whenever handling raw
> pointers.
> 
> > I know that the current approach adds yet another method to the public
> > interface, but I think GetWeakPtr() is a relatively reasonable thing to have
> > there.
> 
> I disagree. Exposing a weak pointer to an object outside of the class is
> actually discouraged, precisely because it's usually a smell for sloppy
lifetime
> management.

Fair enough :) I removed the weak pointer, added Reset(), and some DCHECKs.

[commit-bot: I haz the power, 2017-04-12 12:59:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-12 12:59:44]

Dry run: This issue passed the CQ dry run.

[Bernhard Bauer, 2017-04-12 16:35:34]

Thanks! LGTM with a suggestion:

https://codereview.chromium.org/2802013002/diff/100001/chrome/browser/browsin...
File chrome/browser/browsing_data/browsing_data_remover_impl_unittest.cc
(right):

https://codereview.chromium.org/2802013002/diff/100001/chrome/browser/browsin...
chrome/browser/browsing_data/browsing_data_remover_impl_unittest.cc:1494:
completion_inhibitor.ContinueToCompletion();
You could still keep these if you remove the DCHECK(remover_) in
BrowsingDataRemoverCompletionInhibitor::ContinueToCompletion, right? That might
not be a bad test – verifying that the completion callback can be called even
after the BrowsingDataRemover has been destroyed.

(If you don't want to keep this, turns out that you don't use the
|completion_inhibitor| after you reset it, so you could just destroy it at that
point! Then you wouldn't even need an explicit Reset() method anymore.)

[msramek, 2017-04-12 19:13:48]

The CQ bit was checked by msramek@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-12 19:15:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[msramek, 2017-04-12 19:16:55]

Thanks a lot for your patience, Bernhard :)

https://codereview.chromium.org/2802013002/diff/100001/chrome/browser/browsin...
File chrome/browser/browsing_data/browsing_data_remover_impl_unittest.cc
(right):

https://codereview.chromium.org/2802013002/diff/100001/chrome/browser/browsin...
chrome/browser/browsing_data/browsing_data_remover_impl_unittest.cc:1494:
completion_inhibitor.ContinueToCompletion();
On 2017/04/12 16:35:34, Bernhard Bauer wrote:
> You could still keep these if you remove the DCHECK(remover_) in
> BrowsingDataRemoverCompletionInhibitor::ContinueToCompletion, right? That
might
> not be a bad test – verifying that the completion callback can be called even
> after the BrowsingDataRemover has been destroyed.
> 
> (If you don't want to keep this, turns out that you don't use the
> |completion_inhibitor| after you reset it, so you could just destroy it at
that
> point! Then you wouldn't even need an explicit Reset() method anymore.)

Sigh... I forgot that |continue_to_completion_callback_| still has a weak
pointer bound in, so it still can be called. Reverted this part.

[msramek, 2017-04-12 19:23:05]

msramek@chromium.org changed reviewers:
+ mlerman@chromium.org

[msramek, 2017-04-12 19:23:05]

+Mike PTAL at profiles/.

I'm changing how BrowsingDataRemoverCompletionInhibitor works, and as a result
I'm updating the tests you recently reviewed in
https://codereview.chromium.org/2793913002.

[msramek, 2017-04-12 19:26:43]

Description was changed from

==========
Move BrowsingDataRemoverImpl:: CompletionInhibitor to the public interface

The CompletionInhibitor testing class used to live in BrowsingDataRemoverImpl
to reduce its visibility. However, the class is used both in chrome/browser/ui/
tests and BrowsingDataRemoverImplTest which will move to content/. Therefore,
it must live in the public BrowsingDataRemover.

Note that only exposing this testing class through content/public/test would
not help, as content/public/test must not depend on content/ (non-public).

On the way, this CL also resolves a TODO to make CompletionInhibitor non-static.

BUG=483528, 668114
==========

to

==========
Move BrowsingDataRemoverImpl:: CompletionInhibitor to the public interface

The CompletionInhibitor interface is removed in favor of a simpler
SetWouldCompleteCallbackForTesting() method defined in BrowsingDataRemover.
Its implementation BrowsingDataRemoverCompletionInhibitor is updated to use
the new interface.

This is necessary due to the fact that CompletionInhibitor was used both
in chrome/ and in BrowsingDataRemoverImplTest which will move to content/,
and thus its definition in BrowsingDataRemoverImpl (which will move to
non-public content/) would not be visible in chrome/.

On the way, this CL also resolves a TODO to make CompletionInhibitor non-static.

BUG=483528, 668114
==========

[Mike Lerman, 2017-04-12 19:28:14]

Happy to rely on Bernhard for profile removal stuff.

profiles LGTM

[msramek, 2017-04-12 19:33:39]

Thanks!

Landing now.

[msramek, 2017-04-12 19:33:46]

The CQ bit was unchecked by msramek@chromium.org

[msramek, 2017-04-12 19:33:52]

The CQ bit was checked by msramek@chromium.org

[msramek, 2017-04-12 19:33:52]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org
Link to the patchset: https://codereview.chromium.org/2802013002/#ps160001
(title: "Finishing after shutdown is still OK.")

[commit-bot: I haz the power, 2017-04-12 19:35:39]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-12 19:45:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-12 19:45:41]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[msramek, 2017-04-12 19:48:30]

Description was changed from

==========
Move BrowsingDataRemoverImpl:: CompletionInhibitor to the public interface

The CompletionInhibitor interface is removed in favor of a simpler
SetWouldCompleteCallbackForTesting() method defined in BrowsingDataRemover.
Its implementation BrowsingDataRemoverCompletionInhibitor is updated to use
the new interface.

This is necessary due to the fact that CompletionInhibitor was used both
in chrome/ and in BrowsingDataRemoverImplTest which will move to content/,
and thus its definition in BrowsingDataRemoverImpl (which will move to
non-public content/) would not be visible in chrome/.

On the way, this CL also resolves a TODO to make CompletionInhibitor non-static.

BUG=483528, 668114
==========

to

==========
Move BrowsingDataRemoverImpl:: CompletionInhibitor to the public interface

The CompletionInhibitor interface is removed in favor of a simpler
SetWouldCompleteCallbackForTesting() method defined in BrowsingDataRemover.
Its implementation BrowsingDataRemoverCompletionInhibitor is updated to use
the new interface.

This is necessary due to the fact that CompletionInhibitor was used both
in chrome/ and in BrowsingDataRemoverImplTest which will move to content/,
and thus its definition in BrowsingDataRemoverImpl (which will move to
non-public content/) would not be visible in chrome/.

On the way, this CL also resolves a TODO to make CompletionInhibitor non-static.

TBR=dbeam@chromium.org
BUG=483528, 668114
==========

[msramek, 2017-04-12 19:50:33]

msramek@chromium.org changed reviewers:
+ dbeam@chromium.org

[msramek, 2017-04-12 19:50:34]

Sorry, forgot that options/OWNERS has noparent set.

+Dan, I take the liberty to TBR you, since I only updated a constructor to the
new signature.

[msramek, 2017-04-12 19:50:45]

The CQ bit was checked by msramek@chromium.org

[commit-bot: I haz the power, 2017-04-12 19:51:47]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-12 23:16:12]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1492026645344940,
"parent_rev": "08666168da6fb4eef40bea6fdf1bf737c193b544", "commit_rev":
"bb8dc5c563daf902f09b6f0ab6079d3ec66a089f"}

[commit-bot: I haz the power, 2017-04-12 23:17:06]

Description was changed from

==========
Move BrowsingDataRemoverImpl:: CompletionInhibitor to the public interface

The CompletionInhibitor interface is removed in favor of a simpler
SetWouldCompleteCallbackForTesting() method defined in BrowsingDataRemover.
Its implementation BrowsingDataRemoverCompletionInhibitor is updated to use
the new interface.

This is necessary due to the fact that CompletionInhibitor was used both
in chrome/ and in BrowsingDataRemoverImplTest which will move to content/,
and thus its definition in BrowsingDataRemoverImpl (which will move to
non-public content/) would not be visible in chrome/.

On the way, this CL also resolves a TODO to make CompletionInhibitor non-static.

TBR=dbeam@chromium.org
BUG=483528, 668114
==========

to

==========
Move BrowsingDataRemoverImpl:: CompletionInhibitor to the public interface

The CompletionInhibitor interface is removed in favor of a simpler
SetWouldCompleteCallbackForTesting() method defined in BrowsingDataRemover.
Its implementation BrowsingDataRemoverCompletionInhibitor is updated to use
the new interface.

This is necessary due to the fact that CompletionInhibitor was used both
in chrome/ and in BrowsingDataRemoverImplTest which will move to content/,
and thus its definition in BrowsingDataRemoverImpl (which will move to
non-public content/) would not be visible in chrome/.

On the way, this CL also resolves a TODO to make CompletionInhibitor non-static.

TBR=dbeam@chromium.org
BUG=483528, 668114

Review-Url: https://codereview.chromium.org/2802013002
Cr-Commit-Position: refs/heads/master@{#464199}
Committed:
https://chromium.googlesource.com/chromium/src/+/bb8dc5c563daf902f09b6f0ab607...
==========

[commit-bot: I haz the power, 2017-04-12 23:17:07]

Committed patchset #6 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/bb8dc5c563daf902f09b6f0ab607...