Fix DomDistillerViewerSource to work with PlzNavigate.

components/dom_distiller/content/browser/dom_distiller_viewer_source.cc

 // Copyright 2014 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 "components/dom_distiller/content/browser/dom_distiller_viewer_source.h"
 
 #include <memory>
 #include <string>
 #include <utility>
 #include <vector>
@@ skipping 14 matching lines @@
 #include "components/dom_distiller/core/dom_distiller_request_view_base.h"
 #include "components/dom_distiller/core/dom_distiller_service.h"
 #include "components/dom_distiller/core/experiments.h"
 #include "components/dom_distiller/core/feedback_reporter.h"
 #include "components/dom_distiller/core/task_tracker.h"
 #include "components/dom_distiller/core/url_constants.h"
 #include "components/dom_distiller/core/url_utils.h"
 #include "components/dom_distiller/core/viewer.h"
 #include "content/public/browser/navigation_details.h"
 #include "content/public/browser/navigation_entry.h"
+#include "content/public/browser/navigation_handle.h"
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/render_view_host.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/browser/web_contents_observer.h"
 #include "grit/components_strings.h"
 #include "mojo/public/cpp/bindings/strong_binding.h"
 #include "net/base/url_util.h"
 #include "net/url_request/url_request.h"
 #include "services/shell/public/cpp/interface_provider.h"
 #include "services/shell/public/cpp/interface_registry.h"
 #include "ui/base/l10n/l10n_util.h"
 
 namespace dom_distiller {
 
 // Handles receiving data asynchronously for a specific entry, and passing
 // it along to the data callback for the data source. Lifetime matches that of
 // the current main frame's page in the Viewer instance.
 class DomDistillerViewerSource::RequestViewerHandle
     : public DomDistillerRequestViewBase,
       public content::WebContentsObserver {
  public:
   RequestViewerHandle(content::WebContents* web_contents,
                       const std::string& expected_scheme,
                       const std::string& expected_request_path,
-                      DistilledPagePrefs* distilled_page_prefs);
+                      DistilledPagePrefs* distilled_page_prefs,
+                      std::unique_ptr<DistillerUIHandle> ui_handle);
   ~RequestViewerHandle() override;
 
   // content::WebContentsObserver implementation:
-  void DidNavigateMainFrame(
-      const content::LoadCommittedDetails& details,
-      const content::FrameNavigateParams& params) override;
+  void DidFinishNavigation(
+      content::NavigationHandle* navigation_handle) override;
   void RenderProcessGone(base::TerminationStatus status) override;
   void WebContentsDestroyed() override;
   void DidFinishLoad(content::RenderFrameHost* render_frame_host,
                      const GURL& validated_url) override;
 
  private:
   // Sends JavaScript to the attached Viewer, buffering data if the viewer isn't
   // ready.
   void SendJavaScript(const std::string& buffer) override;
 
   // Cancels the current view request. Once called, no updates will be
   // propagated to the view, and the request to DomDistillerService will be
   // cancelled.
   void Cancel();
 
   // The scheme hosting the current view request;
   std::string expected_scheme_;
 
   // The query path for the current view request.
   std::string expected_request_path_;
 
   // Whether the page is sufficiently initialized to handle updates from the
   // distiller.
   bool waiting_for_page_ready_;
 
   // Temporary store of pending JavaScript if the page isn't ready to receive
   // data from distillation.
   std::string buffer_;
+
+  // An object for accessing chrome-specific UI controls including external
+  // feedback and opening the distiller settings.
+  std::unique_ptr<DistillerUIHandle> distiller_ui_handle_;
 };
 
 DomDistillerViewerSource::RequestViewerHandle::RequestViewerHandle(
     content::WebContents* web_contents,
     const std::string& expected_scheme,
     const std::string& expected_request_path,
-    DistilledPagePrefs* distilled_page_prefs)
+    DistilledPagePrefs* distilled_page_prefs,
+    std::unique_ptr<DistillerUIHandle> ui_handle)
     : DomDistillerRequestViewBase(distilled_page_prefs),
       expected_scheme_(expected_scheme),
       expected_request_path_(expected_request_path),
-      waiting_for_page_ready_(true) {
+      waiting_for_page_ready_(true),
+      distiller_ui_handle_(std::move(ui_handle)) {
   content::WebContentsObserver::Observe(web_contents);
   distilled_page_prefs_->AddObserver(this);
 }
 
 DomDistillerViewerSource::RequestViewerHandle::~RequestViewerHandle() {
   distilled_page_prefs_->RemoveObserver(this);
 }
 
 void DomDistillerViewerSource::RequestViewerHandle::SendJavaScript(
     const std::string& buffer) {
   if (waiting_for_page_ready_) {
     buffer_ += buffer;
   } else {
     DCHECK(buffer_.empty());
     if (web_contents()) {
       RunIsolatedJavaScript(web_contents()->GetMainFrame(), buffer);
     }
   }
 }
 
-void DomDistillerViewerSource::RequestViewerHandle::DidNavigateMainFrame(
-    const content::LoadCommittedDetails& details,
-    const content::FrameNavigateParams& params) {
-  const GURL& navigation = details.entry->GetURL();
-  if (details.is_in_page || (navigation.SchemeIs(expected_scheme_.c_str()) &&
-                             expected_request_path_ == navigation.query())) {
+void DomDistillerViewerSource::RequestViewerHandle::DidFinishNavigation(
+    content::NavigationHandle* navigation_handle) {
+  const GURL& navigation = navigation_handle->GetURL();
+  bool expected_main_view_request =
+      navigation.SchemeIs(expected_scheme_.c_str()) &&
+      expected_request_path_ == navigation.query();
+  if (navigation_handle->IsSamePage() || expected_main_view_request) {
     // In-page navigations, as well as the main view request can be ignored.
+    if (expected_main_view_request) {
+      content::RenderFrameHost* render_frame_host =
+          navigation_handle->GetRenderFrameHost();
+      content::RenderViewHost* render_view_host =
+          render_frame_host->GetRenderViewHost();
+      CHECK_EQ(0, render_view_host->GetEnabledBindings());

[nyquist, 2016/09/19 21:39:51]

This is our security check to ensure we never, ever, mess up and give any
resource rendered through the distiller access to things it should not have
access to.

Would it somehow be possible to keep this verification in StartDataRequest, or
does that not work correctly with PlzNavigate? Maybe the RenderViewHost is not
available anymore at that time?

[jam, 2016/09/19 22:13:05]

Right, the RVH is not available at that time so we can't check.

I chatted with Nasko about this in our PlzNavigate meeting and he was fine with
it. My logic was: this check is for catching developer regressions (i.e. somehow
dom distiller gets hosted in a webui process), and so if this occurs we'll get a
lot of crash reports which will alert us to the bug.

[nyquist, 2016/09/19 22:26:59]

OK. Thanks for checking!

+
+      // Add mojo service for JavaScript functionality. This is the receiving
+      // end of this particular service.
+      render_frame_host->GetInterfaceRegistry()->AddInterface(
+          base::Bind(&CreateDistillerJavaScriptService,
+              render_frame_host,
+              distiller_ui_handle_.get()));
+
+      // Tell the renderer that this is currently a distilled page.
+      mojom::DistillerPageNotifierServicePtr page_notifier_service;
+      render_frame_host->GetRemoteInterfaces()->GetInterface(
+          &page_notifier_service);
+      DCHECK(page_notifier_service);
+      page_notifier_service->NotifyIsDistillerPage();
+    }
     return;
   }
 
   Cancel();
 }
 
 void DomDistillerViewerSource::RequestViewerHandle::RenderProcessGone(
     base::TerminationStatus status) {
   Cancel();
 }
@@ skipping 53 matching lines @@
 
 void DomDistillerViewerSource::StartDataRequest(
     const std::string& path,
     int render_process_id,
     int render_frame_id,
     const content::URLDataSource::GotDataCallback& callback) {
   content::RenderFrameHost* render_frame_host =
       content::RenderFrameHost::FromID(render_process_id, render_frame_id);
   if (!render_frame_host)
     return;
-  content::RenderViewHost* render_view_host =
-      render_frame_host->GetRenderViewHost();
-  DCHECK(render_view_host);
-  CHECK_EQ(0, render_view_host->GetEnabledBindings());
-
   if (kViewerCssPath == path) {
     std::string css = viewer::GetCss();
     callback.Run(base::RefCountedString::TakeString(&css));
     return;
   } else if (kViewerLoadingImagePath == path) {
     std::string image = viewer::GetLoadingImage();
     callback.Run(base::RefCountedString::TakeString(&image));
     return;
   } else if (base::StartsWith(path, kViewerSaveFontScalingPath,
                               base::CompareCase::SENSITIVE)) {
     double scale = 1.0;
     if (base::StringToDouble(
         path.substr(strlen(kViewerSaveFontScalingPath)), &scale)) {
       dom_distiller_service_->GetDistilledPagePrefs()->SetFontScaling(scale);
     }
   }
   content::WebContents* web_contents =
       content::WebContents::FromRenderFrameHost(render_frame_host);
   DCHECK(web_contents);
   // An empty |path| is invalid, but guard against it. If not empty, assume
   // |path| starts with '?', which is stripped away.
   const std::string path_after_query_separator =
       path.size() > 0 ? path.substr(1) : "";
   RequestViewerHandle* request_viewer_handle =
       new RequestViewerHandle(web_contents, scheme_, path_after_query_separator,
-                              dom_distiller_service_->GetDistilledPagePrefs());
+                              dom_distiller_service_->GetDistilledPagePrefs(),
+                              std::move(distiller_ui_handle_));

[nyquist, 2016/09/19 21:39:51]

Maybe I'm misunderstanding this, but I thought that we would create the
DomDistillerViewerSource once through the ProfileImpl in //chrome:
https://cs.chromium.org/chromium/src/chrome/browser/profiles/profile_impl.cc?...

And then, the StartDataRequest would be invoked for any calls to the distiller
after that for that profile.
Moving the distiller_ui_handle_ to the one-off RequestViewerHandle seems to me
like it might make the second call to StartDataRequest not work?

[jam, 2016/09/19 22:13:05]

Thanks for catching that (somehow I misremembered and thought the data source
was per request, double bad since I made this interface!). Fixed.

   std::unique_ptr<ViewerHandle> viewer_handle = viewer::CreateViewRequest(
       dom_distiller_service_, path, request_viewer_handle,
       web_contents->GetContainerBounds().size());
 
   GURL current_url(url_utils::GetValueForKeyInUrlPathQuery(path, kUrlKey));
   std::string unsafe_page_html = viewer::GetUnsafeArticleTemplateHtml(
       url_utils::GetOriginalUrlFromDistillerUrl(current_url).spec(),
       dom_distiller_service_->GetDistilledPagePrefs()->GetTheme(),
       dom_distiller_service_->GetDistilledPagePrefs()->GetFontFamily());
 
-  // Add mojo service for JavaScript functionality. This is the receiving end
-  // of this particular service.
-  render_frame_host->GetInterfaceRegistry()->AddInterface(
-      base::Bind(&CreateDistillerJavaScriptService,
-          render_frame_host,
-          distiller_ui_handle_.get()));
-
-  // Tell the renderer that this is currently a distilled page.
-  mojom::DistillerPageNotifierServicePtr page_notifier_service;
-  render_frame_host->GetRemoteInterfaces()->GetInterface(
-      &page_notifier_service);
-  DCHECK(page_notifier_service);
-  page_notifier_service->NotifyIsDistillerPage();
-
   if (viewer_handle) {
     // The service returned a |ViewerHandle| and guarantees it will call
     // the |RequestViewerHandle|, so passing ownership to it, to ensure the
     // request is not cancelled. The |RequestViewerHandle| will delete itself
     // after receiving the callback.
     request_viewer_handle->TakeViewerHandle(std::move(viewer_handle));
   } else {
     request_viewer_handle->FlagAsErrorPage();
   }
 
@@ skipping 25 matching lines @@
 std::string DomDistillerViewerSource::GetContentSecurityPolicyStyleSrc()
     const {
   return "style-src 'self' https://fonts.googleapis.com;";
 }
 
 std::string DomDistillerViewerSource::GetContentSecurityPolicyChildSrc() const {
   return "child-src *;";
 }
 
 }  // namespace dom_distiller