Refactor BundledExchangesSource

Bug: 966753
Change-Id: Id51e9b7ffe3ca34b950c7bd2adcbd05af8acc776
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1784418Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Kunihiko Sakamoto <ksakamoto@chromium.org>
Commit-Queue: Tsuyoshi Horo <horo@chromium.org>
Cr-Commit-Position: refs/heads/master@{#694151}

content/browser/frame_host/navigation_request.cc

@@ -1916,13 +1916,9 @@ void NavigationRequest::OnStartChecksComplete(
     const base::FilePath specified_path =
         base::CommandLine::ForCurrentProcess()->GetSwitchValuePath(
             switches::kTrustableBundledExchangesFile);
-    base::FilePath url_path;
-    if (net::FileURLToFilePath(common_params_->url, &url_path) &&
-        url_path == specified_path) {
-      BundledExchangesSource source(specified_path);
-      source.is_trusted = true;
-      bundled_exchanges_handle_ =
-          std::make_unique<BundledExchangesHandle>(source);
+    if (net::FilePathToFileURL(specified_path) == common_params_->url) {
+      bundled_exchanges_handle_ = std::make_unique<BundledExchangesHandle>(
+          BundledExchangesSource::CreateFromTrustedFile(specified_path));
     }
   } else if (base::FeatureList::IsEnabled(features::kBundledHTTPExchanges)) {
     // Production path behind the feature flag.

content/browser/web_package/bundled_exchanges_handle.cc

@@ -152,19 +152,16 @@ class BundledExchangesHandle::PrimaryURLRedirectLoader final
   DISALLOW_COPY_AND_ASSIGN(PrimaryURLRedirectLoader);
 };
 
-BundledExchangesHandle::BundledExchangesHandle()
-    : BundledExchangesHandle(BundledExchangesSource()) {}
+BundledExchangesHandle::BundledExchangesHandle() {}
 
 BundledExchangesHandle::BundledExchangesHandle(
-    const BundledExchangesSource& bundled_exchanges_source)
-    : source_(bundled_exchanges_source) {
+    std::unique_ptr<BundledExchangesSource> bundled_exchanges_source)
+    : source_(std::move(bundled_exchanges_source)),
+      reader_(std::make_unique<BundledExchangesReader>(*source_)) {
+  DCHECK(source_->is_trusted());
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
-  if (bundled_exchanges_source.IsValid()) {
-    reader_ =
-        std::make_unique<BundledExchangesReader>(bundled_exchanges_source);
-    reader_->ReadMetadata(base::BindOnce(
-        &BundledExchangesHandle::OnMetadataReady, weak_factory_.GetWeakPtr()));
-  }
+  reader_->ReadMetadata(base::BindOnce(&BundledExchangesHandle::OnMetadataReady,
+                                       weak_factory_.GetWeakPtr()));
 }
 
 BundledExchangesHandle::~BundledExchangesHandle() {
@@ -175,10 +172,9 @@ std::unique_ptr<NavigationLoaderInterceptor>
 BundledExchangesHandle::CreateInterceptor() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   return std::make_unique<Interceptor>(
-      source_.IsValid()
-          ? base::BindRepeating(&BundledExchangesHandle::CreateURLLoader,
-                                weak_factory_.GetWeakPtr())
-          : base::NullCallback());
+      reader_ ? base::BindRepeating(&BundledExchangesHandle::CreateURLLoader,
+                                    weak_factory_.GetWeakPtr())
+              : base::NullCallback());
 }
 
 void BundledExchangesHandle::CreateURLLoaderFactory(
@@ -199,9 +195,7 @@ void BundledExchangesHandle::CreateURLLoader(
     const network::ResourceRequest& resource_request,
     network::mojom::URLLoaderRequest request,
     network::mojom::URLLoaderClientPtr client) {
-  DCHECK(source_.is_trusted);
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
-
   if (metadata_error_) {
     client->OnComplete(
         network::URLLoaderCompletionStatus(net::ERR_INVALID_BUNDLED_EXCHANGES));
@@ -210,7 +204,7 @@ void BundledExchangesHandle::CreateURLLoader(
 
   if (!url_loader_factory_) {
     // This must be the first request to the bundled exchange file.
-    DCHECK(source_.Match(resource_request.url));
+    DCHECK_EQ(source_->url(), resource_request.url);
     pending_create_url_loader_task_ = base::Bind(
         &BundledExchangesHandle::CreateURLLoader, base::Unretained(this),
         resource_request, base::Passed(&request), base::Passed(&client));
@@ -220,7 +214,7 @@ void BundledExchangesHandle::CreateURLLoader(
   // Currently |source_| must be a local file. And the bundle's primary URL
   // can't be a local file URL. So while handling redirected request to the
   // primary URL, |resource_request.url| must not be same as the |source|'s URL.
-  if (!source_.Match(resource_request.url)) {
+  if (source_->url() != resource_request.url) {
     url_loader_factory_->CreateLoaderAndStart(
         std::move(request), /*routing_id=*/0, /*request_id=*/0, /*options=*/0,
         resource_request, std::move(client),

content/browser/web_package/bundled_exchanges_handle.h

@@ -30,7 +30,8 @@ class NavigationLoaderInterceptor;
 class BundledExchangesHandle final {
  public:
   BundledExchangesHandle();
-  explicit BundledExchangesHandle(const BundledExchangesSource& source);
+  explicit BundledExchangesHandle(
+      std::unique_ptr<BundledExchangesSource> bundled_exchanges_source);
   ~BundledExchangesHandle();
 
   // Creates a NavigationLoaderInterceptor instance to handle the request for
@@ -54,10 +55,9 @@ class BundledExchangesHandle final {
                        network::mojom::URLLoaderClientPtr client);
   void OnMetadataReady(data_decoder::mojom::BundleMetadataParseErrorPtr error);
 
-  const BundledExchangesSource source_;
-
   base::OnceClosure pending_create_url_loader_task_;
 
+  std::unique_ptr<BundledExchangesSource> source_;
   std::unique_ptr<BundledExchangesReader> reader_;
   std::unique_ptr<BundledExchangesURLLoaderFactory> url_loader_factory_;
   GURL primary_url_;

content/browser/web_package/bundled_exchanges_reader.cc

@@ -142,7 +142,7 @@ BundledExchangesReader::BundledExchangesReader(
     : parser_(ServiceManagerConnection::GetForProcess()
                   ? ServiceManagerConnection::GetForProcess()->GetConnector()
                   : nullptr),
-      file_(base::MakeRefCounted<SharedFile>(source.file_path)) {}
+      file_(base::MakeRefCounted<SharedFile>(source.file_path())) {}
 
 BundledExchangesReader::~BundledExchangesReader() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);

content/browser/web_package/bundled_exchanges_reader.h

@@ -25,7 +25,7 @@
 
 namespace content {
 
-struct BundledExchangesSource;
+class BundledExchangesSource;
 
 // A class to handle a BundledExchanges that is specified by |source|.
 // It asks the utility process to parse metadata and response structures, and

content/browser/web_package/bundled_exchanges_source.cc

@@ -3,49 +3,28 @@
 // found in the LICENSE file.
 
 #include "content/browser/web_package/bundled_exchanges_source.h"
+
+#include "base/memory/ptr_util.h"
 #include "net/base/filename_util.h"
-#include "net/base/url_util.h"
 #include "url/gurl.h"
 
 namespace content {
 
-namespace {
-
-bool MatchByFilePath(const GURL& url, const base::FilePath file_path) {
-  if (!url.SchemeIsFile())
-    return false;
-  base::FilePath url_file_path;
-  return net::FileURLToFilePath(url, &url_file_path) &&
-         file_path == url_file_path;
+// static
+std::unique_ptr<BundledExchangesSource>
+BundledExchangesSource::CreateFromTrustedFile(const base::FilePath& file_path) {
+  return base::WrapUnique(new BundledExchangesSource(
+      true, file_path, net::FilePathToFileURL(file_path)));
 }
 
-}  // namespace
-
-BundledExchangesSource::BundledExchangesSource() {}
-
-BundledExchangesSource::BundledExchangesSource(const base::FilePath& path)
-    : file_path(path) {
-  DCHECK(!file_path.empty());
+std::unique_ptr<BundledExchangesSource> BundledExchangesSource::Clone() const {
+  return base::WrapUnique(
+      new BundledExchangesSource(is_trusted_, file_path_, url_));
 }
 
-BundledExchangesSource::BundledExchangesSource(
-    const BundledExchangesSource& src) = default;
-
-bool BundledExchangesSource::Match(const GURL& url) const {
-  if (!IsValid())
-    return false;
-
-  GURL request_url = net::SimplifyUrlForRequest(url);
-
-  if (!file_path.empty())
-    return MatchByFilePath(request_url, file_path);
-
-  NOTREACHED();
-  return false;
-}
-
-bool BundledExchangesSource::IsValid() const {
-  return !file_path.empty();
-}
+BundledExchangesSource::BundledExchangesSource(bool is_trusted,
+                                               const base::FilePath& file_path,
+                                               const GURL& url)
+    : is_trusted_(is_trusted), file_path_(file_path), url_(url) {}
 
 }  // namespace content

content/browser/web_package/bundled_exchanges_source.h

@@ -8,37 +8,43 @@
 #include "base/files/file_path.h"
 #include "base/macros.h"
 #include "content/common/content_export.h"
-
-class GURL;
+#include "url/gurl.h"
 
 namespace content {
 
-// A struct to abstract required information to access a BundledExchanges.
-struct CONTENT_EXPORT BundledExchangesSource {
-  // Constructs an invalid instance that does not match any.
-  BundledExchangesSource();
-
-  // Constructs a valid instance that match file URL for the given |file_path|.
-  explicit BundledExchangesSource(const base::FilePath& file_path);
-
-  // Copy constructor.
-  explicit BundledExchangesSource(const BundledExchangesSource& src);
+// A class to abstract required information to access a BundledExchanges.
+class CONTENT_EXPORT BundledExchangesSource {
+ public:
+  static std::unique_ptr<BundledExchangesSource> CreateFromTrustedFile(
+      const base::FilePath& file_path);
 
-  // Checks if this instance is valid and can match a URL.
-  bool IsValid() const;
+  std::unique_ptr<BundledExchangesSource> Clone() const;
 
-  // Checks if the given |url| is for the BundledExchanges itself that this
-  // instance represents. Note that this does not mean the |url| is for an
-  // exchange provided by the BundledExchanges.
-  bool Match(const GURL& url) const;
+  ~BundledExchangesSource() = default;
 
   // A flag to represent if this source can be trusted, i.e. using the URL in
   // the BundledExchanges as the origin for the content. Otherwise, we will use
   // the origin that serves the BundledExchanges itself. For instance, if the
   // BundledExchanges is in a local file system, file:// should be the origin.
-  bool is_trusted = false;
+  //
+  // Currently this flag is always true because we only support
+  // trustable-bundled-exchanges-file loading. TODO(horo): Implement
+  // untrusted bundled exchanges file loading.
+  bool is_trusted() const { return is_trusted_; }
+
+  const base::FilePath& file_path() const { return file_path_; }
+  const GURL& url() const { return url_; }
+
+ private:
+  BundledExchangesSource(bool is_trusted,
+                         const base::FilePath& file_path,
+                         const GURL& url);
+
+  const bool is_trusted_;
+  const base::FilePath file_path_;
+  const GURL url_;
 
-  const base::FilePath file_path;
+  DISALLOW_COPY_AND_ASSIGN(BundledExchangesSource);
 };
 
 }  // namespace content

content/browser/web_package/mock_bundled_exchanges_reader_factory.cc

@@ -158,8 +158,9 @@ class MockBundledExchangesReaderFactoryImpl final
       return nullptr;
     }
 
-    BundledExchangesSource source(temp_file_path_);
-    auto reader = std::make_unique<BundledExchangesReader>(source);
+    auto source =
+        BundledExchangesSource::CreateFromTrustedFile(temp_file_path_);
+    auto reader = std::make_unique<BundledExchangesReader>(*source);
 
     std::unique_ptr<MockParserFactory> factory =
         std::make_unique<MockParserFactory>();