Fix various issues from r343263.

chrome/browser/ui/webui/print_preview/print_preview_distiller.cc

 // Copyright 2015 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 "chrome/browser/ui/webui/print_preview/print_preview_distiller.h"
 
 #include <string>
 
-#include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/dom_distiller/tab_utils.h"
 #include "chrome/browser/printing/print_preview_dialog_controller.h"
 #include "chrome/browser/printing/print_preview_message_handler.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/web_contents_sizer.h"
 #include "chrome/common/prerender_messages.h"
 #include "components/dom_distiller/content/browser/distiller_javascript_utils.h"
 #include "components/printing/common/print_messages.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/render_process_host.h"
 #include "content/public/browser/render_view_host.h"
 #include "content/public/browser/session_storage_namespace.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/browser/web_contents_delegate.h"
 #include "content/public/browser/web_contents_observer.h"
 
 using content::OpenURLParams;
 using content::RenderViewHost;
 using content::SessionStorageNamespace;
 using content::WebContents;
 
+namespace {
+
+WebContents* CreateWebContents(
+    WebContents* source_web_contents) {
+  // TODO(ajwong): Remove the temporary map once prerendering is aware of
+  // multiple session storage namespaces per tab.
+  content::SessionStorageNamespaceMap session_storage_namespace_map;
+  session_storage_namespace_map[std::string()] =
+      source_web_contents->GetController().GetDefaultSessionStorageNamespace();
+  return WebContents::CreateWithSessionStorage(
+      WebContents::CreateParams(source_web_contents->GetBrowserContext()),
+      session_storage_namespace_map);
+}
+
+}  // namespace
+
 class PrintPreviewDistiller::WebContentsDelegateImpl
     : public content::WebContentsDelegate,
       public content::NotificationObserver,
       public content::WebContentsObserver {
  public:
-  explicit WebContentsDelegateImpl(WebContents* web_contents,

[Lei Zhang, 2015/08/14 05:44:35]

nit: not explicit

-                                   scoped_ptr<base::DictionaryValue> settings,
-                                   const base::Closure on_failed_callback)
+  WebContentsDelegateImpl(WebContents* web_contents,
+                          scoped_ptr<base::DictionaryValue> settings,
+                          const base::Closure& on_failed_callback)
       : content::WebContentsObserver(web_contents),
         settings_(settings.Pass()),
         on_failed_callback_(on_failed_callback) {
     web_contents->SetDelegate(this);
 
     // Close ourselves when the application is shutting down.
     notification_registrar_.Add(this, chrome::NOTIFICATION_APP_TERMINATING,
                                 content::NotificationService::AllSources());
 
     // Register to inform new RenderViews that we're rendering.
     notification_registrar_.Add(
         this, content::NOTIFICATION_WEB_CONTENTS_RENDER_VIEW_HOST_CREATED,
         content::Source<WebContents>(web_contents));
   }
 
   ~WebContentsDelegateImpl() override { web_contents()->SetDelegate(nullptr); }
 
   // content::WebContentsDelegate implementation.
   WebContents* OpenURLFromTab(WebContents* source,
                               const OpenURLParams& params) override {
     on_failed_callback_.Run();
     return nullptr;
   }
 
-  void CloseContents(content::WebContents* contents) override {
+  void CloseContents(WebContents* contents) override {
     on_failed_callback_.Run();
   }
 
   void CanDownload(const GURL& url,
                    const std::string& request_method,
                    const base::Callback<void(bool)>& callback) override {
     on_failed_callback_.Run();
     // Cancel the download.
     callback.Run(false);
   }
@@ skipping 36 matching lines @@
                                const std::string& protocol,
                                const GURL& url,
                                bool user_gesture) override {
     on_failed_callback_.Run();
   }
 
   void RenderFrameCreated(
       content::RenderFrameHost* render_frame_host) override {
     // When a new RenderFrame is created for a distilled rendering
     // WebContents, tell the new RenderFrame it's being used for
-    // prerendering before any navigations occur.  Note that this is
+    // prerendering before any navigations occurs. Note that this is

[PhistucK, 2015/08/14 06:34:03]

I think you introduced a grammar error here - "navigations occur", not "occurs".

     // always triggered before the first navigation, so there's no
     // need to send the message just after the WebContents is created.
     render_frame_host->Send(new PrerenderMsg_SetIsPrerendering(
         render_frame_host->GetRoutingID(), true));
   }
 
   void DidFinishLoad(content::RenderFrameHost* render_frame_host,
                      const GURL& validated_url) override {
     // Ask the page to trigger an anchor navigation once the distilled
     // contents are added to the page.
     dom_distiller::RunIsolatedJavaScript(
         web_contents()->GetMainFrame(),
-        "navigate_on_initial_content_load = true;");
+        "didNavigateOnInitialContentLoad()");

[Lei Zhang, 2015/08/14 05:44:35]

I haven't tested this to make sure it works.

[csaavedra, 2015/08/14 08:21:37]

This won't work, as this JS might run before the distiller's JS and the method
will be undefined.

   }
 
   void DidNavigateMainFrame(
       const content::LoadCommittedDetails& details,
       const content::FrameNavigateParams& params) override {
     // The second content loads signals that the distilled contents have
     // been delivered to the page via inline JavaScript execution.
     if (web_contents()->GetController().GetEntryCount() > 1) {
       RenderViewHost* rvh = web_contents()->GetRenderViewHost();
       rvh->Send(new PrintMsg_InitiatePrintPreview(rvh->GetRoutingID(), false));
@@ skipping 42 matching lines @@
       default:
         NOTREACHED() << "Unexpected notification sent.";
         break;
     }
   }
 
  private:
   scoped_ptr<base::DictionaryValue> settings_;
   content::NotificationRegistrar notification_registrar_;
 
-  // The callback called when the preview failed.
-  base::Closure on_failed_callback_;

[Lei Zhang, 2015/08/14 05:44:35]

const since it never changes

+  // The callback called when the distilling failed.
+  const base::Closure on_failed_callback_;
 };
 
 PrintPreviewDistiller::PrintPreviewDistiller(
     WebContents* source_web_contents,
-    const base::Closure on_failed_callback,
+    const base::Closure& on_failed_callback,
     scoped_ptr<base::DictionaryValue> settings) {
-  content::SessionStorageNamespace* session_storage_namespace =

[Lei Zhang, 2015/08/14 05:44:35]

Since this is derived from |source_web_content|, you can probably retrieve it
later instead of retrieving it here and passing it around.

-      source_web_contents->GetController().GetDefaultSessionStorageNamespace();
-  CreateDestinationWebContents(session_storage_namespace, source_web_contents,
-                               settings.Pass(), on_failed_callback);
+  CreateDestinationWebContents(source_web_contents,
+                               settings.Pass(),
+                               on_failed_callback);
+  DistillAndView(source_web_contents, web_contents_.get());
+}
 
-  DCHECK(web_contents_);

[Lei Zhang, 2015/08/14 05:44:35]

DistillAndView() will DCHECK its arguments, so this is redundant.

-  ::DistillAndView(source_web_contents, web_contents_.get());
+PrintPreviewDistiller::~PrintPreviewDistiller() {
+  printing::PrintPreviewDialogController* dialog_controller =
+      printing::PrintPreviewDialogController::GetInstance();
+  if (!dialog_controller)
+    return;
+
+  dialog_controller->RemoveProxyDialogForWebContents(web_contents_.get());
 }
 
 void PrintPreviewDistiller::CreateDestinationWebContents(
-    SessionStorageNamespace* session_storage_namespace,
     WebContents* source_web_contents,
     scoped_ptr<base::DictionaryValue> settings,
-    const base::Closure on_failed_callback) {
+    const base::Closure& on_failed_callback) {
   DCHECK(!web_contents_);
 
-  web_contents_.reset(
-      CreateWebContents(session_storage_namespace, source_web_contents));
+  web_contents_.reset(CreateWebContents(source_web_contents));
 
   printing::PrintPreviewMessageHandler::CreateForWebContents(
       web_contents_.get());
 
   web_contents_delegate_.reset(new WebContentsDelegateImpl(
       web_contents_.get(), settings.Pass(), on_failed_callback));
 
   // Set the size of the distilled WebContents.
   ResizeWebContents(web_contents_.get(), gfx::Size(1, 1));
 
   printing::PrintPreviewDialogController* dialog_controller =
       printing::PrintPreviewDialogController::GetInstance();
   if (!dialog_controller)
     return;
 
   dialog_controller->AddProxyDialogForWebContents(web_contents_.get(),
                                                   source_web_contents);
 }
-
-PrintPreviewDistiller::~PrintPreviewDistiller() {

[Lei Zhang, 2015/08/14 05:44:35]

Please put the dtor right below the ctor, in the same order as the header file.

-  if (web_contents_) {
-    printing::PrintPreviewDialogController* dialog_controller =
-        printing::PrintPreviewDialogController::GetInstance();
-    if (!dialog_controller)
-      return;
-
-    dialog_controller->RemoveProxyDialogForWebContents(web_contents_.get());
-  }
-}
-
-WebContents* PrintPreviewDistiller::CreateWebContents(

[Lei Zhang, 2015/08/14 05:44:35]

This is a standalone function, so it might as well be just that.

-    SessionStorageNamespace* session_storage_namespace,
-    WebContents* source_web_contents) {
-  // TODO(ajwong): Remove the temporary map once prerendering is aware of
-  // multiple session storage namespaces per tab.
-  content::SessionStorageNamespaceMap session_storage_namespace_map;
-  Profile* profile =
-      Profile::FromBrowserContext(source_web_contents->GetBrowserContext());
-  session_storage_namespace_map[std::string()] = session_storage_namespace;
-  return WebContents::CreateWithSessionStorage(
-      WebContents::CreateParams(profile), session_storage_namespace_map);
-}