[mparch] Move WebContents::GetManifest to Page.

This is a per-page concept and should be moved there. Once https://bugs.chromium.org/p/chromium/issues/detail?id=1217621 is fixed this can use that helper instead of indirecting through GetMainFrame. The favicon code may need additional changes to be able to identify the correct page to call this on. For now, it just preserves behavior by using the primary page. Bug: 1201237 Change-Id: I907e604d2223c70effa58388bd172fa881f678fa Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2955119 Reviewed-by: Alex Moshchuk <alexmos@chromium.org> Owners-Override: Alex Moshchuk <alexmos@chromium.org> Commit-Queue: Jeremy Roman <jbroman@chromium.org> Cr-Commit-Position: refs/heads/master@{#893233}
FairyWorld · Jun 17, 2021 · 4bd173d · 4bd173d
1 parent fa2b2e4
commit 4bd173d
Show file tree

Hide file tree

Showing 13 changed files with 52 additions and 28 deletions.
diff --git a/chrome/browser/chrome_service_worker_browsertest.cc b/chrome/browser/chrome_service_worker_browsertest.cc
@@ -39,6 +39,7 @@
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/browser_task_traits.h"
 #include "content/public/browser/browser_thread.h"
+#include "content/public/browser/page.h"
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/service_worker_context.h"
 #include "content/public/browser/storage_partition.h"
@@ -515,8 +516,13 @@ class ChromeServiceWorkerLinkFetchTest : public ChromeServiceWorkerFetchTest {
 
   std::string GetManifestAndIssuedRequests() {
     base::RunLoop run_loop;
-    browser()->tab_strip_model()->GetActiveWebContents()->GetManifest(
-        base::BindOnce(&ManifestCallbackAndRun, run_loop.QuitClosure()));
+    browser()
+        ->tab_strip_model()
+        ->GetActiveWebContents()
+        ->GetMainFrame()
+        ->GetPage()
+        .GetManifest(
+            base::BindOnce(&ManifestCallbackAndRun, run_loop.QuitClosure()));
     run_loop.Run();
     return ExecuteScriptAndExtractString(
         "if (issuedRequests.length != 0) reportRequests();"

diff --git a/components/favicon/content/content_favicon_driver.cc b/components/favicon/content/content_favicon_driver.cc
@@ -103,7 +103,11 @@ int ContentFaviconDriver::DownloadImage(const GURL& url,
 
 void ContentFaviconDriver::DownloadManifest(const GURL& url,
                                             ManifestDownloadCallback callback) {
-  web_contents()->GetManifest(
+  // TODO(crbug.com/1201237): This appears to be reachable from pages other
+  // than the primary page. This code should likely be refactored so that either
+  // this is unreachable from other pages, or the correct page is plumbed in
+  // here.
+  web_contents()->GetMainFrame()->GetPage().GetManifest(
       base::BindOnce(&ContentFaviconDriver::OnDidDownloadManifest,
                      base::Unretained(this), std::move(callback)));
 }

diff --git a/components/favicon/content/content_favicon_driver.h b/components/favicon/content/content_favicon_driver.h
@@ -18,6 +18,10 @@
 #include "third_party/blink/public/mojom/favicon/favicon_url.mojom.h"
 #include "url/gurl.h"
 
+namespace blink {
+struct Manifest;
+}  // namespace blink
+
 namespace favicon {
 
 class CoreFaviconService;

diff --git a/components/webapps/browser/installable/installable_manager.cc b/components/webapps/browser/installable/installable_manager.cc
@@ -22,6 +22,7 @@
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/manifest_icon_downloader.h"
 #include "content/public/browser/navigation_handle.h"
+#include "content/public/browser/page.h"
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/storage_partition.h"
 #include "content/public/common/content_features.h"
@@ -624,7 +625,9 @@ void InstallableManager::FetchManifest() {
   content::WebContents* web_contents = GetWebContents();
   DCHECK(web_contents);
 
-  web_contents->GetManifest(base::BindOnce(
+  // This uses DidFinishNavigation to abort when the primary page changes.
+  // Therefore this should always be the correct page.
+  web_contents->GetMainFrame()->GetPage().GetManifest(base::BindOnce(
       &InstallableManager::OnDidGetManifest, weak_factory_.GetWeakPtr()));
 }
 

diff --git a/content/browser/manifest/manifest_browsertest.cc b/content/browser/manifest/manifest_browsertest.cc
@@ -15,6 +15,7 @@
 #include "base/strings/utf_string_conversions.h"
 #include "build/build_config.h"
 #include "build/chromeos_buildflags.h"
+#include "content/public/browser/page.h"
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/common/content_switches.h"
@@ -92,8 +93,9 @@ class ManifestBrowserTest : public ContentBrowserTest,
   }
 
   void GetManifestAndWait() {
-    shell()->web_contents()->GetManifest(base::BindOnce(
-        &ManifestBrowserTest::OnGetManifest, base::Unretained(this)));
+    shell()->web_contents()->GetMainFrame()->GetPage().GetManifest(
+        base::BindOnce(&ManifestBrowserTest::OnGetManifest,
+                       base::Unretained(this)));
 
     message_loop_runner_ = new MessageLoopRunner();
     message_loop_runner_->Run();

diff --git a/content/browser/net/accept_header_browsertest.cc b/content/browser/net/accept_header_browsertest.cc
@@ -11,6 +11,7 @@
 #include "base/test/bind.h"
 #include "build/build_config.h"
 #include "content/public/browser/browser_thread.h"
+#include "content/public/browser/page.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/test/browser_test.h"
 #include "content/public/test/browser_test_utils.h"
@@ -191,7 +192,8 @@ IN_PROC_BROWSER_TEST_F(AcceptHeaderTest, Check) {
   // Ensure that if an Accept header is already set, it is not overwritten.
   EXPECT_EQ("custom/type", GetFor("/xhr_with_accept_header"));
 
-  shell()->web_contents()->GetManifest(base::DoNothing());
+  shell()->web_contents()->GetMainFrame()->GetPage().GetManifest(
+      base::DoNothing());
 
   // ResourceType::kSubResource
   EXPECT_EQ("*/*", GetFor("/manifest"));

diff --git a/content/browser/payments/payment_app_info_fetcher.cc b/content/browser/payments/payment_app_info_fetcher.cc
@@ -18,6 +18,7 @@
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/global_routing_id.h"
 #include "content/public/browser/manifest_icon_downloader.h"
+#include "content/public/browser/page.h"
 #include "third_party/abseil-cpp/absl/types/optional.h"
 #include "third_party/blink/public/common/manifest/manifest_icon_selector.h"
 #include "third_party/blink/public/common/storage_key/storage_key.h"
@@ -150,7 +151,7 @@ void PaymentAppInfoFetcher::SelfDeleteFetcher::Start(
     web_contents_helper_ =
         std::make_unique<WebContentsHelper>(top_level_web_content);
 
-    top_level_web_content->GetManifest(
+    top_level_render_frame_host->GetPage().GetManifest(
         base::BindOnce(&PaymentAppInfoFetcher::SelfDeleteFetcher::
                            FetchPaymentAppManifestCallback,
                        weak_ptr_factory_.GetWeakPtr()));

diff --git a/content/browser/renderer_host/page_impl.cc b/content/browser/renderer_host/page_impl.cc
@@ -4,6 +4,7 @@
 
 #include "content/browser/renderer_host/page_impl.h"
 
+#include "content/browser/manifest/manifest_manager_host.h"
 #include "content/browser/renderer_host/render_frame_host_impl.h"
 
 namespace content {
@@ -16,4 +17,10 @@ const GURL& PageImpl::GetManifestURL() {
   return manifest_url_;
 }
 
+void PageImpl::GetManifest(GetManifestCallback callback) {
+  ManifestManagerHost* manifest_manager_host =
+      ManifestManagerHost::GetOrCreateForCurrentDocument(&main_document_);
+  manifest_manager_host->GetManifest(std::move(callback));
+}
+
 }  // namespace content
diff --git a/content/browser/renderer_host/page_impl.h b/content/browser/renderer_host/page_impl.h
@@ -28,6 +28,7 @@ class PageImpl : public Page {
 
   // Page implementation.
   const GURL& GetManifestURL() override;
+  void GetManifest(GetManifestCallback callback) override;
 
   RenderFrameHostImpl* main_document() { return &main_document_; }
 

diff --git a/content/browser/web_contents/web_contents_impl.cc b/content/browser/web_contents/web_contents_impl.cc
@@ -69,7 +69,6 @@
 #include "content/browser/download/save_package.h"
 #include "content/browser/find_request_manager.h"
 #include "content/browser/gpu/gpu_data_manager_impl.h"
-#include "content/browser/manifest/manifest_manager_host.h"
 #include "content/browser/media/audio_stream_broker.h"
 #include "content/browser/media/audio_stream_monitor.h"
 #include "content/browser/media/media_web_contents_observer.h"
@@ -5202,14 +5201,6 @@ bool WebContentsImpl::WasEverAudible() {
   return was_ever_audible_;
 }
 
-void WebContentsImpl::GetManifest(GetManifestCallback callback) {
-  OPTIONAL_TRACE_EVENT0("content", "WebContentsImpl::GetManifest");
-  // TODO(yuzus, 1061899): Move this function to RenderFrameHostImpl.
-  ManifestManagerHost* manifest_manager_host =
-      ManifestManagerHost::GetOrCreateForCurrentDocument(GetMainFrame());
-  manifest_manager_host->GetManifest(std::move(callback));
-}
-
 void WebContentsImpl::ExitFullscreen(bool will_cause_resize) {
   OPTIONAL_TRACE_EVENT0("content", "WebContentsImpl::ExitFullscreen");
   // Clean up related state and initiate the fullscreen exit.

diff --git a/content/browser/web_contents/web_contents_impl.h b/content/browser/web_contents/web_contents_impl.h
@@ -533,7 +533,6 @@ class CONTENT_EXPORT WebContentsImpl : public WebContents,
             blink::mojom::FindOptionsPtr options) override;
   void StopFinding(StopFindAction action) override;
   bool WasEverAudible() override;
-  void GetManifest(GetManifestCallback callback) override;
   bool IsFullscreen() override;
   bool ShouldShowStaleContentOnEviction() override;
   void ExitFullscreen(bool will_cause_resize) override;

diff --git a/content/public/browser/page.h b/content/public/browser/page.h
@@ -5,9 +5,14 @@
 #ifndef CONTENT_PUBLIC_BROWSER_PAGE_H_
 #define CONTENT_PUBLIC_BROWSER_PAGE_H_
 
+#include "base/callback.h"
 #include "content/common/content_export.h"
 #include "url/gurl.h"
 
+namespace blink {
+struct Manifest;
+}  // namespace blink
+
 namespace content {
 
 // Page represents a collection of documents with the same main document.
@@ -55,6 +60,15 @@ class CONTENT_EXPORT Page {
   // See https://w3c.github.io/manifest/#web-application-manifest
   virtual const GURL& GetManifestURL() = 0;
 
+  // The callback invoked when the renderer responds to a request for the main
+  // frame document's manifest. The url will be empty if the document specifies
+  // no manifest, and the manifest will be empty if any other failures occurred.
+  using GetManifestCallback =
+      base::OnceCallback<void(const GURL&, const blink::Manifest&)>;
+
+  // Requests the manifest URL and the Manifest of the main frame's document.
+  virtual void GetManifest(GetManifestCallback callback) = 0;
+
  private:
   // This interface should only be implemented inside content.
   friend class PageImpl;

diff --git a/content/public/browser/web_contents.h b/content/public/browser/web_contents.h
@@ -55,7 +55,6 @@ namespace blink {
 namespace web_pref {
 struct WebPreferences;
 }
-struct Manifest;
 struct UserAgentOverride;
 struct RendererPreferences;
 }  // namespace blink
@@ -1053,15 +1052,6 @@ class WebContents : public PageNavigator,
   // navigation.
   virtual bool WasEverAudible() = 0;
 
-  // The callback invoked when the renderer responds to a request for the main
-  // frame document's manifest. The url will be empty if the document specifies
-  // no manifest, and the manifest will be empty if any other failures occurred.
-  using GetManifestCallback =
-      base::OnceCallback<void(const GURL&, const blink::Manifest&)>;
-
-  // Requests the manifest URL and the Manifest of the main frame's document.
-  virtual void GetManifest(GetManifestCallback callback) = 0;
-
   // Returns whether the renderer is in fullscreen mode.
   virtual bool IsFullscreen() = 0;