Split manifest change notifications from ManifestManager.

Also remove layout test support's direct use of ManifestFetcher,
changing it to use ManifestManager instead.

Bug: 704441
Change-Id: I0417baaf6598a8c7d967e3ce1fd32e78bea22aa0
Reviewed-on: https://chromium-review.googlesource.com/774040Reviewed-by: Avi Drissman <avi@chromium.org>
Commit-Queue: Sam McNally <sammc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#521548}

content/public/test/layouttest_support.h

@@ -23,7 +23,6 @@ struct WebSize;
 class WebURLRequest;
 class WebView;
 class WebWidget;
-class WebURLResponse;
 }
 
 namespace device {
@@ -46,6 +45,7 @@ namespace content {
 class RenderFrame;
 class RendererGamepadProvider;
 class RenderView;
+struct Manifest;
 
 // Turn the browser process into layout test mode.
 void EnableBrowserLayoutTestMode();
@@ -98,10 +98,9 @@ void EnableWebTestProxyCreation(
     const WidgetProxyCreationCallback& widget_proxy_creation_callback,
     const FrameProxyCreationCallback& frame_proxy_creation_callback);
 
-typedef base::Callback<void(const blink::WebURLResponse& response,
-                            const std::string& data)> FetchManifestCallback;
-void FetchManifest(blink::WebView* view, const GURL& url,
-                   const FetchManifestCallback&);
+typedef base::OnceCallback<void(const GURL&, const Manifest&)>
+    FetchManifestCallback;
+void FetchManifest(blink::WebView* view, FetchManifestCallback callback);
 
 // Sets gamepad provider to be used for layout tests.
 void SetMockGamepadProvider(std::unique_ptr<RendererGamepadProvider> provider);

content/renderer/BUILD.gn

@@ -274,6 +274,8 @@ target(link_target_type, "renderer") {
     "loader/web_url_request_util.h",
     "loader/weburlresponse_extradata_impl.cc",
     "loader/weburlresponse_extradata_impl.h",
+    "manifest/manifest_change_notifier.cc",
+    "manifest/manifest_change_notifier.h",
     "manifest/manifest_manager.cc",
     "manifest/manifest_manager.h",
     "manifest/manifest_parser.cc",

content/renderer/manifest/manifest_change_notifier.cc

+// Copyright 2017 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 "content/renderer/manifest/manifest_change_notifier.h"
+
+#include <utility>
+
+#include "base/bind.h"
+#include "content/public/renderer/render_frame.h"
+#include "third_party/WebKit/common/associated_interfaces/associated_interface_provider.h"
+#include "third_party/WebKit/public/web/WebDocument.h"
+#include "third_party/WebKit/public/web/WebLocalFrame.h"
+
+namespace content {
+
+ManifestChangeNotifier::ManifestChangeNotifier(RenderFrame* render_frame)
+    : RenderFrameObserver(render_frame), weak_factory_(this) {}
+
+ManifestChangeNotifier::~ManifestChangeNotifier() = default;
+
+void ManifestChangeNotifier::DidChangeManifest() {
+  if (weak_factory_.HasWeakPtrs())
+    return;
+
+  // Changing the manifest URL can trigger multiple notifications; the manifest
+  // URL update may involve removing the old manifest link before adding the new
+  // one, triggering multiple calls to DidChangeManifest(). Coalesce changes
+  // during a single event loop task to avoid sending spurious notifications to
+  // the browser.
+  //
+  // During document load, coalescing is disabled to maintain relative ordering
+  // of this notification and the favicon URL reporting.
+  if (!render_frame()->GetWebFrame()->IsLoading()) {
+    render_frame()
+        ->GetTaskRunner(blink::TaskType::kUnspecedLoading)
+        ->PostTask(FROM_HERE,
+                   base::BindOnce(&ManifestChangeNotifier::ReportManifestChange,
+                                  weak_factory_.GetWeakPtr()));
+    return;
+  }
+  ReportManifestChange();
+}
+
+void ManifestChangeNotifier::OnDestruct() {
+  delete this;
+}
+
+void ManifestChangeNotifier::ReportManifestChange() {
+  auto manifest_url =
+      render_frame()->GetWebFrame()->GetDocument().ManifestURL();
+  if (manifest_url.IsNull()) {
+    GetManifestChangeObserver().ManifestUrlChanged(base::nullopt);
+  } else {
+    GetManifestChangeObserver().ManifestUrlChanged(GURL(manifest_url));
+  }
+}
+
+mojom::ManifestUrlChangeObserver&
+ManifestChangeNotifier::GetManifestChangeObserver() {
+  if (!manifest_change_observer_) {
+    render_frame()->GetRemoteAssociatedInterfaces()->GetInterface(
+        &manifest_change_observer_);
+  }
+  return *manifest_change_observer_;
+}
+
+}  // namespace content

content/renderer/manifest/manifest_change_notifier.h

+// Copyright 2017 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 CONTENT_RENDERER_MANIFEST_MANIFEST_CHANGE_NOTIFIER_H_
+#define CONTENT_RENDERER_MANIFEST_MANIFEST_CHANGE_NOTIFIER_H_
+
+#include "base/macros.h"
+#include "base/memory/weak_ptr.h"
+#include "content/common/manifest_observer.mojom.h"
+#include "content/public/renderer/render_frame_observer.h"
+
+namespace content {
+
+class ManifestChangeNotifier : public RenderFrameObserver {
+ public:
+  explicit ManifestChangeNotifier(RenderFrame* render_frame);
+  ~ManifestChangeNotifier() override;
+
+ private:
+  // RenderFrameObserver implementation.
+  void DidChangeManifest() override;
+  void OnDestruct() override;
+
+  void ReportManifestChange();
+  mojom::ManifestUrlChangeObserver& GetManifestChangeObserver();
+
+  mojom::ManifestUrlChangeObserverAssociatedPtr manifest_change_observer_;
+
+  base::WeakPtrFactory<ManifestChangeNotifier> weak_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(ManifestChangeNotifier);
+};
+
+}  // namespace content
+
+#endif  // CONTENT_RENDERER_MANIFEST_MANIFEST_CHANGE_NOTIFIER_H_

content/renderer/manifest/manifest_manager.cc

@@ -23,8 +23,7 @@ namespace content {
 ManifestManager::ManifestManager(RenderFrame* render_frame)
     : RenderFrameObserver(render_frame),
       may_have_manifest_(false),
-      manifest_dirty_(true),
-      weak_factory_(this) {}
+      manifest_dirty_(true) {}
 
 ManifestManager::~ManifestManager() {
   if (fetcher_)
@@ -85,30 +84,6 @@ void ManifestManager::DidChangeManifest() {
   manifest_dirty_ = true;
   manifest_url_ = GURL();
   manifest_debug_info_ = nullptr;
-
-  if (!render_frame()->IsMainFrame())
-    return;
-
-  if (weak_factory_.HasWeakPtrs())
-    return;
-
-  // Changing the manifest URL can trigger multiple notifications; the manifest
-  // URL update may involve removing the old manifest link before adding the new
-  // one, triggering multiple calls to DidChangeManifest(). Coalesce changes
-  // during a single event loop task to avoid sending spurious notifications to
-  // the browser.
-  //
-  // During document load, coalescing is disabled to maintain relative ordering
-  // of this notification and the favicon URL reporting.
-  if (!render_frame()->GetWebFrame()->IsLoading()) {
-    render_frame()
-        ->GetTaskRunner(blink::TaskType::kUnspecedLoading)
-        ->PostTask(FROM_HERE,
-                   base::BindOnce(&ManifestManager::ReportManifestChange,
-                                  weak_factory_.GetWeakPtr()));
-    return;
-  }
-  ReportManifestChange();
 }
 
 void ManifestManager::DidCommitProvisionalLoad(
@@ -149,6 +124,7 @@ void ManifestManager::OnManifestFetchComplete(
     const GURL& document_url,
     const blink::WebURLResponse& response,
     const std::string& data) {
+  fetcher_.reset();
   if (response.IsNull() && data.empty()) {
     manifest_debug_info_ = nullptr;
     ManifestUmaUtil::FetchFailed(ManifestUmaUtil::FETCH_UNSPECIFIED_REASON);
@@ -162,7 +138,6 @@ void ManifestManager::OnManifestFetchComplete(
   ManifestParser parser(data_piece, response_url, document_url);
   parser.Parse();
 
-  fetcher_.reset();
   manifest_debug_info_ = blink::mojom::ManifestDebugInfo::New();
   manifest_debug_info_->raw_manifest = data;
   parser.TakeErrors(&manifest_debug_info_->errors);
@@ -220,22 +195,4 @@ void ManifestManager::BindToRequest(
 
 void ManifestManager::OnDestruct() {}
 
-void ManifestManager::ReportManifestChange() {
-  auto manifest_url =
-      render_frame()->GetWebFrame()->GetDocument().ManifestURL();
-  if (manifest_url.IsNull()) {
-    GetManifestChangeObserver().ManifestUrlChanged(base::nullopt);
-  } else {
-    GetManifestChangeObserver().ManifestUrlChanged(GURL(manifest_url));
-  }
-}
-
-mojom::ManifestUrlChangeObserver& ManifestManager::GetManifestChangeObserver() {
-  if (!manifest_change_observer_) {
-    render_frame()->GetRemoteAssociatedInterfaces()->GetInterface(
-        &manifest_change_observer_);
-  }
-  return *manifest_change_observer_;
-}
-
 }  // namespace content

content/renderer/manifest/manifest_manager.h

@@ -12,7 +12,6 @@
 #include "base/callback_forward.h"
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
-#include "content/common/manifest_observer.mojom.h"
 #include "content/public/common/manifest.h"
 #include "content/public/renderer/render_frame_observer.h"
 #include "mojo/public/cpp/bindings/binding_set.h"
@@ -73,9 +72,6 @@ class ManifestManager : public RenderFrameObserver,
                                const std::string& data);
   void ResolveCallbacks(ResolveState state);
 
-  void ReportManifestChange();
-  mojom::ManifestUrlChangeObserver& GetManifestChangeObserver();
-
   std::unique_ptr<ManifestFetcher> fetcher_;
 
   // Whether the RenderFrame may have an associated Manifest. If true, the frame
@@ -96,14 +92,10 @@ class ManifestManager : public RenderFrameObserver,
   // Current Manifest debug information.
   blink::mojom::ManifestDebugInfoPtr manifest_debug_info_;
 
-  mojom::ManifestUrlChangeObserverAssociatedPtr manifest_change_observer_;
-
   std::vector<InternalRequestManifestCallback> pending_callbacks_;
 
   mojo::BindingSet<blink::mojom::ManifestManager> bindings_;
 
-  base::WeakPtrFactory<ManifestManager> weak_factory_;
-
   DISALLOW_COPY_AND_ASSIGN(ManifestManager);
 };
 

content/renderer/render_frame_impl.cc

@@ -116,6 +116,7 @@
 #include "content/renderer/loader/web_url_loader_impl.h"
 #include "content/renderer/loader/web_url_request_util.h"
 #include "content/renderer/loader/weburlresponse_extradata_impl.h"
+#include "content/renderer/manifest/manifest_change_notifier.h"
 #include "content/renderer/manifest/manifest_manager.h"
 #include "content/renderer/media/audio_device_factory.h"
 #include "content/renderer/media/audio_ipc_factory.h"
@@ -1354,6 +1355,11 @@ RenderFrameImpl::RenderFrameImpl(CreateParams params)
 #endif
 
   manifest_manager_ = std::make_unique<ManifestManager>(this);
+  if (IsMainFrame()) {
+    // Manages its own lifetime.
+    new ManifestChangeNotifier(this);
+  }
+
   memset(&peak_memory_metrics_, 0,
          sizeof(RenderThreadImpl::RendererMemoryMetrics));
 }

content/shell/renderer/layout_test/blink_test_runner.cc

@@ -617,10 +617,8 @@ bool BlinkTestRunner::AllowExternalPages() {
 
 void BlinkTestRunner::FetchManifest(
     blink::WebView* view,
-      const GURL& url,
-      const base::Callback<void(const blink::WebURLResponse& response,
-                                const std::string& data)>& callback) {
-  ::content::FetchManifest(view, url, callback);
+    base::OnceCallback<void(const GURL&, const Manifest&)> callback) {
+  ::content::FetchManifest(view, std::move(callback));
 }
 
 void BlinkTestRunner::SetPermission(const std::string& name,

content/shell/renderer/layout_test/blink_test_runner.h

@@ -138,9 +138,7 @@ class BlinkTestRunner : public RenderViewObserver,
   bool AllowExternalPages() override;
   void FetchManifest(
       blink::WebView* view,
-      const GURL& url,
-      const base::Callback<void(const blink::WebURLResponse& response,
-                                const std::string& data)>& callback) override;
+      base::OnceCallback<void(const GURL&, const Manifest&)> callback) override;
   void SetPermission(const std::string& name,
                      const std::string& value,
                      const GURL& origin,

content/shell/test_runner/test_runner_for_specific_view.cc

@@ -316,16 +316,15 @@ void TestRunnerForSpecificView::GetManifestThen(
 
   delegate()->FetchManifest(
       web_view(),
-      web_view()->MainFrame()->ToWebLocalFrame()->GetDocument().ManifestURL(),
-      base::Bind(&TestRunnerForSpecificView::GetManifestCallback,
+      base::BindOnce(&TestRunnerForSpecificView::GetManifestCallback,
                      weak_factory_.GetWeakPtr(),
-                 base::Passed(std::move(persistent_callback))));
+                     std::move(persistent_callback)));
 }
 
 void TestRunnerForSpecificView::GetManifestCallback(
     v8::UniquePersistent<v8::Function> callback,
-    const blink::WebURLResponse& response,
-    const std::string& data) {
+    const GURL& manifest_url,
+    const content::Manifest& manifest) {
   PostV8CallbackWithArgs(std::move(callback), 0, nullptr);
 }
 

content/shell/test_runner/test_runner_for_specific_view.h

@@ -15,14 +15,18 @@
 #include "base/memory/weak_ptr.h"
 #include "v8/include/v8.h"
 
+class GURL;
 class SkBitmap;
 
 namespace blink {
 class WebLocalFrame;
-class WebURLResponse;
 class WebView;
 }
 
+namespace content {
+struct Manifest;
+}
+
 namespace gin {
 class Arguments;
 }
@@ -93,8 +97,8 @@ class TestRunnerForSpecificView {
 
   void GetManifestThen(v8::Local<v8::Function> callback);
   void GetManifestCallback(v8::UniquePersistent<v8::Function> callback,
-                           const blink::WebURLResponse& response,
-                           const std::string& data);
+                           const GURL& manifest_url,
+                           const content::Manifest& manifest);
 
   // Calls |callback| with a DOMString[] representing the events recorded since
   // the last call to this function.

content/shell/test_runner/web_test_delegate.h

@@ -32,10 +32,13 @@ class WebPlugin;
 struct WebPluginParams;
 struct WebSize;
 class WebURLRequest;
-class WebURLResponse;
 class WebView;
 }
 
+namespace content {
+struct Manifest;
+}
+
 namespace device {
 class MotionData;
 class OrientationData;
@@ -250,9 +253,8 @@ class WebTestDelegate {
   // Fetch the manifest for a given WebView from the given url.
   virtual void FetchManifest(
       blink::WebView* view,
-      const GURL& url,
-      const base::Callback<void(const blink::WebURLResponse& response,
-                                const std::string& data)>& callback) = 0;
+      base::OnceCallback<void(const GURL&, const content::Manifest&)>
+          callback) = 0;
 
   // Sends a message to the LayoutTestPermissionManager in order for it to
   // update its database.

content/test/BUILD.gn

@@ -1548,7 +1548,6 @@ test("content_unittests") {
     "../renderer/loader/url_response_body_consumer_unittest.cc",
     "../renderer/loader/web_data_consumer_handle_impl_unittest.cc",
     "../renderer/loader/web_url_loader_impl_unittest.cc",
-    "../renderer/manifest/manifest_parser_unittest.cc",
     "../renderer/media/audio_ipc_factory_unittest.cc",
     "../renderer/media/audio_message_filter_unittest.cc",
     "../renderer/media/audio_renderer_mixer_manager_unittest.cc",

content/test/layouttest_support.cc

@@ -30,7 +30,6 @@
 #include "content/public/common/page_state.h"
 #include "content/public/common/screen_info.h"
 #include "content/public/renderer/renderer_gamepad_provider.h"
-#include "content/renderer/fetchers/manifest_fetcher.h"
 #include "content/renderer/gpu/render_widget_compositor.h"
 #include "content/renderer/input/render_widget_input_handler_delegate.h"
 #include "content/renderer/layout_test_dependencies.h"
@@ -243,29 +242,10 @@ void EnableWebTestProxyCreation(
   RenderFrameImpl::InstallCreateHook(CreateWebFrameTestProxy);
 }
 
-void FetchManifestDoneCallback(std::unique_ptr<ManifestFetcher> fetcher,
-                               const FetchManifestCallback& callback,
-                               const blink::WebURLResponse& response,
-                               const std::string& data) {
-  // |fetcher| will be autodeleted here as it is going out of scope.
-  callback.Run(response, data);
-}
-
-void FetchManifest(blink::WebView* view, const GURL& url,
-                   const FetchManifestCallback& callback) {
-  ManifestFetcher* fetcher = new ManifestFetcher(url);
-  std::unique_ptr<ManifestFetcher> autodeleter(fetcher);
-
-  // Start is called on fetcher which is also bound to the callback.
-  // A raw pointer is used instead of a scoped_ptr as base::Passes passes
-  // ownership and thus nulls the scoped_ptr. On MSVS this happens before
-  // the call to Start, resulting in a crash.
-  CHECK(view->MainFrame()->IsWebLocalFrame())
-      << "This function cannot be called if the main frame is not a "
-         "local frame.";
-  fetcher->Start(view->MainFrame()->ToWebLocalFrame(), false,
-                 base::Bind(&FetchManifestDoneCallback,
-                            base::Passed(&autodeleter), callback));
+void FetchManifest(blink::WebView* view, FetchManifestCallback callback) {
+  RenderFrameImpl::FromWebFrame(view->MainFrame())
+      ->GetManifestManager()
+      .RequestManifest(std::move(callback));
 }
 
 void SetMockGamepadProvider(std::unique_ptr<RendererGamepadProvider> provider) {