Improve syncing between dom distiller and the print preview request

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 <stdint.h>
 #include <string>
 #include <utility>
 
 #include "base/command_line.h"
 #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"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/web_contents_sizer.h"
 #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/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/render_widget_host.h"
 #include "content/public/browser/resource_request_details.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;
+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,
                                    const base::Closure on_failed_callback)
       : content::WebContentsObserver(web_contents),
         settings_(std::move(settings)),
-        on_failed_callback_(on_failed_callback) {
+        on_failed_callback_(on_failed_callback),
+        distilled_article_ready_(false),
+        initial_content_loaded_(false) {
     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));
@@ skipping 72 matching lines @@
     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;");

[mdjones, 2016/02/23 19:23:30]

Is this still needed now that you get the article update events?

[mvendramini_hp, 2016/02/23 19:45:20]

Maybe we could remove everything related to initial_content_loaded_ altogether -
it does not break the logic, the patch still works, I've tested.

mdjones@ and nyquist@ WDYT?

[nyquist, 2016/02/24 01:53:08]

Yeah, I think that would be great. It would hopefully simplify this code a bit.

[mvendramini_hp, 2016/02/24 12:30:54]

Done.

+    initial_content_loaded_ = true;
+    MaybePrintPreview();
+  }
+
+  void MaybePrintPreview() {
+    // Wait until we are done distilling the article and the target
+    // WebContents is ready for printing.
+
+    // initial_content_loaded_ is expected to be always
+    // true (set by DidFinishLoad) before attempting to
+    // carry out the print preview
+    DCHECK(initial_content_loaded_);
+
+    if (!distilled_article_ready_)
+      return;
+
+    RenderViewHost* rvh = web_contents()->GetRenderViewHost();
+    rvh->Send(new PrintMsg_InitiatePrintPreview(rvh->GetRoutingID(), false));
+    rvh->Send(new PrintMsg_PrintPreview(rvh->GetRoutingID(), *settings_));
   }
 
   void DidNavigateMainFrame(

[mdjones, 2016/02/23 19:23:30]

Similarly, this method seems unnecessary now that you get the article update
events.

[mvendramini_hp, 2016/02/23 19:45:20]

Yes, it could be removed, (see my former comment) - i've tested removing this
and it's harmless to the patch - at least for the description test page.

Please advise.

       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_));
+      MaybePrintPreview();
     }
   }
 
   void DidGetRedirectForResourceRequest(
       content::RenderFrameHost* render_frame_host,
       const content::ResourceRedirectDetails& details) override {
     if (details.resource_type != content::RESOURCE_TYPE_MAIN_FRAME)
       return;
     // Redirects are unsupported for distilled content renderers.
     on_failed_callback_.Run();
@@ skipping 29 matching lines @@
         }
         break;
       }
 
       default:
         NOTREACHED() << "Unexpected notification sent.";
         break;
     }
   }
 
+  void OnArticleReady(
+      const dom_distiller::DistilledArticleProto* article_proto) override {
+    distilled_article_ready_ = true;
+    MaybePrintPreview();
+  }
+
+  void OnArticleUpdated(
+      dom_distiller::ArticleDistillationUpdate article_update) override {}
+
  private:
   scoped_ptr<base::DictionaryValue> settings_;
   content::NotificationRegistrar notification_registrar_;
 
   // The callback called when the preview failed.
   base::Closure on_failed_callback_;
+
+  bool distilled_article_ready_;
+  bool initial_content_loaded_;
 };
 
 const base::Feature PrintPreviewDistiller::kFeature = {
     "PrintPreviewDistiller", base::FEATURE_ENABLED_BY_DEFAULT,
 };
 
 bool PrintPreviewDistiller::IsEnabled() {
   return base::FeatureList::IsEnabled(kFeature);
 }
 
 PrintPreviewDistiller::PrintPreviewDistiller(
     WebContents* source_web_contents,
     const base::Closure on_failed_callback,
     scoped_ptr<base::DictionaryValue> settings) {
   content::SessionStorageNamespace* session_storage_namespace =
       source_web_contents->GetController().GetDefaultSessionStorageNamespace();
   CreateDestinationWebContents(session_storage_namespace, source_web_contents,
                                std::move(settings), on_failed_callback);
 
   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(
     SessionStorageNamespace* session_storage_namespace,
     WebContents* source_web_contents,
     scoped_ptr<base::DictionaryValue> settings,
     const base::Closure on_failed_callback) {
   DCHECK(!web_contents_);
 
   web_contents_.reset(
@@ skipping 33 matching lines @@
     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);
 }