Move BrowsingDataRemover to content/

content/public/browser/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_
+#ifndef CONTENT_PUBLIC_BROWSER_BROWSING_DATA_REMOVER_H_
+#define CONTENT_PUBLIC_BROWSER_BROWSING_DATA_REMOVER_H_
 
 #include <memory>
+
 #include "base/callback_forward.h"
 #include "base/memory/weak_ptr.h"
 #include "base/time/time.h"
 #include "build/build_config.h"
-#include "chrome/browser/browsing_data/browsing_data_remover_delegate.h"
-#include "components/keyed_service/core/keyed_service.h"
+#include "content/common/content_export.h"
+#include "content/public/browser/browsing_data_remover_delegate.h"

[jam, 2017/04/20 00:39:48]

nit: just forward declare

[msramek, 2017/04/21 15:22:12]

Done. Also added a few more forward declarations that are now necessary.

 
 namespace content {
+
 class BrowsingDataFilterBuilder;
-}
 
 ////////////////////////////////////////////////////////////////////////////////
 // BrowsingDataRemover is responsible for removing data related to browsing:
 // visits in url database, downloads, cookies ...
 //
 //  USAGE:
 //
 //  0. Instantiation.
 //
-//       BrowsingDataRemover* remover =
-//           BrowsingDataRemoverFactory::GetForBrowserContext(browser_context);
+//       content::BrowsingDataRemover* remover =
+//           content::BrowserContext::GetBrowsingDataRemover(browser_context);
 //
 //  1. No observer.
 //
 //       remover->Remove(base::Time(), base::Time::Max(), REMOVE_COOKIES, ALL);
 //
 //  2. Using an observer to report when one's own removal task is finished.
 //
-//       class CookiesDeleter : public BrowsingDataRemover::Observer {
+//       class CookiesDeleter : public content::BrowsingDataRemover::Observer {
 //         CookiesDeleter() { remover->AddObserver(this); }
 //         ~CookiesDeleter() { remover->RemoveObserver(this); }
 //
 //         void DeleteCookies() {
 //           remover->RemoveAndReply(base::Time(), base::Time::Max(),
 //                                   REMOVE_COOKIES, ALL, this);
 //         }
 //
 //         void OnBrowsingDataRemoverDone() {
 //           LOG(INFO) << "Cookies were deleted.";
 //         }
 //       }
 //
 ////////////////////////////////////////////////////////////////////////////////
 //
 // TODO(crbug.com/668114): BrowsingDataRemover does not currently support plugin
 // data deletion. Use PluginDataRemover instead.
-class BrowsingDataRemover : public KeyedService {
+class CONTENT_EXPORT BrowsingDataRemover {

[jam, 2017/04/20 00:39:48]

nit: not needed on interfaces, so remove along with the include

[msramek, 2017/04/21 15:22:12]

Done.

  public:
   // Mask used for Remove.
   enum DataType {
     // Storage datatypes.
     DATA_TYPE_APP_CACHE = 1 << 0,
     DATA_TYPE_FILE_SYSTEMS = 1 << 1,
     DATA_TYPE_INDEXED_DB = 1 << 2,
     DATA_TYPE_LOCAL_STORAGE = 1 << 3,
     DATA_TYPE_WEB_SQL = 1 << 4,
     DATA_TYPE_SERVICE_WORKERS = 1 << 5,
@@ skipping 45 matching lines @@
   // reorder the entries, as this enum is passed to UMA.
   enum CookieOrCacheDeletionChoice {
     NEITHER_COOKIES_NOR_CACHE,
     ONLY_COOKIES,
     ONLY_CACHE,
     BOTH_COOKIES_AND_CACHE,
     MAX_CHOICE_VALUE
   };
 
   // Observer is notified when its own removal task is done.
   class Observer {

[jam, 2017/04/20 00:39:48]

for this class and the next one, can we just use callbacks instead of adding two
single-method interfaces nested in this interface?

we used to have single-method interfaces before bind came along, but now it's
much more verbose (and one has to worry about lifetime)

[msramek, 2017/04/21 15:22:12]

The second class (CompletionInhibitor) can be removed. I have already done the
change you suggest in a separate CL, then rebased this one on top of it, and it
apparently accidentally sneaked the class back in while addressing merge
conflicts. Removed.

Regarding the first class (Observer), I was doing some refactoring in the past
where I considered replacing it with a callback, but it seemed to me that
keeping it as a class was better. Here's the discussion:
https://docs.google.com/document/d/1oIbmZJlq5m5FvHcN7trCpp0h4mtydA0UfZasxEIe3...

I'm happy to hear your thoughts on this. However, even if we did end up
converting it to a callback, I would still prefer having a follow-up CL, since
it also requires updating numerous callsites.

[jam, 2017/04/21 22:58:30]

Thanks, I read the discussion above. The major point seems to be adding a weak
pointer factory to the callsites. I went through the existing callsites, and of
the 10 or so, almost all either had a weakptrfactory already or were refcounted.
So I think it'll be simpler to use a callback. However I agree with you to not
bloat this further, so doing it in a followup is fine.

[msramek, 2017/04/25 01:29:09]

Alright! I'll send you a followup to convert them to callbacks soon.

    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() {}
   };
 
-  // Called by the embedder to provide the delegate that will take care of
-  // deleting embedder-specific data.
+  // 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(
+        BrowsingDataRemover* remover,
+        const base::Closure& continue_to_completion) = 0;
+
+   protected:
+    virtual ~CompletionInhibitor() {}
+  };
+
+  // A delegate that will take care of deleting embedder-specific data.
   virtual void SetEmbedderDelegate(
-      std::unique_ptr<BrowsingDataRemoverDelegate> embedder_delegate) = 0;
+      BrowsingDataRemoverDelegate* embedder_delegate) = 0;
   virtual BrowsingDataRemoverDelegate* GetEmbedderDelegate() const = 0;

[jam, 2017/04/20 00:39:48]

it's a bit odd that the embedder sets the delegate, and then needs to ask for it
again? i.e. why does it have to reach back to content to know about its class?

[msramek, 2017/04/21 15:22:12]

We don't, and removing it really made the code a bit cleaner :)

 
   // Determines whether |origin| matches the |origin_type_mask| according to
   // the |special_storage_policy|.
   virtual bool DoesOriginMatchMask(
       int origin_type_mask,
       const GURL& origin,
       storage::SpecialStoragePolicy* special_storage_policy) const = 0;
 
   // Removes browsing data within the given |time_range|, with datatypes being
   // specified by |remove_mask| and origin types by |origin_type_mask|.
@@ skipping 11 matching lines @@
                               Observer* observer) = 0;
 
   // Like Remove(), but in case of URL-keyed only removes data whose URL match
   // |filter_builder| (e.g. are on certain origin or domain).
   // RemoveWithFilter() currently only works with FILTERABLE_DATATYPES.
   virtual void RemoveWithFilter(
       const base::Time& delete_begin,
       const base::Time& delete_end,
       int remove_mask,
       int origin_type_mask,
-      std::unique_ptr<content::BrowsingDataFilterBuilder> filter_builder) = 0;
+      std::unique_ptr<BrowsingDataFilterBuilder> filter_builder) = 0;
 
   // A version of the above that in addition informs the |observer| when the
   // removal task is finished.
   virtual void RemoveWithFilterAndReply(
       const base::Time& delete_begin,
       const base::Time& delete_end,
       int remove_mask,
       int origin_type_mask,
-      std::unique_ptr<content::BrowsingDataFilterBuilder> filter_builder,
+      std::unique_ptr<BrowsingDataFilterBuilder> filter_builder,
       Observer* observer) = 0;
 
   // Observers.
   virtual void AddObserver(Observer* observer) = 0;
   virtual void RemoveObserver(Observer* observer) = 0;
 
   // A |callback| that will be called just before a deletion task is completed
   // and observers are notified. The receiver must respond by calling
   // |continue_to_completion| to finish the task. Used in tests to artificially
   // prolong execution.
   virtual void SetWouldCompleteCallbackForTesting(
       const base::Callback<void(const base::Closure& continue_to_completion)>&
           callback) = 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_
+}  // namespace content
+
+#endif  // CONTENT_PUBLIC_BROWSER_BROWSING_DATA_REMOVER_H_