Move BrowsingDataRemoverImpl:: CompletionInhibitor to the public interface

chrome/browser/browsing_data/browsing_data_remover.h

Index: chrome/browser/browsing_data/browsing_data_remover.h
diff --git a/chrome/browser/browsing_data/browsing_data_remover.h b/chrome/browser/browsing_data/browsing_data_remover.h
index 4edae603f8cec0bf941a4b7d46dc32c73dbfe4fd..dae8f2a50550e79b47e876a2c491c14138561e7a 100644
--- a/chrome/browser/browsing_data/browsing_data_remover.h
+++ b/chrome/browser/browsing_data/browsing_data_remover.h
@@ -127,6 +127,22 @@ class BrowsingDataRemover : public KeyedService {
     virtual ~Observer() {}
   };
 
+  // The completion inhibitor can artificially delay completion of the browsing
+  // data removal process. It is used during testing to simulate scenarios in
+  // which the deletion stalls or takes a very long time.
+  class CompletionInhibitor {
+   public:
+    // Invoked when a |remover| is just about to complete clearing browser data,
+    // and will be prevented from completing until after the callback
+    // |continue_to_completion| is run.
+    virtual void OnBrowsingDataRemoverWouldComplete(

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

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]

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.
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:17]

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 13:20:08]

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.

+        BrowsingDataRemover* remover,
+        const base::Closure& continue_to_completion) = 0;
+
+   protected:
+    virtual ~CompletionInhibitor() {}
+  };
+
   // Called by the embedder to provide the delegate that will take care of
   // deleting embedder-specific data.
   virtual void SetEmbedderDelegate(
@@ -179,6 +195,12 @@ class BrowsingDataRemover : public KeyedService {
   virtual void AddObserver(Observer* observer) = 0;
   virtual void RemoveObserver(Observer* observer) = 0;
 
+  // Sets a CompletionInhibitor, which will be notified each time an instance is
+  // about to complete a browsing data removal process, and will be able to
+  // artificially delay the completion.
+  virtual void SetCompletionInhibitorForTesting(
+      CompletionInhibitor* inhibitor) = 0;
+
   // Parameters of the last call are exposed to be used by tests. Removal and
   // origin type masks equal to -1 mean that no removal has ever been executed.
   // TODO(msramek): If other consumers than tests are interested in this,