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 73 matching lines @@
         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;");
+    initial_content_loaded_ = true;

[nyquist, 2016/02/11 15:57:53]

It feels like this should also call MaybePrintPreview(). WDYT?

[mvendramini_hp, 2016/02/11 19:00:46]

I think it's unnecessary, but it wouldn't hurt to put it there just to be safe.

This was a concern raised previously by mdjones - please take a look at his
comments @ 2016/01/20 18:14:16, on that day I conducted a test and replied him
with my findings and further insights: 
https://codereview.chromium.org/1575473002/diff/20001/chrome/browser/ui/webui...

[nyquist, 2016/02/17 23:09:38]

So if you want to do that instead, then I think you should remove one of the
members. It looks very unsymmetric to me to have two members, but only trigger
on one of them.

So either:
- remove initial_content_loaded_
- DCHECK(initial_content_loaded_) in MaybePrintPreview() instead of having it as
part of the if-statement
- Add a call to MaybePrintPreview here.

I don't think it adds a lot of overhead to call MaybePrintPreview if you were
concerned about performance though. It'll just bail out after the first
if-check.

To make the code easier to read though, you might want to just use a DCHECK
instead, since it's an assumption you make.

[mvendramini_hp, 2016/02/18 12:27:02]

I agree it looks unsymmetric, and I think removing would look better and
simpler; though I went with the latter, for defensive coding reasons;

+  }
+
+  void MaybePrintPreview() {
+    // Wait until we are done distilling the article and the target
+    // WebContents is ready for printing.
+    if (!distilled_article_ready_ || !initial_content_loaded_)
+      return;
+    RenderViewHost* rvh = web_contents()->GetRenderViewHost();

[nyquist, 2016/02/11 15:57:53]

Nit: I think this would read easier if you added a newline before this line, to
separate the exceptional case above.

[mvendramini_hp, 2016/02/11 19:00:46]

Okay, done!

+    rvh->Send(new PrintMsg_InitiatePrintPreview(rvh->GetRoutingID(), false));
+    rvh->Send(new PrintMsg_PrintPreview(rvh->GetRoutingID(), *settings_));
   }
 
   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_));
+      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);
 }