[DIAL MRP] Use data_decoder to parse custom DIAL launch messages from

the Cast SDK.

This patch also changes the XML parsing done by DIAL discovery to use
the same batching ID so that the data_decoder service instance will be
shared with the MRP.


Bug: 808720
Change-Id: Ieed0aa79bfb35874d5bb68fc35690baeae54f561
Reviewed-on: https://chromium-review.googlesource.com/1147107
Commit-Queue: Derek Cheng <imcheng@chromium.org>
Reviewed-by: mark a. foltz <mfoltz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#578834}

chrome/browser/media/router/BUILD.gn

@@ -70,6 +70,8 @@ static_library("router") {
       "//ui/base:ui_features",
     ]
     sources += [
+      "data_decoder_util.cc",
+      "data_decoder_util.h",
       "event_page_request_manager.cc",
       "event_page_request_manager.h",
       "event_page_request_manager_factory.cc",

chrome/browser/media/router/data_decoder_util.cc

+// Copyright 2018 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/media/router/data_decoder_util.h"
+
+#include "services/service_manager/public/cpp/connector.h"
+
+namespace media_router {
+
+namespace {
+// The batch ID used by data_decoder_util functions.
+constexpr char kDataDecoderServiceBatchId[] = "media_router";
+}  // namespace
+
+DataDecoder::DataDecoder(service_manager::Connector* connector)
+    : connector_(connector->Clone()) {}
+
+DataDecoder::~DataDecoder() = default;
+
+void DataDecoder::ParseXml(const std::string& unsafe_xml,
+                           data_decoder::XmlParserCallback callback) {
+  data_decoder::ParseXml(connector_.get(), unsafe_xml, std::move(callback),
+                         kDataDecoderServiceBatchId);
+}
+
+void DataDecoder::ParseJson(
+    const std::string& unsafe_json,
+    const data_decoder::SafeJsonParser::SuccessCallback& success_callback,
+    const data_decoder::SafeJsonParser::ErrorCallback& error_callback) {
+  data_decoder::SafeJsonParser::ParseBatch(connector_.get(), unsafe_json,
+                                           success_callback, error_callback,
+                                           kDataDecoderServiceBatchId);
+}
+
+}  // namespace media_router

chrome/browser/media/router/data_decoder_util.h

+// Copyright 2018 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_DATA_DECODER_UTIL_H_
+#define CHROME_BROWSER_MEDIA_ROUTER_DATA_DECODER_UTIL_H_
+
+#include <string>
+
+#include "services/data_decoder/public/cpp/safe_json_parser.h"
+#include "services/data_decoder/public/cpp/safe_xml_parser.h"
+
+namespace service_manager {
+class Connector;
+}
+
+namespace media_router {
+
+// A wrapper over their data_decoder functions for parsing XML/JSON that batches
+// all calls with a shared batch ID.
+// Thread safety: A newly constructed DataDecoder is not bound to any thread. On
+// first use, it becomes bound to the calling thread.
+class DataDecoder {
+ public:
+  // |connector|: Connector object to be cloned in the constructor.
+  explicit DataDecoder(service_manager::Connector* connector);
+  ~DataDecoder();
+
+  void ParseXml(const std::string& unsafe_xml,
+                data_decoder::XmlParserCallback callback);
+
+  void ParseJson(
+      const std::string& unsafe_json,
+      const data_decoder::SafeJsonParser::SuccessCallback& success_callback,
+      const data_decoder::SafeJsonParser::ErrorCallback& error_callback);
+
+ private:
+  std::unique_ptr<service_manager::Connector> connector_;
+  DISALLOW_COPY_AND_ASSIGN(DataDecoder);
+};
+
+}  // namespace media_router
+
+#endif  // CHROME_BROWSER_MEDIA_ROUTER_DATA_DECODER_UTIL_H_

chrome/browser/media/router/discovery/dial/device_description_service.cc

@@ -10,10 +10,10 @@
 
 #include "base/stl_util.h"
 #include "base/strings/string_util.h"
+#include "chrome/browser/media/router/data_decoder_util.h"
 #include "chrome/browser/media/router/discovery/dial/device_description_fetcher.h"
 #include "chrome/browser/media/router/discovery/dial/safe_dial_device_description_parser.h"
 #include "chrome/browser/media/router/media_router_metrics.h"
-#include "content/public/common/service_manager_connection.h"
 #include "net/base/ip_address.h"
 #include "url/gurl.h"
 
@@ -99,12 +99,12 @@ ParsingError ValidateParsedDeviceDescription(
 }  // namespace
 
 DeviceDescriptionService::DeviceDescriptionService(
-    service_manager::Connector* connector,
+    DataDecoder* data_decoder,
     const DeviceDescriptionParseSuccessCallback& success_cb,
     const DeviceDescriptionParseErrorCallback& error_cb)
     : success_cb_(success_cb),
       error_cb_(error_cb),
-      device_description_parser_(connector) {}
+      device_description_parser_(data_decoder) {}
 
 DeviceDescriptionService::~DeviceDescriptionService() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);

chrome/browser/media/router/discovery/dial/device_description_service.h

@@ -17,12 +17,9 @@
 #include "chrome/browser/media/router/discovery/dial/parsed_dial_device_description.h"
 #include "chrome/browser/media/router/discovery/dial/safe_dial_device_description_parser.h"
 
-namespace service_manager {
-class Connector;
-}
-
 namespace media_router {
 
+class DataDecoder;
 class DeviceDescriptionFetcher;
 class SafeDialDeviceDescriptionParser;
 
@@ -64,7 +61,7 @@ class DeviceDescriptionService {
                                    const std::string& error_message)>;
 
   DeviceDescriptionService(
-      service_manager::Connector* connector,
+      DataDecoder* data_decoder,
       const DeviceDescriptionParseSuccessCallback& success_cb,
       const DeviceDescriptionParseErrorCallback& error_cb);
   virtual ~DeviceDescriptionService();

chrome/browser/media/router/discovery/dial/dial_app_discovery_service.cc

@@ -7,6 +7,7 @@
 #include "base/stl_util.h"
 #include "base/strings/string_util.h"
 #include "base/time/default_clock.h"
+#include "chrome/browser/media/router/data_decoder_util.h"
 #include "net/http/http_status_code.h"
 #include "url/gurl.h"
 
@@ -33,9 +34,8 @@ DialAppInfoResult::DialAppInfoResult(DialAppInfoResult&& other) = default;
 
 DialAppInfoResult::~DialAppInfoResult() = default;
 
-DialAppDiscoveryService::DialAppDiscoveryService(
-    service_manager::Connector* connector)
-    : parser_(std::make_unique<SafeDialAppInfoParser>(connector)) {}
+DialAppDiscoveryService::DialAppDiscoveryService(DataDecoder* data_decoder)
+    : parser_(std::make_unique<SafeDialAppInfoParser>(data_decoder)) {}
 
 DialAppDiscoveryService::~DialAppDiscoveryService() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);

chrome/browser/media/router/discovery/dial/dial_app_discovery_service.h

@@ -20,12 +20,10 @@
 #include "chrome/common/media_router/discovery/media_sink_internal.h"
 #include "url/gurl.h"
 
-namespace service_manager {
-class Connector;
-}
-
 namespace media_router {
 
+class DataDecoder;
+
 // Represents DIAL app status on receiver device.
 enum class DialAppInfoResultCode {
   kOk = 0,
@@ -63,7 +61,7 @@ class DialAppDiscoveryService {
                               const std::string& app_name,
                               DialAppInfoResult result)>;
 
-  explicit DialAppDiscoveryService(service_manager::Connector* connector);
+  explicit DialAppDiscoveryService(DataDecoder* data_decoder);
 
   virtual ~DialAppDiscoveryService();
 

chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc

@@ -43,11 +43,8 @@ void DialMediaSinkService::Start(
 std::unique_ptr<DialMediaSinkServiceImpl, base::OnTaskRunnerDeleter>
 DialMediaSinkService::CreateImpl(
     const OnSinksDiscoveredCallback& sink_discovery_cb) {
-  // Clone the connector so it can be used on the IO thread.
-  std::unique_ptr<service_manager::Connector> connector =
-      content::ServiceManagerConnection::GetForProcess()
-          ->GetConnector()
-          ->Clone();
+  service_manager::Connector* connector =
+      content::ServiceManagerConnection::GetForProcess()->GetConnector();
 
   // Note: The SequencedTaskRunner needs to be IO thread because DialRegistry
   // runs on IO thread.
@@ -55,8 +52,7 @@ DialMediaSinkService::CreateImpl(
       content::BrowserThread::GetTaskRunnerForThread(
           content::BrowserThread::IO);
   return std::unique_ptr<DialMediaSinkServiceImpl, base::OnTaskRunnerDeleter>(
-      new DialMediaSinkServiceImpl(std::move(connector), sink_discovery_cb,
-                                   task_runner),
+      new DialMediaSinkServiceImpl(connector, sink_discovery_cb, task_runner),
       base::OnTaskRunnerDeleter(task_runner));
 }
 

chrome/browser/media/router/discovery/dial/dial_media_sink_service_impl.cc

@@ -8,6 +8,7 @@
 
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
+#include "chrome/browser/media/router/data_decoder_util.h"
 #include "chrome/browser/media/router/discovery/dial/dial_device_data.h"
 #include "services/service_manager/public/cpp/connector.h"
 
@@ -50,11 +51,11 @@ SinkAppStatus GetSinkAppStatusFromResponse(const DialAppInfoResult& result) {
 }  // namespace
 
 DialMediaSinkServiceImpl::DialMediaSinkServiceImpl(
-    std::unique_ptr<service_manager::Connector> connector,
+    service_manager::Connector* connector,
     const OnSinksDiscoveredCallback& on_sinks_discovered_cb,
     const scoped_refptr<base::SequencedTaskRunner>& task_runner)
     : MediaSinkServiceBase(on_sinks_discovered_cb),
-      connector_(std::move(connector)),
+      data_decoder_(std::make_unique<DataDecoder>(connector)),
       task_runner_(task_runner) {
   DETACH_FROM_SEQUENCE(sequence_checker_);
 }
@@ -74,7 +75,7 @@ void DialMediaSinkServiceImpl::Start() {
     return;
 
   description_service_ = std::make_unique<DeviceDescriptionService>(
-      connector_.get(),
+      data_decoder_.get(),
       base::BindRepeating(
           &DialMediaSinkServiceImpl::OnDeviceDescriptionAvailable,
           base::Unretained(this)),
@@ -82,7 +83,7 @@ void DialMediaSinkServiceImpl::Start() {
                           base::Unretained(this)));
 
   app_discovery_service_ =
-      std::make_unique<DialAppDiscoveryService>(connector_.get());
+      std::make_unique<DialAppDiscoveryService>(data_decoder_.get());
 
   StartTimer();
 

chrome/browser/media/router/discovery/dial/dial_media_sink_service_impl.h

@@ -26,6 +26,7 @@ class Connector;
 
 namespace media_router {
 
+class DataDecoder;
 class DeviceDescriptionService;
 class DialRegistry;
 
@@ -56,7 +57,7 @@ class DialMediaSinkServiceImpl : public MediaSinkServiceBase,
   // Note that both callbacks are invoked on |task_runner|.
   // |task_runner|: The SequencedTaskRunner this class runs in.
   DialMediaSinkServiceImpl(
-      std::unique_ptr<service_manager::Connector> connector,
+      service_manager::Connector* connector,
       const OnSinksDiscoveredCallback& on_sinks_discovered_cb,
       const scoped_refptr<base::SequencedTaskRunner>& task_runner);
   ~DialMediaSinkServiceImpl() override;
@@ -171,8 +172,9 @@ class DialMediaSinkServiceImpl : public MediaSinkServiceBase,
   void OnDiscoveryComplete() override;
   void RecordDeviceCounts() override;
 
-  // Connector to ServiceManager for safe XML parsing requests.
-  std::unique_ptr<service_manager::Connector> connector_;
+  // Used for parsing XML. Shared by |description_service_| and
+  // |app_discovery_service_|.
+  std::unique_ptr<DataDecoder> data_decoder_;
 
   // Initialized in |Start()|.
   std::unique_ptr<DeviceDescriptionService> description_service_;

chrome/browser/media/router/discovery/dial/dial_media_sink_service_impl_unittest.cc

@@ -9,7 +9,9 @@
 #include "chrome/browser/media/router/discovery/dial/dial_registry.h"
 #include "chrome/browser/media/router/test/test_helper.h"
 #include "content/public/test/test_browser_thread_bundle.h"
+#include "services/data_decoder/data_decoder_service.h"
 #include "services/service_manager/public/cpp/connector.h"
+#include "services/service_manager/public/cpp/test/test_connector_factory.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -46,8 +48,12 @@ class DialMediaSinkServiceImplTest : public ::testing::Test {
  public:
   DialMediaSinkServiceImplTest()
       : thread_bundle_(content::TestBrowserThreadBundle::IO_MAINLOOP),
+        connector_factory_(
+            service_manager::TestConnectorFactory::CreateForUniqueService(
+                std::make_unique<data_decoder::DataDecoderService>())),
+        connector_(connector_factory_->CreateConnector()),
         media_sink_service_(new DialMediaSinkServiceImpl(
-            std::unique_ptr<service_manager::Connector>(),
+            connector_.get(),
             mock_sink_discovered_cb_.Get(),
             base::SequencedTaskRunnerHandle::Get())) {}
 
@@ -99,6 +105,8 @@ class DialMediaSinkServiceImplTest : public ::testing::Test {
 
  protected:
   const content::TestBrowserThreadBundle thread_bundle_;
+  std::unique_ptr<service_manager::TestConnectorFactory> connector_factory_;
+  std::unique_ptr<service_manager::Connector> connector_;
 
   base::MockCallback<OnSinksDiscoveredCallback> mock_sink_discovered_cb_;
   base::MockCallback<

chrome/browser/media/router/discovery/dial/safe_dial_app_info_parser.cc

@@ -9,8 +9,7 @@
 #include "base/bind.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
-#include "base/unguessable_token.h"
-#include "services/data_decoder/public/cpp/safe_xml_parser.h"
+#include "chrome/browser/media/router/data_decoder_util.h"
 #include "url/gurl.h"
 
 namespace media_router {
@@ -77,13 +76,8 @@ SafeDialAppInfoParser::ParsingResult ValidateParsedAppInfo(
 
 }  // namespace
 
-// Note we generate a random batch ID so that parsing operations started from
-// this SafeDialAppInfoParser instance run in the same utility process.
-SafeDialAppInfoParser::SafeDialAppInfoParser(
-    service_manager::Connector* connector)
-    : connector_(connector),
-      xml_parser_batch_id_(base::UnguessableToken::Create().ToString()),
-      weak_factory_(this) {}
+SafeDialAppInfoParser::SafeDialAppInfoParser(DataDecoder* data_decoder)
+    : data_decoder_(data_decoder), weak_factory_(this) {}
 
 SafeDialAppInfoParser::~SafeDialAppInfoParser() {}
 
@@ -92,11 +86,10 @@ void SafeDialAppInfoParser::Parse(const std::string& xml_text,
   DVLOG(2) << "Parsing app info...";
   DCHECK(callback);
 
-  data_decoder::ParseXml(
-      connector_, xml_text,
+  data_decoder_->ParseXml(
+      xml_text,
       base::BindOnce(&SafeDialAppInfoParser::OnXmlParsingDone,
-                     weak_factory_.GetWeakPtr(), std::move(callback)),
-      xml_parser_batch_id_);
+                     weak_factory_.GetWeakPtr(), std::move(callback)));
 }
 
 void SafeDialAppInfoParser::OnXmlParsingDone(

chrome/browser/media/router/discovery/dial/safe_dial_app_info_parser.h

@@ -15,12 +15,10 @@
 #include "base/values.h"
 #include "chrome/browser/media/router/discovery/dial/parsed_dial_app_info.h"
 
-namespace service_manager {
-class Connector;
-}
-
 namespace media_router {
 
+class DataDecoder;
+
 // SafeDialAppInfoParser parses the given app info XML file safely via a utility
 // process.
 // Spec for DIAL app info XML:
@@ -37,8 +35,7 @@ class SafeDialAppInfoParser {
     kInvalidState = 5
   };
 
-  // |connector| should be a valid connector to the ServiceManager.
-  explicit SafeDialAppInfoParser(service_manager::Connector* connector);
+  explicit SafeDialAppInfoParser(DataDecoder* data_decoder);
   virtual ~SafeDialAppInfoParser();
 
   // Callback function invoked when done parsing DIAL app info XML.
@@ -64,12 +61,8 @@ class SafeDialAppInfoParser {
                         std::unique_ptr<base::Value> value,
                         const base::Optional<std::string>& error);
 
-  // Connector to the ServiceManager, used to retrieve the XmlParser service.
-  service_manager::Connector* const connector_;
-
-  // The batch ID used to group XML parsing calls to SafeXmlParser into a single
-  // process.
-  std::string xml_parser_batch_id_;
+  // Used for parsing XML. Not owned by |this|.
+  DataDecoder* const data_decoder_;
 
   base::WeakPtrFactory<SafeDialAppInfoParser> weak_factory_;
 

chrome/browser/media/router/discovery/dial/safe_dial_app_info_parser_unittest.cc

@@ -10,6 +10,7 @@
 #include "base/macros.h"
 #include "base/run_loop.h"
 #include "base/strings/string_util.h"
+#include "chrome/browser/media/router/data_decoder_util.h"
 #include "content/public/test/test_browser_thread_bundle.h"
 #include "services/data_decoder/data_decoder_service.h"
 #include "services/data_decoder/public/cpp/safe_xml_parser.h"
@@ -105,7 +106,8 @@ class SafeDialAppInfoParserTest : public testing::Test {
       const std::string& xml,
       SafeDialAppInfoParser::ParsingResult expected_result) {
     base::RunLoop run_loop;
-    SafeDialAppInfoParser parser(connector_.get());
+    DataDecoder data_decoder(connector_.get());
+    SafeDialAppInfoParser parser(&data_decoder);
     parser.Parse(xml,
                  base::BindOnce(&SafeDialAppInfoParserTest::OnParsingCompleted,
                                 base::Unretained(this), expected_result));

chrome/browser/media/router/discovery/dial/safe_dial_device_description_parser.cc

@@ -8,8 +8,7 @@
 
 #include "base/bind.h"
 #include "base/strings/stringprintf.h"
-#include "base/unguessable_token.h"
-#include "services/data_decoder/public/cpp/safe_xml_parser.h"
+#include "chrome/browser/media/router/data_decoder_util.h"
 #include "url/gurl.h"
 
 namespace media_router {
@@ -35,13 +34,9 @@ void NotifyParsingError(SafeDialDeviceDescriptionParser::ParseCallback callback,
 
 }  // namespace
 
-// Note we generate a random batch ID so that parsing operations started from
-// this SafeDialDescriptionParser instance run in the same utility process.
 SafeDialDeviceDescriptionParser::SafeDialDeviceDescriptionParser(
-    service_manager::Connector* connector)
-    : connector_(connector),
-      xml_parser_batch_id_(base::UnguessableToken::Create().ToString()),
-      weak_factory_(this) {}
+    DataDecoder* data_decoder)
+    : data_decoder_(data_decoder), weak_factory_(this) {}
 
 SafeDialDeviceDescriptionParser::~SafeDialDeviceDescriptionParser() {}
 
@@ -51,11 +46,10 @@ void SafeDialDeviceDescriptionParser::Parse(const std::string& xml_text,
   DVLOG(2) << "Parsing device description...";
   DCHECK(callback);
 
-  data_decoder::ParseXml(
-      connector_, xml_text,
+  data_decoder_->ParseXml(
+      xml_text,
       base::BindOnce(&SafeDialDeviceDescriptionParser::OnXmlParsingDone,
-                     weak_factory_.GetWeakPtr(), std::move(callback), app_url),
-      xml_parser_batch_id_);
+                     weak_factory_.GetWeakPtr(), std::move(callback), app_url));
 }
 
 void SafeDialDeviceDescriptionParser::OnXmlParsingDone(

chrome/browser/media/router/discovery/dial/safe_dial_device_description_parser.h

@@ -17,12 +17,10 @@
 
 class GURL;
 
-namespace service_manager {
-class Connector;
-}
-
 namespace media_router {
 
+class DataDecoder;
+
 // SafeDialDeviceDescriptionParser parses the given device description XML file
 // safely via a utility process.
 // Spec for DIAL device description:
@@ -48,9 +46,7 @@ class SafeDialDeviceDescriptionParser {
     kTotalCount = 11,
   };
 
-  // |connector| should be a valid connector to the ServiceManager.
-  explicit SafeDialDeviceDescriptionParser(
-      service_manager::Connector* connector);
+  explicit SafeDialDeviceDescriptionParser(DataDecoder* data_decoder);
   ~SafeDialDeviceDescriptionParser();
 
   // Callback function invoked when done parsing some device description XML.
@@ -82,12 +78,8 @@ class SafeDialDeviceDescriptionParser {
                         std::unique_ptr<base::Value> value,
                         const base::Optional<std::string>& error);
 
-  // Connector to the ServiceManager, used to retrieve the XmlParser service.
-  service_manager::Connector* const connector_;
-
-  // The batch ID used to group XML parsing calls to SafeXmlParser into a single
-  // process.
-  std::string xml_parser_batch_id_;
+  // Used for parsing XML. Not owned by |this|.
+  DataDecoder* const data_decoder_;
 
   base::WeakPtrFactory<SafeDialDeviceDescriptionParser> weak_factory_;
 

chrome/browser/media/router/discovery/dial/safe_dial_device_description_parser_unittest.cc

@@ -10,6 +10,7 @@
 #include "base/macros.h"
 #include "base/run_loop.h"
 #include "base/strings/string_util.h"
+#include "chrome/browser/media/router/data_decoder_util.h"
 #include "content/public/test/test_browser_thread_bundle.h"
 #include "services/data_decoder/data_decoder_service.h"
 #include "services/service_manager/public/cpp/test/test_connector_factory.h"
@@ -91,7 +92,8 @@ class SafeDialDeviceDescriptionParserTest : public testing::Test {
     ParsedDialDeviceDescription device_description;
     SafeDialDeviceDescriptionParser::ParsingError error;
     base::RunLoop run_loop;
-    SafeDialDeviceDescriptionParser parser(connector_.get());
+    DataDecoder data_decoder(connector_.get());
+    SafeDialDeviceDescriptionParser parser(&data_decoder);
     parser.Parse(
         xml, app_url,
         base::BindOnce(

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

@@ -19,7 +19,9 @@
 #include "chrome/common/media_router/media_source_helper.h"
 #include "components/cast_channel/cast_socket_service.h"
 #include "content/public/browser/browser_thread.h"
+#include "content/public/common/service_manager_connection.h"
 #include "extensions/common/extension.h"
+#include "services/service_manager/public/cpp/connector.h"
 #if defined(OS_WIN)
 #include "chrome/browser/media/router/mojo/media_route_provider_util_win.h"
 #endif
@@ -278,11 +280,15 @@ void MediaRouterDesktop::InitializeDialMediaRouteProvider() {
   auto* dial_media_sink_service =
       media_sink_service_->GetDialMediaSinkServiceImpl();
   auto task_runner = dial_media_sink_service->task_runner();
+
+  service_manager::Connector* connector =
+      content::ServiceManagerConnection::GetForProcess()->GetConnector();
   dial_provider_ =
       std::unique_ptr<DialMediaRouteProvider, base::OnTaskRunnerDeleter>(
           new DialMediaRouteProvider(mojo::MakeRequest(&dial_provider_ptr),
                                      media_router_ptr.PassInterface(),
-                                     dial_media_sink_service, task_runner),
+                                     dial_media_sink_service, connector,
+                                     task_runner),
           base::OnTaskRunnerDeleter(task_runner));
   RegisterMediaRouteProvider(MediaRouteProviderId::DIAL,
                              std::move(dial_provider_ptr), base::DoNothing());

chrome/browser/media/router/providers/dial/dial_internal_message_util.cc

@@ -164,43 +164,39 @@ base::Value CreateDialMessageCommon(DialInternalMessageType type,
 
 // static
 std::unique_ptr<DialInternalMessage> DialInternalMessage::From(
-    const std::string& message) {
-  // TODO(https://crbug.com/816628): This may need to be parsed out of process.
-  std::unique_ptr<base::Value> message_value = base::JSONReader::Read(message);
-  if (!message_value) {
-    DVLOG(2) << "Failed to read JSON message: " << message;
+    base::Value message,
+    std::string* error) {
+  DCHECK(error);
+  base::Value* type_value =
+      message.FindKeyOfType("type", base::Value::Type::STRING);
+  if (!type_value) {
+    *error = "Missing type value";
     return nullptr;
   }
 
-  base::Value* type_value = message_value->FindKey("type");
-  if (!type_value || !type_value->is_string()) {
-    DVLOG(2) << "Missing type value";
-    return nullptr;
-  }
-
-  std::string str_type = type_value->GetString();
   DialInternalMessageType message_type =
-      StringToDialInternalMessageType(str_type);
+      StringToDialInternalMessageType(type_value->GetString());
   if (message_type == DialInternalMessageType::kOther) {
-    DVLOG(2) << __func__ << ": Unsupported message type: " << str_type;
+    *error = "Unsupported message type";
     return nullptr;
   }
 
-  base::Value* client_id_value = message_value->FindKey("clientId");
-  if (!client_id_value || !client_id_value->is_string()) {
-    DVLOG(2) << "Missing clientId";
+  base::Value* client_id_value =
+      message.FindKeyOfType("clientId", base::Value::Type::STRING);
+  if (!client_id_value) {
+    *error = "Missing clientId";
     return nullptr;
   }
 
-  // "message" is optional.
   base::Optional<base::Value> message_body;
-  base::Value* message_body_value = message_value->FindKey("message");
+  base::Value* message_body_value = message.FindKey("message");
   if (message_body_value)
-    message_body = message_body_value->Clone();
+    message_body = std::move(*message_body_value);
 
   int sequence_number = -1;
-  base::Value* sequence_number_value = message_value->FindKey("sequenceNumber");
-  if (sequence_number_value && sequence_number_value->is_int())
+  base::Value* sequence_number_value =
+      message.FindKeyOfType("sequenceNumber", base::Value::Type::INTEGER);
+  if (sequence_number_value)
     sequence_number = sequence_number_value->GetInt();
 
   return std::make_unique<DialInternalMessage>(
@@ -224,18 +220,20 @@ CustomDialLaunchMessageBody CustomDialLaunchMessageBody::From(
   DCHECK(message.type == DialInternalMessageType::kCustomDialLaunch);
 
   const base::Optional<base::Value>& body = message.body;
-  if (!body)
+  if (!body || !body->is_dict())
     return CustomDialLaunchMessageBody();
 
-  const base::Value* do_launch_value = body->FindKey("doLaunch");
-  if (!do_launch_value || !do_launch_value->is_bool())
+  const base::Value* do_launch_value =
+      body->FindKeyOfType("doLaunch", base::Value::Type::BOOLEAN);
+  if (!do_launch_value)
     return CustomDialLaunchMessageBody();
 
   bool do_launch = do_launch_value->GetBool();
 
   base::Optional<std::string> launch_parameter;
-  const base::Value* launch_parameter_value = body->FindKey("launchParameter");
-  if (launch_parameter_value && launch_parameter_value->is_string())
+  const base::Value* launch_parameter_value =
+      body->FindKeyOfType("launchParameter", base::Value::Type::STRING);
+  if (launch_parameter_value)
     launch_parameter = launch_parameter_value->GetString();
 
   return CustomDialLaunchMessageBody(do_launch, launch_parameter);
@@ -256,12 +254,13 @@ bool DialInternalMessageUtil::IsStopSessionMessage(
   if (message.type != DialInternalMessageType::kV2Message)
     return false;
 
-  if (!message.body)
+  const base::Optional<base::Value>& body = message.body;
+  if (!body || !body->is_dict())
     return false;
 
-  const base::Value* request_type = message.body->FindKey("type");
-  return request_type && request_type->is_string() &&
-         request_type->GetString() == "STOP";
+  const base::Value* request_type =
+      body->FindKeyOfType("type", base::Value::Type::STRING);
+  return request_type && request_type->GetString() == "STOP";
 }
 
 // static

chrome/browser/media/router/providers/dial/dial_internal_message_util.h

@@ -45,9 +45,11 @@ enum class DialReceiverAction {
 
 // Parsed custom DIAL launch internal message coming from a Cast SDK client.
 struct DialInternalMessage {
-  // Returns a DialInternalMessage for |message|, or nullptr is |message| is not
-  // a valid custom DIAL launch internal message.
-  static std::unique_ptr<DialInternalMessage> From(const std::string& message);
+  // Returns a DialInternalMessage for |message|. If |message| is not a valid
+  // custom DIAL launch internal message, returns nullptr and sets |error| with
+  // an error reason.
+  static std::unique_ptr<DialInternalMessage> From(base::Value message,
+                                                   std::string* error);
 
   DialInternalMessage(DialInternalMessageType type,
                       base::Optional<base::Value> body,

chrome/browser/media/router/providers/dial/dial_internal_message_util_unittest.cc

@@ -53,7 +53,7 @@ TEST_F(DialInternalMessageUtilTest, ParseClientConnectMessage) {
           "clientId":"15212681945883010"
         })";
 
-  auto message = DialInternalMessage::From(kClientConnectMessage);
+  auto message = ParseDialInternalMessage(kClientConnectMessage);
   ASSERT_TRUE(message);
   EXPECT_EQ(DialInternalMessageType::kClientConnect, message->type);
   EXPECT_EQ(base::Value("15212681945883010"), message->body);
@@ -74,7 +74,7 @@ TEST_F(DialInternalMessageUtilTest, ParseCustomDialLaunchMessage) {
     "clientId":"152127444812943594"
   })";
 
-  auto message = DialInternalMessage::From(kCustomDialLaunchMessage);
+  auto message = ParseDialInternalMessage(kCustomDialLaunchMessage);
   ASSERT_TRUE(message);
   EXPECT_EQ(DialInternalMessageType::kCustomDialLaunch, message->type);
   EXPECT_EQ("152127444812943594", message->client_id);
@@ -98,7 +98,7 @@ TEST_F(DialInternalMessageUtilTest, ParseV2StopSessionMessage) {
     "clientId":"152127444812943594"
   })";
 
-  auto message = DialInternalMessage::From(kV2StopSessionMessage);
+  auto message = ParseDialInternalMessage(kV2StopSessionMessage);
   ASSERT_TRUE(message);
   EXPECT_EQ(DialInternalMessageType::kV2Message, message->type);
   EXPECT_EQ("152127444812943594", message->client_id);

chrome/browser/media/router/providers/dial/dial_media_route_provider.cc

@@ -9,7 +9,9 @@
 #include "base/containers/flat_map.h"
 #include "base/no_destructor.h"
 #include "base/stl_util.h"
+#include "chrome/browser/media/router/data_decoder_util.h"
 #include "chrome/common/media_router/media_source_helper.h"
+#include "services/service_manager/public/cpp/connector.h"
 #include "url/origin.h"
 
 namespace media_router {
@@ -20,6 +22,11 @@ url::Origin CreateOrigin(const std::string& url) {
   return url::Origin::Create(GURL(url));
 }
 
+void ReportParseError(const std::string& error) {
+  // TODO(crbug.com/808720): Record UMA metric for parse result.
+  DVLOG(2) << "Failed to parse DIAL internal message: " << error;
+}
+
 static constexpr int kMaxPendingDialLaunches = 10;
 
 }  // namespace
@@ -28,9 +35,11 @@ DialMediaRouteProvider::DialMediaRouteProvider(
     mojom::MediaRouteProviderRequest request,
     mojom::MediaRouterPtrInfo media_router,
     DialMediaSinkServiceImpl* media_sink_service,
+    service_manager::Connector* connector,
     const scoped_refptr<base::SequencedTaskRunner>& task_runner)
     : binding_(this),
       media_sink_service_(media_sink_service),
+      data_decoder_(std::make_unique<DataDecoder>(connector)),
       weak_ptr_factory_(this) {
   DETACH_FROM_SEQUENCE(sequence_checker_);
   DCHECK(media_sink_service_);
@@ -178,18 +187,30 @@ void DialMediaRouteProvider::SendRouteMessage(
     const std::string& message,
     SendRouteMessageCallback callback) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  data_decoder_->ParseJson(
+      message,
+      base::BindRepeating(&DialMediaRouteProvider::HandleParsedRouteMessage,
+                          weak_ptr_factory_.GetWeakPtr(), media_route_id),
+      base::BindRepeating(&ReportParseError));
+  // TODO(https://crbug.com/866551): SendRouteMessageCallback is no longer used.
+  // Always invoke it with true until it is removed.
+  std::move(callback).Run(true);
+}
 
-  auto internal_message = DialInternalMessage::From(message);
+void DialMediaRouteProvider::HandleParsedRouteMessage(
+    const MediaRoute::Id& route_id,
+    std::unique_ptr<base::Value> message) {
+  std::string error;
+  std::unique_ptr<DialInternalMessage> internal_message =
+      DialInternalMessage::From(std::move(*message), &error);
   if (!internal_message) {
-    DVLOG(2) << "Fail to parse message";
-    std::move(callback).Run(true);
+    ReportParseError(error);
     return;
   }
 
-  const DialActivity* activity = activity_manager_->GetActivity(media_route_id);
+  const DialActivity* activity = activity_manager_->GetActivity(route_id);
   if (!activity) {
-    DVLOG(2) << "No activity record found with route_id " << media_route_id;
-    std::move(callback).Run(true);
+    DVLOG(2) << "No activity record found with route_id " << route_id;
     return;
   }
 
@@ -198,11 +219,10 @@ void DialMediaRouteProvider::SendRouteMessage(
       media_sink_service_->GetSinkById(route.media_sink_id());
   if (!sink) {
     DVLOG(2) << __func__ << ": Sink not found: " << route.media_sink_id();
-    std::move(callback).Run(true);
     return;
   }
 
-  DVLOG(2) << __func__ << ": Recieved message from:" << media_route_id;
+  DVLOG(2) << __func__ << ": Recieved message from:" << route_id;
   // TODO(https://crbug.com/816628): Investigate whether the direct use of
   // PresentationConnection in this class to communicate with the SDK client can
   // result in eliminating the need for CLIENT_CONNECT messages.
@@ -214,8 +234,6 @@ void DialMediaRouteProvider::SendRouteMessage(
   } else if (DialInternalMessageUtil::IsStopSessionMessage(*internal_message)) {
     DoTerminateRoute(*activity, *sink, base::DoNothing());
   }
-
-  std::move(callback).Run(true);
 }
 
 void DialMediaRouteProvider::HandleClientConnect(

chrome/browser/media/router/providers/dial/dial_media_route_provider.h

@@ -28,6 +28,8 @@ class Origin;
 
 namespace media_router {
 
+class DataDecoder;
+
 // MediaRouteProvider for DIAL sinks.
 // DialMediaRouteProvider supports custom DIAL launch, which is a
 // way for websites that uses Cast SDK to launch apps on DIAL devices.
@@ -45,10 +47,16 @@ namespace media_router {
 //    communicate with the app on the device via its own mechanism.
 class DialMediaRouteProvider : public mojom::MediaRouteProvider {
  public:
+  // |request|: Request to bind to |this|.
+  // |media_router|: Pointer to MediaRouter.
+  // |media_sink_service|: DIAL MediaSinkService providing information on sinks.
+  // |connector|: Connector object for accessing data_decoder services.
+  // |task_runner|: The task runner on which |this| runs.
   DialMediaRouteProvider(
       mojom::MediaRouteProviderRequest request,
       mojom::MediaRouterPtrInfo media_router,
       DialMediaSinkServiceImpl* media_sink_service,
+      service_manager::Connector* connector,
       const scoped_refptr<base::SequencedTaskRunner>& task_runner);
   ~DialMediaRouteProvider() override;
 
@@ -140,6 +148,8 @@ class DialMediaRouteProvider : public mojom::MediaRouteProvider {
                              const std::vector<MediaSinkInternal>& sinks,
                              const std::vector<url::Origin>& origins);
 
+  void HandleParsedRouteMessage(const MediaRoute::Id& route_id,
+                                std::unique_ptr<base::Value> message);
   void HandleClientConnect(const DialActivity& activity,
                            const MediaSinkInternal& sink);
   void SendCustomDialLaunchMessage(const MediaRoute::Id& route_id,
@@ -187,6 +197,9 @@ class DialMediaRouteProvider : public mojom::MediaRouteProvider {
   std::unique_ptr<DialActivityManager> activity_manager_;
   std::unique_ptr<BufferedMessageSender> message_sender_;
 
+  // Used for parsing Custom DIAL launch JSON messages.
+  std::unique_ptr<DataDecoder> data_decoder_;
+
   SEQUENCE_CHECKER(sequence_checker_);
   base::WeakPtrFactory<DialMediaRouteProvider> weak_ptr_factory_;
   DISALLOW_COPY_AND_ASSIGN(DialMediaRouteProvider);

chrome/browser/media/router/providers/dial/dial_media_route_provider_unittest.cc

@@ -13,7 +13,10 @@
 #include "chrome/browser/media/router/route_message_util.h"
 #include "chrome/browser/media/router/test/test_helper.h"
 #include "net/http/http_status_code.h"
+#include "services/data_decoder/data_decoder_service.h"
+#include "services/data_decoder/public/cpp/testing_json_parser.h"
 #include "services/service_manager/public/cpp/connector.h"
+#include "services/service_manager/public/cpp/test/test_connector_factory.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -26,8 +29,8 @@ namespace media_router {
 
 class TestDialMediaSinkServiceImpl : public DialMediaSinkServiceImpl {
  public:
-  TestDialMediaSinkServiceImpl()
-      : DialMediaSinkServiceImpl(/* connector */ nullptr,
+  explicit TestDialMediaSinkServiceImpl(service_manager::Connector* connector)
+      : DialMediaSinkServiceImpl(connector,
                                  base::DoNothing(),
                                  /* task_runner */ nullptr) {}
 
@@ -80,7 +83,12 @@ class TestDialMediaSinkServiceImpl : public DialMediaSinkServiceImpl {
 
 class DialMediaRouteProviderTest : public ::testing::Test {
  public:
-  DialMediaRouteProviderTest() {}
+  DialMediaRouteProviderTest()
+      : connector_factory_(
+            service_manager::TestConnectorFactory::CreateForUniqueService(
+                std::make_unique<data_decoder::DataDecoderService>())),
+        connector_(connector_factory_->CreateConnector()),
+        mock_sink_service_(connector_.get()) {}
 
   void SetUp() override {
     mojom::MediaRouterPtr router_ptr;
@@ -90,7 +98,8 @@ class DialMediaRouteProviderTest : public ::testing::Test {
     EXPECT_CALL(mock_router_, OnSinkAvailabilityUpdated(_, _));
     provider_ = std::make_unique<DialMediaRouteProvider>(
         mojo::MakeRequest(&provider_ptr_), router_ptr.PassInterface(),
-        &mock_sink_service_, base::SequencedTaskRunnerHandle::Get());
+        &mock_sink_service_, connector_.get(),
+        base::SequencedTaskRunnerHandle::Get());
 
     auto activity_manager =
         std::make_unique<TestDialActivityManager>(&loader_factory_);
@@ -126,7 +135,7 @@ class DialMediaRouteProviderTest : public ::testing::Test {
   void ExpectDialInternalMessageType(const RouteMessagePtr& message,
                                      DialInternalMessageType expected_type) {
     ASSERT_TRUE(message->message);
-    auto internal_message = DialInternalMessage::From(message->message.value());
+    auto internal_message = ParseDialInternalMessage(*message->message);
     ASSERT_TRUE(internal_message);
     EXPECT_EQ(expected_type, internal_message->type);
   }
@@ -188,6 +197,7 @@ class DialMediaRouteProviderTest : public ::testing::Test {
                 DoFetchDialAppInfo(_, _));
     provider_->SendRouteMessage(route_id, kClientConnectMessage,
                                 base::DoNothing());
+    base::RunLoop().RunUntilIdle();
     auto app_info_cb =
         mock_sink_service_.app_discovery_service()->PassCallback();
     ASSERT_FALSE(app_info_cb.is_null());
@@ -209,8 +219,9 @@ class DialMediaRouteProviderTest : public ::testing::Test {
     ASSERT_EQ(1u, received_messages.size());
     ExpectDialInternalMessageType(received_messages[0],
                                   DialInternalMessageType::kCustomDialLaunch);
+    std::string error_unused;
     auto internal_message =
-        DialInternalMessage::From(received_messages[0]->message.value());
+        ParseDialInternalMessage(*received_messages[0]->message);
     ASSERT_TRUE(internal_message);
     EXPECT_GE(internal_message->sequence_number, 0);
     custom_dial_launch_seq_number_ = internal_message->sequence_number;
@@ -244,6 +255,7 @@ class DialMediaRouteProviderTest : public ::testing::Test {
         base::StringPrintf(kCustomDialLaunchMessage,
                            custom_dial_launch_seq_number_),
         base::DoNothing());
+    base::RunLoop().RunUntilIdle();
 
     // Simulate a successful launch response.
     app_instance_url_ = GURL(app_launch_url.spec() + "/run");
@@ -258,6 +270,7 @@ class DialMediaRouteProviderTest : public ::testing::Test {
     base::RunLoop().RunUntilIdle();
 
     ASSERT_EQ(1u, routes.size());
+    // TODO(https://crbug.com/867935): Replace with operator== / EXPECT_TRUE.
     EXPECT_TRUE(routes[0].Equals(*route_));
   }
 
@@ -366,12 +379,17 @@ class DialMediaRouteProviderTest : public ::testing::Test {
 
  protected:
   base::test::ScopedTaskEnvironment environment_;
+  std::unique_ptr<service_manager::TestConnectorFactory> connector_factory_;
+  std::unique_ptr<service_manager::Connector> connector_;
+
   network::TestURLLoaderFactory loader_factory_;
 
   mojom::MediaRouteProviderPtr provider_ptr_;
   MockMojoMediaRouter mock_router_;
   std::unique_ptr<mojo::Binding<mojom::MediaRouter>> router_binding_;
 
+  data_decoder::TestingJsonParser::ScopedFactoryOverride parser_override_;
+
   TestDialMediaSinkServiceImpl mock_sink_service_;
   TestDialActivityManager* activity_manager_ = nullptr;
   std::unique_ptr<DialMediaRouteProvider> provider_;

chrome/browser/media/router/test/test_helper.cc

@@ -9,6 +9,7 @@
 #include "testing/gmock/include/gmock/gmock.h"
 
 #if !defined(OS_ANDROID)
+#include "base/json/json_reader.h"
 #include "services/network/public/cpp/simple_url_loader.h"
 #include "url/gurl.h"
 #endif
@@ -172,6 +173,15 @@ std::unique_ptr<ParsedDialAppInfo> CreateParsedDialAppInfoPtr(
       CreateParsedDialAppInfo(name, app_state));
 }
 
+std::unique_ptr<DialInternalMessage> ParseDialInternalMessage(
+    const std::string& message) {
+  auto message_value = base::JSONReader::Read(message);
+  std::string error_unused;
+  return message_value ? DialInternalMessage::From(std::move(*message_value),
+                                                   &error_unused)
+                       : nullptr;
+}
+
 #endif  // !defined(OS_ANDROID)
 
 }  // namespace media_router

chrome/browser/media/router/test/test_helper.h

@@ -22,13 +22,13 @@
 #include "third_party/blink/public/mojom/presentation/presentation.mojom.h"
 
 #if !defined(OS_ANDROID)
-
 #include "chrome/browser/media/router/discovery/dial/dial_media_sink_service.h"
 #include "chrome/browser/media/router/discovery/dial/dial_url_fetcher.h"
 #include "chrome/browser/media/router/discovery/mdns/cast_media_sink_service.h"
 #include "chrome/browser/media/router/discovery/mdns/cast_media_sink_service_impl.h"
 #include "chrome/browser/media/router/providers/cast/cast_app_discovery_service.h"
 #include "chrome/browser/media/router/providers/dial/dial_activity_manager.h"
+#include "chrome/browser/media/router/providers/dial/dial_internal_message_util.h"
 #include "chrome/common/media_router/discovery/media_sink_internal.h"
 #include "net/base/ip_endpoint.h"
 #include "services/network/test/test_url_loader_factory.h"
@@ -238,6 +238,10 @@ ParsedDialAppInfo CreateParsedDialAppInfo(const std::string& name,
 std::unique_ptr<ParsedDialAppInfo> CreateParsedDialAppInfoPtr(
     const std::string& name,
     DialAppState app_state);
+
+std::unique_ptr<DialInternalMessage> ParseDialInternalMessage(
+    const std::string& message);
+
 #endif  // !defined(OS_ANDROID)
 
 }  // namespace media_router