Add incremental updates for multipage distillation.

components/dom_distiller/core/distiller.cc

 // Copyright 2013 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/core/distiller.h"
 
 #include <map>
+#include <vector>
 
 #include "base/bind.h"
 #include "base/callback.h"
 #include "base/location.h"
 #include "base/message_loop/message_loop.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/values.h"
 #include "components/dom_distiller/core/distiller_page.h"
 #include "components/dom_distiller/core/distiller_url_fetcher.h"
@@ skipping 71 matching lines @@
 
 DistillerImpl::DistilledPageData* DistillerImpl::GetPageAtIndex(size_t index)
     const {
   DCHECK_LT(index, pages_.size());
   DistilledPageData* page_data = pages_[index];
   DCHECK(page_data);
   return page_data;
 }
 
 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();
 }
 
 void DistillerImpl::DistillNextPage() {
   if (!waiting_pages_.empty()) {
     std::map<int, GURL>::iterator front = waiting_pages_.begin();
     int page_num = front->first;
     const GURL url = front->second;
@@ skipping 17 matching lines @@
 void DistillerImpl::OnPageDistillationFinished(
     int page_num,
     const GURL& page_url,
     scoped_ptr<DistilledPageInfo> distilled_page,
     bool distillation_successful) {
   DCHECK(distilled_page.get());
   DCHECK(started_pages_index_.find(page_num) != started_pages_index_.end());
   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()) {
       // The pages should be in same origin.
       DCHECK_EQ(next_page_url.GetOrigin(), page_url.GetOrigin());
       AddToDistillationQueue(page_num + 1, next_page_url);
     }
 
     GURL prev_page_url(distilled_page->prev_page_url);
     if (prev_page_url.is_valid()) {
@@ skipping 45 matching lines @@
       std::find(page_data->image_fetchers_.begin(),
                 page_data->image_fetchers_.end(),
                 url_fetcher);
 
   DCHECK(fetcher_it != page_data->image_fetchers_.end());
   // Delete the |url_fetcher| by DeleteSoon since the OnFetchImageDone
   // callback is invoked by the |url_fetcher|.
   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);
 
   AddPageIfDone(page_num);
 }
 
 void DistillerImpl::AddPageIfDone(int page_num) {
   DCHECK(started_pages_index_.find(page_num) != started_pages_index_.end());
   DCHECK(finished_pages_index_.find(page_num) == finished_pages_index_.end());
   DistilledPageData* page_data = GetPageAtIndex(started_pages_index_[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(
         new DistilledArticleProto());
     // Stitch the pages back into the article.
     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);
         first_page = false;
       }
 
       finished_pages_index_.erase(it++);
     }
 
     pages_.clear();
     DCHECK_LE(static_cast<size_t>(article_proto->pages_size()),
               max_pages_in_article_);
 
     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();
   }
 }
 
 }  // namespace dom_distiller