Add support for distilling arbitrary URLs in DOM Distiller Viewer.

Add support for distilling arbitrary URLs in DOM Distiller Viewer.

This adds support for requesting to view arbitrary URLs in the DOM
Distiller viewer. Previously, an entry first had to be added to the
list of articles.

Also fixes the CSP for the viewer to be in the correct format.

BUG=319881
TBR=rsleevi@chromium.org,joi@chromium.org

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

[nyquist, 2014-02-05 20:31:49]

PTAL

[shashi, 2014-02-05 22:28:07]

https://codereview.chromium.org/151003006/diff/1/chrome/browser/dom_distiller...
File chrome/browser/dom_distiller/dom_distiller_viewer_source_browsertest.cc
(right):

https://codereview.chromium.org/151003006/diff/1/chrome/browser/dom_distiller...
chrome/browser/dom_distiller/dom_distiller_viewer_source_browsertest.cc:162:
const GURL url(std::string(chrome::kDomDistillerScheme) + "://" +
nit: can be refactored into a function, e.g. GetDistillerViewURLFromEntryId().

https://codereview.chromium.org/151003006/diff/1/chrome/browser/dom_distiller...
chrome/browser/dom_distiller/dom_distiller_viewer_source_browsertest.cc:188:
net::EscapeQueryParamValue(view_url, true));
nit: can be refactored to a function GetDistillerViewUrlFromUrl()

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

https://codereview.chromium.org/151003006/diff/1/components/dom_distiller/con...
components/dom_distiller/content/dom_distiller_viewer_source.cc:187: GURL
dummy_url(scheme_ + "://dummy/?" + request->url().query());
nit: Instead of using dummy, maybe use dom-distiller-viewer-internal or some
other scheme, so that when I see this in the log, I can make sense of the url.

Also add a comment, that host is not passed when starting data request so we are
modifying the path to include the host component. This does feel a bit hacky
though.

https://codereview.chromium.org/151003006/diff/1/components/dom_distiller/con...
components/dom_distiller/content/dom_distiller_viewer_source.cc:189: GURL url =
net::AppendOrReplaceQueryParameter(
Maybe a unit_test to make sure this works with urls that already have
request_id. also testing when request_id is actually in the distilled url: e.g. 
?url="http://example.com/request_id=abc"

https://codereview.chromium.org/151003006/diff/1/components/dom_distiller/web...
File components/dom_distiller/webui/dom_distiller_handler.cc (left):

https://codereview.chromium.org/151003006/diff/1/components/dom_distiller/web...
components/dom_distiller/webui/dom_distiller_handler.cc:58:
DCHECK(url.is_valid());
why remove the DCHECK?

https://codereview.chromium.org/151003006/diff/1/components/dom_distiller/web...
File components/dom_distiller/webui/dom_distiller_handler.cc (right):

https://codereview.chromium.org/151003006/diff/1/components/dom_distiller/web...
components/dom_distiller/webui/dom_distiller_handler.cc:66: std::string(kUrlKey)
+ "=" +
nit: can be refactored and maybe use: net::AppendOrReplaceQueryParameter

https://codereview.chromium.org/151003006/diff/1/components/dom_distiller/web...
components/dom_distiller/webui/dom_distiller_handler.cc:83:
net::EscapeQueryParamValue(entry_id, true));
nit: can be refactored and maybe use: net::AppendOrReplaceQueryParameter

https://codereview.chromium.org/151003006/diff/1/components/dom_distiller_str...
File components/dom_distiller_strings.grdp (right):

https://codereview.chromium.org/151003006/diff/1/components/dom_distiller_str...
components/dom_distiller_strings.grdp:19: <message
name="IDS_DOM_DISTILLER_WEBUI_VIEW_URL" desc="The label for for the button for
viewing a URL on the DOM Distiller debug page.">
s/for for/for

[cjhopman, 2014-02-05 23:03:05]

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

https://codereview.chromium.org/151003006/diff/1/components/dom_distiller/con...
components/dom_distiller/content/dom_distiller_viewer_source.cc:42:
substitutions.push_back(std::string("/") + kCssPath);  // $2
why not put this slash in the html file?

https://codereview.chromium.org/151003006/diff/1/components/dom_distiller/con...
components/dom_distiller/content/dom_distiller_viewer_source.cc:190: dummy_url,
kRequestIdKey, request->url().host());
It looks like the request id is never used.

https://codereview.chromium.org/151003006/diff/1/components/dom_distiller/con...
components/dom_distiller/content/dom_distiller_viewer_source.cc:202: GURL
dummy_url(scheme_ + "://dummy/?" + path);
I don't really like how we are passing data around here. I'd really like to see
us figure out the long-term plan for this view stuff.

https://codereview.chromium.org/151003006/diff/1/components/dom_distiller/con...
components/dom_distiller/content/dom_distiller_viewer_source.cc:202: GURL
dummy_url(scheme_ + "://dummy/?" + path);
Could you comment on what you need this dummy_url for? Or extract it to a nicely
named function?

Maybe just add a function like "GetValueForKeyInURLPathQuery" either here or in
url_utils that constructs the dummy_url itself.

[shashi, 2014-02-07 19:02:24]

PTAL, gentle reminder.

[nyquist, 2014-02-25 20:30:24]

PTAL

https://codereview.chromium.org/151003006/diff/1/chrome/browser/dom_distiller...
File chrome/browser/dom_distiller/dom_distiller_viewer_source_browsertest.cc
(right):

https://codereview.chromium.org/151003006/diff/1/chrome/browser/dom_distiller...
chrome/browser/dom_distiller/dom_distiller_viewer_source_browsertest.cc:162:
const GURL url(std::string(chrome::kDomDistillerScheme) + "://" +
On 2014/02/05 22:28:08, shashi wrote:
> nit: can be refactored into a function, e.g. GetDistillerViewURLFromEntryId().

Done.

https://codereview.chromium.org/151003006/diff/1/chrome/browser/dom_distiller...
chrome/browser/dom_distiller/dom_distiller_viewer_source_browsertest.cc:188:
net::EscapeQueryParamValue(view_url, true));
On 2014/02/05 22:28:08, shashi wrote:
> nit: can be refactored to a function GetDistillerViewUrlFromUrl()

Done.

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

https://codereview.chromium.org/151003006/diff/1/components/dom_distiller/con...
components/dom_distiller/content/dom_distiller_viewer_source.cc:42:
substitutions.push_back(std::string("/") + kCssPath);  // $2
On 2014/02/05 23:03:06, cjhopman wrote:
> why not put this slash in the html file?

Done.

https://codereview.chromium.org/151003006/diff/1/components/dom_distiller/con...
components/dom_distiller/content/dom_distiller_viewer_source.cc:187: GURL
dummy_url(scheme_ + "://dummy/?" + request->url().query());
On 2014/02/05 22:28:08, shashi wrote:
> nit: Instead of using dummy, maybe use dom-distiller-viewer-internal or some
> other scheme, so that when I see this in the log, I can make sense of the url.
> 
> Also add a comment, that host is not passed when starting data request so we
are
> modifying the path to include the host component. This does feel a bit hacky
> though.
> 

Done.

https://codereview.chromium.org/151003006/diff/1/components/dom_distiller/con...
components/dom_distiller/content/dom_distiller_viewer_source.cc:189: GURL url =
net::AppendOrReplaceQueryParameter(
On 2014/02/05 22:28:08, shashi wrote:
> Maybe a unit_test to make sure this works with urls that already have
> request_id. also testing when request_id is actually in the distilled url:
e.g. 
> ?url="http://example.com/request_id=abc"
> 
> 

Removed request_id now.

https://codereview.chromium.org/151003006/diff/1/components/dom_distiller/con...
components/dom_distiller/content/dom_distiller_viewer_source.cc:190: dummy_url,
kRequestIdKey, request->url().host());
On 2014/02/05 23:03:06, cjhopman wrote:
> It looks like the request id is never used.

Done.

https://codereview.chromium.org/151003006/diff/1/components/dom_distiller/con...
components/dom_distiller/content/dom_distiller_viewer_source.cc:202: GURL
dummy_url(scheme_ + "://dummy/?" + path);
On 2014/02/05 23:03:06, cjhopman wrote:
> Could you comment on what you need this dummy_url for? Or extract it to a
nicely
> named function?
> 
> Maybe just add a function like "GetValueForKeyInURLPathQuery" either here or
in
> url_utils that constructs the dummy_url itself.

Done.

https://codereview.chromium.org/151003006/diff/1/components/dom_distiller/web...
File components/dom_distiller/webui/dom_distiller_handler.cc (left):

https://codereview.chromium.org/151003006/diff/1/components/dom_distiller/web...
components/dom_distiller/webui/dom_distiller_handler.cc:58:
DCHECK(url.is_valid());
On 2014/02/05 22:28:08, shashi wrote:
> why remove the DCHECK?

Done.

https://codereview.chromium.org/151003006/diff/1/components/dom_distiller/web...
File components/dom_distiller/webui/dom_distiller_handler.cc (right):

https://codereview.chromium.org/151003006/diff/1/components/dom_distiller/web...
components/dom_distiller/webui/dom_distiller_handler.cc:66: std::string(kUrlKey)
+ "=" +
On 2014/02/05 22:28:08, shashi wrote:
> nit: can be refactored and maybe use: net::AppendOrReplaceQueryParameter

Done.

https://codereview.chromium.org/151003006/diff/1/components/dom_distiller/web...
components/dom_distiller/webui/dom_distiller_handler.cc:83:
net::EscapeQueryParamValue(entry_id, true));
On 2014/02/05 22:28:08, shashi wrote:
> nit: can be refactored and maybe use: net::AppendOrReplaceQueryParameter

Done.

https://codereview.chromium.org/151003006/diff/1/components/dom_distiller_str...
File components/dom_distiller_strings.grdp (right):

https://codereview.chromium.org/151003006/diff/1/components/dom_distiller_str...
components/dom_distiller_strings.grdp:19: <message
name="IDS_DOM_DISTILLER_WEBUI_VIEW_URL" desc="The label for for the button for
viewing a URL on the DOM Distiller debug page.">
On 2014/02/05 22:28:08, shashi wrote:
> s/for for/for

Done.

[shashi, 2014-02-25 21:48:07]

https://codereview.chromium.org/151003006/diff/1/components/dom_distiller/web...
File components/dom_distiller/webui/dom_distiller_handler.cc (right):

https://codereview.chromium.org/151003006/diff/1/components/dom_distiller/web...
components/dom_distiller/webui/dom_distiller_handler.cc:66: std::string(kUrlKey)
+ "=" +
Is this refactored.
On 2014/02/25 20:30:24, nyquist wrote:
> On 2014/02/05 22:28:08, shashi wrote:
> > nit: can be refactored and maybe use: net::AppendOrReplaceQueryParameter
> 
> Done.

https://codereview.chromium.org/151003006/diff/1/components/dom_distiller/web...
components/dom_distiller/webui/dom_distiller_handler.cc:83:
net::EscapeQueryParamValue(entry_id, true));
Is this refactored?
On 2014/02/25 20:30:24, nyquist wrote:
> On 2014/02/05 22:28:08, shashi wrote:
> > nit: can be refactored and maybe use: net::AppendOrReplaceQueryParameter
> 
> Done.

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

https://codereview.chromium.org/151003006/diff/230001/components/dom_distille...
components/dom_distiller/content/dom_distiller_viewer_source.cc:200: bool
has_url = !requested_url_str.empty() && requested_url.is_valid();
nit is requested_url.is_valid() sufficient here?
https://code.google.com/p/chromium/codesearch#chromium/src/url/gurl.h&l=68

https://codereview.chromium.org/151003006/diff/230001/components/dom_distille...
components/dom_distiller/content/dom_distiller_viewer_source.cc:223: GURL
dummy_url(std::string(kInternalScheme) + "://dummy/" + path);
It will be nice to have a comment here, that using a dummy url to parse the key,
value in path.

https://codereview.chromium.org/151003006/diff/230001/components/dom_distille...
File components/dom_distiller/content/dom_distiller_viewer_source_unittest.cc
(right):

https://codereview.chromium.org/151003006/diff/230001/components/dom_distille...
components/dom_distiller/content/dom_distiller_viewer_source_unittest.cc:114:
view_request_delegate.get());
Thank you for adding these tests :).

I was thinking what would be the behavior for
?kUrlKey="chrome-dom-distiller://[some_entry_id]" or some webui scheme. Will
there be an error? Clearly this cannot be tested here but in a browser test. You
do not need to add a browser test for that in this CL.

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

https://codereview.chromium.org/151003006/diff/230001/components/dom_distille...
components/dom_distiller/core/dom_distiller_service.h:68: virtual void
RemoveObserver(DomDistillerObserver* observer) = 0;
Should the interface also have DISALLOW_COPY_AND_ASSIGN?

https://codereview.chromium.org/151003006/diff/230001/components/dom_distille...
File components/dom_distiller/webui/dom_distiller_handler.cc (right):

https://codereview.chromium.org/151003006/diff/230001/components/dom_distille...
components/dom_distiller/webui/dom_distiller_handler.cc:66: GURL
view_url(article_scheme_ + "://" + base::GenerateGUID() + "/?" +
Probably want to use the GetDistillerViewUrl* functions here.

https://codereview.chromium.org/151003006/diff/230001/components/dom_distille...
components/dom_distiller/webui/dom_distiller_handler.cc:83: GURL
url(article_scheme_ + "://" + base::GenerateGUID() + "/?" +
Probably want to use the GetDistillerViewUrl* functions here.

https://codereview.chromium.org/151003006/diff/230001/components/dom_distille...
components/dom_distiller/webui/dom_distiller_handler.cc:122: const GURL
DomDistillerHandler::GetDistillerViewUrlFromEntryId(
Is this function used anywhere?

https://codereview.chromium.org/151003006/diff/230001/components/dom_distille...
components/dom_distiller/webui/dom_distiller_handler.cc:130: GURL
url(article_scheme_ + "://" + base::GenerateGUID());
Same for this function.

[cjhopman, 2014-02-25 22:25:13]

https://codereview.chromium.org/151003006/diff/230001/components/dom_distille...
File components/dom_distiller/content/dom_distiller_viewer_source.h (right):

https://codereview.chromium.org/151003006/diff/230001/components/dom_distille...
components/dom_distiller/content/dom_distiller_viewer_source.h:30: // Overridden
from content::URLDataSource:
Making the URLDataSource overrides private means that users (particularly your
tests) need to access those methods through a pointer to an URLDataSource (not a
pointer to a DomDistillerViewerSource).

https://codereview.chromium.org/151003006/diff/230001/components/dom_distille...
components/dom_distiller/content/dom_distiller_viewer_source.h:50: std::string
GetValueForKeyInURLPathQuery(const std::string& path,
static

https://codereview.chromium.org/151003006/diff/230001/components/dom_distille...
components/dom_distiller/content/dom_distiller_viewer_source.h:54:
FRIEND_TEST_ALL_PREFIXES(DomDistillerViewerSourceTest, TestPathUtil);
Only this test should be a friend.

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

https://codereview.chromium.org/151003006/diff/230001/components/dom_distille...
components/dom_distiller/core/dom_distiller_service.h:34: // Calls to,
construction and destruction of this service must happen on the
This comma reads like a typo; i.e. that you meant "calls to construction and
destruction". How about "Construction, destruction, and usage of this ..."

https://codereview.chromium.org/151003006/diff/230001/components/dom_distille...
components/dom_distiller/core/dom_distiller_service.h:35: // same thread. All
callbacks will also be called on the caller's thread.
I'd say: Callbacks will be called on that same thread.

That is, refer back to the thread in the previous sentence rather than
introducing a new "thread"

https://codereview.chromium.org/151003006/diff/230001/components/dom_distille...
components/dom_distiller/core/dom_distiller_service.h:45: // available offline.
Clarify the callback part of this: is it always called? is it called with false
if the article isn't available offline?

https://codereview.chromium.org/151003006/diff/230001/components/dom_distille...
components/dom_distiller/core/dom_distiller_service.h:59: // destroyed (or when
this service is destroyed).
We should be clear about how we will handle the ViewRequestDelegate (either here
or in its declaration).

For example, is it possible to get a callback into the delegate after I've
deleted the handle?

https://codereview.chromium.org/151003006/diff/230001/components/dom_distille...
File components/dom_distiller/webui/dom_distiller_handler.cc (right):

https://codereview.chromium.org/151003006/diff/230001/components/dom_distille...
components/dom_distiller/webui/dom_distiller_handler.cc:122: const GURL
DomDistillerHandler::GetDistillerViewUrlFromEntryId(
On 2014/02/25 21:48:08, shashi wrote:
> Is this function used anywhere?

Looks like this is also defined in the tests.

[nyquist, 2014-02-27 00:07:37]

PTAL

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

https://codereview.chromium.org/151003006/diff/230001/components/dom_distille...
components/dom_distiller/content/dom_distiller_viewer_source.cc:200: bool
has_url = !requested_url_str.empty() && requested_url.is_valid();
On 2014/02/25 21:48:08, shashi wrote:
> nit is requested_url.is_valid() sufficient here?
> https://code.google.com/p/chromium/codesearch#chromium/src/url/gurl.h&l=68

Done.

https://codereview.chromium.org/151003006/diff/230001/components/dom_distille...
components/dom_distiller/content/dom_distiller_viewer_source.cc:223: GURL
dummy_url(std::string(kInternalScheme) + "://dummy/" + path);
On 2014/02/25 21:48:08, shashi wrote:
> It will be nice to have a comment here, that using a dummy url to parse the
key,
> value in path.

Done. Also moved more of this into the constant, which cleaned up a little bit.

https://codereview.chromium.org/151003006/diff/230001/components/dom_distille...
File components/dom_distiller/content/dom_distiller_viewer_source.h (right):

https://codereview.chromium.org/151003006/diff/230001/components/dom_distille...
components/dom_distiller/content/dom_distiller_viewer_source.h:30: // Overridden
from content::URLDataSource:
On 2014/02/25 22:25:14, cjhopman wrote:
> Making the URLDataSource overrides private means that users (particularly your
> tests) need to access those methods through a pointer to an URLDataSource (not
a
> pointer to a DomDistillerViewerSource).

Moved the methods from content::URLDataSource to be public.

Also, added a single friend class which accesses the method I need to test in
the unit tests.

https://codereview.chromium.org/151003006/diff/230001/components/dom_distille...
components/dom_distiller/content/dom_distiller_viewer_source.h:50: std::string
GetValueForKeyInURLPathQuery(const std::string& path,
On 2014/02/25 22:25:14, cjhopman wrote:
> static

Done.

https://codereview.chromium.org/151003006/diff/230001/components/dom_distille...
components/dom_distiller/content/dom_distiller_viewer_source.h:54:
FRIEND_TEST_ALL_PREFIXES(DomDistillerViewerSourceTest, TestPathUtil);
On 2014/02/25 22:25:14, cjhopman wrote:
> Only this test should be a friend.

See new implementation with a single friend class. WDYT?

https://codereview.chromium.org/151003006/diff/230001/components/dom_distille...
File components/dom_distiller/content/dom_distiller_viewer_source_unittest.cc
(right):

https://codereview.chromium.org/151003006/diff/230001/components/dom_distille...
components/dom_distiller/content/dom_distiller_viewer_source_unittest.cc:114:
view_request_delegate.get());
On 2014/02/25 21:48:08, shashi wrote:
> Thank you for adding these tests :).
> 
> I was thinking what would be the behavior for
> ?kUrlKey="chrome-dom-distiller://[some_entry_id]" or some webui scheme. Will
> there be an error? Clearly this cannot be tested here but in a browser test.
You
> do not need to add a browser test for that in this CL.

Added a unit test for both internal chrome:// pages and recursive calls.

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

https://codereview.chromium.org/151003006/diff/230001/components/dom_distille...
components/dom_distiller/core/dom_distiller_service.h:34: // Calls to,
construction and destruction of this service must happen on the
On 2014/02/25 22:25:14, cjhopman wrote:
> This comma reads like a typo; i.e. that you meant "calls to construction and
> destruction". How about "Construction, destruction, and usage of this ..."

Done.

https://codereview.chromium.org/151003006/diff/230001/components/dom_distille...
components/dom_distiller/core/dom_distiller_service.h:35: // same thread. All
callbacks will also be called on the caller's thread.
On 2014/02/25 22:25:14, cjhopman wrote:
> I'd say: Callbacks will be called on that same thread.
> 
> That is, refer back to the thread in the previous sentence rather than
> introducing a new "thread"

Done.

https://codereview.chromium.org/151003006/diff/230001/components/dom_distille...
components/dom_distiller/core/dom_distiller_service.h:45: // available offline.
On 2014/02/25 22:25:14, cjhopman wrote:
> Clarify the callback part of this: is it always called? is it called with
false
> if the article isn't available offline?

Done.

https://codereview.chromium.org/151003006/diff/230001/components/dom_distille...
components/dom_distiller/core/dom_distiller_service.h:59: // destroyed (or when
this service is destroyed).
On 2014/02/25 22:25:14, cjhopman wrote:
> We should be clear about how we will handle the ViewRequestDelegate (either
here
> or in its declaration).
> 
> For example, is it possible to get a callback into the delegate after I've
> deleted the handle? 

Done.

https://codereview.chromium.org/151003006/diff/230001/components/dom_distille...
components/dom_distiller/core/dom_distiller_service.h:68: virtual void
RemoveObserver(DomDistillerObserver* observer) = 0;
On 2014/02/25 21:48:08, shashi wrote:
> Should the interface also have DISALLOW_COPY_AND_ASSIGN?

Done.

https://codereview.chromium.org/151003006/diff/230001/components/dom_distille...
File components/dom_distiller/webui/dom_distiller_handler.cc (right):

https://codereview.chromium.org/151003006/diff/230001/components/dom_distille...
components/dom_distiller/webui/dom_distiller_handler.cc:66: GURL
view_url(article_scheme_ + "://" + base::GenerateGUID() + "/?" +
On 2014/02/25 21:48:08, shashi wrote:
> Probably want to use the GetDistillerViewUrl* functions here.

Done.

https://codereview.chromium.org/151003006/diff/230001/components/dom_distille...
components/dom_distiller/webui/dom_distiller_handler.cc:83: GURL
url(article_scheme_ + "://" + base::GenerateGUID() + "/?" +
On 2014/02/25 21:48:08, shashi wrote:
> Probably want to use the GetDistillerViewUrl* functions here.

Done.

https://codereview.chromium.org/151003006/diff/230001/components/dom_distille...
components/dom_distiller/webui/dom_distiller_handler.cc:122: const GURL
DomDistillerHandler::GetDistillerViewUrlFromEntryId(
On 2014/02/25 22:25:14, cjhopman wrote:
> On 2014/02/25 21:48:08, shashi wrote:
> > Is this function used anywhere?
> 
> Looks like this is also defined in the tests.
> 

Done.
Created dom_distiller::url_utils for this and put it in
//components/dom_distiller/core.

https://codereview.chromium.org/151003006/diff/230001/components/dom_distille...
components/dom_distiller/webui/dom_distiller_handler.cc:130: GURL
url(article_scheme_ + "://" + base::GenerateGUID());
On 2014/02/25 21:48:08, shashi wrote:
> Same for this function.

Done.

[shashi, 2014-02-27 04:29:08]

Mostly nits.

https://chromiumcodereview.appspot.com/151003006/diff/250001/chrome/browser/d...
File chrome/browser/dom_distiller/dom_distiller_viewer_source_browsertest.cc
(right):

https://chromiumcodereview.appspot.com/151003006/diff/250001/chrome/browser/d...
chrome/browser/dom_distiller/dom_distiller_viewer_source_browsertest.cc:25:
#include "components/dom_distiller/core/url_constants.h"
nit: is this import needed?

https://chromiumcodereview.appspot.com/151003006/diff/250001/chrome/browser/d...
chrome/browser/dom_distiller/dom_distiller_viewer_source_browsertest.cc:25:
#include "components/dom_distiller/core/url_constants.h"
nit: is this import needed?

https://chromiumcodereview.appspot.com/151003006/diff/250001/chrome/browser/d...
chrome/browser/dom_distiller/dom_distiller_viewer_source_browsertest.cc:31:
#include "net/base/escape.h"
nit: is this import needed?

https://chromiumcodereview.appspot.com/151003006/diff/250001/chrome/browser/d...
chrome/browser/dom_distiller/dom_distiller_viewer_source_browsertest.cc:32:
#include "net/base/url_util.h"
nit: is this import needed?

https://chromiumcodereview.appspot.com/151003006/diff/250001/components/dom_d...
File components/dom_distiller/content/dom_distiller_viewer_source.cc (right):

https://chromiumcodereview.appspot.com/151003006/diff/250001/components/dom_d...
components/dom_distiller/content/dom_distiller_viewer_source.cc:26: #include
"net/base/url_util.h"
nit: is this import needed?

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

https://chromiumcodereview.appspot.com/151003006/diff/250001/components/dom_d...
components/dom_distiller/content/dom_distiller_viewer_source.h:10: #include
"base/gtest_prod_util.h"
nit: is this import needed?

https://chromiumcodereview.appspot.com/151003006/diff/250001/components/dom_d...
components/dom_distiller/content/dom_distiller_viewer_source.h:47: // viewing
distilled content either based on the URL or the entry ID.
nit: Do you mean: Based on the |path|

https://chromiumcodereview.appspot.com/151003006/diff/250001/components/dom_d...
File components/dom_distiller/core/dom_distiller_service.h (right):

https://chromiumcodereview.appspot.com/151003006/diff/250001/components/dom_d...
components/dom_distiller/core/dom_distiller_service.h:73:
DISALLOW_COPY_AND_ASSIGN(DomDistillerServiceInterface);
private: before DISALLOW_COPY_AND_ASSIGN.

This now feels like http://www.boost.org/doc/libs/1_41_0/boost/noncopyable.hpp
:).

https://chromiumcodereview.appspot.com/151003006/diff/250001/components/dom_d...
File components/dom_distiller/core/url_utils.cc (right):

https://chromiumcodereview.appspot.com/151003006/diff/250001/components/dom_d...
components/dom_distiller/core/url_utils.cc:36: std::string
GetValueForKeyInURLPathQuery(const std::string& path,
Should it be Url instead of URL?

https://chromiumcodereview.appspot.com/151003006/diff/250001/components/dom_d...
File components/dom_distiller/core/url_utils.h (right):

https://chromiumcodereview.appspot.com/151003006/diff/250001/components/dom_d...
components/dom_distiller/core/url_utils.h:23: std::string
GetValueForKeyInURLPathQuery(const std::string& path,
Just noticed that we have Url and URL as two spellings.

https://chromiumcodereview.appspot.com/151003006/diff/250001/components/dom_d...
components/dom_distiller/core/url_utils.h:26: // Return whether it should be
possible to distill the given URL.
nit: |url| ?

https://chromiumcodereview.appspot.com/151003006/diff/250001/components/dom_d...
File components/dom_distiller/webui/dom_distiller_handler.cc (right):

https://chromiumcodereview.appspot.com/151003006/diff/250001/components/dom_d...
components/dom_distiller/webui/dom_distiller_handler.cc:10: #include
"base/guid.h"
nit: is this import needed?

https://chromiumcodereview.appspot.com/151003006/diff/250001/components/dom_d...
components/dom_distiller/webui/dom_distiller_handler.cc:10: #include
"base/guid.h"
nit: is this import needed?

https://chromiumcodereview.appspot.com/151003006/diff/250001/components/dom_d...
components/dom_distiller/webui/dom_distiller_handler.cc:14: #include
"components/dom_distiller/core/url_constants.h"
nit: is this import needed?

https://chromiumcodereview.appspot.com/151003006/diff/250001/components/dom_d...
components/dom_distiller/webui/dom_distiller_handler.cc:14: #include
"components/dom_distiller/core/url_constants.h"
nit: is this import needed?

https://chromiumcodereview.appspot.com/151003006/diff/250001/components/dom_d...
components/dom_distiller/webui/dom_distiller_handler.cc:68:
DCHECK(view_url.is_valid());
nit: should it be DCHECK(url_utils::IsDistillableUrl(view_url))?

https://chromiumcodereview.appspot.com/151003006/diff/250001/components/dom_d...
components/dom_distiller/webui/dom_distiller_handler.cc:105:
entry->SetString("title", net::EscapeForHTML(title));
Is there a unit test somewhere to test all values that we display under
chrome-distiller are actually escaped?. Like initialize a mock service with
article : { 'title': "<javascript> alert('foo'); </javascript>", 'html' : }

Note: you do not need to add this test in the current CL.

[cjhopman, 2014-02-27 17:35:19]

lgtm

[nyquist, 2014-02-27 19:50:19]

Addressed all nits. Added nasko@ for security review.

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

https://codereview.chromium.org/151003006/diff/250001/chrome/browser/dom_dist...
chrome/browser/dom_distiller/dom_distiller_viewer_source_browsertest.cc:25:
#include "components/dom_distiller/core/url_constants.h"
On 2014/02/27 04:29:08, shashi wrote:
> nit: is this import needed?

Done.

https://codereview.chromium.org/151003006/diff/250001/chrome/browser/dom_dist...
chrome/browser/dom_distiller/dom_distiller_viewer_source_browsertest.cc:31:
#include "net/base/escape.h"
On 2014/02/27 04:29:08, shashi wrote:
> nit: is this import needed?

Done.

https://codereview.chromium.org/151003006/diff/250001/chrome/browser/dom_dist...
chrome/browser/dom_distiller/dom_distiller_viewer_source_browsertest.cc:32:
#include "net/base/url_util.h"
On 2014/02/27 04:29:08, shashi wrote:
> nit: is this import needed?

Done.

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

https://codereview.chromium.org/151003006/diff/250001/components/dom_distille...
components/dom_distiller/content/dom_distiller_viewer_source.cc:26: #include
"net/base/url_util.h"
On 2014/02/27 04:29:08, shashi wrote:
> nit: is this import needed?

Done.

https://codereview.chromium.org/151003006/diff/250001/components/dom_distille...
File components/dom_distiller/content/dom_distiller_viewer_source.h (right):

https://codereview.chromium.org/151003006/diff/250001/components/dom_distille...
components/dom_distiller/content/dom_distiller_viewer_source.h:47: // viewing
distilled content either based on the URL or the entry ID.
On 2014/02/27 04:29:08, shashi wrote:
> nit: Do you mean: Based on the |path|

Done.

https://codereview.chromium.org/151003006/diff/250001/components/dom_distille...
File components/dom_distiller/core/dom_distiller_service.h (right):

https://codereview.chromium.org/151003006/diff/250001/components/dom_distille...
components/dom_distiller/core/dom_distiller_service.h:73:
DISALLOW_COPY_AND_ASSIGN(DomDistillerServiceInterface);
On 2014/02/27 04:29:08, shashi wrote:
> private: before DISALLOW_COPY_AND_ASSIGN.
> 
> This now feels like http://www.boost.org/doc/libs/1_41_0/boost/noncopyable.hpp
> :).

Done.

https://codereview.chromium.org/151003006/diff/250001/components/dom_distille...
File components/dom_distiller/core/url_utils.cc (right):

https://codereview.chromium.org/151003006/diff/250001/components/dom_distille...
components/dom_distiller/core/url_utils.cc:36: std::string
GetValueForKeyInURLPathQuery(const std::string& path,
On 2014/02/27 04:29:08, shashi wrote:
> Should it be Url instead of URL?

Done.

https://codereview.chromium.org/151003006/diff/250001/components/dom_distille...
File components/dom_distiller/core/url_utils.h (right):

https://codereview.chromium.org/151003006/diff/250001/components/dom_distille...
components/dom_distiller/core/url_utils.h:23: std::string
GetValueForKeyInURLPathQuery(const std::string& path,
On 2014/02/27 04:29:08, shashi wrote:
> Just noticed that we have Url and URL as two spellings.

Done.

https://codereview.chromium.org/151003006/diff/250001/components/dom_distille...
components/dom_distiller/core/url_utils.h:26: // Return whether it should be
possible to distill the given URL.
On 2014/02/27 04:29:08, shashi wrote:
> nit: |url| ?

Done.

https://codereview.chromium.org/151003006/diff/250001/components/dom_distille...
File components/dom_distiller/webui/dom_distiller_handler.cc (right):

https://codereview.chromium.org/151003006/diff/250001/components/dom_distille...
components/dom_distiller/webui/dom_distiller_handler.cc:10: #include
"base/guid.h"
On 2014/02/27 04:29:08, shashi wrote:
> nit: is this import needed?

Done.

https://codereview.chromium.org/151003006/diff/250001/components/dom_distille...
components/dom_distiller/webui/dom_distiller_handler.cc:14: #include
"components/dom_distiller/core/url_constants.h"
On 2014/02/27 04:29:08, shashi wrote:
> nit: is this import needed?

Done.

https://codereview.chromium.org/151003006/diff/250001/components/dom_distille...
components/dom_distiller/webui/dom_distiller_handler.cc:68:
DCHECK(view_url.is_valid());
On 2014/02/27 04:29:08, shashi wrote:
> nit: should it be DCHECK(url_utils::IsDistillableUrl(view_url))?

It should be that on the check of gurl above. This URL is just the one we
create. Thanks!

https://codereview.chromium.org/151003006/diff/250001/components/dom_distille...
components/dom_distiller/webui/dom_distiller_handler.cc:105:
entry->SetString("title", net::EscapeForHTML(title));
On 2014/02/27 04:29:08, shashi wrote:
> Is there a unit test somewhere to test all values that we display under
> chrome-distiller are actually escaped?. Like initialize a mock service with
> article : { 'title': "<javascript> alert('foo'); </javascript>", 'html' : }
> 
> Note: you do not need to add this test in the current CL.

Good suggestion. I'll add a TODO for that.

[shashi, 2014-02-27 20:13:24]

lgtm, thanks!

[nasko, 2014-02-27 22:40:44]

I can't claim I understand the full CL, but looking from security and general
code cleanliness point of view, it looks pretty good. Few comments only.

https://codereview.chromium.org/151003006/diff/290001/components/dom_distille...
File components/dom_distiller/core/dom_distiller_service.h (left):

https://codereview.chromium.org/151003006/diff/290001/components/dom_distille...
components/dom_distiller/core/dom_distiller_service.h:92:
DISALLOW_COPY_AND_ASSIGN(DomDistillerService);
Why is this no longer needed?

https://codereview.chromium.org/151003006/diff/290001/components/dom_distille...
File components/dom_distiller/core/url_utils.cc (right):

https://codereview.chromium.org/151003006/diff/290001/components/dom_distille...
components/dom_distiller/core/url_utils.cc:26: GURL url(scheme + "://" +
base::GenerateGUID());
Do you always want to generate new GUID for each call to this function? It means
that for the same entry id, you will get different results for each call.

https://codereview.chromium.org/151003006/diff/290001/components/dom_distille...
File components/dom_distiller/webui/dom_distiller_handler.cc (right):

https://codereview.chromium.org/151003006/diff/290001/components/dom_distille...
components/dom_distiller/webui/dom_distiller_handler.cc:61: args->GetString(0,
&url);
nit: You aren't checking the success of GetString.

https://codereview.chromium.org/151003006/diff/290001/components/dom_distille...
components/dom_distiller/webui/dom_distiller_handler.cc:63: if
(url_utils::IsDistillableUrl(gurl)) {
nit: Just a personal preference, but it reads better if you invert the boolean
check and all the code moves out of the code block. It makes the if statement
very simple and concise.

https://codereview.chromium.org/151003006/diff/290001/components/dom_distille...
components/dom_distiller/webui/dom_distiller_handler.cc:66:
DCHECK(view_url.is_valid());
DCHECKs should almost always be followed by an if statement to handle the
failure condition. Why try to load if you know it is not a valid URL?

[nyquist, 2014-02-28 16:46:51]

nasko: PTAL

https://codereview.chromium.org/151003006/diff/290001/components/dom_distille...
File components/dom_distiller/core/dom_distiller_service.h (left):

https://codereview.chromium.org/151003006/diff/290001/components/dom_distille...
components/dom_distiller/core/dom_distiller_service.h:92:
DISALLOW_COPY_AND_ASSIGN(DomDistillerService);
On 2014/02/27 22:40:44, Nasko Oskov wrote:
> Why is this no longer needed?

Done. Kept this in addition to the one in the pure virtual interface.

https://codereview.chromium.org/151003006/diff/290001/components/dom_distille...
File components/dom_distiller/core/url_utils.cc (right):

https://codereview.chromium.org/151003006/diff/290001/components/dom_distille...
components/dom_distiller/core/url_utils.cc:26: GURL url(scheme + "://" +
base::GenerateGUID());
On 2014/02/27 22:40:44, Nasko Oskov wrote:
> Do you always want to generate new GUID for each call to this function? It
means
> that for the same entry id, you will get different results for each call.

Yes, that's the idea. If this will be run in an extension, they will then always
get separate origins. The idea is to move the logic for this into something that
contains a map of these, and maybe unique CSS styles, etc. going forward. Doing
this from the start makes sure that all of these distilled pages will have their
own origin.

https://codereview.chromium.org/151003006/diff/290001/components/dom_distille...
File components/dom_distiller/webui/dom_distiller_handler.cc (right):

https://codereview.chromium.org/151003006/diff/290001/components/dom_distille...
components/dom_distiller/webui/dom_distiller_handler.cc:61: args->GetString(0,
&url);
On 2014/02/27 22:40:44, Nasko Oskov wrote:
> nit: You aren't checking the success of GetString.

Done.

https://codereview.chromium.org/151003006/diff/290001/components/dom_distille...
components/dom_distiller/webui/dom_distiller_handler.cc:63: if
(url_utils::IsDistillableUrl(gurl)) {
On 2014/02/27 22:40:44, Nasko Oskov wrote:
> nit: Just a personal preference, but it reads better if you invert the boolean
> check and all the code moves out of the code block. It makes the if statement
> very simple and concise.

I couldn't find a pretty way to write the method, but by extracting parts of it
to a method in an anonymous namespace at least the high level logic should be
easy to read. I hope you like this better.

On the personal note, after I grew up I've usually followed the "positive branch
first", if there's both if-else. If the else-block can make it become an early
return, that's a different story. In this case, I ended up checking three
conditionals in the end (GetString, IsUrlDistillable and is_valid), so I did
what I felt was easiest for new readers.

https://codereview.chromium.org/151003006/diff/290001/components/dom_distille...
components/dom_distiller/webui/dom_distiller_handler.cc:66:
DCHECK(view_url.is_valid());
On 2014/02/27 22:40:44, Nasko Oskov wrote:
> DCHECKs should almost always be followed by an if statement to handle the
> failure condition. Why try to load if you know it is not a valid URL?

Removed DCHECK, and instead handle an error. The style guide says we should not
handle DCHECK fails:
http://www.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREACHED-
"you should not handle DCHECK() failures, even if failure would result in a
crash"

But since this is not really a crash, but more security/validation, and in
addition what to do in a failure is very well defined, I decided to handle the
error instead.

[nasko, 2014-02-28 18:10:18]

LGTM

[nyquist, 2014-02-28 20:10:44]

The CQ bit was checked by nyquist@chromium.org

[commit-bot: I haz the power, 2014-02-28 20:11:21]

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

[commit-bot: I haz the power, 2014-02-28 20:37:18]

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

[commit-bot: I haz the power, 2014-02-28 20:37:18]

Try jobs failed on following builders: linux_chromium_clang_dbg

[nyquist, 2014-03-01 00:00:32]

The CQ bit was checked by nyquist@chromium.org

[commit-bot: I haz the power, 2014-03-01 00:01:38]

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

[commit-bot: I haz the power, 2014-03-01 00:44:21]

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

[commit-bot: I haz the power, 2014-03-01 00:44:21]

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

[nyquist, 2014-03-03 07:56:15]

The CQ bit was checked by nyquist@chromium.org

[commit-bot: I haz the power, 2014-03-03 07:56:21]

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

[commit-bot: I haz the power, 2014-03-03 12:30:54]

Change committed as 254458