Refactor BrowsingDataRemover to mock out WebappRegistry.

chrome/browser/browsing_data/browsing_data_remover_unittest.cc

 // Copyright (c) 2012 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.
 
 #include "chrome/browser/browsing_data/browsing_data_remover.h"
 
 #include <stddef.h>
 #include <stdint.h>
 
 #include <set>
@@ skipping 58 matching lines @@
 #include "net/ssl/channel_id_store.h"
 #include "net/ssl/ssl_client_cert_type.h"
 #include "net/url_request/url_request_context.h"
 #include "net/url_request/url_request_context_getter.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/skia/include/core/SkBitmap.h"
 #include "ui/gfx/favicon_size.h"
 #include "url/origin.h"
 
+#if BUILDFLAG(ANDROID_JAVA_UI)
+#include "chrome/browser/android/webapps/webapp_registry.h"
+#endif
+
 #if defined(OS_CHROMEOS)
 #include "chrome/browser/chromeos/login/users/mock_user_manager.h"
 #include "chrome/browser/chromeos/login/users/scoped_user_manager_enabler.h"
 #include "chrome/browser/chromeos/settings/cros_settings.h"
 #include "chrome/browser/chromeos/settings/device_settings_service.h"
 #include "chromeos/dbus/dbus_thread_manager.h"
 #include "chromeos/dbus/mock_cryptohome_client.h"
 #include "components/signin/core/account_id/account_id.h"
 #endif
 
@@ skipping 164 matching lines @@
  private:
   void AsyncRunCallback(const base::Closure& callback) {
     callback.Run();
   }
 
   StoragePartitionRemovalData storage_partition_removal_data_;
 
   DISALLOW_COPY_AND_ASSIGN(TestStoragePartition);
 };
 
+#if BUILDFLAG(ANDROID_JAVA_UI)
+class TestWebappRegistry : public WebappRegistry {
+ public:
+  TestWebappRegistry() : WebappRegistry() { }
+
+  void UnregisterWebapps(const base::Closure& callback) override {
+    // Mocks out a JNI call and runs the callback.
+    callback.Run();

[gone, 2016/03/10 00:18:06]

Should this be a delayed task, like with the real call?  This will immediately
run the callback on the same thread.

[dominickn, 2016/03/10 00:30:45]

My reasoning was that I wanted to block as little as possible in C++ unit tests.
This mocking should only happen in circumstances where the Java calls won't
work. Do you think this is valid?

[gone, 2016/03/10 00:45:07]

Although the Java calls won't work, I'd normally argue that getting your mocked
class to work as similarly as possible could catch different behavior.  If, for
example, the real code did something non-trivial immediately when the mock
callback was called, this could make things occur in a different order than in
the real world.

In this case, however, I'm not sure there's much benefit with the current code
path because the BrowsingDataRemover is already waiting for other things
asynchronously.

So, your call.  I can swing either way.

+  }
+};
+#endif
+
 // Custom matcher to test the equivalence of two URL filters. Since those are
 // blackbox predicates, we can only approximate the equivalence by testing
 // whether the filter give the same answer for several URLs. This is currently
 // good enough for our testing purposes, to distinguish whitelists
 // and blacklists, empty and non-empty filters and such.
 // TODO(msramek): BrowsingDataRemover and some of its backends support URL
 // filters, but its constructor currently only takes a single URL and constructs
 // its own url filter. If an url filter was directly passed to
 // BrowsingDataRemover (what should eventually be the case), we can use the same
 // instance in the test as well, and thus simply test base::Callback::Equals()
@@ skipping 662 matching lines @@
 
 class BrowsingDataRemoverTest : public testing::Test {
  public:
   BrowsingDataRemoverTest()
       : profile_(new TestingProfile()),
         clear_domain_reliability_tester_(GetProfile()) {
     callback_subscription_ =
         BrowsingDataRemover::RegisterOnBrowsingDataRemovedCallback(
             base::Bind(&BrowsingDataRemoverTest::NotifyWithDetails,
                        base::Unretained(this)));
+
+#if BUILDFLAG(ANDROID_JAVA_UI)
+    BrowsingDataRemover* remover =
+        BrowsingDataRemoverFactory::GetForBrowserContext(profile_.get());
+    remover->OverrideWebappRegistryForTesting(
+        scoped_ptr<WebappRegistry>(new TestWebappRegistry()));
+#endif
   }
 
   ~BrowsingDataRemoverTest() override {}
 
   void TearDown() override {
 #if defined(ENABLE_EXTENSIONS)
     mock_policy_ = nullptr;
 #endif
 
     // TestingProfile contains a DOMStorageContext.  BrowserContext's destructor
@@ skipping 1321 matching lines @@
   EXPECT_CALL(*tester.store(), RemoveLoginsByURLAndTimeImpl(_, _, _))
       .WillOnce(Return(password_manager::PasswordStoreChangeList()));
   EXPECT_CALL(*tester.store(), DisableAutoSignInForAllLoginsImpl())
       .WillOnce(Return(password_manager::PasswordStoreChangeList()));
 
   BlockUntilBrowsingDataRemoved(BrowsingDataRemover::EVERYTHING,
                                 BrowsingDataRemover::REMOVE_COOKIES |
                                     BrowsingDataRemover::REMOVE_PASSWORDS,
                                 false);
 }