Improve syncing between dom distiller and the print preview request

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 f3076d92fcdc5cc124ac5afff6cd3a56280d1502..0d6f887940b24f0a1be3c878f00309a9c95d3ccd 100644
--- a/chrome/browser/ui/webui/print_preview/print_preview_distiller.cc
+++ b/chrome/browser/ui/webui/print_preview/print_preview_distiller.cc
@@ -12,6 +12,7 @@
 #include "base/feature_list.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/chrome_notification_types.h"
+#include "chrome/browser/dom_distiller/dom_distiller_service_factory.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"
@@ -20,7 +21,10 @@
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/prerender_messages.h"
 #include "components/dom_distiller/content/browser/distiller_javascript_utils.h"
+#include "components/dom_distiller/core/dom_distiller_service.h"
+#include "components/dom_distiller/core/task_tracker.h"
 #include "components/printing/common/print_messages.h"
+#include "content/public/browser/navigation_details.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/render_process_host.h"
@@ -36,11 +40,14 @@ using content::OpenURLParams;
 using content::RenderViewHost;
 using content::SessionStorageNamespace;
 using content::WebContents;
+using dom_distiller::DomDistillerService;
+using dom_distiller::DomDistillerServiceFactory;
 
 class PrintPreviewDistiller::WebContentsDelegateImpl
     : public content::WebContentsDelegate,
       public content::NotificationObserver,
-      public content::WebContentsObserver {
+      public content::WebContentsObserver,
+      public dom_distiller::ViewRequestDelegate {
  public:
   explicit WebContentsDelegateImpl(WebContents* web_contents,
                                    scoped_ptr<base::DictionaryValue> settings,
@@ -134,25 +141,13 @@ class PrintPreviewDistiller::WebContentsDelegateImpl
         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;");
-  }
+  void MaybePrintPreview() {
+    // Wait until we are done distilling the article and the target
+    // WebContents is ready for printing.
 
-  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));
-      rvh->Send(new PrintMsg_PrintPreview(rvh->GetRoutingID(), *settings_));
-    }
+    RenderViewHost* rvh = web_contents()->GetRenderViewHost();
+    rvh->Send(new PrintMsg_InitiatePrintPreview(rvh->GetRoutingID(), false));

[wychen, 2016/04/06 06:13:15]

The resources might still be not ready here, like images or web fonts.

Some signals in the renderer might be useful, like
didMeaningfulLayout(WebMeaningfulLayout::FinishedLoading), but it cannot be used
as is because our content is dynamically loaded.

This could be in another CL.

[mvendramini_hp, 2016/04/06 13:39:04]

Also, at this point, MaybePrintPreview is no longer a descriptive name here -
since there is no longer any decision making in this function, there is no
longer justification for calling it "Maybe".

How about changing it to "DoPrintPreview" or similar? Your concern above should
perhaps be implemented in where we decide to call this function (currently from
DidNavigateMainFrame)

wychen@ mdjones@ WDYT ?

[wychen, 2016/04/06 19:48:06]

Yeah. The name is confusing now. Renaming it would be appropriate.

Do you plan to address the issue about resource readiness in this CL?

+    rvh->Send(new PrintMsg_PrintPreview(rvh->GetRoutingID(), *settings_));
   }
 
   void DidGetRedirectForResourceRequest(
@@ -168,6 +163,16 @@ class PrintPreviewDistiller::WebContentsDelegateImpl
     on_failed_callback_.Run();
   }
 
+  void DidNavigateMainFrame(
+      const ::content::LoadCommittedDetails& details,
+      const content::FrameNavigateParams& params) override {
+    // The navigation to notify print preview that content is on the page will
+    // be an in-page navigation.
+    if (!details.is_in_page)
+      return;
+    MaybePrintPreview();
+  }
+
   void Observe(int type,
                const content::NotificationSource& source,
                const content::NotificationDetails& details) override {
@@ -201,6 +206,12 @@ class PrintPreviewDistiller::WebContentsDelegateImpl
     }
   }
 
+  void OnArticleReady(
+      const dom_distiller::DistilledArticleProto* article_proto) override {}
+
+  void OnArticleUpdated(
+      dom_distiller::ArticleDistillationUpdate article_update) override {}
+
  private:
   scoped_ptr<base::DictionaryValue> settings_;
   content::NotificationRegistrar notification_registrar_;
@@ -228,6 +239,15 @@ PrintPreviewDistiller::PrintPreviewDistiller(
 
   DCHECK(web_contents_);
   ::DistillAndView(source_web_contents, web_contents_.get());
+
+  DomDistillerService* dom_distiller_service =
+      DomDistillerServiceFactory::GetForBrowserContext(
+          source_web_contents->GetBrowserContext());
+
+  const GURL& url = source_web_contents->GetLastCommittedURL();
+
+  viewer_handle_ = dom_distiller_service->AddViewRequestDelegate(
+      web_contents_delegate_.get(), url);
 }
 
 void PrintPreviewDistiller::CreateDestinationWebContents(