Move BrowsingDataRemoverImpl:: CompletionInhibitor to the public interface

chrome/browser/browsing_data/browsing_data_remover.h

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef CHROME_BROWSER_BROWSING_DATA_BROWSING_DATA_REMOVER_H_
 #define CHROME_BROWSER_BROWSING_DATA_BROWSING_DATA_REMOVER_H_
 
 #include <memory>
 #include "base/callback_forward.h"
 #include "base/time/time.h"
@@ skipping 109 matching lines @@
   class Observer {
    public:
     // Called when a removal task is finished. Note that every removal task can
     // only have one observer attached to it, and only that one is called.
     virtual void OnBrowsingDataRemoverDone() = 0;
 
    protected:
     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(
       std::unique_ptr<BrowsingDataRemoverDelegate> embedder_delegate) = 0;
   virtual BrowsingDataRemoverDelegate* GetEmbedderDelegate() const = 0;
 
   // Determines whether |origin| matches the |origin_type_mask| according to
   // the |special_storage_policy|.
   virtual bool DoesOriginMatchMask(
       int origin_type_mask,
@@ skipping 32 matching lines @@
       const base::Time& delete_end,
       int remove_mask,
       int origin_type_mask,
       std::unique_ptr<content::BrowsingDataFilterBuilder> filter_builder,
       Observer* observer) = 0;
 
   // Observers.
   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,
   // consider returning them in OnBrowsingDataRemoverDone() callback. If not,
   // consider simplifying this interface by removing these methods and changing
   // the tests to record the parameters using GMock instead.
   virtual const base::Time& GetLastUsedBeginTime() = 0;
   virtual const base::Time& GetLastUsedEndTime() = 0;
   virtual int GetLastUsedRemovalMask() = 0;
   virtual int GetLastUsedOriginTypeMask() = 0;
 };
 
 #endif // CHROME_BROWSER_BROWSING_DATA_BROWSING_DATA_REMOVER_H_