Fix various issues from r343263.

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

Index: chrome/browser/ui/webui/print_preview/print_preview_distiller.cc
diff --git a/chrome/browser/ui/webui/print_preview/print_preview_distiller.cc b/chrome/browser/ui/webui/print_preview/print_preview_distiller.cc
index b71b6736dde1ef75ae84b18c8c795b3ebd490758..852caf8fffe0db357b05175e85f6192d3628b440 100644
--- a/chrome/browser/ui/webui/print_preview/print_preview_distiller.cc
+++ b/chrome/browser/ui/webui/print_preview/print_preview_distiller.cc
@@ -6,7 +6,6 @@
 
 #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"
@@ -30,14 +29,30 @@ 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) {
@@ -62,7 +77,7 @@ class PrintPreviewDistiller::WebContentsDelegateImpl
     return nullptr;
   }
 
-  void CloseContents(content::WebContents* contents) override {
+  void CloseContents(WebContents* contents) override {
     on_failed_callback_.Run();
   }
 
@@ -119,7 +134,7 @@ class PrintPreviewDistiller::WebContentsDelegateImpl
       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(
@@ -132,7 +147,7 @@ class PrintPreviewDistiller::WebContentsDelegateImpl
     // 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(
@@ -195,32 +210,36 @@ class PrintPreviewDistiller::WebContentsDelegateImpl
   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());
+}
+
+PrintPreviewDistiller::~PrintPreviewDistiller() {
+  printing::PrintPreviewDialogController* dialog_controller =
+      printing::PrintPreviewDialogController::GetInstance();
+  if (!dialog_controller)
+    return;
 
-  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());
+  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());
@@ -239,27 +258,3 @@ void PrintPreviewDistiller::CreateDestinationWebContents(
   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);
-}