Remove more SafeJsonParser and SafeXmlParser usage

This adds new ParseXml() and ParseXmlIsolated() calls to the
DataDecoder client API, which works without Service Manager.

Media Router code and all other ParseXml clients are ported to use
this API. MediaRouter is also moved off of SafeJsonParser.

No net functional changes.

Bug: 977637
Change-Id: I3a04a343d9f24747deeb9eac61df246d4e0129c2
Tbr: dmazzoni@chromium.org
Tbr: tbarzic@chromium.org
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1884593
Commit-Queue: Ken Rockot <rockot@google.com>
Reviewed-by: mark a. foltz <mfoltz@chromium.org>
Reviewed-by: Nicolas Ouellet-Payeur <nicolaso@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#711943}

chrome/browser/browser_switcher/ieem_sitelist_parser.cc

@@ -6,11 +6,9 @@
 
 #include "base/bind.h"
 #include "content/public/browser/browser_thread.h"
-#include "content/public/browser/system_connector.h"
+#include "services/data_decoder/public/cpp/data_decoder.h"
 #include "services/data_decoder/public/cpp/safe_xml_parser.h"
-#include "services/data_decoder/public/mojom/constants.mojom.h"
 #include "services/data_decoder/public/mojom/xml_parser.mojom.h"
-#include "services/service_manager/public/cpp/connector.h"
 
 namespace browser_switcher {
 
@@ -128,23 +126,22 @@ void ParseIeFileVersionTwo(const base::Value& xml, ParsedXml* result) {
 }
 
 void RawXmlParsed(base::OnceCallback<void(ParsedXml)> callback,
-                  std::unique_ptr<base::Value> xml,
-                  const base::Optional<std::string>& error) {
-  if (error) {
+                  data_decoder::DataDecoder::ValueOrError xml) {
+  if (!xml.value) {
     // Copies the string, but it should only be around 20 characters.
-    std::move(callback).Run(ParsedXml({}, *error));
+    std::move(callback).Run(ParsedXml({}, *xml.error));
     return;
   }
-  DCHECK(xml);
   DCHECK(data_decoder::IsXmlElementOfType(
-      *xml, data_decoder::mojom::XmlParser::kElementType));
+      *xml.value, data_decoder::mojom::XmlParser::kElementType));
   ParsedXml result;
-  if (data_decoder::IsXmlElementNamed(*xml, kSchema1RulesElement)) {
+  if (data_decoder::IsXmlElementNamed(*xml.value, kSchema1RulesElement)) {
     // Enterprise Mode schema v.1 has <rules> element at its top level.
-    ParseIeFileVersionOne(*xml, &result);
-  } else if (data_decoder::IsXmlElementNamed(*xml, kSchema2SiteListElement)) {
+    ParseIeFileVersionOne(*xml.value, &result);
+  } else if (data_decoder::IsXmlElementNamed(*xml.value,
+                                             kSchema2SiteListElement)) {
     // Enterprise Mode schema v.2 has <site-list> element at its top level.
-    ParseIeFileVersionTwo(*xml, &result);
+    ParseIeFileVersionTwo(*xml.value, &result);
   } else {
     result.error = kInvalidRootElement;
   }
@@ -162,8 +159,8 @@ ParsedXml::~ParsedXml() = default;
 
 void ParseIeemXml(const std::string& xml,
                   base::OnceCallback<void(ParsedXml)> callback) {
-  data_decoder::ParseXml(content::GetSystemConnector(), xml,
-                         base::BindOnce(&RawXmlParsed, std::move(callback)));
+  data_decoder::DataDecoder::ParseXmlIsolated(
+      xml, base::BindOnce(&RawXmlParsed, std::move(callback)));
 }
 
 }  // namespace browser_switcher

chrome/browser/chromeos/extensions/device_local_account_external_policy_loader_unittest.cc

@@ -40,6 +40,7 @@
 #include "extensions/common/extension.h"
 #include "extensions/common/extension_urls.h"
 #include "extensions/common/manifest.h"
+#include "services/data_decoder/public/cpp/test_support/in_process_data_decoder.h"
 #include "services/network/public/cpp/weak_wrapper_shared_url_loader_factory.h"
 #include "services/network/test/test_url_loader_factory.h"
 #include "testing/gmock/include/gmock/gmock.h"
@@ -166,6 +167,7 @@ class DeviceLocalAccountExternalPolicyLoaderTest : public testing::Test {
   void SetForceInstallListPolicy();
 
   content::BrowserTaskEnvironment task_environment_;
+  data_decoder::test::InProcessDataDecoder in_process_data_decoder_;
   std::unique_ptr<TestingProfile> profile_;
   base::ScopedTempDir temp_dir_;
   base::FilePath cache_dir_;

chrome/browser/chromeos/extensions/external_cache_impl.cc

@@ -24,7 +24,6 @@
 #include "content/public/browser/notification_details.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/notification_source.h"
-#include "content/public/browser/system_connector.h"
 #include "extensions/browser/notification_types.h"
 #include "extensions/browser/updater/extension_downloader.h"
 #include "extensions/common/extension.h"
@@ -250,12 +249,6 @@ bool ExternalCacheImpl::GetExtensionExistingVersion(const std::string& id,
   return false;
 }
 
-service_manager::Connector* ExternalCacheImpl::GetConnector() {
-  if (use_null_connector_)
-    return nullptr;
-  return content::GetSystemConnector();
-}
-
 void ExternalCacheImpl::UpdateExtensionLoader() {
   VLOG(1) << "Notify ExternalCacheImpl delegate about cache update";
   if (delegate_)
@@ -269,8 +262,7 @@ void ExternalCacheImpl::CheckCache() {
   // If url_loader_factory_ is missing we can't download anything.
   if (url_loader_factory_) {
     downloader_ = ChromeExtensionDownloaderFactory::CreateForURLLoaderFactory(
-        url_loader_factory_, this, GetConnector(),
-        extensions::GetExternalVerifierFormat());
+        url_loader_factory_, this, extensions::GetExternalVerifierFormat());
   }
 
   cached_extensions_->Clear();

chrome/browser/chromeos/extensions/external_cache_impl.h

@@ -34,10 +34,6 @@ namespace network {
 class SharedURLLoaderFactory;
 }
 
-namespace service_manager {
-class Connector;
-}
-
 namespace chromeos {
 
 class ExternalCacheDelegate;
@@ -101,13 +97,7 @@ class ExternalCacheImpl : public ExternalCache,
 
   void set_flush_on_put(bool flush_on_put) { flush_on_put_ = flush_on_put; }
 
-  void use_null_connector_for_test() { use_null_connector_ = true; }
-
  private:
-  // Gets service manager connector this external cache instance should use.
-  // Might be null in tests - see use_null_connector_for_test().
-  service_manager::Connector* GetConnector();
-
   // Notifies the that the cache has been updated, providing
   // extensions loader with an updated list of extensions.
   void UpdateExtensionLoader();
@@ -147,8 +137,6 @@ class ExternalCacheImpl : public ExternalCache,
   // Whether to flush the crx file after putting into |local_cache_|.
   bool flush_on_put_ = false;
 
-  bool use_null_connector_ = false;
-
   // This is the list of extensions currently configured.
   std::unique_ptr<base::DictionaryValue> extensions_;
 

chrome/browser/chromeos/extensions/external_cache_impl_unittest.cc

@@ -140,7 +140,6 @@ TEST_F(ExternalCacheImplTest, Basic) {
       cache_dir, url_loader_factory(),
       base::CreateSequencedTaskRunner({base::ThreadPool(), base::MayBlock()}),
       this, true, false);
-  external_cache.use_null_connector_for_test();
 
   std::unique_ptr<base::DictionaryValue> prefs(new base::DictionaryValue);
   prefs->Set(kTestExtensionId1, CreateEntryWithUpdateUrl(true));
@@ -263,7 +262,6 @@ TEST_F(ExternalCacheImplTest, PreserveInstalled) {
       cache_dir, url_loader_factory(),
       base::CreateSequencedTaskRunner({base::ThreadPool(), base::MayBlock()}),
       this, true, false);
-  external_cache.use_null_connector_for_test();
 
   std::unique_ptr<base::DictionaryValue> prefs(new base::DictionaryValue);
   prefs->Set(kTestExtensionId1, CreateEntryWithUpdateUrl(true));

chrome/browser/chromeos/login/demo_mode/demo_extensions_external_loader_unittest.cc

@@ -40,6 +40,7 @@
 #include "extensions/browser/notification_types.h"
 #include "extensions/common/extension.h"
 #include "extensions/common/manifest.h"
+#include "services/data_decoder/public/cpp/test_support/in_process_data_decoder.h"
 #include "services/network/public/cpp/weak_wrapper_shared_url_loader_factory.h"
 #include "services/network/test/test_url_loader_factory.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -223,6 +224,7 @@ class DemoExtensionsExternalLoaderTest : public testing::Test {
   scoped_refptr<network::WeakWrapperSharedURLLoaderFactory>
       test_shared_loader_factory_;
 
+  data_decoder::test::InProcessDataDecoder in_process_data_decoder_;
   content::InProcessUtilityThreadHelper in_process_utility_thread_helper_;
 
   user_manager::ScopedUserManager scoped_user_manager_;

chrome/browser/extensions/updater/chrome_extension_downloader_factory.cc

@@ -17,7 +17,6 @@
 #include "components/signin/public/identity_manager/identity_manager.h"
 #include "components/update_client/update_query_params.h"
 #include "content/public/browser/storage_partition.h"
-#include "content/public/browser/system_connector.h"
 #include "extensions/browser/extension_prefs.h"
 #include "extensions/browser/updater/extension_downloader.h"
 #include "extensions/common/verifier_formats.h"
@@ -31,12 +30,11 @@ std::unique_ptr<ExtensionDownloader>
 ChromeExtensionDownloaderFactory::CreateForURLLoaderFactory(
     scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory,
     ExtensionDownloaderDelegate* delegate,
-    service_manager::Connector* connector,
     crx_file::VerifierFormat required_verifier_format,
     const base::FilePath& profile_path) {
-  std::unique_ptr<ExtensionDownloader> downloader(new ExtensionDownloader(
-      delegate, std::move(url_loader_factory), connector,
-      required_verifier_format, profile_path));
+  std::unique_ptr<ExtensionDownloader> downloader(
+      new ExtensionDownloader(delegate, std::move(url_loader_factory),
+                              required_verifier_format, profile_path));
 #if BUILDFLAG(GOOGLE_CHROME_BRANDING)
   std::string brand;
   google_brand::GetBrand(&brand);
@@ -62,8 +60,7 @@ ChromeExtensionDownloaderFactory::CreateForProfile(
   std::unique_ptr<ExtensionDownloader> downloader = CreateForURLLoaderFactory(
       content::BrowserContext::GetDefaultStoragePartition(profile)
           ->GetURLLoaderFactoryForBrowserProcess(),
-      delegate, content::GetSystemConnector(),
-      extensions::GetPolicyVerifierFormat(), profile->GetPath());
+      delegate, extensions::GetPolicyVerifierFormat(), profile->GetPath());
 
   // NOTE: It is not obvious why it is OK to pass raw pointers to the token
   // service and identity manager here. The logic is as follows:

chrome/browser/extensions/updater/chrome_extension_downloader_factory.h

@@ -25,10 +25,6 @@ namespace network {
 class SharedURLLoaderFactory;
 }  // namespace network
 
-namespace service_manager {
-class Connector;
-}
-
 // This provides a simple static interface for constructing an
 // ExtensionDownloader suitable for use from within Chrome.
 class ChromeExtensionDownloaderFactory {
@@ -47,7 +43,6 @@ class ChromeExtensionDownloaderFactory {
   CreateForURLLoaderFactory(
       scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory,
       extensions::ExtensionDownloaderDelegate* delegate,
-      service_manager::Connector* connector,
       crx_file::VerifierFormat required_verifier_format,
       const base::FilePath& profile_path = base::FilePath());
 

chrome/browser/extensions/updater/extension_updater_unittest.cc

@@ -335,7 +335,7 @@ class MockService : public TestExtensionService {
             test_shared_url_loader_factory_,
             downloader_delegate_override_ ? downloader_delegate_override_
                                           : delegate,
-            /*connector=*/nullptr, GetTestVerifierFormat());
+            GetTestVerifierFormat());
     return downloader;
   }
 

chrome/browser/media/router/data_decoder_util.cc

@@ -4,28 +4,13 @@
 
 #include "chrome/browser/media/router/data_decoder_util.h"
 
-#include "services/service_manager/public/cpp/connector.h"
+#include "base/no_destructor.h"
 
 namespace media_router {
 
-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,
-    data_decoder::SafeJsonParser::SuccessCallback success_callback,
-    data_decoder::SafeJsonParser::ErrorCallback error_callback) {
-  data_decoder::SafeJsonParser::ParseBatch(
-      connector_.get(), unsafe_json, std::move(success_callback),
-      std::move(error_callback), kDataDecoderServiceBatchId);
+data_decoder::DataDecoder& GetDataDecoder() {
+  static base::NoDestructor<data_decoder::DataDecoder> decoder;
+  return *decoder;
 }
 
 }  // namespace media_router

chrome/browser/media/router/data_decoder_util.h

@@ -5,43 +5,13 @@
 #ifndef CHROME_BROWSER_MEDIA_ROUTER_DATA_DECODER_UTIL_H_
 #define CHROME_BROWSER_MEDIA_ROUTER_DATA_DECODER_UTIL_H_
 
-#include <string>
-
-#include "base/token.h"
-#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;
-}
+#include "services/data_decoder/public/cpp/data_decoder.h"
 
 namespace media_router {
 
-// The batch ID used by data_decoder_util functions.
-static constexpr base::Token kDataDecoderServiceBatchId{0xabf3003d50bb0170ull,
-                                                        0x0c659c570136566eull};
-
-// 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,
-                 data_decoder::SafeJsonParser::SuccessCallback success_callback,
-                 data_decoder::SafeJsonParser::ErrorCallback error_callback);
-
- private:
-  std::unique_ptr<service_manager::Connector> connector_;
-  DISALLOW_COPY_AND_ASSIGN(DataDecoder);
-};
+// Returns a shared DataDecoder instance used to handle all decoding operations
+// related to Media Router.
+data_decoder::DataDecoder& GetDataDecoder();
 
 }  // namespace media_router
 

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

@@ -14,7 +14,6 @@
 #endif
 
 #include "base/stl_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"
@@ -80,12 +79,9 @@ ParsingError ValidateParsedDeviceDescription(
 }  // namespace
 
 DeviceDescriptionService::DeviceDescriptionService(
-    DataDecoder* data_decoder,
     const DeviceDescriptionParseSuccessCallback& success_cb,
     const DeviceDescriptionParseErrorCallback& error_cb)
-    : success_cb_(success_cb),
-      error_cb_(error_cb),
-      device_description_parser_(data_decoder) {}
+    : success_cb_(success_cb), error_cb_(error_cb) {}
 
 DeviceDescriptionService::~DeviceDescriptionService() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);

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

@@ -19,7 +19,6 @@
 
 namespace media_router {
 
-class DataDecoder;
 class DeviceDescriptionFetcher;
 class SafeDialDeviceDescriptionParser;
 
@@ -61,7 +60,6 @@ class DeviceDescriptionService {
                                    const std::string& error_message)>;
 
   DeviceDescriptionService(
-      DataDecoder* data_decoder,
       const DeviceDescriptionParseSuccessCallback& success_cb,
       const DeviceDescriptionParseErrorCallback& error_cb);
   virtual ~DeviceDescriptionService();

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

@@ -55,7 +55,7 @@ class TestDeviceDescriptionService : public DeviceDescriptionService {
   TestDeviceDescriptionService(
       const DeviceDescriptionParseSuccessCallback& success_cb,
       const DeviceDescriptionParseErrorCallback& error_cb)
-      : DeviceDescriptionService(nullptr, success_cb, error_cb) {}
+      : DeviceDescriptionService(success_cb, error_cb) {}
 
   MOCK_METHOD2(ParseDeviceDescription,
                void(const DialDeviceData&, const DialDeviceDescriptionData&));

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

@@ -36,8 +36,8 @@ DialAppInfoResult::DialAppInfoResult(DialAppInfoResult&& other) = default;
 
 DialAppInfoResult::~DialAppInfoResult() = default;
 
-DialAppDiscoveryService::DialAppDiscoveryService(DataDecoder* data_decoder)
-    : parser_(std::make_unique<SafeDialAppInfoParser>(data_decoder)) {}
+DialAppDiscoveryService::DialAppDiscoveryService()
+    : parser_(std::make_unique<SafeDialAppInfoParser>()) {}
 
 DialAppDiscoveryService::~DialAppDiscoveryService() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);

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

@@ -22,8 +22,6 @@
 
 namespace media_router {
 
-class DataDecoder;
-
 // Represents DIAL app status on receiver device.
 // These values are persisted to logs. Entries should not be renumbered and
 // numeric values should never be reused.
@@ -64,7 +62,7 @@ class DialAppDiscoveryService {
                               const std::string& app_name,
                               DialAppInfoResult result)>;
 
-  explicit DialAppDiscoveryService(DataDecoder* data_decoder);
+  DialAppDiscoveryService();
 
   virtual ~DialAppDiscoveryService();