Extract EndpointFetcher from Handler.

Refactor the AccessCodeCastHandler to only depend on the
AccessCodeSinkService. Additionally remove dependencies of the discovery
server interface from the generic MDNS discovery of the Media Router.

I was not able to simply solve the circular dependency from the
//chrome//browser (see b/220386256) -- however discovery no longer needs
this circular dependency (so the Media Router team doesn't have to deal
with it).

Bug: b/218327526
Change-Id: If939fe5bf1ccf8beb01b9222df09b2b9f9e3e983
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3475163
Reviewed-by: Takumi Fujimoto <takumif@chromium.org>
Reviewed-by: Brian Malcolm <bmalcolm@chromium.org>
Commit-Queue: George Benz <gbj@google.com>
Cr-Commit-Position: refs/heads/main@{#985104}

chrome/browser/BUILD.gn

@@ -4420,8 +4420,7 @@ static_library("browser") {
       "//chrome/browser/cart:mojo_bindings",
       "//chrome/browser/enterprise/signals:utils",
       "//chrome/browser/first_party_sets",
-      "//chrome/browser/media/router/discovery:discovery",
-      "//chrome/browser/media/router/discovery/access_code:access_code_sink_service_factory",
+      "//chrome/browser/media/router/discovery/access_code:access_code_sink_service",
       "//chrome/browser/new_tab_page/chrome_colors:generate_chrome_colors_info",
       "//chrome/browser/new_tab_page/chrome_colors:generate_colors_info",
       "//chrome/browser/policy:path_parser",
@@ -4484,8 +4483,8 @@ static_library("browser") {
       "//extensions/buildflags",
     ]
     allow_circular_includes_from += [
-      # TODO(crbug.com/1030821): Eliminate usages of browser.h from Media Router.
-      "//chrome/browser/media/router/discovery:discovery",
+      # TODO(b/220386256): Remove circular dependency from the browser.
+      "//chrome/browser/media/router/discovery/access_code:access_code_sink_service",
 
       # TODO(crbug.com/1200215): Remove cycles and simplify all dependencies.
       "//chrome/browser/web_applications",

chrome/browser/media/router/BUILD.gn

@@ -195,6 +195,7 @@ if (!is_android) {
     testonly = true
     deps = [
       "discovery",
+      "//chrome/browser/media/router/discovery/access_code:discovery_resources_proto",
       "//chrome/test:test_support",
       "//components/cast_channel:cast_channel",
       "//components/cast_channel:test_support",
@@ -206,6 +207,8 @@ if (!is_android) {
     ]
     public_deps = [ ":router" ]
     sources = [
+      "discovery/access_code/access_code_test_util.cc",
+      "discovery/access_code/access_code_test_util.h",
       "discovery/mdns/cast_media_sink_service_test_helpers.cc",
       "discovery/mdns/cast_media_sink_service_test_helpers.h",
       "providers/test/test_media_route_provider.cc",
@@ -304,6 +307,7 @@ source_set("unittests") {
       "//chrome/browser/media/router/discovery:discovery",
       "//chrome/browser/media/router/discovery/access_code:access_code_cast_feature",
       "//chrome/browser/media/router/discovery/access_code:access_code_sink_service",
+      "//chrome/browser/media/router/discovery/access_code:discovery_resources_proto",
       "//chrome/test:test_support",
       "//components/sync_preferences:test_support",
       "//content/test:test_support",

chrome/browser/media/router/discovery/BUILD.gn

@@ -16,35 +16,20 @@ static_library("discovery") {
     "//chrome/browser:browser_process",
     "//chrome/browser/media/router:data_decoder_util",
     "//chrome/browser/media/router:media_router_feature",
-    "//chrome/browser/media/router/discovery/access_code:discovery_resources_proto",
-    "//chrome/browser/profiles:profile",
-    "//chrome/browser/signin:identity_manager_provider",
-    "//chrome/browser/ui/webui/access_code_cast:mojo_bindings",
-    "//chrome/common:channel_info",
     "//chrome/common:constants",
     "//components/cast_channel",
-    "//components/leveldb_proto:leveldb_proto",
     "//components/media_router/browser",
     "//components/media_router/common",
     "//components/media_router/common/mojom:logger",
     "//components/net_log",
     "//components/prefs",
-    "//components/signin/public/base:base",
-    "//components/signin/public/identity_manager:identity_manager",
-    "//components/user_manager:user_manager",
     "//components/version_info",
     "//content/public/browser",
     "//content/public/common",
     "//services/network/public/cpp",
     "//third_party/openscreen/src/cast/common/channel/proto:channel_proto",
   ]
   sources = [
-    "access_code/access_code_cast_discovery_interface.cc",
-    "access_code/access_code_cast_discovery_interface.h",
-    "access_code/access_code_media_sink_util.cc",
-    "access_code/access_code_media_sink_util.h",
-    "access_code/access_code_test_util.cc",
-    "access_code/access_code_test_util.h",
     "dial/device_description_fetcher.cc",
     "dial/device_description_fetcher.h",
     "dial/device_description_service.cc",

chrome/browser/media/router/discovery/access_code/BUILD.gn

@@ -26,8 +26,14 @@ static_library("access_code_cast_feature") {
 if (!is_android) {
   static_library("access_code_sink_service") {
     sources = [
+      "access_code_cast_discovery_interface.cc",
+      "access_code_cast_discovery_interface.h",
       "access_code_cast_sink_service.cc",
       "access_code_cast_sink_service.h",
+      "access_code_cast_sink_service_factory.cc",
+      "access_code_cast_sink_service_factory.h",
+      "access_code_media_sink_util.cc",
+      "access_code_media_sink_util.h",
     ]
     public_deps = [
       "//base",
@@ -41,25 +47,18 @@ if (!is_android) {
       ":access_code_cast_feature",
       "//chrome/browser/media/router:router",
       "//chrome/browser/media/router/discovery:discovery",
-    ]
-  }
-
-  static_library("access_code_sink_service_factory") {
-    sources = [
-      "access_code_cast_sink_service_factory.cc",
-      "access_code_cast_sink_service_factory.h",
-    ]
-    public_deps = [
-      "//base",
+      "//chrome/browser/media/router/discovery/access_code:discovery_resources_proto",
       "//chrome/browser/profiles:profile",
+      "//chrome/browser/signin:identity_manager_provider",
+      "//chrome/browser/ui/webui/access_code_cast:mojo_bindings",
+      "//chrome/common:channel_info",
+      "//components/cast_channel:cast_channel",
       "//components/keyed_service/content:content",
-      "//components/media_router/browser",
-      "//components/media_router/common",
-    ]
-    deps = [
-      ":access_code_cast_feature",
-      ":access_code_sink_service",
-      "//chrome/browser/media/router:router",
+      "//components/leveldb_proto:leveldb_proto",
+      "//components/signin/public/base:base",
+      "//components/signin/public/identity_manager:identity_manager",
+      "//components/user_manager:user_manager",
+      "//components/version_info:channel",
     ]
   }
 }

chrome/browser/media/router/discovery/access_code/access_code_cast_discovery_interface.cc

@@ -161,6 +161,7 @@ AccessCodeCastDiscoveryInterface::AccessCodeCastDiscoveryInterface(
       logger_(logger),
       endpoint_fetcher_(CreateEndpointFetcher(access_code)) {
   DCHECK(profile_);
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 }
 
 AccessCodeCastDiscoveryInterface::AccessCodeCastDiscoveryInterface(
@@ -307,6 +308,8 @@ void AccessCodeCastDiscoveryInterface::SetNetworkInfoField(
 std::unique_ptr<EndpointFetcher>
 AccessCodeCastDiscoveryInterface::CreateEndpointFetcher(
     const std::string& access_code) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+
   std::vector<std::string> discovery_scopes;
   discovery_scopes.push_back(kDiscoveryOAuth2Scope);
 
@@ -320,6 +323,7 @@ AccessCodeCastDiscoveryInterface::CreateEndpointFetcher(
 void AccessCodeCastDiscoveryInterface::ValidateDiscoveryAccessCode(
     DiscoveryDeviceCallback callback) {
   DCHECK(!callback_);
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   callback_ = std::move(callback);
 
   auto* const fetcher_ptr = endpoint_fetcher_.get();

chrome/browser/media/router/discovery/access_code/access_code_cast_sink_service.cc

@@ -7,12 +7,14 @@
 #include <algorithm>
 
 #include "base/bind.h"
+#include "base/task/bind_post_task.h"
 #include "base/task/sequenced_task_runner.h"
 #include "base/task/single_thread_task_runner.h"
 #include "base/task/task_runner_util.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "base/time/time.h"
 #include "chrome/browser/media/router/discovery/access_code/access_code_cast_feature.h"
+#include "chrome/browser/media/router/discovery/access_code/access_code_media_sink_util.h"
 #include "chrome/browser/media/router/discovery/mdns/media_sink_util.h"
 #include "chrome/browser/media/router/discovery/media_sink_discovery_metrics.h"
 #include "chrome/browser/media/router/providers/cast/dual_media_sink_service.h"
@@ -22,6 +24,8 @@
 #include "components/media_router/common/media_route.h"
 #include "components/media_router/common/media_sink.h"
 #include "components/media_router/common/mojom/media_router.mojom.h"
+#include "content/public/browser/browser_thread.h"
+#include "mojo/public/cpp/bindings/callback_helpers.h"
 
 namespace media_router {
 
@@ -172,7 +176,7 @@ void AccessCodeCastSinkService::HandleMediaRouteDiscoveredByAccessCode(
   // discovery.
   auto it = pending_callbacks_.find(sink->id());
   if (it != pending_callbacks_.end()) {
-    std::move(it->second).Run(true);
+    std::move(it->second).Run(AddSinkResultCode::OK, sink->id());
     pending_callbacks_.erase(sink->id());
   } else {
     if (sink->cast_data().discovered_by_access_code) {
@@ -218,9 +222,19 @@ void AccessCodeCastSinkService::OnAccessCodeRouteRemoved(
   }
 }
 
+void AccessCodeCastSinkService::DiscoverSink(const std::string& access_code,
+                                             AddSinkResultCallback callback) {
+  discovery_server_interface_ =
+      std::make_unique<AccessCodeCastDiscoveryInterface>(
+          profile_, access_code, media_router_->GetLogger());
+  discovery_server_interface_->ValidateDiscoveryAccessCode(
+      base::BindOnce(&AccessCodeCastSinkService::OnAccessCodeValidated,
+                     weak_ptr_factory_.GetWeakPtr(), std::move(callback)));
+}
+
 void AccessCodeCastSinkService::AddSinkToMediaRouter(
     const MediaSinkInternal& sink,
-    ChannelOpenedCallback callback) {
+    AddSinkResultCallback add_sink_callback) {
   // Check to see if the media sink already exists in the media router.
   base::PostTaskAndReplyWithResult(
       cast_media_sink_service_impl_->task_runner().get(), FROM_HERE,
@@ -229,12 +243,39 @@ void AccessCodeCastSinkService::AddSinkToMediaRouter(
                      sink.id()),
       base::BindOnce(&AccessCodeCastSinkService::OpenChannelIfNecessary,
                      weak_ptr_factory_.GetWeakPtr(), sink,
-                     std::move(callback)));
+                     std::move(add_sink_callback)));
+}
+
+void AccessCodeCastSinkService::OnAccessCodeValidated(
+    AddSinkResultCallback add_sink_callback,
+    absl::optional<DiscoveryDevice> discovery_device,
+    AddSinkResultCode result_code) {
+  if (result_code != AddSinkResultCode::OK) {
+    std::move(add_sink_callback).Run(result_code, absl::nullopt);
+    return;
+  }
+  if (!discovery_device.has_value()) {
+    std::move(add_sink_callback)
+        .Run(AddSinkResultCode::EMPTY_RESPONSE, absl::nullopt);
+    return;
+  }
+  std::pair<absl::optional<MediaSinkInternal>, CreateCastMediaSinkResult>
+      creation_result = CreateAccessCodeMediaSink(discovery_device.value());
+
+  if (!creation_result.first.has_value() ||
+      creation_result.second != CreateCastMediaSinkResult::kOk) {
+    std::move(add_sink_callback)
+        .Run(AddSinkResultCode::SINK_CREATION_ERROR, absl::nullopt);
+    return;
+  }
+  auto media_sink = creation_result.first.value();
+
+  AddSinkToMediaRouter(media_sink, std::move(add_sink_callback));
 }
 
 void AccessCodeCastSinkService::OpenChannelIfNecessary(
     const MediaSinkInternal& sink,
-    ChannelOpenedCallback callback,
+    AddSinkResultCallback add_sink_callback,
     bool has_sink) {
   if (has_sink) {
     media_router_->GetLogger()->LogInfo(
@@ -258,12 +299,26 @@ void AccessCodeCastSinkService::OpenChannelIfNecessary(
           "terminate it.",
           sink.id(), "", "");
       media_router_->TerminateRoute(route_it->media_route_id());
-      pending_callbacks_.emplace(sink.id(), std::move(callback));
+      pending_callbacks_.emplace(sink.id(), std::move(add_sink_callback));
     } else {
-      std::move(callback).Run(true);
+      std::move(add_sink_callback).Run(AddSinkResultCode::OK, sink.id());
     }
     return;
   }
+  // Task runner for the current thread.
+  scoped_refptr<base::SequencedTaskRunner> current_task_runner =
+      base::SequencedTaskRunnerHandle::Get();
+
+  // The OnChannelOpenedResult() callback needs to be be bound with
+  // BindPostTask() to ensure that the callback is invoked on this specific task
+  // runner.
+  auto channel_cb = base::BindOnce(
+      &AccessCodeCastSinkService::OnChannelOpenedResult,
+      weak_ptr_factory_.GetWeakPtr(), std::move(add_sink_callback), sink.id());
+
+  auto returned_channel_cb =
+      base::BindPostTask(current_task_runner, std::move(channel_cb));
+
   auto backoff_entry = std::make_unique<net::BackoffEntry>(&backoff_policy_);
   media_router_->GetLogger()->LogInfo(
       mojom::LogCategory::kDiscovery, kLoggerComponent,
@@ -273,7 +328,8 @@ void AccessCodeCastSinkService::OpenChannelIfNecessary(
       base::BindOnce(&CastMediaSinkServiceImpl::OpenChannel,
                      base::Unretained(cast_media_sink_service_impl_), sink,
                      std::move(backoff_entry), SinkSource::kAccessCode,
-                     std::move(callback), CreateCastSocketOpenParams(sink)));
+                     std::move(returned_channel_cb),
+                     CreateCastSocketOpenParams(sink)));
 }
 
 cast_channel::CastSocketOpenParams
@@ -286,6 +342,24 @@ AccessCodeCastSinkService::CreateCastSocketOpenParams(
       cast_channel::CastDeviceCapability::NONE);
 }
 
+void AccessCodeCastSinkService::OnChannelOpenedResult(
+    AddSinkResultCallback add_sink_callback,
+    MediaSink::Id sink_id,
+    bool channel_opened) {
+  if (!channel_opened) {
+    media_router_->GetLogger()->LogError(
+        mojom::LogCategory::kDiscovery, kLoggerComponent,
+        "The channel failed to open.", sink_id, "", "");
+    std::move(add_sink_callback)
+        .Run(AddSinkResultCode::CHANNEL_OPEN_ERROR, absl::nullopt);
+    return;
+  }
+  media_router_->GetLogger()->LogInfo(
+      mojom::LogCategory::kDiscovery, kLoggerComponent,
+      "The channel successfully opened.", sink_id, "", "");
+  std::move(add_sink_callback).Run(AddSinkResultCode::OK, sink_id);
+}
+
 void AccessCodeCastSinkService::Shutdown() {
   // There's no guarantee that MediaRouter is still in the
   // MediaRoutesObserver. |media_routes_observer_| accesses MediaRouter in its