Add support for displaying distilled articles.

Add support for displaying distilled articles.

Adds support in the content::URLDataSource for DOM Distiller for displaying
distilled articles.

Also adds preliminary error handling.

Depends on https://codereview.chromium.org/105723002/

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

[nyquist, 2014-01-14 22:13:47]

thakis: PTAL

[nyquist, 2014-01-14 22:14:30]

tsepez: security review

[Tom Sepez, 2014-01-14 22:25:56]

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

https://codereview.chromium.org/134873008/diff/1/components/dom_distiller/con...
components/dom_distiller/content/dom_distiller_viewer_source.cc:71: std::string
title;
Do we know that title doesn't itself contain markup?

[nyquist, 2014-01-14 22:26:52]

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

https://codereview.chromium.org/134873008/diff/1/components/dom_distiller/con...
components/dom_distiller/content/dom_distiller_viewer_source.cc:71: std::string
title;
On 2014/01/14 22:25:56, Tom Sepez wrote:
> Do we know that title doesn't itself contain markup?

No, we don't. It should be sanitized.

[Nico, 2014-01-15 05:31:47]

lgtm with Tom's comment addressed. I don't have anything useful to say.

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

https://codereview.chromium.org/134873008/diff/1/components/dom_distiller/con...
components/dom_distiller/content/dom_distiller_viewer_source.cc:37: }
nit: newline above, this add "  // namespace"

https://codereview.chromium.org/134873008/diff/1/components/dom_distiller/con...
components/dom_distiller/content/dom_distiller_viewer_source.cc:118: //
|RequestViewerHandle| will never be called, so cleanup now.
nit "clean up now"

https://codereview.chromium.org/134873008/diff/1/components/dom_distiller/con...
components/dom_distiller/content/dom_distiller_viewer_source.cc:145:
path->assign(request->url().host());
nit: imho `*path = ...` is way more readable

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

https://codereview.chromium.org/134873008/diff/1/components/dom_distiller/con...
components/dom_distiller/content/resources/dom_distiller_viewer.html:6: +-->
why the "+"? :-)

[nyquist, 2014-01-15 22:21:42]

PTAL (tsepez). Addressed all comments.

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

https://codereview.chromium.org/134873008/diff/1/components/dom_distiller/con...
components/dom_distiller/content/dom_distiller_viewer_source.cc:37: }
On 2014/01/15 05:31:48, Nico wrote:
> nit: newline above, this add "  // namespace"

Done.

https://codereview.chromium.org/134873008/diff/1/components/dom_distiller/con...
components/dom_distiller/content/dom_distiller_viewer_source.cc:71: std::string
title;
On 2014/01/14 22:26:53, nyquist wrote:
> On 2014/01/14 22:25:56, Tom Sepez wrote:
> > Do we know that title doesn't itself contain markup?
> 
> No, we don't. It should be sanitized.

Done.

https://codereview.chromium.org/134873008/diff/1/components/dom_distiller/con...
components/dom_distiller/content/dom_distiller_viewer_source.cc:118: //
|RequestViewerHandle| will never be called, so cleanup now.
On 2014/01/15 05:31:48, Nico wrote:
> nit "clean up now"

Done.

https://codereview.chromium.org/134873008/diff/1/components/dom_distiller/con...
components/dom_distiller/content/dom_distiller_viewer_source.cc:145:
path->assign(request->url().host());
On 2014/01/15 05:31:48, Nico wrote:
> nit: imho `*path = ...` is way more readable

Done.

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

https://codereview.chromium.org/134873008/diff/1/components/dom_distiller/con...
components/dom_distiller/content/resources/dom_distiller_viewer.html:6: +-->
On 2014/01/15 05:31:48, Nico wrote:
> why the "+"? :-)

It looks pretty, don't you think? It kind of gives a hint to what will come
later on the line...

Removed.

[Tom Sepez, 2014-01-15 22:28:39]

lgtm

[nyquist, 2014-01-28 02:31:10]

cjhopman: PTAL //components/dom_distiller
rsleevi: PTAL for //net/base DEPS include.

[cjhopman, 2014-01-28 02:56:55]

lgtm

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

https://codereview.chromium.org/134873008/diff/220001/components/dom_distille...
components/dom_distiller/content/dom_distiller_viewer_source.cc:62: // This
holds the callback to where the data retreived is sent back.
s/retreived/retrieved/

https://codereview.chromium.org/134873008/diff/220001/components/dom_distille...
components/dom_distiller/content/dom_distiller_viewer_source.cc:117:
RequestViewerHandle* request_viewer_handle =
What do you think about making this a scoped_ptr, and then in the if
(viewer_handle) case you explicitly release() it and comment that it will delete
itself in the OnArticleReady callback? I think that might make the lifetime
slightly clearer.

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

https://codereview.chromium.org/134873008/diff/220001/components/dom_distille...
components/dom_distiller/content/resources/dom_distiller_viewer.html:5: ound in
the LICENSE file.
s/ound/found/

[Ryan Sleevi, 2014-01-28 06:48:14]

What did you need me to review? I don't see any net/ changes?

[nyquist, 2014-01-28 16:49:53]

On 2014/01/28 06:48:14, Ryan Sleevi wrote:
> What did you need me to review? I don't see any net/ changes?

rsleevi:
Adding net/base dependency in: components/dom_distiller/content/DEPS
I think I did it correctly though, it is in
components/dom_distiller/content/dom_distiller_viewer_source.cc:
77     title = net::EscapeForHTML(proto->title());

proto->title() is completely unsafe, extracted from the source web page, and
|title| is supposed to be displayed to our users in both <head><title>... tag
and probably inside <h1> or something on the page.

I guess I could just TBR adding new DEPS, but just wanted to verify I was using
EscapeForHTML correctly.

[nyquist, 2014-01-28 18:16:12]

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

https://codereview.chromium.org/134873008/diff/220001/components/dom_distille...
components/dom_distiller/content/dom_distiller_viewer_source.cc:62: // This
holds the callback to where the data retreived is sent back.
On 2014/01/28 02:56:56, cjhopman (OOO til jan 27) wrote:
> s/retreived/retrieved/

Done.

https://codereview.chromium.org/134873008/diff/220001/components/dom_distille...
components/dom_distiller/content/dom_distiller_viewer_source.cc:117:
RequestViewerHandle* request_viewer_handle =
On 2014/01/28 02:56:56, cjhopman (OOO til jan 27) wrote:
> What do you think about making this a scoped_ptr, and then in the if
> (viewer_handle) case you explicitly release() it and comment that it will
delete
> itself in the OnArticleReady callback? I think that might make the lifetime
> slightly clearer.

Done.

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

https://codereview.chromium.org/134873008/diff/220001/components/dom_distille...
components/dom_distiller/content/resources/dom_distiller_viewer.html:5: ound in
the LICENSE file.
On 2014/01/28 02:56:56, cjhopman (OOO til jan 27) wrote:
> s/ound/found/

Done.

[Ryan Sleevi, 2014-01-28 20:31:52]

On 2014/01/28 16:49:53, nyquist wrote:
> On 2014/01/28 06:48:14, Ryan Sleevi wrote:
> > What did you need me to review? I don't see any net/ changes?
> 
> rsleevi:
> Adding net/base dependency in: components/dom_distiller/content/DEPS
> I think I did it correctly though, it is in
> components/dom_distiller/content/dom_distiller_viewer_source.cc:
> 77     title = net::EscapeForHTML(proto->title());
> 
> proto->title() is completely unsafe, extracted from the source web page, and
> |title| is supposed to be displayed to our users in both <head><title>... tag
> and probably inside <h1> or something on the page.
> 
> I guess I could just TBR adding new DEPS, but just wanted to verify I was
using
> EscapeForHTML correctly.

Then yes, LGTM . EscapeForHTML is just a simple string replacement. No need for
a net/ OWNERS review for DEPS change (more a need for the module owner of
whereever you're adding the DEPS)

[commit-bot: I haz the power, 2014-01-28 21:36:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nyquist@chromium.org/134873008/250001

[commit-bot: I haz the power, 2014-01-29 06:20:27]

Change committed as 247645