[dom_distiller] Add support for incremental viewer.

components/dom_distiller/content/dom_distiller_viewer_source.cc

 // Copyright 2014 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 "components/dom_distiller/content/dom_distiller_viewer_source.h"
 
 #include <sstream>
 #include <string>
 #include <vector>
 
 #include "base/memory/ref_counted_memory.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/message_loop/message_loop.h"
+#include "base/strings/utf_string_conversions.h"
 #include "components/dom_distiller/core/task_tracker.h"
 #include "components/dom_distiller/core/url_constants.h"
 #include "components/dom_distiller/core/viewer.h"
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/render_view_host.h"
+#include "content/public/browser/web_contents.h"
+#include "content/public/browser/web_contents_observer.h"
 #include "net/url_request/url_request.h"
 
 namespace dom_distiller {
 
 // Handles receiving data asynchronously for a specific entry, and passing
-// it along to the data callback for the data source.
+// it along to the data callback for the data source. Lifetime matches that of
+// the RenderFrameHost for the Viewer instance.

[Charlie Reis, 2014/05/09 21:32:46]

What are your expectations in the case that the WebContents does a cross-process
navigation, and thus changes its top-level RenderFrameHost?  Will this class
stick around and continue processing (as it appears to), or should it stop and
go away?

I'm guessing you want to keep going as long as the WebContents is around (in
which case the comment is wrong), but I don't understand the case enough to say
for sure.

I'm also not sure if you need to restrict where the WebContents can navigate
(e.g., is it a problem if it redirects to the user's bank account?).

[Yaron, 2014/05/12 18:48:17]

It should go away. My intention was to have this basically have 1:1
corresponding with one visible instance of the viewer. 
Not necessary.
Any navigation could kill this object. If I'm missing a case, please let me
know. I suppose RenderProcessGone is one in the case of a tab crash. Is
RenderViewHostChange also necessary? Will we not get AboutToNavigateRenderView
cause a navigation would be necessary for this to happen?

[Charlie Reis, 2014/05/12 20:04:01]

Oh, I misunderstand.  That sounds good; one comment about what event to listen
for below.
Yes, RenderProcessGone would be good, in case of a crash. 
AboutToNavigateRenderView isn't right-- I've commented on that below.

 class DomDistillerViewerSource::RequestViewerHandle
-    : public ViewRequestDelegate {
+    : public ViewRequestDelegate,
+      public content::WebContentsObserver {
  public:
   explicit RequestViewerHandle(
+      int render_process_id,
+      int render_frame_id,
       const content::URLDataSource::GotDataCallback& callback);
   virtual ~RequestViewerHandle();
 
   // ViewRequestDelegate implementation.
   virtual void OnArticleReady(
       const DistilledArticleProto* article_proto) OVERRIDE;
 
   virtual void OnArticleUpdated(
       ArticleDistillationUpdate article_update) OVERRIDE;
 
   void TakeViewerHandle(scoped_ptr<ViewerHandle> viewer_handle);
 
+  // WebContentsObserver:
+  virtual void AboutToNavigateRenderView(
+      content::RenderViewHost* render_view_host) OVERRIDE;
+  virtual void WebContentsDestroyed(content::WebContents* web_contents)
+       OVERRIDE;
+  virtual void DidFinishLoad(
+      int64 frame_id,
+      const GURL& validated_url,
+      bool is_main_frame,
+      content::RenderViewHost* render_view_host) OVERRIDE;
+
  private:
+  // Sends JavaScript to the attached Viewer, buffering data if the viewer isn't
+  // ready.
+  void SendJavaScript(const std::string& buffer);
+
+  // Cancel's the current view request. Once called, no updates will be

[Charlie Reis, 2014/05/09 21:32:46]

nit: Cancels

[Yaron, 2014/05/12 18:48:17]

Done.

[Yaron, 2014/05/12 18:48:17]

Done.

+  // propagated to the view, and the request to DomDistillerService will be
+  // cancelled.
+  void Cancel();
+
   // The handle to the view request towards the DomDistillerService. It
   // needs to be kept around to ensure the distillation request finishes.
   scoped_ptr<ViewerHandle> viewer_handle_;
 
-  // This holds the callback to where the data retrieved is sent back.
+  // WebContents associated with the Viewer's render process.
+  content::WebContents* web_contents_;
+
+  // Holds the callback to where the data retrieved is sent back.
   content::URLDataSource::GotDataCallback callback_;
+
+  // Number of pages that have been rendered by the viewer.

[Charlie Reis, 2014/05/09 21:32:46]

I'm not sure what this comment is referring to.  Are you talking about how many
per-frame documents have loaded, how many NavigationEntries have committed in
the WebContents over time, or something else?  (Conceptual pages in the
article?)

[Yaron, 2014/05/12 18:48:17]

Conceptual pages of the article. Expanded the comment slightly.

+  int page_count_;
+
+  // Whether the page is sufficiently initialized to handle updates from the
+  // distiller.
+  bool waiting_for_page_ready_;
+
+  // Temporary store of pending JavaScript if the page isn't ready to receive
+  // data from distillation.
+  std::string buffer_;
 };
 
 DomDistillerViewerSource::RequestViewerHandle::RequestViewerHandle(
+    int render_process_id,
+    int render_frame_id,
     const content::URLDataSource::GotDataCallback& callback)
-    : callback_(callback) {
+    : web_contents_(content::WebContents::FromRenderFrameHost(
+          content::RenderFrameHost::FromID(render_process_id,
+                                           render_frame_id))),
+      callback_(callback),
+      page_count_(0),
+      waiting_for_page_ready_(true) {
+  content::WebContentsObserver::Observe(web_contents_);
 }
 
 DomDistillerViewerSource::RequestViewerHandle::~RequestViewerHandle() {
+  // Balanced with constructor although can be a no-op if frame navigated away.
+  content::WebContentsObserver::Observe(NULL);
+}
+
+void DomDistillerViewerSource::RequestViewerHandle::SendJavaScript(
+    const std::string& buffer) {
+  if (waiting_for_page_ready_) {
+    buffer_ += buffer;
+  } else {
+    if (web_contents_) {
+      web_contents_->GetMainFrame()->ExecuteJavaScript(
+          base::UTF8ToUTF16(buffer));
+    }
+  }
+}
+
+void DomDistillerViewerSource::RequestViewerHandle::WebContentsDestroyed(
+    content::WebContents* web_contents) {
+  Cancel();
+}
+
+void DomDistillerViewerSource::RequestViewerHandle::AboutToNavigateRenderView(
+        content::RenderViewHost* render_view_host) {

[Charlie Reis, 2014/05/09 21:32:46]

nit: Wrong indent.

[Yaron, 2014/05/12 18:48:17]

Done.

+  Cancel();
+}
+
+void DomDistillerViewerSource::RequestViewerHandle::Cancel() {
+  // Ensure we don't send any incremental updates to the Viewer.
+  web_contents_ = NULL;
+
+  // No need to listen for notifications.
+  content::WebContentsObserver::Observe(NULL);
+
+  // Schedule the Viewer for deletion. Ensures distillation is cancelled, and
+  // any pending data stored in |buffer_| is released.
+  base::MessageLoop::current()->DeleteSoon(FROM_HERE, this);
+}
+
+void DomDistillerViewerSource::RequestViewerHandle::DidFinishLoad(
+     int64 frame_id,
+     const GURL& validated_url,
+     bool is_main_frame,
+     content::RenderViewHost* render_view_host)  {
+  if (!is_main_frame || web_contents_ == NULL) {
+    return;
+  }
+  waiting_for_page_ready_ = false;
+  if (buffer_.empty()) return;

[Charlie Reis, 2014/05/09 21:32:46]

nit: Return should be on its own line.

[Yaron, 2014/05/12 18:48:17]

Done.

+
+  if (web_contents_) {
+    web_contents_->GetMainFrame()->ExecuteJavaScript(
+        base::UTF8ToUTF16(buffer_));
+  }
+  buffer_.clear();
 }
 
 void DomDistillerViewerSource::RequestViewerHandle::OnArticleReady(
     const DistilledArticleProto* article_proto) {
-  std::string unsafe_page_html = viewer::GetUnsafeHtml(article_proto);
-  callback_.Run(base::RefCountedString::TakeString(&unsafe_page_html));
-  base::MessageLoop::current()->DeleteSoon(FROM_HERE, this);
+  if (page_count_ == 0) {
+    // This is a single-page article.
+    std::string unsafe_page_html = viewer::GetUnsafeArticleHtml(article_proto);
+    callback_.Run(base::RefCountedString::TakeString(&unsafe_page_html));
+  } else if (page_count_ == article_proto->pages_size()) {
+    // We may still be showing the "Loading" indicator.
+    SendJavaScript(viewer::GetToggleLoadingIndicatorJs(true));
+  } else {
+    // It's possible that we didn't get some incremental updates from the
+    // distiller. Ensure all remaining pages are flushed to the viewer.
+    for (;page_count_ < article_proto->pages_size();
+        page_count_++) {

[Charlie Reis, 2014/05/09 21:32:46]

nit: Won't this fit on the previous line?

[Yaron, 2014/05/12 18:48:17]

Done.

+      const DistilledPageProto& page = article_proto->pages(page_count_);
+      SendJavaScript(
+          viewer::GetUnsafeIncrementalDistilledPageJs(
+              &page,
+              page_count_ == article_proto->pages_size()));
+    }
+  }
+  // No need to hold on to the ViewerHandle now that distillation is complete.
+  viewer_handle_.reset();
 }
 
 void DomDistillerViewerSource::RequestViewerHandle::OnArticleUpdated(
     ArticleDistillationUpdate article_update) {
-  // TODO(nyquist): Add support for displaying pages incrementally.
+  for (;page_count_ < static_cast<int>(article_update.GetPagesSize());
+       page_count_++) {
+    const DistilledPageProto& page =
+        article_update.GetDistilledPage(page_count_);
+    if (page_count_ == 0) {
+      std::string unsafe_page_html = viewer::GetUnsafePartialArticleHtml(&page);
+      callback_.Run(base::RefCountedString::TakeString(&unsafe_page_html));
+    } else {
+      SendJavaScript(
+            viewer::GetUnsafeIncrementalDistilledPageJs(&page, false));

[Charlie Reis, 2014/05/09 21:32:46]

nit: Wrong indent.

[Yaron, 2014/05/12 18:48:17]

Done.

+    }
+  }
 }
 
 void DomDistillerViewerSource::RequestViewerHandle::TakeViewerHandle(
     scoped_ptr<ViewerHandle> viewer_handle) {
   viewer_handle_ = viewer_handle.Pass();
 }
 
 DomDistillerViewerSource::DomDistillerViewerSource(
     DomDistillerServiceInterface* dom_distiller_service,
     const std::string& scheme)
@@ skipping 13 matching lines @@
     int render_frame_id,
     const content::URLDataSource::GotDataCallback& callback) {
   content::RenderFrameHost* render_frame_host =
       content::RenderFrameHost::FromID(render_process_id, render_frame_id);
   DCHECK(render_frame_host);
   content::RenderViewHost* render_view_host =
       render_frame_host->GetRenderViewHost();
   DCHECK(render_view_host);
   CHECK_EQ(0, render_view_host->GetEnabledBindings());
 
-  if (kCssPath == path) {
+  if (kViewerCssPath == path) {
     std::string css = viewer::GetCss();
     callback.Run(base::RefCountedString::TakeString(&css));
     return;
+  } else if (kViewerJsPath == path) {

[Charlie Reis, 2014/05/09 21:32:46]

nit: No need for an else if the previous block returns.

[Yaron, 2014/05/12 18:48:17]

Done.

+    std::string js = viewer::GetJavaScript();
+    callback.Run(base::RefCountedString::TakeString(&js));
+    return;
   }
-
   RequestViewerHandle* request_viewer_handle =
-      new RequestViewerHandle(callback);
+      new RequestViewerHandle(render_process_id, render_frame_id, callback);

[Charlie Reis, 2014/05/09 21:32:46]

Maybe it's easier to just pass
content::WebContents::FromRenderFrameHost(render_frame_host) here, since the
RequestViewerHandle doesn't seem to care about render_process_id or
render_frame_id?

[Yaron, 2014/05/12 18:48:17]

Done.

   scoped_ptr<ViewerHandle> viewer_handle = viewer::CreateViewRequest(
       dom_distiller_service_, path, request_viewer_handle);
 
   if (viewer_handle) {
     // The service returned a |ViewerHandle| and guarantees it will call
     // the |RequestViewerHandle|, so passing ownership to it, to ensure the
     // request is not cancelled. The |RequestViewerHandle| will delete itself
     // after receiving the callback.
     request_viewer_handle->TakeViewerHandle(viewer_handle.Pass());
   } else {
     // The service did not return a |ViewerHandle|, which means the
     // |RequestViewerHandle| will never be called, so clean up now.
     delete request_viewer_handle;
 
     std::string error_page_html = viewer::GetErrorPageHtml();
     callback.Run(base::RefCountedString::TakeString(&error_page_html));
   }
 };
 
 std::string DomDistillerViewerSource::GetMimeType(
     const std::string& path) const {
-  if (path == kCssPath)
+  if (kViewerCssPath == path)
     return "text/css";
+  else if (kViewerJsPath == path)
+    return "text/javascript";
   return "text/html";
 }
 
 bool DomDistillerViewerSource::ShouldServiceRequest(
     const net::URLRequest* request) const {
   return request->url().SchemeIs(scheme_.c_str());
 }
 
 // TODO(nyquist): Start tracking requests using this method.
 void DomDistillerViewerSource::WillServiceRequest(
     const net::URLRequest* request,
     std::string* path) const {
 }
 
 std::string DomDistillerViewerSource::GetContentSecurityPolicyObjectSrc()
     const {
   return "object-src 'none'; style-src 'self';";
 }
 
 }  // namespace dom_distiller