Add support for multipage distillation.

Add support for multipage distillation.

Add support for distilling multiple pages, currently support is really
basic and just distills pages sequentially.
BUG=288015
TEST=included
NOTRY=true
R=cjhopman@chromium.org

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

[shashi, 2014-01-29 05:20:42]

Initial implementation, viewer related changes will be landed in a separate
patch.

[Yaron, 2014-01-29 20:03:41]

https://chromiumcodereview.appspot.com/146843010/diff/2/components/dom_distil...
File components/dom_distiller/content/distiller_page_web_contents.cc (right):

https://chromiumcodereview.appspot.com/146843010/diff/2/components/dom_distil...
components/dom_distiller/content/distiller_page_web_contents.cc:65: if
(validated_url != web_contents_->GetURL()) {
Why are these necessary? To ensure the page wasn't redirected?

https://chromiumcodereview.appspot.com/146843010/diff/2/components/dom_distil...
File components/dom_distiller/core/distiller.cc (right):

https://chromiumcodereview.appspot.com/146843010/diff/2/components/dom_distil...
components/dom_distiller/core/distiller.cc:92: DistilledPageProto* page_proto =
article_proto_->add_pages();
Why do you add the page before distillation occurs? If a page fails to laod then
you'd have a hanging entry. Is that so we can try and distill at view time?

https://chromiumcodereview.appspot.com/146843010/diff/2/components/dom_distil...
components/dom_distiller/core/distiller.cc:126: DistilledPageProto* current_page
= GetLastPage();
While we aren't planning to do in-parallel distillations, I don't like this
approach of making an assumptions across tasks/callbacks. Unless you have reason
to add the page first (per comment above), I think it's better to do it here.

https://chromiumcodereview.appspot.com/146843010/diff/2/components/dom_distil...
components/dom_distiller/core/distiller.cc:200: DistillationTaskComplete();
Naming feels wrong because of this case. I had to read the function to realize
why this wasn't a bug.

https://chromiumcodereview.appspot.com/146843010/diff/2/third_party/readabili...
File third_party/readability/js/readability.js (right):

https://chromiumcodereview.appspot.com/146843010/diff/2/third_party/readabili...
third_party/readability/js/readability.js:159: // TODO(bengr): Remove this
assignment of null to nextPageLink when
Please remove this to-do and subsequent null-ing out

[cjhopman, 2014-01-29 21:44:12]

https://chromiumcodereview.appspot.com/146843010/diff/50001/components/dom_di...
File components/dom_distiller/content/distiller_page_web_contents.cc (right):

https://chromiumcodereview.appspot.com/146843010/diff/50001/components/dom_di...
components/dom_distiller/content/distiller_page_web_contents.cc:65: if
(validated_url != web_contents_->GetURL()) {
What is this for?

https://chromiumcodereview.appspot.com/146843010/diff/50001/components/dom_di...
File components/dom_distiller/core/distiller.h (right):

https://chromiumcodereview.appspot.com/146843010/diff/50001/components/dom_di...
components/dom_distiller/core/distiller.h:58: // Distills a article from a page
and associated pages.
This class is starting to handle a lot of things:
loading a page and injecting javascript to distill it
creating and tracking image fetchers
managing the process of distilling multiple pages

I think we should split this up into two classes:
1. handles loading a URL with a DistillerPage and executing javascript on that
page to extract content/title/image_urls
and
2. manages multi-page + creation/tracking of image fetchers

wdyt?

https://chromiumcodereview.appspot.com/146843010/diff/50001/third_party/reada...
File third_party/readability/js/readability.js (right):

https://chromiumcodereview.appspot.com/146843010/diff/50001/third_party/reada...
third_party/readability/js/readability.js:1746: result[0] =
readability.getArticleTitle();
We should probably change this to a dictionary at some point.

[shashi, 2014-01-29 22:51:37]

https://chromiumcodereview.appspot.com/146843010/diff/2/components/dom_distil...
File components/dom_distiller/content/distiller_page_web_contents.cc (right):

https://chromiumcodereview.appspot.com/146843010/diff/2/components/dom_distil...
components/dom_distiller/content/distiller_page_web_contents.cc:65: if
(validated_url != web_contents_->GetURL()) {
On 2014/01/29 20:03:41, Yaron wrote:
> Why are these necessary? To ensure the page wasn't redirected?
Done, I can use is_main_frame.

https://chromiumcodereview.appspot.com/146843010/diff/2/components/dom_distil...
File components/dom_distiller/core/distiller.cc (right):

https://chromiumcodereview.appspot.com/146843010/diff/2/components/dom_distil...
components/dom_distiller/core/distiller.cc:92: DistilledPageProto* page_proto =
article_proto_->add_pages();
On 2014/01/29 20:03:41, Yaron wrote:
> Why do you add the page before distillation occurs? If a page fails to laod
then
> you'd have a hanging entry. Is that so we can try and distill at view time?

Done. No, it was just inline with previous code that used it to pass the URL of
the current page, changed the OnExecuteJavaScriptDone to take a url.

https://chromiumcodereview.appspot.com/146843010/diff/2/components/dom_distil...
components/dom_distiller/core/distiller.cc:126: DistilledPageProto* current_page
= GetLastPage();
Parallel distillation will require creating multiple distiller pages and some
more work, but I have addressed the style issue here.
On 2014/01/29 20:03:41, Yaron wrote:
> While we aren't planning to do in-parallel distillations, I don't like this
> approach of making an assumptions across tasks/callbacks. Unless you have
reason
> to add the page first (per comment above), I think it's better to do it here.

https://chromiumcodereview.appspot.com/146843010/diff/2/components/dom_distil...
components/dom_distiller/core/distiller.cc:200: DistillationTaskComplete();
Changed, hopefully better.
On 2014/01/29 20:03:41, Yaron wrote:
> Naming feels wrong because of this case. I had to read the function to realize
> why this wasn't a bug.

https://chromiumcodereview.appspot.com/146843010/diff/2/third_party/readabili...
File third_party/readability/js/readability.js (right):

https://chromiumcodereview.appspot.com/146843010/diff/2/third_party/readabili...
third_party/readability/js/readability.js:159: // TODO(bengr): Remove this
assignment of null to nextPageLink when
On 2014/01/29 20:03:41, Yaron wrote:
> Please remove this to-do and subsequent null-ing out

Done.

https://chromiumcodereview.appspot.com/146843010/diff/50001/components/dom_di...
File components/dom_distiller/content/distiller_page_web_contents.cc (right):

https://chromiumcodereview.appspot.com/146843010/diff/50001/components/dom_di...
components/dom_distiller/content/distiller_page_web_contents.cc:65: if
(validated_url != web_contents_->GetURL()) {
On 2014/01/29 21:44:13, cjhopman wrote:
> What is this for?
What I need is is_main_frame, somehow when I tested before my debug build was
crashing due to an unknown error.

https://chromiumcodereview.appspot.com/146843010/diff/50001/components/dom_di...
File components/dom_distiller/core/distiller.h (right):

https://chromiumcodereview.appspot.com/146843010/diff/50001/components/dom_di...
components/dom_distiller/core/distiller.h:58: // Distills a article from a page
and associated pages.
Makes sense, but I will prefer to do it in a separate CL, this CL is already a
bit big, plus I will be able to unit_tests for the new classes.
On 2014/01/29 21:44:13, cjhopman wrote:
> This class is starting to handle a lot of things:
> loading a page and injecting javascript to distill it
> creating and tracking image fetchers
> managing the process of distilling multiple pages
> 
> I think we should split this up into two classes:
> 1. handles loading a URL with a DistillerPage and executing javascript on that
> page to extract content/title/image_urls
> and
> 2. manages multi-page + creation/tracking of image fetchers
> 
> wdyt?

https://chromiumcodereview.appspot.com/146843010/diff/50001/third_party/reada...
File third_party/readability/js/readability.js (right):

https://chromiumcodereview.appspot.com/146843010/diff/50001/third_party/reada...
third_party/readability/js/readability.js:1746: result[0] =
readability.getArticleTitle();
On 2014/01/29 21:44:13, cjhopman wrote:
> We should probably change this to a dictionary at some point.
I actually tried to change it to dictionary ,but most WebUI hooks seem to use
arrays, I am not sure the exact reason. I definitely agree this should be a
dictionary.

[shashi, 2014-01-29 23:21:09]

Note: on Chris' suggestion, I am refactoring the distiller into two classes:
PageDistiller and Distiller(which is article distiller), so hold off on review
for the distiller.

[shashi, 2014-01-30 01:39:25]

Done the refactoring, all tests pass, ready for review, will land it next week.

[shashi, 2014-02-03 20:58:47]

Gentle reminder, PTAL.

[cjhopman, 2014-02-03 21:47:21]

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

https://chromiumcodereview.appspot.com/146843010/diff/200001/components/dom_d...
components/dom_distiller/core/distiller.cc:23: // Maximum number of distilled
pages in a article.
s/ a / an /

https://chromiumcodereview.appspot.com/146843010/diff/200001/components/dom_d...
components/dom_distiller/core/distiller.cc:74:
base::MessageLoop::current()->PostTask(
Do we need to post a task here? Can't we just call DistillNextPage directly?

https://chromiumcodereview.appspot.com/146843010/diff/200001/components/dom_d...
components/dom_distiller/core/distiller.cc:154: delete fetcher;
It looks like there is a lot going on in this class that makes it not
cancellable and not safe to be destroyed while it is doing work (I don't think
you are adding anything like that, though). This will need to be fixed, maybe we
should file a bug to make the distiller cancellable.

https://chromiumcodereview.appspot.com/146843010/diff/200001/components/dom_d...
components/dom_distiller/core/distiller.cc:159: void
DistillerImpl::CheckIfAllCallbacksAreFinished() {
I don't like this function name. I would expect this to return a bool of whether
or not all callbacks are finished.

Maybe change to something like RunDistillerCallbackIfFinished or
MaybeRunDistillerCallback or some such.

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

https://chromiumcodereview.appspot.com/146843010/diff/200001/components/dom_d...
components/dom_distiller/core/distiller.h:58: public
PageDistiller::PageDistillerCallback {
This should not subclass a Callback.

https://chromiumcodereview.appspot.com/146843010/diff/200001/components/dom_d...
components/dom_distiller/core/distiller.h:76: //
PageDistiller::PageDistillerCallback implementation.
This comment isn't really right. PageDistillerCallback isn't really an
interface-y thing so there is no "implementation" of it. This is the function
passed as the PageDistillerCallback (I don't think it needs any comment, the
name is enough, unless you want to comment on what it does/how it works).

https://chromiumcodereview.appspot.com/146843010/diff/200001/components/dom_d...
components/dom_distiller/core/distiller.h:88: void DistillNextPage(const GURL&
url);
You should be thinking about how to handle the case where the url that we are
given to distill is not the first page of the article.

https://chromiumcodereview.appspot.com/146843010/diff/200001/components/dom_d...
components/dom_distiller/core/distiller.h:105: std::set<std::string>
processed_urls_;
I'd say that this should probably be a hash_set (base/containers/hash_tables.h).
image_fetchers should probably be a hash_map, too.

(then again, with a max size of 32, it doesn't really matter and a simple list
and linear search would probably actually be faster).

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

https://chromiumcodereview.appspot.com/146843010/diff/200001/components/dom_d...
components/dom_distiller/core/distiller_unittest.cc:173: vector<int>
image_indices;
Could we have a test for distilling a page with no images? I.e. have DistillPage
with no images and then DistillPageWithImages.

https://chromiumcodereview.appspot.com/146843010/diff/200001/components/dom_d...
components/dom_distiller/core/distiller_unittest.cc:258: vector<int>
image_indices;
Don't have images here if they aren't needed for this test.

https://chromiumcodereview.appspot.com/146843010/diff/200001/components/dom_d...
File components/dom_distiller/core/dom_distiller_service_unittest.cc (right):

https://chromiumcodereview.appspot.com/146843010/diff/200001/components/dom_d...
components/dom_distiller/core/dom_distiller_service_unittest.cc:389:
EXPECT_TRUE(store_->GetEntryByUrl(pages_url[0], &entry));
Do we really care about the state of the store before the distiller callback is
run? shouldn't that behavior be verified in some other test?

https://chromiumcodereview.appspot.com/146843010/diff/200001/components/dom_d...
File components/dom_distiller/core/page_distiller.h (right):

https://chromiumcodereview.appspot.com/146843010/diff/200001/components/dom_d...
components/dom_distiller/core/page_distiller.h:33: // Distills a single page and
and what?

[shashi, 2014-02-03 23:19:28]

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

https://chromiumcodereview.appspot.com/146843010/diff/200001/components/dom_d...
components/dom_distiller/core/distiller.cc:23: // Maximum number of distilled
pages in a article.
On 2014/02/03 21:47:22, cjhopman wrote:
> s/ a / an /

Done.

https://chromiumcodereview.appspot.com/146843010/diff/200001/components/dom_d...
components/dom_distiller/core/distiller.cc:74:
base::MessageLoop::current()->PostTask(
On 2014/02/03 21:47:22, cjhopman wrote:
> Do we need to post a task here? Can't we just call DistillNextPage directly?
I was afraid that it may recurse, because OnPageDistillationFinish also calls
this method, but now looking at the implementation, I see that it will not
happen.

https://chromiumcodereview.appspot.com/146843010/diff/200001/components/dom_d...
components/dom_distiller/core/distiller.cc:154: delete fetcher;
Done, filed:  http://crbug.com/340431
On 2014/02/03 21:47:22, cjhopman wrote:
> It looks like there is a lot going on in this class that makes it not
> cancellable and not safe to be destroyed while it is doing work (I don't think
> you are adding anything like that, though). This will need to be fixed, maybe
we
> should file a bug to make the distiller cancellable.

https://chromiumcodereview.appspot.com/146843010/diff/200001/components/dom_d...
components/dom_distiller/core/distiller.cc:159: void
DistillerImpl::CheckIfAllCallbacksAreFinished() {
On 2014/02/03 21:47:22, cjhopman wrote:
> I don't like this function name. I would expect this to return a bool of
whether
> or not all callbacks are finished.
> 
> Maybe change to something like RunDistillerCallbackIfFinished or
> MaybeRunDistillerCallback or some such.

Done.

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

https://chromiumcodereview.appspot.com/146843010/diff/200001/components/dom_d...
components/dom_distiller/core/distiller.h:58: public
PageDistiller::PageDistillerCallback {
On 2014/02/03 21:47:22, cjhopman wrote:
> This should not subclass a Callback.

Done.

https://chromiumcodereview.appspot.com/146843010/diff/200001/components/dom_d...
components/dom_distiller/core/distiller.h:76: //
PageDistiller::PageDistillerCallback implementation.
On 2014/02/03 21:47:22, cjhopman wrote:
> This comment isn't really right. PageDistillerCallback isn't really an
> interface-y thing so there is no "implementation" of it. This is the function
> passed as the PageDistillerCallback (I don't think it needs any comment, the
> name is enough, unless you want to comment on what it does/how it works).

Done.

https://chromiumcodereview.appspot.com/146843010/diff/200001/components/dom_d...
components/dom_distiller/core/distiller.h:88: void DistillNextPage(const GURL&
url);
On 2014/02/03 21:47:22, cjhopman wrote:
> You should be thinking about how to handle the case where the url that we are
> given to distill is not the first page of the article.

Good point, currently there is only a next page heuristic, I think what we are
talking about a previous page heuristic. I will have to first add heuristic for
the previous link from the current page. Currently readability does not detect
previous pages. I think depending on heurisitic and the behavior we desire, we
can use page number: negative page numbers and positive page numbers:
map[page_num] = DistilledPageInfo_instance. Once everything is done, we can
create the article and attach pages in sequence.

https://chromiumcodereview.appspot.com/146843010/diff/200001/components/dom_d...
components/dom_distiller/core/distiller.h:105: std::set<std::string>
processed_urls_;
On 2014/02/03 21:47:22, cjhopman wrote:
> I'd say that this should probably be a hash_set
(base/containers/hash_tables.h).
> image_fetchers should probably be a hash_map, too.
> 
> (then again, with a max size of 32, it doesn't really matter and a simple list
> and linear search would probably actually be faster).

Done changed to use vector and std::find.

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

https://chromiumcodereview.appspot.com/146843010/diff/200001/components/dom_d...
components/dom_distiller/core/distiller_unittest.cc:173: vector<int>
image_indices;
On 2014/02/03 21:47:22, cjhopman wrote:
> Could we have a test for distilling a page with no images? I.e. have
DistillPage
> with no images and then DistillPageWithImages.

Done.

https://chromiumcodereview.appspot.com/146843010/diff/200001/components/dom_d...
components/dom_distiller/core/distiller_unittest.cc:258: vector<int>
image_indices;
On 2014/02/03 21:47:22, cjhopman wrote:
> Don't have images here if they aren't needed for this test.

Done.

https://chromiumcodereview.appspot.com/146843010/diff/200001/components/dom_d...
File components/dom_distiller/core/dom_distiller_service_unittest.cc (right):

https://chromiumcodereview.appspot.com/146843010/diff/200001/components/dom_d...
components/dom_distiller/core/dom_distiller_service_unittest.cc:389:
EXPECT_TRUE(store_->GetEntryByUrl(pages_url[0], &entry));
On 2014/02/03 21:47:22, cjhopman wrote:
> Do we really care about the state of the store before the distiller callback
is
> run? shouldn't that behavior be verified in some other test?

It was to test that initial entry was added for the first page of the article,
removed since it is tested elsewhere.

https://chromiumcodereview.appspot.com/146843010/diff/200001/components/dom_d...
File components/dom_distiller/core/page_distiller.h (right):

https://chromiumcodereview.appspot.com/146843010/diff/200001/components/dom_d...
components/dom_distiller/core/page_distiller.h:33: // Distills a single page and
On 2014/02/03 21:47:22, cjhopman wrote:
> and what?
Done, typo.

[cjhopman, 2014-02-03 23:56:52]

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

https://chromiumcodereview.appspot.com/146843010/diff/200001/components/dom_d...
components/dom_distiller/core/distiller.cc:74:
base::MessageLoop::current()->PostTask(
On 2014/02/03 23:19:29, shashi wrote:
> On 2014/02/03 21:47:22, cjhopman wrote:
> > Do we need to post a task here? Can't we just call DistillNextPage directly?
> I was afraid that it may recurse, because OnPageDistillationFinish also calls
> this method, but now looking at the implementation, I see that it will not
> happen.
Ah, I see now. 

Now I think that we should either be posting the task to
PageDistiller::DistillPage (like you were) or PageDistiller should post back its
PageDistillerCallback. I think I'd prefer the latter (as that would make it
harder to misuse PageDistiller).

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

https://chromiumcodereview.appspot.com/146843010/diff/200001/components/dom_d...
components/dom_distiller/core/distiller.h:105: std::set<std::string>
processed_urls_;
On 2014/02/03 23:19:29, shashi wrote:
> On 2014/02/03 21:47:22, cjhopman wrote:
> > I'd say that this should probably be a hash_set
> (base/containers/hash_tables.h).
> > image_fetchers should probably be a hash_map, too.
> > 
> > (then again, with a max size of 32, it doesn't really matter and a simple
list
> > and linear search would probably actually be faster).
> 
> Done changed to use vector and std::find.
> 
Sorry, my comment was unclear. I really don't think this should use vector+find.
Please use set or, preferably, hash_set.

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

https://chromiumcodereview.appspot.com/146843010/diff/100020/components/dom_d...
components/dom_distiller/core/distiller.h:71: void
OnFetchImageDone(DistilledPageProto* distilled_page_proto,
nit: should be private

https://chromiumcodereview.appspot.com/146843010/diff/100020/components/dom_d...
components/dom_distiller/core/distiller.h:75: void
OnPageDistillationFinished(const GURL& page_url,
nit: should be private

https://chromiumcodereview.appspot.com/146843010/diff/100020/components/dom_d...
components/dom_distiller/core/distiller.h:87: // Checks if all distillation
callbacks are finished and runs the
nit: This reads a little awkward, how about "Runs |distillation_cb_| if all
distillation and image fetches are complete."

https://chromiumcodereview.appspot.com/146843010/diff/100020/third_party/read...
File third_party/readability/js/readability.js (left):

https://chromiumcodereview.appspot.com/146843010/diff/100020/third_party/read...
third_party/readability/js/readability.js:1: 
Since this is modified, it needs to contain a notice that you've modified it. I
believe that just adding the chromium copyright header is the correct approach.

Also, make sure that the README.chromium is updated to reflect the fact that
changes have been made to this file.

[shashi, 2014-02-04 01:39:36]

PTAL

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

https://chromiumcodereview.appspot.com/146843010/diff/200001/components/dom_d...
components/dom_distiller/core/distiller.cc:74:
base::MessageLoop::current()->PostTask(
On 2014/02/03 23:56:53, cjhopman wrote:
> On 2014/02/03 23:19:29, shashi wrote:
> > On 2014/02/03 21:47:22, cjhopman wrote:
> > > Do we need to post a task here? Can't we just call DistillNextPage
directly?
> > I was afraid that it may recurse, because OnPageDistillationFinish also
calls
> > this method, but now looking at the implementation, I see that it will not
> > happen.
> Ah, I see now. 
> 
> Now I think that we should either be posting the task to
> PageDistiller::DistillPage (like you were) or PageDistiller should post back
its
> PageDistillerCallback. I think I'd prefer the latter (as that would make it
> harder to misuse PageDistiller).

Done.

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

https://chromiumcodereview.appspot.com/146843010/diff/200001/components/dom_d...
components/dom_distiller/core/distiller.h:105: std::set<std::string>
processed_urls_;
Ah, changed back to use hash_set.
On 2014/02/03 23:56:53, cjhopman wrote:
> On 2014/02/03 23:19:29, shashi wrote:
> > On 2014/02/03 21:47:22, cjhopman wrote:
> > > I'd say that this should probably be a hash_set
> > (base/containers/hash_tables.h).
> > > image_fetchers should probably be a hash_map, too.
> > > 
> > > (then again, with a max size of 32, it doesn't really matter and a simple
> list
> > > and linear search would probably actually be faster).
> > 
> > Done changed to use vector and std::find.
> > 
> Sorry, my comment was unclear. I really don't think this should use
vector+find.
> Please use set or, preferably, hash_set.

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

https://chromiumcodereview.appspot.com/146843010/diff/100020/components/dom_d...
components/dom_distiller/core/distiller.h:71: void
OnFetchImageDone(DistilledPageProto* distilled_page_proto,
On 2014/02/03 23:56:53, cjhopman wrote:
> nit: should be private

Done.

https://chromiumcodereview.appspot.com/146843010/diff/100020/components/dom_d...
components/dom_distiller/core/distiller.h:75: void
OnPageDistillationFinished(const GURL& page_url,
On 2014/02/03 23:56:53, cjhopman wrote:
> nit: should be private

Done.

https://chromiumcodereview.appspot.com/146843010/diff/100020/components/dom_d...
components/dom_distiller/core/distiller.h:87: // Checks if all distillation
callbacks are finished and runs the
On 2014/02/03 23:56:53, cjhopman wrote:
> nit: This reads a little awkward, how about "Runs |distillation_cb_| if all
> distillation and image fetches are complete."

Done.

https://chromiumcodereview.appspot.com/146843010/diff/100020/third_party/read...
File third_party/readability/js/readability.js (left):

https://chromiumcodereview.appspot.com/146843010/diff/100020/third_party/read...
third_party/readability/js/readability.js:1: 
On 2014/02/03 23:56:53, cjhopman wrote:
> Since this is modified, it needs to contain a notice that you've modified it.
I
> believe that just adding the chromium copyright header is the correct
approach.
> 
> Also, make sure that the README.chromium is updated to reflect the fact that
> changes have been made to this file.

Done.

[cjhopman, 2014-02-04 01:48:26]

lgtm

[shashi, 2014-02-04 02:07:07]

The CQ bit was checked by shashishekhar@chromium.org

[commit-bot: I haz the power, 2014-02-04 02:17:55]

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

[commit-bot: I haz the power, 2014-02-04 05:20:15]

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

[commit-bot: I haz the power, 2014-02-04 05:20:15]

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

[shashi, 2014-02-04 05:21:40]

The CQ bit was checked by shashishekhar@chromium.org

[commit-bot: I haz the power, 2014-02-04 05:21:46]

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

[commit-bot: I haz the power, 2014-02-04 07:56:50]

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

[commit-bot: I haz the power, 2014-02-04 07:56:51]

Retried try job too often on linux_rel for step(s) base_unittests,
browser_tests, interactive_ui_tests, net_unittests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[shashi, 2014-02-04 19:00:04]

The CQ bit was checked by shashishekhar@chromium.org

[commit-bot: I haz the power, 2014-02-04 19:03:17]

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

[commit-bot: I haz the power, 2014-02-04 22:51:02]

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

[commit-bot: I haz the power, 2014-02-04 22:51:02]

Retried try job too often on mac_rel for step(s) base_unittests, browser_tests,
interactive_ui_tests, net_unittests, sync_integration_tests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[shashi, 2014-02-04 22:52:42]

The CQ bit was checked by shashishekhar@chromium.org

[commit-bot: I haz the power, 2014-02-04 22:57:49]

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

[commit-bot: I haz the power, 2014-02-05 01:22:51]

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

[shashi, 2014-02-05 01:33:13]

The CQ bit was unchecked by shashishekhar@chromium.org

[shashi, 2014-02-05 01:33:17]

The CQ bit was checked by shashishekhar@chromium.org

[shashi, 2014-02-05 02:30:09]

The CQ bit was unchecked by shashishekhar@chromium.org

[shashi, 2014-02-05 02:30:23]

The CQ bit was checked by shashishekhar@chromium.org

[shashi, 2014-02-05 05:52:38]

The CQ bit was unchecked by shashishekhar@chromium.org

[shashi, 2014-02-05 05:55:22]

The CQ bit was checked by shashishekhar@chromium.org

[shashi, 2014-02-05 05:57:01]

Committed patchset #9 manually as r248867 (presubmit successful).