Move BrowsingDataRemoverImpl:: CompletionInhibitor to the public interface

chrome/browser/browsing_data/browsing_data_remover_test_util.cc

Index: chrome/browser/browsing_data/browsing_data_remover_test_util.cc
diff --git a/chrome/browser/browsing_data/browsing_data_remover_test_util.cc b/chrome/browser/browsing_data/browsing_data_remover_test_util.cc
index 9bdc5a399774bed2d039ade83657402c2fa234a9..676d539887956eb84eee7a2a9e1d26694720f829 100644
--- a/chrome/browser/browsing_data/browsing_data_remover_test_util.cc
+++ b/chrome/browser/browsing_data/browsing_data_remover_test_util.cc
@@ -22,14 +22,25 @@ void BrowsingDataRemoverCompletionObserver::OnBrowsingDataRemoverDone() {
   message_loop_runner_->Quit();
 }
 
-BrowsingDataRemoverCompletionInhibitor::BrowsingDataRemoverCompletionInhibitor()
+BrowsingDataRemoverCompletionInhibitor::BrowsingDataRemoverCompletionInhibitor(
+    BrowsingDataRemover* remover)
     : message_loop_runner_(new content::MessageLoopRunner) {
-  BrowsingDataRemoverImpl::set_completion_inhibitor_for_testing(this);
+  DCHECK(remover);
+  remover_ = remover->GetWeakPtr();
+  remover_->SetWouldCompleteCallbackForTesting(
+      base::Bind(&BrowsingDataRemoverCompletionInhibitor::
+                     OnBrowsingDataRemoverWouldComplete,
+                 base::Unretained(this)));
 }
 
 BrowsingDataRemoverCompletionInhibitor::
     ~BrowsingDataRemoverCompletionInhibitor() {
-  BrowsingDataRemoverImpl::set_completion_inhibitor_for_testing(nullptr);
+  // BrowsingDataRemoverCompletionInhibitor might outlive BrowsingDataRemover
+  // in some tests.

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

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.)

[msramek, 2017/04/11 13:13:03]

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.

[Bernhard Bauer, 2017/04/11 15:58:41]

(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 😃)
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]

Huh. I remember seeing it now, but I guess I wasn't paying attention... :(
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]

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.
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 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:18:35]

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."
Fair enough :) I removed the weak pointer, added Reset(), and some DCHECKs.

+  if (!remover_)
+    return;
+  remover_->SetWouldCompleteCallbackForTesting(
+      base::Callback<void(const base::Closure&)>());
 }
 
 void BrowsingDataRemoverCompletionInhibitor::BlockUntilNearCompletion() {
@@ -44,7 +55,6 @@ void BrowsingDataRemoverCompletionInhibitor::ContinueToCompletion() {
 }
 
 void BrowsingDataRemoverCompletionInhibitor::OnBrowsingDataRemoverWouldComplete(
-    BrowsingDataRemoverImpl* remover,
     const base::Closure& continue_to_completion) {
   DCHECK(continue_to_completion_callback_.is_null());
   continue_to_completion_callback_ = continue_to_completion;