[dom_distiller] Add support for incremental viewer.

[dom_distiller] Add support for incremental viewer.

The viewed page can now be rendered all at once, or with a single page
update, followed by additional page content sent via HTML. While more
pages are loading, a progress indicator is present.

The title for a page was added to DistilledPageProto so its accessible
during partial distillation. This was already stored in
DistilledPageData so it's just moved.

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

[Yaron, 2014-04-29 04:05:01]

First look. Need to add some comments and tests

[nyquist, 2014-04-29 23:58:36]

https://codereview.chromium.org/260073009/diff/1/components/dom_distiller/con...
File components/dom_distiller/content/dom_distiller_viewer_source.cc (right):

https://codereview.chromium.org/260073009/diff/1/components/dom_distiller/con...
components/dom_distiller/content/dom_distiller_viewer_source.cc:77: std::string
unsafe_page_html = viewer::GetUnsafeArticleHtml(article_proto);
Add a comment. Maybe something like: "This is a single-page article."

https://codereview.chromium.org/260073009/diff/1/components/dom_distiller/con...
components/dom_distiller/content/dom_distiller_viewer_source.cc:89: for
(;page_count_ < article_proto->pages_size();
Add a comment saying something like: "Got OnArticleReady before all the
incremental updates. Pushing all we got to the renderer. This maybe the case
when the distiller does not provide incremental updates."

https://codereview.chromium.org/260073009/diff/1/components/dom_distiller/con...
components/dom_distiller/content/dom_distiller_viewer_source.cc:155: } else if
(kViewerJsPath == path) {
I think you also should add JS to the mime type handling?

https://codereview.chromium.org/260073009/diff/1/components/dom_distiller/con...
File components/dom_distiller/content/resources/dom_distiller_viewer.html
(right):

https://codereview.chromium.org/260073009/diff/1/components/dom_distiller/con...
components/dom_distiller/content/resources/dom_distiller_viewer.html:13: <script
src="dom_distiller_viewer.js"></script>
Could you use /dom_distiller_viewer.js so we know exactly where the request is
going?
Also, for CSS we inject the filename. How would you feel about doing the same
for JS? And if not, maybe we should at some point change them both so they don't
differ?

https://codereview.chromium.org/260073009/diff/1/components/dom_distiller/con...
components/dom_distiller/content/resources/dom_distiller_viewer.html:24: <div
id="loadingIndicator" style="display:$5">Loading...</div>
Should the string "Loading..." be internationalized?

[Yaron, 2014-05-07 05:04:29]

nyquist: ptal, now with a test
creis: can you take a look at the RenderFrameHost/WebContentsObserver stuff in
components/dom_distiller/content/dom_distiller_viewer_source.cc. In case you
need context (you may have reviewed a previous CL), this is running with
untrusted content in a chrome page of sorts so we were instructed not to use
webui, or set up any of those bindings. This includes a simplified version where
we can send a base html page but have a handle to the page live on so we can
push incremental updates. When the page goes away, we destroy the backing object
( DomDistillerViewerSource::RequestViewerHandle)

https://codereview.chromium.org/260073009/diff/1/components/dom_distiller/con...
File components/dom_distiller/content/dom_distiller_viewer_source.cc (right):

https://codereview.chromium.org/260073009/diff/1/components/dom_distiller/con...
components/dom_distiller/content/dom_distiller_viewer_source.cc:77: std::string
unsafe_page_html = viewer::GetUnsafeArticleHtml(article_proto);
On 2014/04/29 23:58:37, nyquist wrote:
> Add a comment. Maybe something like: "This is a single-page article."

Done.

https://codereview.chromium.org/260073009/diff/1/components/dom_distiller/con...
components/dom_distiller/content/dom_distiller_viewer_source.cc:89: for
(;page_count_ < article_proto->pages_size();
On 2014/04/29 23:58:37, nyquist wrote:
> Add a comment saying something like: "Got OnArticleReady before all the
> incremental updates. Pushing all we got to the renderer. This maybe the case
> when the distiller does not provide incremental updates."

Done-ish.

https://codereview.chromium.org/260073009/diff/1/components/dom_distiller/con...
components/dom_distiller/content/dom_distiller_viewer_source.cc:155: } else if
(kViewerJsPath == path) {
On 2014/04/29 23:58:37, nyquist wrote:
> I think you also should add JS to the mime type handling?

Done.

https://codereview.chromium.org/260073009/diff/1/components/dom_distiller/con...
File components/dom_distiller/content/resources/dom_distiller_viewer.html
(right):

https://codereview.chromium.org/260073009/diff/1/components/dom_distiller/con...
components/dom_distiller/content/resources/dom_distiller_viewer.html:13: <script
src="dom_distiller_viewer.js"></script>
On 2014/04/29 23:58:37, nyquist wrote:
> Could you use /dom_distiller_viewer.js so we know exactly where the request is
> going?
> Also, for CSS we inject the filename. How would you feel about doing the same
> for JS? And if not, maybe we should at some point change them both so they
don't
> differ?

Done.

https://codereview.chromium.org/260073009/diff/1/components/dom_distiller/con...
components/dom_distiller/content/resources/dom_distiller_viewer.html:24: <div
id="loadingIndicator" style="display:$5">Loading...</div>
On 2014/04/29 23:58:37, nyquist wrote:
> Should the string "Loading..." be internationalized?

Done.

https://codereview.chromium.org/260073009/diff/20001/components/dom_distiller...
File components/dom_distiller/core/css/distilledpage.css (right):

https://codereview.chromium.org/260073009/diff/20001/components/dom_distiller...
components/dom_distiller/core/css/distilledpage.css:18: .hidden{display:none}
note to self: should probably namespace these to avoid colliding with styles
from the page

[Charlie Reis, 2014-05-07 20:00:17]

[+avi,jam]

I think it would be good to avoid dealing with RenderFrameHosts directly here. 
It appears you only really need it to execute JavaScript in the main frame, in
which case we can expose that API on WebContents.

Two reasons for going down that path:
1) You won't have to care about whether there are multiple RenderFrameHosts,
possibly in separate processes.
2) We're considering removing RenderFrameHost from the public content API.

[Avi (use Gerrit), 2014-05-07 21:00:25]

On 2014/05/07 20:00:17, Charlie Reis wrote:
> [+avi,jam]
> 
> I think it would be good to avoid dealing with RenderFrameHosts directly here.

> It appears you only really need it to execute JavaScript in the main frame, in
> which case we can expose that API on WebContents.
> 
> Two reasons for going down that path:
> 1) You won't have to care about whether there are multiple RenderFrameHosts,
> possibly in separate processes.
> 2) We're considering removing RenderFrameHost from the public content API.

We already have a million places in the code that do
web_contents->GetMainFrame()->ExecuteJavascript(). Unless we're moving all of
those now, I would rather this CL do it exactly as all the other places do it,
and we'll figure out how to change all the places at once.

[Charlie Reis, 2014-05-07 21:22:33]

On 2014/05/07 21:00:25, Avi wrote:
> On 2014/05/07 20:00:17, Charlie Reis wrote:
> > [+avi,jam]
> > 
> > I think it would be good to avoid dealing with RenderFrameHosts directly
here.
> 
> > It appears you only really need it to execute JavaScript in the main frame,
in
> > which case we can expose that API on WebContents.
> > 
> > Two reasons for going down that path:
> > 1) You won't have to care about whether there are multiple RenderFrameHosts,
> > possibly in separate processes.
> > 2) We're considering removing RenderFrameHost from the public content API.
> 
> We already have a million places in the code that do
> web_contents->GetMainFrame()->ExecuteJavascript(). Unless we're moving all of
> those now, I would rather this CL do it exactly as all the other places do it,
> and we'll figure out how to change all the places at once.

Good point.  Either way, seems like it's safer to track WebContents rather than
RenderFrameHost in this code.

[Yaron, 2014-05-09 17:50:49]

ok, changed to track webcontents

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

Thanks, that seems more reasonable to me.  A few questions and some minor nits
below in the file you mentioned.

https://codereview.chromium.org/260073009/diff/100001/components/dom_distille...
File components/dom_distiller/content/dom_distiller_viewer_source.cc (right):

https://codereview.chromium.org/260073009/diff/100001/components/dom_distille...
components/dom_distiller/content/dom_distiller_viewer_source.cc:28: // the
RenderFrameHost for the Viewer instance.
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?).

https://codereview.chromium.org/260073009/diff/100001/components/dom_distille...
components/dom_distiller/content/dom_distiller_viewer_source.cc:64: // Cancel's
the current view request. Once called, no updates will be
nit: Cancels

https://codereview.chromium.org/260073009/diff/100001/components/dom_distille...
components/dom_distiller/content/dom_distiller_viewer_source.cc:79: // Number of
pages that have been rendered by the viewer.
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?)

https://codereview.chromium.org/260073009/diff/100001/components/dom_distille...
components/dom_distiller/content/dom_distiller_viewer_source.cc:127:
content::RenderViewHost* render_view_host) {
nit: Wrong indent.

https://codereview.chromium.org/260073009/diff/100001/components/dom_distille...
components/dom_distiller/content/dom_distiller_viewer_source.cc:152: if
(buffer_.empty()) return;
nit: Return should be on its own line.

https://codereview.chromium.org/260073009/diff/100001/components/dom_distille...
components/dom_distiller/content/dom_distiller_viewer_source.cc:174:
page_count_++) {
nit: Won't this fit on the previous line?

https://codereview.chromium.org/260073009/diff/100001/components/dom_distille...
components/dom_distiller/content/dom_distiller_viewer_source.cc:197:
viewer::GetUnsafeIncrementalDistilledPageJs(&page, false));
nit: Wrong indent.

https://codereview.chromium.org/260073009/diff/100001/components/dom_distille...
components/dom_distiller/content/dom_distiller_viewer_source.cc:237: } else if
(kViewerJsPath == path) {
nit: No need for an else if the previous block returns.

https://codereview.chromium.org/260073009/diff/100001/components/dom_distille...
components/dom_distiller/content/dom_distiller_viewer_source.cc:243: new
RequestViewerHandle(render_process_id, render_frame_id, callback);
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:16]

All comments addressed. Avi: can you look at
https://codereview.chromium.org/260073009/diff/100001/components/dom_distille...
since Charlie appears to be out this week

https://codereview.chromium.org/260073009/diff/100001/components/dom_distille...
File components/dom_distiller/content/dom_distiller_viewer_source.cc (right):

https://codereview.chromium.org/260073009/diff/100001/components/dom_distille...
components/dom_distiller/content/dom_distiller_viewer_source.cc:28: // the
RenderFrameHost for the Viewer instance.
On 2014/05/09 21:32:46, Charlie Reis (OOO May 17) wrote:
> 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?

It should go away. My intention was to have this basically have 1:1
corresponding with one visible instance of the viewer. 
> 
> 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.

Not necessary.
> 
> 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?).

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?

https://codereview.chromium.org/260073009/diff/100001/components/dom_distille...
components/dom_distiller/content/dom_distiller_viewer_source.cc:64: // Cancel's
the current view request. Once called, no updates will be
On 2014/05/09 21:32:46, Charlie Reis (OOO May 17) wrote:
> nit: Cancels

Done.

https://codereview.chromium.org/260073009/diff/100001/components/dom_distille...
components/dom_distiller/content/dom_distiller_viewer_source.cc:64: // Cancel's
the current view request. Once called, no updates will be
On 2014/05/09 21:32:46, Charlie Reis (OOO May 17) wrote:
> nit: Cancels

Done.

https://codereview.chromium.org/260073009/diff/100001/components/dom_distille...
components/dom_distiller/content/dom_distiller_viewer_source.cc:79: // Number of
pages that have been rendered by the viewer.
On 2014/05/09 21:32:46, Charlie Reis (OOO May 17) wrote:
> 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?)

Conceptual pages of the article. Expanded the comment slightly.

https://codereview.chromium.org/260073009/diff/100001/components/dom_distille...
components/dom_distiller/content/dom_distiller_viewer_source.cc:127:
content::RenderViewHost* render_view_host) {
On 2014/05/09 21:32:46, Charlie Reis (OOO May 17) wrote:
> nit: Wrong indent.

Done.

https://codereview.chromium.org/260073009/diff/100001/components/dom_distille...
components/dom_distiller/content/dom_distiller_viewer_source.cc:152: if
(buffer_.empty()) return;
On 2014/05/09 21:32:46, Charlie Reis (OOO May 17) wrote:
> nit: Return should be on its own line.

Done.

https://codereview.chromium.org/260073009/diff/100001/components/dom_distille...
components/dom_distiller/content/dom_distiller_viewer_source.cc:174:
page_count_++) {
On 2014/05/09 21:32:46, Charlie Reis (OOO May 17) wrote:
> nit: Won't this fit on the previous line?

Done.

https://codereview.chromium.org/260073009/diff/100001/components/dom_distille...
components/dom_distiller/content/dom_distiller_viewer_source.cc:197:
viewer::GetUnsafeIncrementalDistilledPageJs(&page, false));
On 2014/05/09 21:32:46, Charlie Reis (OOO May 17) wrote:
> nit: Wrong indent.

Done.

https://codereview.chromium.org/260073009/diff/100001/components/dom_distille...
components/dom_distiller/content/dom_distiller_viewer_source.cc:237: } else if
(kViewerJsPath == path) {
On 2014/05/09 21:32:46, Charlie Reis (OOO May 17) wrote:
> nit: No need for an else if the previous block returns.

Done.

https://codereview.chromium.org/260073009/diff/100001/components/dom_distille...
components/dom_distiller/content/dom_distiller_viewer_source.cc:243: new
RequestViewerHandle(render_process_id, render_frame_id, callback);
On 2014/05/09 21:32:46, Charlie Reis (OOO May 17) wrote:
> 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?

Done.

[Charlie Reis, 2014-05-12 20:04:00]

I'm still around until the 17th, though my time will indeed be tight this week. 
A few comments below.

https://codereview.chromium.org/260073009/diff/100001/components/dom_distille...
File components/dom_distiller/content/dom_distiller_viewer_source.cc (right):

https://codereview.chromium.org/260073009/diff/100001/components/dom_distille...
components/dom_distiller/content/dom_distiller_viewer_source.cc:28: // the
RenderFrameHost for the Viewer instance.
On 2014/05/12 18:48:17, Yaron wrote:
> On 2014/05/09 21:32:46, Charlie Reis (OOO May 17) wrote:
> > 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?
> 
> It should go away. My intention was to have this basically have 1:1
> corresponding with one visible instance of the viewer. 

Oh, I misunderstand.  That sounds good; one comment about what event to listen
for below.

> > 
> > 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.
> 
> Not necessary.
> > 
> > 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?).
> 
> 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?
> 

Yes, RenderProcessGone would be good, in case of a crash. 
AboutToNavigateRenderView isn't right-- I've commented on that below.

https://codereview.chromium.org/260073009/diff/110001/components/dom_distille...
File components/dom_distiller/content/dom_distiller_viewer_source.cc (right):

https://codereview.chromium.org/260073009/diff/110001/components/dom_distille...
components/dom_distiller/content/dom_distiller_viewer_source.cc:119: void
DomDistillerViewerSource::RequestViewerHandle::AboutToNavigateRenderView(
This doesn't sound right to me, since not all navigations go through this (e.g.,
renderer-initiated navigations).  Plus a navigation might fail or turn out to be
a download or 204, in which case we haven't left the page even if this has
fired.

If you're hoping to watch for navigations in the main frame, I would recommend
DidNavigateMainFrame instead.  That will hear all navigations in the main frame,
regardless of whether they're cross-process or not.  (Note that it will include
same-page navigations like fragment URLs and push state as well, but you can
check LoadCommittedDetails::is_in_page if you don't want to cancel for those.)

[Yaron, 2014-05-12 21:58:39]

thanks for the quick review. switched to DidNavigateMainFrame

https://codereview.chromium.org/260073009/diff/110001/components/dom_distille...
File components/dom_distiller/content/dom_distiller_viewer_source.cc (right):

https://codereview.chromium.org/260073009/diff/110001/components/dom_distille...
components/dom_distiller/content/dom_distiller_viewer_source.cc:119: void
DomDistillerViewerSource::RequestViewerHandle::AboutToNavigateRenderView(
On 2014/05/12 20:04:01, Charlie Reis(OOO as of May 17) wrote:
> This doesn't sound right to me, since not all navigations go through this
(e.g.,
> renderer-initiated navigations).  Plus a navigation might fail or turn out to
be
> a download or 204, in which case we haven't left the page even if this has
> fired.
> 
> If you're hoping to watch for navigations in the main frame, I would recommend
> DidNavigateMainFrame instead.  That will hear all navigations in the main
frame,
> regardless of whether they're cross-process or not.  (Note that it will
include
> same-page navigations like fragment URLs and push state as well, but you can
> check LoadCommittedDetails::is_in_page if you don't want to cancel for those.)

Done.

[Charlie Reis, 2014-05-12 22:12:51]

I'm still not 100% sure I understand the lifetime, but it seems to match what
you're describing.  My guess: is this observer added after the page of interest
has committed and before its DidFinishLoad occurs?  This code seems like it will
work if so.  (If the observer were added before the page of interest commits,
we'd call Cancel when it committed.)

Nothing else jumps out at me in that class, so dom_distiller_viewer_source.cc
LGTM from a content API perspective.

https://codereview.chromium.org/260073009/diff/130001/components/dom_distille...
File components/dom_distiller/content/dom_distiller_viewer_source.cc (right):

https://codereview.chromium.org/260073009/diff/130001/components/dom_distille...
components/dom_distiller/content/dom_distiller_viewer_source.cc:29: // the
RenderFrameHost for the Viewer instance.
nit: Lifetime matches that of the current main frame's page in the Viewer
instance.

[nyquist, 2014-05-13 05:01:14]

https://codereview.chromium.org/260073009/diff/130001/chrome/browser/dom_dist...
File chrome/browser/dom_distiller/dom_distiller_viewer_source_browsertest.cc
(right):

https://codereview.chromium.org/260073009/diff/130001/chrome/browser/dom_dist...
chrome/browser/dom_distiller/dom_distiller_viewer_source_browsertest.cc:177:
NULL;
Nit: Indent.

https://codereview.chromium.org/260073009/diff/130001/components/dom_distille...
File components/dom_distiller/DEPS (right):

https://codereview.chromium.org/260073009/diff/130001/components/dom_distille...
components/dom_distiller/DEPS:2: "+content/public/test",
Do these includes work for iOS?

https://codereview.chromium.org/260073009/diff/130001/components/dom_distille...
File components/dom_distiller/content/dom_distiller_viewer_source.cc (right):

https://codereview.chromium.org/260073009/diff/130001/components/dom_distille...
components/dom_distiller/content/dom_distiller_viewer_source.cc:200: if
(page_count_ == 0) {
Nit: Add a comment like:
// This is the first page, so send web page scaffolding too.

https://codereview.chromium.org/260073009/diff/130001/components/dom_distille...
components/dom_distiller/content/dom_distiller_viewer_source.cc:279: if
(kViewerCssPath == path)
Nit: Following the style of the rest of this CL, add {} in this function maybe?

https://codereview.chromium.org/260073009/diff/130001/components/dom_distille...
File components/dom_distiller/content/resources/dom_distiller_viewer.html
(right):

https://codereview.chromium.org/260073009/diff/130001/components/dom_distille...
components/dom_distiller/content/resources/dom_distiller_viewer.html:13: <script
src="/$3"></script>
Nit: I think this is fine, but maybe we should be consistent of specifying type?
Either remove text/css above or add text/javascript here? I'm fine with keeping
as it is now too though.

https://codereview.chromium.org/260073009/diff/130001/components/dom_distille...
File components/dom_distiller/core/fake_distiller.h (right):

https://codereview.chromium.org/260073009/diff/130001/components/dom_distille...
components/dom_distiller/core/fake_distiller.h:50: void
RunDistillerCallback(scoped_ptr<DistilledArticleProto> proto);
What about renaming this to RunDistillerFinishedCallback now?

https://codereview.chromium.org/260073009/diff/130001/components/dom_distille...
File components/dom_distiller/core/viewer.h (right):

https://codereview.chromium.org/260073009/diff/130001/components/dom_distille...
components/dom_distiller/core/viewer.h:45: const bool is_last_page);
Nit: What is |is_last_page| for here and in the method below?

[Yaron, 2014-05-13 17:49:22]

On 2014/05/12 22:12:51, Charlie Reis(OOO as of May 17) wrote:
> I'm still not 100% sure I understand the lifetime, but it seems to match what
> you're describing.  My guess: is this observer added after the page of
interest
> has committed and before its DidFinishLoad occurs?  This code seems like it
will
> work if so.  (If the observer were added before the page of interest commits,
> we'd call Cancel when it committed.)

Oops, you're right. I did break it with this revision. The observer is
registered prior to the initial commit.

It doesn't seem possible for the first commit to get redirected (cause we're
loading a page that we directly control - chrome-distiller://...) but that still
seems hacky. Any recommendations on how to best handle?

> 
> Nothing else jumps out at me in that class, so dom_distiller_viewer_source.cc
> LGTM from a content API perspective.
> 
>
https://codereview.chromium.org/260073009/diff/130001/components/dom_distille...
> File components/dom_distiller/content/dom_distiller_viewer_source.cc (right):
> 
>
https://codereview.chromium.org/260073009/diff/130001/components/dom_distille...
> components/dom_distiller/content/dom_distiller_viewer_source.cc:29: // the
> RenderFrameHost for the Viewer instance.
> nit: Lifetime matches that of the current main frame's page in the Viewer
> instance.

[Charlie Reis, 2014-05-13 22:59:48]

On 2014/05/13 17:49:22, Yaron wrote:
> On 2014/05/12 22:12:51, Charlie Reis(OOO as of May 17) wrote:
> > I'm still not 100% sure I understand the lifetime, but it seems to match
what
> > you're describing.  My guess: is this observer added after the page of
> interest
> > has committed and before its DidFinishLoad occurs?  This code seems like it
> will
> > work if so.  (If the observer were added before the page of interest
commits,
> > we'd call Cancel when it committed.)
> 
> Oops, you're right. I did break it with this revision. The observer is
> registered prior to the initial commit.
> 
> It doesn't seem possible for the first commit to get redirected (cause we're
> loading a page that we directly control - chrome-distiller://...) but that
still
> seems hacky. Any recommendations on how to best handle?
> 

If we expect a single commit from a known URL, you could check for that and then
cancel on any future commits or commits of other URLs.  Yes, checking for which
URL commits is fragile in general, but here it's more of a sanity check with the
"one commit only" rule being the main thing to watch for.

(I don't remember the details of the chrome-distiller:// scheme, though it's
probably been explained to me in the past.  We obviously need to be careful
about preventing that process from being able to navigate to other chrome://
URLs (even cross-process), as well as getting any WebUI bindings as you
mentioned.)

[Yaron, 2014-05-14 17:42:33]

Thanks Charlie. Ignored the navigation and addressed Tommy's comments in 2nd
oldest patchset. Last patchset is a rebase

https://codereview.chromium.org/260073009/diff/130001/chrome/browser/dom_dist...
File chrome/browser/dom_distiller/dom_distiller_viewer_source_browsertest.cc
(right):

https://codereview.chromium.org/260073009/diff/130001/chrome/browser/dom_dist...
chrome/browser/dom_distiller/dom_distiller_viewer_source_browsertest.cc:177:
NULL;
On 2014/05/13 05:01:15, nyquist wrote:
> Nit: Indent.

Done.

https://codereview.chromium.org/260073009/diff/130001/components/dom_distille...
File components/dom_distiller/content/dom_distiller_viewer_source.cc (right):

https://codereview.chromium.org/260073009/diff/130001/components/dom_distille...
components/dom_distiller/content/dom_distiller_viewer_source.cc:29: // the
RenderFrameHost for the Viewer instance.
On 2014/05/12 22:12:52, Charlie Reis(OOO as of May 17) wrote:
> nit: Lifetime matches that of the current main frame's page in the Viewer
> instance.

Done.

https://codereview.chromium.org/260073009/diff/130001/components/dom_distille...
components/dom_distiller/content/dom_distiller_viewer_source.cc:200: if
(page_count_ == 0) {
On 2014/05/13 05:01:15, nyquist wrote:
> Nit: Add a comment like:
> // This is the first page, so send web page scaffolding too.

Done.

https://codereview.chromium.org/260073009/diff/130001/components/dom_distille...
components/dom_distiller/content/dom_distiller_viewer_source.cc:279: if
(kViewerCssPath == path)
On 2014/05/13 05:01:15, nyquist wrote:
> Nit: Following the style of the rest of this CL, add {} in this function
maybe?

Done.

https://codereview.chromium.org/260073009/diff/130001/components/dom_distille...
File components/dom_distiller/content/resources/dom_distiller_viewer.html
(right):

https://codereview.chromium.org/260073009/diff/130001/components/dom_distille...
components/dom_distiller/content/resources/dom_distiller_viewer.html:13: <script
src="/$3"></script>
On 2014/05/13 05:01:15, nyquist wrote:
> Nit: I think this is fine, but maybe we should be consistent of specifying
type?
> Either remove text/css above or add text/javascript here? I'm fine with
keeping
> as it is now too though.

It's really unneeded. Removing "type" from above.

https://codereview.chromium.org/260073009/diff/130001/components/dom_distille...
File components/dom_distiller/core/fake_distiller.h (right):

https://codereview.chromium.org/260073009/diff/130001/components/dom_distille...
components/dom_distiller/core/fake_distiller.h:50: void
RunDistillerCallback(scoped_ptr<DistilledArticleProto> proto);
On 2014/05/13 05:01:15, nyquist wrote:
> What about renaming this to RunDistillerFinishedCallback now?

Follow-on?

https://codereview.chromium.org/260073009/diff/130001/components/dom_distille...
File components/dom_distiller/core/viewer.h (right):

https://codereview.chromium.org/260073009/diff/130001/components/dom_distille...
components/dom_distiller/core/viewer.h:45: const bool is_last_page);
On 2014/05/13 05:01:15, nyquist wrote:
> Nit: What is |is_last_page| for here and in the method below?

Done.

[Yaron, 2014-05-14 17:42:48]

Thanks Charlie. Ignored the navigation and addressed Tommy's comments in 2nd
oldest patchset. Last patchset is a rebase

[Yaron, 2014-05-14 17:42:57]

Thanks Charlie. Ignored the navigation and addressed Tommy's comments in 2nd
oldest patchset. Last patchset is a rebase

[nyquist, 2014-05-14 17:53:10]

lgtm

[nyquist, 2014-05-14 17:54:34]

https://codereview.chromium.org/260073009/diff/170001/components/dom_distille...
File components/dom_distiller/content/dom_distiller_viewer_source.cc (right):

https://codereview.chromium.org/260073009/diff/170001/components/dom_distille...
components/dom_distiller/content/dom_distiller_viewer_source.cc:316:
std::string* path) const {
Oh, and for future reference, you could abuse the |path| here and put the
hostname into it, and read it out again in StartDataRequest. That way it could
be mapped on the hostname as well in the RequestViewerHandle. But I'm okay with
not doing that now.

[Yaron, 2014-05-14 18:12:00]

The CQ bit was checked by yfriedman@chromium.org

[commit-bot: I haz the power, 2014-05-14 18:12:27]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/yfriedman@chromium.org/260073009/170001

[commit-bot: I haz the power, 2014-05-14 22:55:49]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-05-14 23:00:56]

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

[commit-bot: I haz the power, 2014-05-14 23:00:56]

Try jobs failed on following builders:
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)

[Yaron, 2014-05-14 23:36:25]

The CQ bit was checked by yfriedman@chromium.org

[commit-bot: I haz the power, 2014-05-14 23:37:24]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/yfriedman@chromium.org/260073009/170001

[commit-bot: I haz the power, 2014-05-15 01:07:37]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)

[commit-bot: I haz the power, 2014-05-15 01:10:09]

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

[commit-bot: I haz the power, 2014-05-15 01:10:09]

Try jobs failed on following builders:
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)

[Yaron, 2014-05-15 01:10:43]

The CQ bit was checked by yfriedman@chromium.org

[commit-bot: I haz the power, 2014-05-15 01:13:02]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/yfriedman@chromium.org/260073009/170001

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

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)

[commit-bot: I haz the power, 2014-05-15 02:06:41]

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

[commit-bot: I haz the power, 2014-05-15 02:06:42]

Try jobs failed on following builders:
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)

[Yaron, 2014-05-15 02:27:08]

The CQ bit was checked by yfriedman@chromium.org

[commit-bot: I haz the power, 2014-05-15 02:28:50]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/yfriedman@chromium.org/260073009/170001

[commit-bot: I haz the power, 2014-05-15 08:03:25]

Change committed as 270622