Add incremental updates for multipage distillation.

components/dom_distiller/core/distiller.cc

Index: components/dom_distiller/core/distiller.cc
diff --git a/components/dom_distiller/core/distiller.cc b/components/dom_distiller/core/distiller.cc
index 8bdc905a8231236647c96882d3c8638e0dec2641..6a30b9652ef540383156f127942ab761ae41d681 100644
--- a/components/dom_distiller/core/distiller.cc
+++ b/components/dom_distiller/core/distiller.cc
@@ -5,6 +5,7 @@
 #include "components/dom_distiller/core/distiller.h"
 
 #include <map>
+#include <vector>
 
 #include "base/bind.h"
 #include "base/callback.h"
@@ -96,9 +97,11 @@ DistillerImpl::DistilledPageData* DistillerImpl::GetPageAtIndex(size_t index)
 }
 
 void DistillerImpl::DistillPage(const GURL& url,
-                            const DistillerCallback& distillation_cb) {
+                                const ArticleDistillationCallback& article_cb,
+                                const PageDistillationCallback& page_cb) {
   DCHECK(AreAllPagesFinished());
-  distillation_cb_ = distillation_cb;
+  article_cb_ = article_cb;
+  page_cb_ = page_cb;
 
   AddToDistillationQueue(0, url);
   DistillNextPage();
@@ -136,13 +139,14 @@ void DistillerImpl::OnPageDistillationFinished(
   if (distillation_successful) {
     DistilledPageData* page_data =
         GetPageAtIndex(started_pages_index_[page_num]);
-    DistilledPageProto* current_page = new DistilledPageProto();
-    page_data->proto.reset(current_page);
+    base::RefCountedData<DistilledPageProto>* current_page =

[cjhopman, 2014/02/28 02:38:14]

This isn't used again so just create the new one when assigning to
page_data->proto

[shashi, 2014/03/02 02:53:40]

Done.

+        new base::RefCountedData<DistilledPageProto>();
+    page_data->proto = current_page;
     page_data->page_num = page_num;
     page_data->title = distilled_page->title;
 
-    current_page->set_url(page_url.spec());
-    current_page->set_html(distilled_page->html);
+    page_data->proto->data.set_url(page_url.spec());
+    page_data->proto->data.set_html(distilled_page->html);
 
     GURL next_page_url(distilled_page->next_page_url);
     if (next_page_url.is_valid()) {
@@ -208,7 +212,7 @@ void DistillerImpl::OnFetchImageDone(int page_num,
   page_data->image_fetchers_.weak_erase(fetcher_it);
   base::MessageLoop::current()->DeleteSoon(FROM_HERE, url_fetcher);
 
-  DistilledPageProto_Image* image = page_data->proto->add_image();
+  DistilledPageProto_Image* image = page_data->proto->data.add_image();
   image->set_name(id);
   image->set_data(response);
 
@@ -222,12 +226,40 @@ void DistillerImpl::AddPageIfDone(int page_num) {
   if (page_data->image_fetchers_.empty()) {
     finished_pages_index_[page_num] = started_pages_index_[page_num];
     started_pages_index_.erase(page_num);
+    bool has_prev_page = false;

[cjhopman, 2014/02/28 02:38:14]

I think I'd extract all this to a new method so that here it is just:

page_cb.Run(CreateArticleDistillationUpdate());

[shashi, 2014/03/02 02:53:40]

Done.

+    bool has_next_page = false;
+    if (!finished_pages_index_.empty()) {
+      int prev_page_num = finished_pages_index_.begin()->first - 1;
+      int next_page_num = finished_pages_index_.rbegin()->first + 1;
+      has_prev_page =
+          (waiting_pages_.find(prev_page_num) != waiting_pages_.end()) ||

[cjhopman, 2014/02/28 02:38:14]

Can't we just use IsPageNumberInUse?

[shashi, 2014/03/02 02:53:40]

We can since finished_pages is sorted :), done.

+          started_pages_index_.find(prev_page_num) !=
+              started_pages_index_.end();
+
+      has_next_page =
+          (waiting_pages_.find(next_page_num) != waiting_pages_.end()) ||
+          started_pages_index_.find(next_page_num) !=
+              started_pages_index_.end();
+    }
+
+    std::vector<ArticleDistillationUpdate::RefCountedDistilledPageProto>
+        update_pages;
+    for (std::map<int, size_t>::const_iterator it =
+             finished_pages_index_.begin();
+         it != finished_pages_index_.end();
+         ++it) {
+      update_pages.push_back(pages_[it->second]->proto);
+    }
+    ArticleDistillationUpdate article_update(
+        update_pages, has_next_page, has_prev_page);
+    DCHECK_EQ(article_update.getPagesSize(), finished_pages_index_.size());
+    page_cb_.Run(article_update);
     RunDistillerCallbackIfDone();

[cjhopman, 2014/02/28 02:38:14]

Do we really want to call both the update and finished when we are done?

[shashi, 2014/03/02 02:53:40]

For last page, we can skip the update, I did not make that optimization. I can
add this check, but needs to be decided wrt. API behavior.

if(has_next_page || has_prev_page) {
  Fire incremental
} else {
   Fire complete.
}

   }
 }
 
 void DistillerImpl::RunDistillerCallbackIfDone() {
-  DCHECK(!distillation_cb_.is_null());
+  DCHECK(!article_cb_.is_null());
   if (AreAllPagesFinished()) {
     bool first_page = true;
     scoped_ptr<DistilledArticleProto> article_proto(
@@ -236,7 +268,7 @@ void DistillerImpl::RunDistillerCallbackIfDone() {
     for (std::map<int, size_t>::iterator it = finished_pages_index_.begin();
          it != finished_pages_index_.end();) {
       DistilledPageData* page_data = GetPageAtIndex(it->second);
-      *(article_proto->add_pages()) = *(page_data->proto);
+      *(article_proto->add_pages()) = page_data->proto.get()->data;
 
       if (first_page) {
         article_proto->set_title(page_data->title);
@@ -252,8 +284,8 @@ void DistillerImpl::RunDistillerCallbackIfDone() {
 
     DCHECK(pages_.empty());
     DCHECK(finished_pages_index_.empty());
-    distillation_cb_.Run(article_proto.Pass());
-    distillation_cb_.Reset();
+    article_cb_.Run(article_proto.Pass());
+    article_cb_.Reset();
   }
 }