[Media Router] Factor out calls to EventPageRequestManager out of...

[Media Router] Factor out calls to EventPageRequestManager out of MediaRouterMojoImpl into its own class

On desktop, the calls to the MRP are queued with EventPageRequestManager until
the Mojo connections with the extension are established. This CL factors out the
calls to EventPageRequestManager into MediaRouterDesktop, a subclass of
MediaRouterMojoImpl, so that MediaRouterMojoImpl is not aware of extension-
related details.

Changes to unit tests:
- Move tests from MRMojoImplTest to MRMojoTest so that they can shared with
  MRDesktopTest
- Merge MRMojoExtensionTest into MRDesktopTest

Bug: 737320
Change-Id: I8271932b0418af2490a2bbea49e444120d9e2aa6
Reviewed-on: https://chromium-review.googlesource.com/572501Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Derek Cheng <imcheng@chromium.org>
Commit-Queue: Takumi Fujimoto <takumif@chromium.org>
Cr-Commit-Position: refs/heads/master@{#491861}

chrome/browser/extensions/chrome_extensions_interface_registration.cc

@@ -7,7 +7,7 @@
 #include "base/bind.h"
 #include "base/logging.h"
 #include "chrome/browser/media/router/media_router_feature.h"  // nogncheck
-#include "chrome/browser/media/router/mojo/media_router_mojo_impl.h"  // nogncheck
+#include "chrome/browser/media/router/mojo/media_router_desktop.h"  // nogncheck
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/render_process_host.h"
 #include "extensions/common/extension.h"
@@ -29,7 +29,7 @@ void RegisterChromeInterfacesForExtension(
       extension->permissions_data()->HasAPIPermission(
           APIPermission::kMediaRouterPrivate)) {
     registry->AddInterface(
-        base::Bind(&media_router::MediaRouterMojoImpl::BindToRequest,
+        base::Bind(&media_router::MediaRouterDesktop::BindToRequest,
                    base::RetainedRef(extension), context));
   }
 }

chrome/browser/media/router/BUILD.gn

@@ -82,6 +82,8 @@ static_library("router") {
       "media_router_ui_service_factory.h",
       "mojo/media_route_provider_util_win.cc",
       "mojo/media_route_provider_util_win.h",
+      "mojo/media_router_desktop.cc",
+      "mojo/media_router_desktop.h",
       "mojo/media_router_mojo_impl.cc",
       "mojo/media_router_mojo_impl.h",
       "mojo/media_router_mojo_metrics.cc",

chrome/browser/media/router/event_page_request_manager.cc

@@ -50,9 +50,15 @@ void EventPageRequestManager::OnMojoConnectionsReady() {
   }
 
   mojo_connections_ready_ = true;
-  for (auto& next_request : pending_requests_)
-    std::move(next_request).Run();
-  pending_requests_.clear();
+
+  std::deque<base::OnceClosure> requests;
+  requests.swap(pending_requests_);
+  for (base::OnceClosure& request : requests) {
+    DCHECK(mojo_connections_ready_);
+    // The requests should not queue additional requests when executed.
+    std::move(request).Run();
+  }
+  DCHECK(pending_requests_.empty());
   wakeup_attempt_count_ = 0;
 }
 

chrome/browser/media/router/event_page_request_manager.h

@@ -56,6 +56,12 @@ class EventPageRequestManager : public KeyedService {
     return media_route_provider_extension_id_;
   }
 
+  bool mojo_connections_ready() const { return mojo_connections_ready_; }
+
+  void set_mojo_connections_ready_for_test(bool ready) {
+    mojo_connections_ready_ = ready;
+  }
+
  protected:
   explicit EventPageRequestManager(content::BrowserContext* context);
 

chrome/browser/media/router/media_router_factory.cc

@@ -13,7 +13,7 @@
 #include "chrome/browser/media/android/router/media_router_android.h"
 #else
 #include "chrome/browser/media/router/event_page_request_manager_factory.h"
-#include "chrome/browser/media/router/mojo/media_router_mojo_impl.h"
+#include "chrome/browser/media/router/mojo/media_router_desktop.h"
 #endif
 
 using content::BrowserContext;
@@ -76,7 +76,7 @@ KeyedService* MediaRouterFactory::BuildServiceInstanceFor(
 #if defined(OS_ANDROID)
   media_router = new MediaRouterAndroid(context);
 #else
-  media_router = new MediaRouterMojoImpl(context);
+  media_router = new MediaRouterDesktop(context);
 #endif
   media_router->Initialize();
   return media_router;

chrome/browser/media/router/mojo/media_router_desktop.h

+// Copyright 2017 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_MEDIA_ROUTER_MOJO_MEDIA_ROUTER_DESKTOP_H_
+#define CHROME_BROWSER_MEDIA_ROUTER_MOJO_MEDIA_ROUTER_DESKTOP_H_
+
+#include "build/build_config.h"
+#include "chrome/browser/media/router/mojo/media_router_mojo_impl.h"
+
+namespace content {
+class RenderFrameHost;
+}
+
+namespace extensions {
+class Extension;
+}
+
+namespace media_router {
+class EventPageRequestManager;
+
+// MediaRouter implementation that uses the MediaRouteProvider implemented in
+// the component extension.
+class MediaRouterDesktop : public MediaRouterMojoImpl {
+ public:
+  ~MediaRouterDesktop() override;
+
+  // Sets up the MediaRouter instance owned by |context| to handle
+  // MediaRouterObserver requests from the component extension given by
+  // |extension|. Creates the MediaRouterMojoImpl instance if it does not
+  // exist.
+  // Called by the Mojo module registry.
+  // |extension|: The component extension, used for querying
+  //     suspension state.
+  // |context|: The BrowserContext which owns the extension process.
+  // |request|: The Mojo connection request used for binding.
+  static void BindToRequest(const extensions::Extension* extension,
+                            content::BrowserContext* context,
+                            mojom::MediaRouterRequest request,
+                            content::RenderFrameHost* source);
+
+  // MediaRouter implementation.
+  void OnUserGesture() override;
+
+ protected:
+  // These methods override MediaRouterMojoImpl methods. They call the base
+  // methods if the connection to the MediaRouteProvider is valid. Otherwise,
+  // they queue themselves to be called again later by |request_manager_| when
+  // the connection is valid.
+  void DoCreateRoute(const MediaSource::Id& source_id,
+                     const MediaSink::Id& sink_id,
+                     const url::Origin& origin,
+                     int tab_id,
+                     std::vector<MediaRouteResponseCallback> callbacks,
+                     base::TimeDelta timeout,
+                     bool incognito) override;
+  void DoJoinRoute(const MediaSource::Id& source_id,
+                   const std::string& presentation_id,
+                   const url::Origin& origin,
+                   int tab_id,
+                   std::vector<MediaRouteResponseCallback> callbacks,
+                   base::TimeDelta timeout,
+                   bool incognito) override;
+  void DoConnectRouteByRouteId(
+      const MediaSource::Id& source_id,
+      const MediaRoute::Id& route_id,
+      const url::Origin& origin,
+      int tab_id,
+      std::vector<MediaRouteResponseCallback> callbacks,
+      base::TimeDelta timeout,
+      bool incognito) override;
+  void DoTerminateRoute(const MediaRoute::Id& route_id) override;
+  void DoDetachRoute(const MediaRoute::Id& route_id) override;
+  void DoSendRouteMessage(const MediaRoute::Id& route_id,
+                          const std::string& message,
+                          SendRouteMessageCallback callback) override;
+  void DoSendRouteBinaryMessage(const MediaRoute::Id& route_id,
+                                std::unique_ptr<std::vector<uint8_t>> data,
+                                SendRouteMessageCallback callback) override;
+  void DoStartListeningForRouteMessages(
+      const MediaRoute::Id& route_id) override;
+  void DoStopListeningForRouteMessages(const MediaRoute::Id& route_id) override;
+  void DoStartObservingMediaSinks(const MediaSource::Id& source_id) override;
+  void DoStopObservingMediaSinks(const MediaSource::Id& source_id) override;
+  void DoStartObservingMediaRoutes(const MediaSource::Id& source_id) override;
+  void DoStopObservingMediaRoutes(const MediaSource::Id& source_id) override;
+  void DoSearchSinks(const MediaSink::Id& sink_id,
+                     const MediaSource::Id& source_id,
+                     const std::string& search_input,
+                     const std::string& domain,
+                     MediaSinkSearchResponseCallback sink_callback) override;
+  void DoCreateMediaRouteController(
+      const MediaRoute::Id& route_id,
+      mojom::MediaControllerRequest mojo_media_controller_request,
+      mojom::MediaStatusObserverPtr mojo_observer) override;
+  void DoProvideSinks(const std::string& provider_name,
+                      std::vector<MediaSinkInternal> sinks) override;
+  void DoUpdateMediaSinks(const MediaSource::Id& source_id) override;
+#if defined(OS_WIN)
+  void DoEnsureMdnsDiscoveryEnabled();
+#endif
+
+  // Error handler callback for |binding_| and |media_route_provider_|.
+  void OnConnectionError() override;
+
+  // Issues 0+ calls to |media_route_provider_| to ensure its state is in sync
+  // with MediaRouter on a best-effort basis.
+  // The extension might have become out of sync with MediaRouter due to one
+  // of few reasons:
+  // (1) The extension crashed and lost unpersisted changes.
+  // (2) The extension was updated; temporary data is cleared.
+  // (3) The extension has an unforseen bug which causes temporary data to be
+  //     persisted incorrectly on suspension.
+  void SyncStateToMediaRouteProvider() override;
+
+ private:
+  friend class MediaRouterDesktopTest;
+  friend class MediaRouterDesktopTestTest;
+  friend class MediaRouterFactory;
+
+  enum class FirewallCheck {
+    // Skips the firewall check for the benefit of unit tests so they do not
+    // have to depend on the system's firewall configuration.
+    SKIP_FOR_TESTING,
+    // Perform the firewall check (default).
+    RUN,
+  };
+
+  MediaRouterDesktop(content::BrowserContext* context,
+                     FirewallCheck check_firewall = FirewallCheck::RUN);
+
+  // mojom::MediaRouter implementation.
+  // Notifies |request_manager_| that the Mojo connection to MediaRouteProvider
+  // is valid.
+  void RegisterMediaRouteProvider(
+      mojom::MediaRouteProviderPtr media_route_provider_ptr,
+      mojom::MediaRouter::RegisterMediaRouteProviderCallback callback) override;
+
+  // Binds |this| to a Mojo interface request, so that clients can acquire a
+  // handle to a MediaRouter instance via the Mojo service connector.
+  // Passes the extension's ID to the event page request manager.
+  void BindToMojoRequest(mojo::InterfaceRequest<mojom::MediaRouter> request,
+                         const extensions::Extension& extension);
+
+#if defined(OS_WIN)
+  // Ensures that mDNS discovery is enabled in the MRPM extension. This can be
+  // called many times but the MRPM will only be called once per registration
+  // period.
+  void EnsureMdnsDiscoveryEnabled();
+
+  // Callback used to enable mDNS in the MRPM if a firewall prompt will not be
+  // triggered. If a firewall prompt would be triggered, enabling mDNS won't
+  // happen until the user is clearly interacting with MR.
+  void OnFirewallCheckComplete(bool firewall_can_use_local_ports);
+#endif
+
+  // Request manager responsible for waking the component extension and calling
+  // the requests to it.
+  EventPageRequestManager* const request_manager_;
+
+  // Binds |this| to a Mojo connection stub for mojom::MediaRouter.
+  mojo::Binding<mojom::MediaRouter> binding_;
+
+  // A flag to ensure that we record the provider version once, during the
+  // initial event page wakeup attempt.
+  bool provider_version_was_recorded_ = false;
+
+#if defined(OS_WIN)
+  // A pair of flags to ensure that mDNS discovery is only enabled on Windows
+  // when there will be appropriate context for the user to associate a firewall
+  // prompt with Media Router. |should_enable_mdns_discovery_| can only go from
+  // |false| to |true|. On Windows, |is_mdns_enabled_| is set to |false| in
+  // RegisterMediaRouteProvider and only set to |true| when we successfully call
+  // the extension to enable mDNS.
+  bool is_mdns_enabled_ = false;
+  bool should_enable_mdns_discovery_ = false;
+#endif
+
+  base::WeakPtrFactory<MediaRouterDesktop> weak_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(MediaRouterDesktop);
+};
+
+}  // namespace media_router
+
+#endif  // CHROME_BROWSER_MEDIA_ROUTER_MOJO_MEDIA_ROUTER_DESKTOP_H_

chrome/browser/media/router/mojo/media_router_desktop_unittest.cc

+// Copyright 2017 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 <stddef.h>
+#include <stdint.h>
+
+#include <memory>
+#include <string>
+#include <utility>
+
+#include "chrome/browser/media/router/mojo/media_router_desktop.h"
+
+#include "build/build_config.h"
+#include "chrome/browser/media/router/event_page_request_manager.h"
+#include "chrome/browser/media/router/event_page_request_manager_factory.h"
+#include "chrome/browser/media/router/mojo/media_router_mojo_test.h"
+#include "chrome/common/media_router/media_source_helper.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+using testing::_;
+using testing::Invoke;
+using testing::Mock;
+using testing::Return;
+
+namespace media_router {
+
+namespace {
+
+const char kSource[] = "source1";
+const char kOrigin[] = "http://origin/";
+
+class NullMessageObserver : public RouteMessageObserver {
+ public:
+  NullMessageObserver(MediaRouter* router, const MediaRoute::Id& route_id)
+      : RouteMessageObserver(router, route_id) {}
+  ~NullMessageObserver() final {}
+
+  void OnMessagesReceived(
+      const std::vector<content::PresentationConnectionMessage>& messages)
+      final {}
+};
+
+class TestEventPageRequestManager : public EventPageRequestManager {
+ public:
+  static std::unique_ptr<KeyedService> Create(
+      content::BrowserContext* context) {
+    return base::MakeUnique<TestEventPageRequestManager>(context);
+  }
+
+  explicit TestEventPageRequestManager(content::BrowserContext* context)
+      : EventPageRequestManager(context) {}
+  ~TestEventPageRequestManager() = default;
+
+  MOCK_METHOD1(SetExtensionId, void(const std::string& extension_id));
+  void RunOrDefer(base::OnceClosure request,
+                  MediaRouteProviderWakeReason wake_reason) override {
+    RunOrDeferInternal(request, wake_reason);
+  }
+  MOCK_METHOD2(RunOrDeferInternal,
+               void(base::OnceClosure& request,
+                    MediaRouteProviderWakeReason wake_reason));
+  MOCK_METHOD0(OnMojoConnectionsReady, void());
+  MOCK_METHOD0(OnMojoConnectionError, void());
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(TestEventPageRequestManager);
+};
+
+}  // namespace
+
+class MediaRouterDesktopTest : public MediaRouterMojoTest {
+ public:
+  MediaRouterDesktopTest() {
+    EventPageRequestManagerFactory::GetInstance()->SetTestingFactory(
+        profile(), &TestEventPageRequestManager::Create);
+    request_manager_ = static_cast<TestEventPageRequestManager*>(
+        EventPageRequestManagerFactory::GetApiForBrowserContext(profile()));
+    request_manager_->set_mojo_connections_ready_for_test(true);
+    ON_CALL(*request_manager_, RunOrDeferInternal(_, _))
+        .WillByDefault(Invoke([](base::OnceClosure& request,
+                                 MediaRouteProviderWakeReason wake_reason) {
+          std::move(request).Run();
+        }));
+  }
+
+  ~MediaRouterDesktopTest() override {}
+
+ protected:
+  std::unique_ptr<MediaRouterMojoImpl> CreateMediaRouter() override {
+    return std::unique_ptr<MediaRouterMojoImpl>(
+        new MediaRouterDesktop(profile()));
+  }
+
+  TestEventPageRequestManager* request_manager_ = nullptr;
+};
+
+TEST_F(MediaRouterDesktopTest, CreateRoute) {
+  TestCreateRoute();
+}
+
+TEST_F(MediaRouterDesktopTest, JoinRoute) {
+  TestJoinRoute();
+}
+
+TEST_F(MediaRouterDesktopTest, ConnectRouteByRouteId) {
+  TestConnectRouteByRouteId();
+}
+
+TEST_F(MediaRouterDesktopTest, TerminateRoute) {
+  TestTerminateRoute();
+}
+
+TEST_F(MediaRouterDesktopTest, SendRouteMessage) {
+  TestSendRouteMessage();
+}
+
+TEST_F(MediaRouterDesktopTest, SendRouteBinaryMessage) {
+  TestSendRouteBinaryMessage();
+}
+
+TEST_F(MediaRouterDesktopTest, DetachRoute) {
+  TestDetachRoute();
+}
+
+TEST_F(MediaRouterDesktopTest, SearchSinks) {
+  TestSearchSinks();
+}
+
+TEST_F(MediaRouterDesktopTest, ProvideSinks) {
+  TestProvideSinks();
+}
+
+TEST_F(MediaRouterDesktopTest, CreateMediaRouteController) {
+  TestCreateMediaRouteController();
+}
+
+#if defined(OS_WIN)
+TEST_F(MediaRouterDesktopTest, EnableMdnsAfterEachRegister) {
+  // EnableMdnsDiscovery() should not be called when no MRPM is registered yet.
+  EXPECT_CALL(*request_manager_, RunOrDeferInternal(_, _))
+      .WillRepeatedly(Return());
+  router()->OnUserGesture();
+  EXPECT_TRUE(Mock::VerifyAndClearExpectations(request_manager_));
+
+  EXPECT_CALL(mock_media_route_provider_,
+              UpdateMediaSinks(MediaSourceForDesktop().id()));
+  // EnableMdnsDiscovery() is never called except on Windows.
+  EXPECT_CALL(mock_media_route_provider_, EnableMdnsDiscovery());
+  ConnectProviderManagerService();
+  // Should not call EnableMdnsDiscovery(), but will call UpdateMediaSinks.
+  router()->OnUserGesture();
+  base::RunLoop().RunUntilIdle();
+}
+#endif
+
+TEST_F(MediaRouterDesktopTest, UpdateMediaSinksOnUserGesture) {
+#if defined(OS_WIN)
+  EXPECT_CALL(mock_media_route_provider_, EnableMdnsDiscovery());
+#endif
+  EXPECT_CALL(mock_media_route_provider_,
+              UpdateMediaSinks(MediaSourceForDesktop().id()));
+  router()->OnUserGesture();
+  base::RunLoop().RunUntilIdle();
+}
+
+TEST_F(MediaRouterDesktopTest, SyncStateToMediaRouteProvider) {
+  MediaSource media_source(kSource);
+  std::unique_ptr<MockMediaSinksObserver> sinks_observer;
+  std::unique_ptr<MockMediaRoutesObserver> routes_observer;
+  std::unique_ptr<NullMessageObserver> messages_observer;
+
+  {
+    router()->OnSinkAvailabilityUpdated(
+        mojom::MediaRouter::SinkAvailability::PER_SOURCE);
+    EXPECT_CALL(mock_media_route_provider_,
+                StartObservingMediaSinks(media_source.id()));
+    sinks_observer = base::MakeUnique<MockMediaSinksObserver>(
+        router(), media_source, url::Origin(GURL(kOrigin)));
+    EXPECT_TRUE(sinks_observer->Init());
+
+    EXPECT_CALL(mock_media_route_provider_,
+                StartObservingMediaRoutes(media_source.id()));
+    routes_observer =
+        base::MakeUnique<MockMediaRoutesObserver>(router(), media_source.id());
+
+    EXPECT_CALL(mock_media_route_provider_,
+                StartListeningForRouteMessages(media_source.id()));
+    messages_observer =
+        base::MakeUnique<NullMessageObserver>(router(), media_source.id());
+    base::RunLoop().RunUntilIdle();
+    EXPECT_TRUE(Mock::VerifyAndClearExpectations(&mock_media_route_provider_));
+  }
+
+  {
+    EXPECT_CALL(mock_media_route_provider_,
+                StartObservingMediaSinks(media_source.id()));
+    EXPECT_CALL(mock_media_route_provider_, StartObservingMediaRoutes(""));
+    EXPECT_CALL(mock_media_route_provider_,
+                StartObservingMediaRoutes(media_source.id()));
+    EXPECT_CALL(mock_media_route_provider_,
+                StartListeningForRouteMessages(media_source.id()));
+    router()->OnConnectionError();
+    ConnectProviderManagerService();
+    base::RunLoop().RunUntilIdle();
+  }
+}
+
+}  // namespace media_router

chrome/browser/media/router/mojo/media_router_mojo_test.h

@@ -242,29 +242,42 @@ class MediaRouterMojoTest : public ::testing::Test {
   void SetUp() override;
   void TearDown() override;
 
+  // Create a MediaRouter instance to test.
+  virtual std::unique_ptr<MediaRouterMojoImpl> CreateMediaRouter() = 0;
+
   void ProcessEventLoop();
 
   void ConnectProviderManagerService();
 
+  // Tests that calling MediaRouter methods result in calls to corresponding
+  // MediaRouteProvider methods.
+  void TestCreateRoute();
+  void TestJoinRoute();
+  void TestConnectRouteByRouteId();
+  void TestTerminateRoute();
+  void TestSendRouteMessage();
+  void TestSendRouteBinaryMessage();
+  void TestDetachRoute();
+  void TestSearchSinks();
+  void TestProvideSinks();
+  void TestCreateMediaRouteController();
+
   const std::string& extension_id() const { return extension_->id(); }
 
-  MediaRouterMojoImpl* router() const { return mock_media_router_.get(); }
+  MediaRouterMojoImpl* router() const { return media_router_.get(); }
 
   Profile* profile() { return &profile_; }
 
   // Mock objects.
   MockMediaRouteProvider mock_media_route_provider_;
 
-  // Mojo proxy object for |mock_media_router_|
-  media_router::mojom::MediaRouterPtr media_router_proxy_;
-
   RegisterMediaRouteProviderHandler provide_handler_;
 
  private:
   content::TestBrowserThreadBundle test_thread_bundle_;
   scoped_refptr<extensions::Extension> extension_;
   TestingProfile profile_;
-  std::unique_ptr<MediaRouterMojoImpl> mock_media_router_;
+  std::unique_ptr<MediaRouterMojoImpl> media_router_;
   std::unique_ptr<mojo::Binding<mojom::MediaRouteProvider>> binding_;
 
   DISALLOW_COPY_AND_ASSIGN(MediaRouterMojoTest);

chrome/test/BUILD.gn

@@ -3817,6 +3817,7 @@ test("unit_tests") {
       # Move media_router_ui_service_factory_unittest.cc to chrome/browser/ui.
       "../browser/media/router/media_router_ui_service_factory_unittest.cc",
       "../browser/media/router/mojo/media_route_controller_unittest.cc",
+      "../browser/media/router/mojo/media_router_desktop_unittest.cc",
       "../browser/media/router/mojo/media_router_mojo_impl_unittest.cc",
       "../browser/media/router/mojo/media_router_mojo_metrics_unittest.cc",
       "../browser/policy/local_sync_policy_handler_unittest.cc",