Add incremental updates for multipage distillation.

Add incremental updates for multipage distillation.

Add support for providing incremental updates for multipage
distillation. Updates are now broadcasted to the viewers, whenever a new
page in the article finishes distillation. This allows viewer to show
data to the user, before the distillation of the entire article is
completed.
This change required changing management of pages in distiller to be
refcounted.

BUG=288015
NOTRY=true
R=cjhopman@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=255708

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

https://chromiumcodereview.appspot.com/178303004/diff/100002/components/dom_d...
File components/dom_distiller.gypi (right):

https://chromiumcodereview.appspot.com/178303004/diff/100002/components/dom_d...
components/dom_distiller.gypi:54:
'dom_distiller/core/article_distillation_update.h',
nit:sort

https://chromiumcodereview.appspot.com/178303004/diff/100002/components/dom_d...
File components/dom_distiller/content/dom_distiller_viewer_source.h (right):

https://chromiumcodereview.appspot.com/178303004/diff/100002/components/dom_d...
components/dom_distiller/content/dom_distiller_viewer_source.h:8: #include
<string>
nit: not needed

https://chromiumcodereview.appspot.com/178303004/diff/100002/components/dom_d...
File components/dom_distiller/core/article_distillation_update.cc (right):

https://chromiumcodereview.appspot.com/178303004/diff/100002/components/dom_d...
components/dom_distiller/core/article_distillation_update.cc:15: pages_ = pages;
These should be initialized in the initializer list

 ADU::ADU(...) : pages_(pages), ... {}

https://chromiumcodereview.appspot.com/178303004/diff/100002/components/dom_d...
components/dom_distiller/core/article_distillation_update.cc:28: size_t
ArticleDistillationUpdate::getPagesSize() const { return pages_.size(); }
These three could be inlined in the header.

https://chromiumcodereview.appspot.com/178303004/diff/100002/components/dom_d...
File components/dom_distiller/core/article_distillation_update.h (right):

https://chromiumcodereview.appspot.com/178303004/diff/100002/components/dom_d...
components/dom_distiller/core/article_distillation_update.h:27: const
RefCountedDistilledPageProto getDistilledPage(size_t index) const;
this should return a const& (not const value).

https://chromiumcodereview.appspot.com/178303004/diff/100002/components/dom_d...
components/dom_distiller/core/article_distillation_update.h:27: const
RefCountedDistilledPageProto getDistilledPage(size_t index) const;
s/get/Get

https://chromiumcodereview.appspot.com/178303004/diff/100002/components/dom_d...
components/dom_distiller/core/article_distillation_update.h:29: size_t
getPagesSize() const;
capitalize first letters of function names

https://chromiumcodereview.appspot.com/178303004/diff/100002/components/dom_d...
File components/dom_distiller/core/distiller.cc (right):

https://chromiumcodereview.appspot.com/178303004/diff/100002/components/dom_d...
components/dom_distiller/core/distiller.cc:142:
base::RefCountedData<DistilledPageProto>* current_page =
This isn't used again so just create the new one when assigning to
page_data->proto

https://chromiumcodereview.appspot.com/178303004/diff/100002/components/dom_d...
components/dom_distiller/core/distiller.cc:229: bool has_prev_page = false;
I think I'd extract all this to a new method so that here it is just:

page_cb.Run(CreateArticleDistillationUpdate());

https://chromiumcodereview.appspot.com/178303004/diff/100002/components/dom_d...
components/dom_distiller/core/distiller.cc:235:
(waiting_pages_.find(prev_page_num) != waiting_pages_.end()) ||
Can't we just use IsPageNumberInUse?

https://chromiumcodereview.appspot.com/178303004/diff/100002/components/dom_d...
components/dom_distiller/core/distiller.cc:257: RunDistillerCallbackIfDone();
Do we really want to call both the update and finished when we are done?

https://chromiumcodereview.appspot.com/178303004/diff/100002/components/dom_d...
File components/dom_distiller/core/distiller.h (right):

https://chromiumcodereview.appspot.com/178303004/diff/100002/components/dom_d...
components/dom_distiller/core/distiller.h:32: PageDistillationCallback;
How about:

DistillationFinishedCallback
and
DistillationUpdateCallback

i.e. don't tie the incremental updates to the concept of pages

https://chromiumcodereview.appspot.com/178303004/diff/100002/components/dom_d...
components/dom_distiller/core/distiller.h:32: PageDistillationCallback;
Consider making a new class for these callbacks (i.e. DistillerDelegate).

On second thought, I think this is probably fine since its not really exposed to
users (they would use the ViewRequestDelegate).

https://chromiumcodereview.appspot.com/178303004/diff/100002/components/dom_d...
components/dom_distiller/core/distiller.h:36: // Distills a page, and
asynchrounously returns the article HTML to the
s/asynchrounously/asynchronously

https://chromiumcodereview.appspot.com/178303004/diff/100002/components/dom_d...
components/dom_distiller/core/distiller.h:37: // supplied callback.
Update this comment to reflect these changes.

https://chromiumcodereview.appspot.com/178303004/diff/100002/components/dom_d...
File components/dom_distiller/core/distiller_unittest.cc (right):

https://chromiumcodereview.appspot.com/178303004/diff/100002/components/dom_d...
components/dom_distiller/core/distiller_unittest.cc:201:
ACTION_P5(VerifyIncrementalUpdatesMatch,
Why is this an action? It isn't ever used as an action.

https://chromiumcodereview.appspot.com/178303004/diff/100002/components/dom_d...
components/dom_distiller/core/distiller_unittest.cc:203: kNumPages,
nit: the capitalization of this is wrong

https://chromiumcodereview.appspot.com/178303004/diff/100002/components/dom_d...
components/dom_distiller/core/distiller_unittest.cc:207: vector<int> page_nums =
get_pages_in_sequence(start_page_num, kNumPages);
This function contains the following names:
start_page_num
page_num
page_nums
num_pages
kNumPages
update_pages
actual_page_num

This makes it really hard to follow. Please rename some things and maybe use
more descriptive names.

https://chromiumcodereview.appspot.com/178303004/diff/100002/components/dom_d...
components/dom_distiller/core/distiller_unittest.cc:215: for (int page_num = 0;
page_num < incremental_updates.size(); ++page_num) {
This seems like the index of an incremental update, why is it called page_num?

https://chromiumcodereview.appspot.com/178303004/diff/100002/components/dom_d...
components/dom_distiller/core/distiller_unittest.cc:215: for (int page_num = 0;
page_num < incremental_updates.size(); ++page_num) {
incremental_updates is a vector, why not use an iterator? If not that, than at
least store a reference into the vector here so you don't keep repeating the
lookup.

https://chromiumcodereview.appspot.com/178303004/diff/100002/components/dom_d...
components/dom_distiller/core/distiller_unittest.cc:218: vector<int>
update_pages(page_nums.begin(), page_nums.begin() + num_pages);
I'd rename this: expected_pages_in_update or some such

https://chromiumcodereview.appspot.com/178303004/diff/100002/components/dom_d...
components/dom_distiller/core/distiller_unittest.cc:352: TEST_F(DistillerTest,
CheckMaxPageLimit) {
Just noticed: This should probably be three separate tests

https://chromiumcodereview.appspot.com/178303004/diff/100002/components/dom_d...
components/dom_distiller/core/distiller_unittest.cc:612: }
This setup code seems to be repeated three times. Should probably just extract
that out.

https://chromiumcodereview.appspot.com/178303004/diff/100002/components/dom_d...
File components/dom_distiller/core/task_tracker.h (right):

https://chromiumcodereview.appspot.com/178303004/diff/100002/components/dom_d...
components/dom_distiller/core/task_tracker.h:50: // Called when an article that
is currently under distillation is updated.
Hm, this comment is confusing... does that mean the page has been updated? 

Maybe:
OnArticleUpdated is called when a new part of the distilled article contents is
available. See the ArticleDistillationUpdate documentation for what information
is available when this is called.

I don't know if that second sentence is any good.

https://chromiumcodereview.appspot.com/178303004/diff/100002/components/dom_d...
components/dom_distiller/core/task_tracker.h:51: virtual void
OnArticleUpdated(ArticleDistillationUpdate article_update) = 0;
this should be taking a const& to the ArticleDistillationUpdate

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

PTAL

https://chromiumcodereview.appspot.com/178303004/diff/100002/components/dom_d...
File components/dom_distiller.gypi (right):

https://chromiumcodereview.appspot.com/178303004/diff/100002/components/dom_d...
components/dom_distiller.gypi:54:
'dom_distiller/core/article_distillation_update.h',
On 2014/02/28 02:38:14, cjhopman wrote:
> nit:sort

Done.

https://chromiumcodereview.appspot.com/178303004/diff/100002/components/dom_d...
File components/dom_distiller/content/dom_distiller_viewer_source.h (right):

https://chromiumcodereview.appspot.com/178303004/diff/100002/components/dom_d...
components/dom_distiller/content/dom_distiller_viewer_source.h:8: #include
<string>
On 2014/02/28 02:38:14, cjhopman wrote:
> nit: not needed
It is cpplint complains about it: since this file uses std::string.

https://chromiumcodereview.appspot.com/178303004/diff/100002/components/dom_d...
File components/dom_distiller/core/article_distillation_update.cc (right):

https://chromiumcodereview.appspot.com/178303004/diff/100002/components/dom_d...
components/dom_distiller/core/article_distillation_update.cc:15: pages_ = pages;
On 2014/02/28 02:38:14, cjhopman wrote:
> These should be initialized in the initializer list
> 
>  ADU::ADU(...) : pages_(pages), ... {}

Done.

https://chromiumcodereview.appspot.com/178303004/diff/100002/components/dom_d...
components/dom_distiller/core/article_distillation_update.cc:28: size_t
ArticleDistillationUpdate::getPagesSize() const { return pages_.size(); }
On 2014/02/28 02:38:14, cjhopman wrote:
> These three could be inlined in the header.

Done.

https://chromiumcodereview.appspot.com/178303004/diff/100002/components/dom_d...
File components/dom_distiller/core/article_distillation_update.h (right):

https://chromiumcodereview.appspot.com/178303004/diff/100002/components/dom_d...
components/dom_distiller/core/article_distillation_update.h:27: const
RefCountedDistilledPageProto getDistilledPage(size_t index) const;
On 2014/02/28 02:38:14, cjhopman wrote:
> this should return a const& (not const value).

This is a scoped_refptr<>, maybe I should rename it to 
RefPtrToDistilledPageProto ?

https://chromiumcodereview.appspot.com/178303004/diff/100002/components/dom_d...
components/dom_distiller/core/article_distillation_update.h:27: const
RefCountedDistilledPageProto getDistilledPage(size_t index) const;
On 2014/02/28 02:38:14, cjhopman wrote:
> s/get/Get

Done.

https://chromiumcodereview.appspot.com/178303004/diff/100002/components/dom_d...
components/dom_distiller/core/article_distillation_update.h:29: size_t
getPagesSize() const;
On 2014/02/28 02:38:14, cjhopman wrote:
> capitalize first letters of function names

Done.

https://chromiumcodereview.appspot.com/178303004/diff/100002/components/dom_d...
File components/dom_distiller/core/distiller.cc (right):

https://chromiumcodereview.appspot.com/178303004/diff/100002/components/dom_d...
components/dom_distiller/core/distiller.cc:142:
base::RefCountedData<DistilledPageProto>* current_page =
On 2014/02/28 02:38:14, cjhopman wrote:
> This isn't used again so just create the new one when assigning to
> page_data->proto

Done.

https://chromiumcodereview.appspot.com/178303004/diff/100002/components/dom_d...
components/dom_distiller/core/distiller.cc:229: bool has_prev_page = false;
On 2014/02/28 02:38:14, cjhopman wrote:
> I think I'd extract all this to a new method so that here it is just:
> 
> page_cb.Run(CreateArticleDistillationUpdate());

Done.

https://chromiumcodereview.appspot.com/178303004/diff/100002/components/dom_d...
components/dom_distiller/core/distiller.cc:235:
(waiting_pages_.find(prev_page_num) != waiting_pages_.end()) ||
On 2014/02/28 02:38:14, cjhopman wrote:
> Can't we just use IsPageNumberInUse?
We can since finished_pages is sorted :), done.

https://chromiumcodereview.appspot.com/178303004/diff/100002/components/dom_d...
components/dom_distiller/core/distiller.cc:257: RunDistillerCallbackIfDone();
On 2014/02/28 02:38:14, cjhopman wrote:
> Do we really want to call both the update and finished when we are done?

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.
}

https://chromiumcodereview.appspot.com/178303004/diff/100002/components/dom_d...
File components/dom_distiller/core/distiller.h (right):

https://chromiumcodereview.appspot.com/178303004/diff/100002/components/dom_d...
components/dom_distiller/core/distiller.h:32: PageDistillationCallback;
On 2014/02/28 02:38:14, cjhopman wrote:
> How about:
> 
> DistillationFinishedCallback
> and
> DistillationUpdateCallback
> 
> i.e. don't tie the incremental updates to the concept of pages

Done.

https://chromiumcodereview.appspot.com/178303004/diff/100002/components/dom_d...
components/dom_distiller/core/distiller.h:32: PageDistillationCallback;
On 2014/02/28 02:38:14, cjhopman wrote:
> How about:
> 
> DistillationFinishedCallback
> and
> DistillationUpdateCallback
> 
> i.e. don't tie the incremental updates to the concept of pages

Done.

https://chromiumcodereview.appspot.com/178303004/diff/100002/components/dom_d...
components/dom_distiller/core/distiller.h:36: // Distills a page, and
asynchrounously returns the article HTML to the
On 2014/02/28 02:38:14, cjhopman wrote:
> s/asynchrounously/asynchronously

Done.

https://chromiumcodereview.appspot.com/178303004/diff/100002/components/dom_d...
components/dom_distiller/core/distiller.h:37: // supplied callback.
On 2014/02/28 02:38:14, cjhopman wrote:
> Update this comment to reflect these changes.

Done.

https://chromiumcodereview.appspot.com/178303004/diff/100002/components/dom_d...
File components/dom_distiller/core/distiller_unittest.cc (right):

https://chromiumcodereview.appspot.com/178303004/diff/100002/components/dom_d...
components/dom_distiller/core/distiller_unittest.cc:201:
ACTION_P5(VerifyIncrementalUpdatesMatch,
On 2014/02/28 02:38:14, cjhopman wrote:
> Why is this an action? It isn't ever used as an action.

Changed to function.

https://chromiumcodereview.appspot.com/178303004/diff/100002/components/dom_d...
components/dom_distiller/core/distiller_unittest.cc:203: kNumPages,
On 2014/02/28 02:38:14, cjhopman wrote:
> nit: the capitalization of this is wrong

Done.

https://chromiumcodereview.appspot.com/178303004/diff/100002/components/dom_d...
components/dom_distiller/core/distiller_unittest.cc:207: vector<int> page_nums =
get_pages_in_sequence(start_page_num, kNumPages);
On 2014/02/28 02:38:14, cjhopman wrote:
> This function contains the following names:
> start_page_num
> page_num
> page_nums
> num_pages
> kNumPages
> update_pages
> actual_page_num
> 
> This makes it really hard to follow. Please rename some things and maybe use
> more descriptive names.
> 
> 

Done.

https://chromiumcodereview.appspot.com/178303004/diff/100002/components/dom_d...
components/dom_distiller/core/distiller_unittest.cc:215: for (int page_num = 0;
page_num < incremental_updates.size(); ++page_num) {
On 2014/02/28 02:38:14, cjhopman wrote:
> This seems like the index of an incremental update, why is it called page_num?

Done.

https://chromiumcodereview.appspot.com/178303004/diff/100002/components/dom_d...
components/dom_distiller/core/distiller_unittest.cc:215: for (int page_num = 0;
page_num < incremental_updates.size(); ++page_num) {
On 2014/02/28 02:38:14, cjhopman wrote:
> incremental_updates is a vector, why not use an iterator? If not that, than at
> least store a reference into the vector here so you don't keep repeating the
> lookup.

Done.

https://chromiumcodereview.appspot.com/178303004/diff/100002/components/dom_d...
components/dom_distiller/core/distiller_unittest.cc:215: for (int page_num = 0;
page_num < incremental_updates.size(); ++page_num) {
On 2014/02/28 02:38:14, cjhopman wrote:
> This seems like the index of an incremental update, why is it called page_num?

Done.

https://chromiumcodereview.appspot.com/178303004/diff/100002/components/dom_d...
components/dom_distiller/core/distiller_unittest.cc:218: vector<int>
update_pages(page_nums.begin(), page_nums.begin() + num_pages);
On 2014/02/28 02:38:14, cjhopman wrote:
> I'd rename this: expected_pages_in_update or some such

Done.

https://chromiumcodereview.appspot.com/178303004/diff/100002/components/dom_d...
components/dom_distiller/core/distiller_unittest.cc:352: TEST_F(DistillerTest,
CheckMaxPageLimit) {
On 2014/02/28 02:38:14, cjhopman wrote:
> Just noticed: This should probably be three separate tests

Good catch, just realized that I messed up the rebase in
https://codereview.chromium.org/167963003, so an identical patch got included
here. #goto fail; #IfeelBad.
Refactored to two functions.

https://chromiumcodereview.appspot.com/178303004/diff/100002/components/dom_d...
components/dom_distiller/core/distiller_unittest.cc:612: }
On 2014/02/28 02:38:14, cjhopman wrote:
> This setup code seems to be repeated three times. Should probably just extract
> that out.

Good point, done, though it might make it slightly hard to review: I have
extracted it to CreateMultipageDistillerDataWithoutImages;

https://chromiumcodereview.appspot.com/178303004/diff/100002/components/dom_d...
File components/dom_distiller/core/task_tracker.h (right):

https://chromiumcodereview.appspot.com/178303004/diff/100002/components/dom_d...
components/dom_distiller/core/task_tracker.h:51: virtual void
OnArticleUpdated(ArticleDistillationUpdate article_update) = 0;
On 2014/02/28 02:38:15, cjhopman wrote:
> this should be taking a const& to the ArticleDistillationUpdate

We are copying updates since they contain pages which are refcounted pointers.

[nyquist, 2014-03-03 23:00:11]

https://codereview.chromium.org/178303004/diff/200016/components/dom_distille...
File components/dom_distiller/core/article_distillation_update.h (right):

https://codereview.chromium.org/178303004/diff/200016/components/dom_distille...
components/dom_distiller/core/article_distillation_update.h:29: size_t
GetPagesSize() const { return pages_.size(); }
I've mostly seen calls to size() of a local variable to also have hacker case,
so I'd say get_pages_size(), but both exist in our codebase.

https://codereview.chromium.org/178303004/diff/200016/components/dom_distille...
components/dom_distiller/core/article_distillation_update.h:31: bool
HasNextPage() const { return has_next_page_; }
I think this should be has_next_page, and similarly below.

https://codereview.chromium.org/178303004/diff/200016/components/dom_distille...
components/dom_distiller/core/article_distillation_update.h:36: // True, if
article has a next page.
Whether article has a next page. Same below.
Also, could you specify that this means that the article has pages that are not
part of the current set of pages? As it stands right now, it is a little bit
confusing as to what these two booleans really represent.

https://codereview.chromium.org/178303004/diff/200016/components/dom_distille...
File components/dom_distiller/core/distiller.h (right):

https://codereview.chromium.org/178303004/diff/200016/components/dom_distille...
components/dom_distiller/core/distiller.h:148: const ArticleDistillationUpdate
CreateDistillationUpdate() const;
What does this do? Even though it's private a short explanation would be nice.

https://codereview.chromium.org/178303004/diff/200016/components/dom_distille...
File components/dom_distiller/core/distiller_unittest.cc (right):

https://codereview.chromium.org/178303004/diff/200016/components/dom_distille...
components/dom_distiller/core/distiller_unittest.cc:65: vector<int>
get_pages_in_sequence(int start_page_num, int num_pages) {
GetPagesInSequence

https://codereview.chromium.org/178303004/diff/200016/components/dom_distille...
components/dom_distiller/core/distiller_unittest.cc:98: // Updates should
contain a list of pages. Pages in a update should be in
'an update'

https://codereview.chromium.org/178303004/diff/200016/components/dom_distille...
components/dom_distiller/core/distiller_unittest.cc:99: // the correct ascending
page order regardless of start_page_num.
|start_page_num|

[cjhopman, 2014-03-04 02:29:04]

https://codereview.chromium.org/178303004/diff/200016/components/dom_distille...
File components/dom_distiller/core/article_distillation_update.h (right):

https://codereview.chromium.org/178303004/diff/200016/components/dom_distille...
components/dom_distiller/core/article_distillation_update.h:19:
RefPtrToDistilledPageProto;
How would you feel about just doing:

typedef base::RefCountedData<DistilledPageProto> RefCountedPageProto;

or RefCountedDistilledPage;

https://codereview.chromium.org/178303004/diff/200016/components/dom_distille...
components/dom_distiller/core/article_distillation_update.h:29: size_t
GetPagesSize() const { return pages_.size(); }
On 2014/03/03 23:00:12, nyquist wrote:
> I've mostly seen calls to size() of a local variable to also have hacker case,
> so I'd say get_pages_size(), but both exist in our codebase.

Yeah, I was tempted to suggest that but it seemed to violate the style guide. I
would be happy with either one as long as these three are all the same. I think
I suggested this case because it matches GetDistilledPage() above.

https://codereview.chromium.org/178303004/diff/200016/components/dom_distille...
components/dom_distiller/core/article_distillation_update.h:31: bool
HasNextPage() const { return has_next_page_; }
On 2014/03/03 23:00:12, nyquist wrote:
> I think this should be has_next_page, and similarly below.

According to the style guide, this is optional.

[shashi, 2014-03-04 19:47:22]

PTAL

https://chromiumcodereview.appspot.com/178303004/diff/200016/components/dom_d...
File components/dom_distiller/core/article_distillation_update.h (right):

https://chromiumcodereview.appspot.com/178303004/diff/200016/components/dom_d...
components/dom_distiller/core/article_distillation_update.h:19:
RefPtrToDistilledPageProto;
On 2014/03/04 02:29:05, cjhopman wrote:
> How would you feel about just doing:
> 
> typedef base::RefCountedData<DistilledPageProto> RefCountedPageProto;
> 
> or RefCountedDistilledPage;

Done.

https://chromiumcodereview.appspot.com/178303004/diff/200016/components/dom_d...
components/dom_distiller/core/article_distillation_update.h:36: // True, if
article has a next page.
Clarified and moved the comments to the public interface of the class.
On 2014/03/03 23:00:12, nyquist wrote:
> Whether article has a next page. Same below.
> Also, could you specify that this means that the article has pages that are
not
> part of the current set of pages? As it stands right now, it is a little bit
> confusing as to what these two booleans really represent.

https://chromiumcodereview.appspot.com/178303004/diff/200016/components/dom_d...
File components/dom_distiller/core/distiller_unittest.cc (right):

https://chromiumcodereview.appspot.com/178303004/diff/200016/components/dom_d...
components/dom_distiller/core/distiller_unittest.cc:98: // Updates should
contain a list of pages. Pages in a update should be in
On 2014/03/03 23:00:12, nyquist wrote:
> 'an update'

Done.

https://chromiumcodereview.appspot.com/178303004/diff/200016/components/dom_d...
components/dom_distiller/core/distiller_unittest.cc:99: // the correct ascending
page order regardless of start_page_num.
On 2014/03/03 23:00:12, nyquist wrote:
> |start_page_num|

Done.

[cjhopman, 2014-03-05 02:55:34]

https://codereview.chromium.org/178303004/diff/230001/components/dom_distille...
File components/dom_distiller/core/article_distillation_update.h (right):

https://codereview.chromium.org/178303004/diff/230001/components/dom_distille...
components/dom_distiller/core/article_distillation_update.h:27: const
scoped_refptr<RefCountedPageProto> GetDistilledPage(size_t index) const;
The page proto here can be modified by the caller. This should return a "const
DistilledPageProto&".

https://codereview.chromium.org/178303004/diff/230001/components/dom_distille...
File components/dom_distiller/core/distiller.h (right):

https://codereview.chromium.org/178303004/diff/230001/components/dom_distille...
components/dom_distiller/core/distiller.h:100:
scoped_refptr<base::RefCountedData<DistilledPageProto> > proto;
This could use the RefCountedPageProto. It's kind of awkward to use the typedef
from the other class that way so idk if you should do that. This is fine as is,
if you want to keep it.

https://codereview.chromium.org/178303004/diff/230001/components/dom_distille...
components/dom_distiller/core/distiller.h:100:
scoped_refptr<base::RefCountedData<DistilledPageProto> > proto;
proto isn't very descriptive. How about `distilled_page` or
`distilled_page_proto`?

https://codereview.chromium.org/178303004/diff/230001/components/dom_distille...
components/dom_distiller/core/distiller.h:148: // Prepare the distillation
update for the article currently under
How about: Create an ArticleDistillationUpdate for the current distillation
state?

https://codereview.chromium.org/178303004/diff/230001/components/dom_distille...
File components/dom_distiller/core/distiller_unittest.cc (right):

https://codereview.chromium.org/178303004/diff/230001/components/dom_distille...
components/dom_distiller/core/distiller_unittest.cc:33: const char kTitle[] =
"Title";
I believe that there should be no indent for a namespace. (this confused me
below when I saw a function that I thought should be free but was indented like
it was part of a class)

[shashi, 2014-03-05 03:45:38]

PTAL

https://codereview.chromium.org/178303004/diff/200016/components/dom_distille...
File components/dom_distiller/core/distiller.h (right):

https://codereview.chromium.org/178303004/diff/200016/components/dom_distille...
components/dom_distiller/core/distiller.h:148: const ArticleDistillationUpdate
CreateDistillationUpdate() const;
On 2014/03/03 23:00:12, nyquist wrote:
> What does this do? Even though it's private a short explanation would be nice.

Done.

https://codereview.chromium.org/178303004/diff/230001/components/dom_distille...
File components/dom_distiller/core/article_distillation_update.h (right):

https://codereview.chromium.org/178303004/diff/230001/components/dom_distille...
components/dom_distiller/core/article_distillation_update.h:27: const
scoped_refptr<RefCountedPageProto> GetDistilledPage(size_t index) const;
On 2014/03/05 02:55:34, cjhopman wrote:
> The page proto here can be modified by the caller. This should return a "const
> DistilledPageProto&".
Done.

https://codereview.chromium.org/178303004/diff/230001/components/dom_distille...
File components/dom_distiller/core/distiller.h (right):

https://codereview.chromium.org/178303004/diff/230001/components/dom_distille...
components/dom_distiller/core/distiller.h:100:
scoped_refptr<base::RefCountedData<DistilledPageProto> > proto;
On 2014/03/05 02:55:34, cjhopman wrote:
> proto isn't very descriptive. How about `distilled_page` or
> `distilled_page_proto`?

Done.

https://codereview.chromium.org/178303004/diff/230001/components/dom_distille...
components/dom_distiller/core/distiller.h:148: // Prepare the distillation
update for the article currently under
On 2014/03/05 02:55:34, cjhopman wrote:
> How about: Create an ArticleDistillationUpdate for the current distillation
> state?

Done.

https://codereview.chromium.org/178303004/diff/230001/components/dom_distille...
File components/dom_distiller/core/distiller_unittest.cc (right):

https://codereview.chromium.org/178303004/diff/230001/components/dom_distille...
components/dom_distiller/core/distiller_unittest.cc:33: const char kTitle[] =
"Title";
On 2014/03/05 02:55:34, cjhopman wrote:
> I believe that there should be no indent for a namespace. (this confused me
> below when I saw a function that I thought should be free but was indented
like
> it was part of a class)

Done.

[shashi, 2014-03-06 21:27:38]

Gentle reminder.

[cjhopman, 2014-03-06 23:05:33]

lgtm

[shashi, 2014-03-06 23:16:29]

The CQ bit was checked by shashishekhar@chromium.org

[commit-bot: I haz the power, 2014-03-06 23:22:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shashishekhar@chromium.org/178303004/2...

[commit-bot: I haz the power, 2014-03-07 01:52:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-07 01:52:46]

Try jobs failed on following builders: linux_chromium_chromeos_clang_dbg

[shashi, 2014-03-07 16:02:12]

The CQ bit was checked by shashishekhar@chromium.org

[commit-bot: I haz the power, 2014-03-07 16:03:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shashishekhar@chromium.org/178303004/2...

[commit-bot: I haz the power, 2014-03-07 16:06:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-07 16:06:02]

Try jobs failed on following builders: mac_chromium_rel

[shashi, 2014-03-07 16:15:17]

The CQ bit was checked by shashishekhar@chromium.org

[commit-bot: I haz the power, 2014-03-07 16:17:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shashishekhar@chromium.org/178303004/2...

[commit-bot: I haz the power, 2014-03-07 16:57:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-07 16:57:46]

Retried try job too often on win for step(s) compile
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[shashi, 2014-03-07 19:20:32]

The CQ bit was checked by shashishekhar@chromium.org

[commit-bot: I haz the power, 2014-03-07 19:22:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shashishekhar@chromium.org/178303004/2...

[commit-bot: I haz the power, 2014-03-07 20:26:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shashishekhar@chromium.org/178303004/2...

[shashi, 2014-03-07 21:02:43]

The CQ bit was unchecked by shashishekhar@chromium.org

[shashi, 2014-03-07 21:03:00]

The CQ bit was checked by shashishekhar@chromium.org

[shashi, 2014-03-07 21:19:40]

The CQ bit was unchecked by shashishekhar@chromium.org

[shashi, 2014-03-07 21:19:50]

The CQ bit was checked by shashishekhar@chromium.org

[shashi, 2014-03-07 22:10:34]

The CQ bit was unchecked by shashishekhar@chromium.org

[shashi, 2014-03-07 22:10:41]

The CQ bit was checked by shashishekhar@chromium.org

[shashi, 2014-03-07 22:18:13]

Committed patchset #7 manually as r255708 (presubmit successful).