Add the scheme chrome-distiller:// and hook up data source.

Add the scheme chrome-distiller:// and hook up data source.

This adds the scheme chrome-distiller:// and has a minimal implementation
of a data source which should be loaded when a URL with the new scheme is
loaded.

The chrome-distiller:// scheme will be used for displaying distilled
articles, which is extracted content of web pages with long form articles.

Chrome will maintain a list of such articles, and in addition, the user
will be able to display such distilled content on demand for a given URL.

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

[nyquist, 2013-12-04 23:53:35]

thakis: PTAL for overview

Patch set 1 is the "mechanical move" of a CL that depends on the new "list CL".
Patch set 2 and 3 are cleanups of that.

[nyquist, 2013-12-04 23:56:55]

https://codereview.chromium.org/105723002/diff/40001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc (right):

https://codereview.chromium.org/105723002/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc:220: return new
dom_distiller::DomDistillerViewerUi(web_ui, service);
I think this can be added in the next CL instead. Sorry about that. I will
remove it in the next patch set. By removing the |service| argument, I think
this whole block can go away, as |web_ui| is given by default as an argument.

[Nico, 2013-12-05 00:22:38]

lgtm. Maybe the CL description could describe a bit more what this is.

Since this adds a new scheme: abarth, can you do a security review for the new
scheme?

[abarth-chromium, 2013-12-05 04:33:44]

It's not clear to me why this feature needs a new scheme.  Please have someone
on the security team review this CL for security.

[Nico, 2013-12-05 04:34:58]

On Wed, Dec 4, 2013 at 8:33 PM, <abarth@chromium.org> wrote:

> It's not clear to me why this feature needs a new scheme.  Please have
> someone
> on the security team review this CL for security.
>

Can you suggest someone? I added you for this, but apparently you're not
the right person :-) tsepez maybe?


>
> https://codereview.chromium.org/105723002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[abarth-chromium, 2013-12-05 04:40:01]

On 2013/12/05 04:34:58, Nico wrote:
> Can you suggest someone? I added you for this, but apparently you're not
> the right person :-) tsepez maybe?

Yes, tsepez is a good choice.  I'm trying to stay out of the security team's
hair these days.  :)

[Nico, 2013-12-05 04:41:02]

On Wed, Dec 4, 2013 at 8:40 PM, <abarth@chromium.org> wrote:

> On 2013/12/05 04:34:58, Nico wrote:
>
>> Can you suggest someone? I added you for this, but apparently you're not
>> the right person :-) tsepez maybe?
>>
>
> Yes, tsepez is a good choice.  I'm trying to stay out of the security
> team's
> hair these days.  :)
>

That too is probably a good choice ;-) Thanks, I'll add him and remove you.


>
> https://codereview.chromium.org/105723002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Nico, 2013-12-05 04:41:37]

tsepez, this adds a new scheme. Can you do a security review?

[nasko, 2013-12-05 16:59:27]

A drive-by comment.

https://codereview.chromium.org/105723002/diff/70001/components/dom_distiller...
File components/dom_distiller/webui/dom_distiller_viewer_ui.cc (right):

https://codereview.chromium.org/105723002/diff/70001/components/dom_distiller...
components/dom_distiller/webui/dom_distiller_viewer_ui.cc:22: GURL url =
web_contents->GetVisibleURL();
This should be GetLastCommittedURL. GetVisibleURL can return the URL of a
pending navigation, which has no content, so it doesn't make sense to distill
anything.

[Tom Sepez, 2013-12-05 19:25:52]

https://codereview.chromium.org/105723002/diff/90001/components/dom_distiller...
File components/dom_distiller/webui/dom_distiller_handler.cc (right):

https://codereview.chromium.org/105723002/diff/90001/components/dom_distiller...
components/dom_distiller/webui/dom_distiller_handler.cc:57:
web_ui()->GetWebContents()->GetController().LoadURL(url,
@creis - would this load into the same renderer as other open webui?

[Charlie Reis, 2013-12-06 22:09:31]

https://codereview.chromium.org/105723002/diff/90001/components/dom_distiller...
File components/dom_distiller/webui/dom_distiller_handler.cc (right):

https://codereview.chromium.org/105723002/diff/90001/components/dom_distiller...
components/dom_distiller/webui/dom_distiller_handler.cc:57:
web_ui()->GetWebContents()->GetController().LoadURL(url,
On 2013/12/05 19:25:52, Tom Sepez wrote:
> @creis - would this load into the same renderer as other open webui?

That depends on whether chrome-distiller:// URLs have WebUI bindings.  The check
is in RenderProcessHostImpl::IsSuitableHost, which passes the URL to
WebUIControllerFactoryRegistry::GetInstance()->UseWebUIBindingsForURL.

Why do we need a new scheme for this, though?  Adding new schemes tends to be
risky work, and we avoid it whenever possible.  For example, we have to know
whether it's legal to navigate from normal pages to this scheme, or from this
scheme to chrome://.  (There's a warning in
content/public/common/url_constants.cc.)

[nyquist, 2013-12-06 22:25:43]

https://codereview.chromium.org/105723002/diff/90001/components/dom_distiller...
File components/dom_distiller/webui/dom_distiller_handler.cc (right):

https://codereview.chromium.org/105723002/diff/90001/components/dom_distiller...
components/dom_distiller/webui/dom_distiller_handler.cc:57:
web_ui()->GetWebContents()->GetController().LoadURL(url,
On 2013/12/06 22:09:32, creis wrote:
> On 2013/12/05 19:25:52, Tom Sepez wrote:
> > @creis - would this load into the same renderer as other open webui?
> 
> That depends on whether chrome-distiller:// URLs have WebUI bindings.  The
check
> is in RenderProcessHostImpl::IsSuitableHost, which passes the URL to
> WebUIControllerFactoryRegistry::GetInstance()->UseWebUIBindingsForURL.
> 
> Why do we need a new scheme for this, though?  Adding new schemes tends to be
> risky work, and we avoid it whenever possible.  For example, we have to know
> whether it's legal to navigate from normal pages to this scheme, or from this
> scheme to chrome://.  (There's a warning in
> content/public/common/url_constants.cc.)

chrome-distiller:// does use a WebUI to render things. A new scheme was proposed
to ensure that chrome:// does not get compromised since the content that will be
rendered is inherently not trusted.

Also, this will probably in the end be rendered in an iframe, and not as a top
level navigation. However, this CL doesn't support that yet.

[Charlie Reis, 2013-12-06 22:33:02]

https://codereview.chromium.org/105723002/diff/90001/components/dom_distiller...
File components/dom_distiller/webui/dom_distiller_handler.cc (right):

https://codereview.chromium.org/105723002/diff/90001/components/dom_distiller...
components/dom_distiller/webui/dom_distiller_handler.cc:57:
web_ui()->GetWebContents()->GetController().LoadURL(url,
On 2013/12/06 22:25:44, nyquist wrote:
> On 2013/12/06 22:09:32, creis wrote:
> > On 2013/12/05 19:25:52, Tom Sepez wrote:
> > > @creis - would this load into the same renderer as other open webui?
> > 
> > That depends on whether chrome-distiller:// URLs have WebUI bindings.  The
> check
> > is in RenderProcessHostImpl::IsSuitableHost, which passes the URL to
> > WebUIControllerFactoryRegistry::GetInstance()->UseWebUIBindingsForURL.
> > 
> > Why do we need a new scheme for this, though?  Adding new schemes tends to
be
> > risky work, and we avoid it whenever possible.  For example, we have to know
> > whether it's legal to navigate from normal pages to this scheme, or from
this
> > scheme to chrome://.  (There's a warning in
> > content/public/common/url_constants.cc.)
> 
> chrome-distiller:// does use a WebUI to render things. A new scheme was
proposed
> to ensure that chrome:// does not get compromised since the content that will
be
> rendered is inherently not trusted.

I don't think this won't guarantee that.  If the user has many tabs open, it
looks like any WebUI pages could end up sharing a process.

I'm concerned about a WebUI-enabled process that contains untrusted content.  I
don't think we've had to support that in the past.

> Also, this will probably in the end be rendered in an iframe, and not as a top
> level navigation. However, this CL doesn't support that yet.

What would the top-level URL be?  The process is currently decided by the
top-level URL, since we don't have out-of-process iframes working yet.  We
wouldn't be able to grant WebUI bindings to just an iframe and not the rest of
the process.

[nyquist, 2014-01-08 02:00:40]

tsepez, creis: PTAL. Now implemented as data source instead of normal WebUI.

Sorry for the big rebase, but at least the CL is less than half the previous
size.

https://codereview.chromium.org/105723002/diff/90001/components/dom_distiller...
File components/dom_distiller/webui/dom_distiller_handler.cc (right):

https://codereview.chromium.org/105723002/diff/90001/components/dom_distiller...
components/dom_distiller/webui/dom_distiller_handler.cc:57:
web_ui()->GetWebContents()->GetController().LoadURL(url,
On 2013/12/06 22:33:03, Charlie Reis (OOO) wrote:
> On 2013/12/06 22:25:44, nyquist wrote:
> > On 2013/12/06 22:09:32, creis wrote:
> > > On 2013/12/05 19:25:52, Tom Sepez wrote:
> > > > @creis - would this load into the same renderer as other open webui?
> > > 
> > > That depends on whether chrome-distiller:// URLs have WebUI bindings.  The
> > check
> > > is in RenderProcessHostImpl::IsSuitableHost, which passes the URL to
> > > WebUIControllerFactoryRegistry::GetInstance()->UseWebUIBindingsForURL.
> > > 
> > > Why do we need a new scheme for this, though?  Adding new schemes tends to
> be
> > > risky work, and we avoid it whenever possible.  For example, we have to
know
> > > whether it's legal to navigate from normal pages to this scheme, or from
> this
> > > scheme to chrome://.  (There's a warning in
> > > content/public/common/url_constants.cc.)
> > 
> > chrome-distiller:// does use a WebUI to render things. A new scheme was
> proposed
> > to ensure that chrome:// does not get compromised since the content that
will
> be
> > rendered is inherently not trusted.
> 
> I don't think this won't guarantee that.  If the user has many tabs open, it
> looks like any WebUI pages could end up sharing a process.
> 
> I'm concerned about a WebUI-enabled process that contains untrusted content. 
I
> don't think we've had to support that in the past.
> 

Changed to implement as a data source instead.

> > Also, this will probably in the end be rendered in an iframe, and not as a
top
> > level navigation. However, this CL doesn't support that yet.
> 
> What would the top-level URL be?  The process is currently decided by the
> top-level URL, since we don't have out-of-process iframes working yet.  We
> wouldn't be able to grant WebUI bindings to just an iframe and not the rest of
> the process.

chrome-distiller://123-abc for entries. Not decided for on-the-fly URLs yet.

[Tom Sepez, 2014-01-09 00:56:10]

Thanks for taking the time to do it this way.  I believe this is correct. LGTM.

[nasko, 2014-01-16 23:39:11]

I haven't followed the changes in this CL, so looking only at the latest
patchset I've added some comments. 
I have two main concerns:
* some things added to content/ don't belong there
* I can't clearly see how we avoid giving the new scheme privileged WebUI
bindings. It will be good to have an explicit test case for this.

https://codereview.chromium.org/105723002/diff/110001/chrome/browser/content_...
File chrome/browser/content_settings/host_content_settings_map.cc (right):

https://codereview.chromium.org/105723002/diff/110001/chrome/browser/content_...
chrome/browser/content_settings/host_content_settings_map.cc:557:
primary_url.SchemeIs(chrome::kDomDistillerScheme) ||
I'm not sure if we need to exempt the scheme from content settings. If I
understand the feature correctly, we will strip down the DOM to the simplest
form and display that. This is not Chrome internal content, rather untrusted
data from the web, we don't need to exempt this content from content settings.

nit: please keep this in alphabetical order, since most other places where the
schemes appear do it.

https://codereview.chromium.org/105723002/diff/110001/chrome/renderer/chrome_...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/105723002/diff/110001/chrome/renderer/chrome_...
chrome/renderer/chrome_content_renderer_client.cc:311: if
(CommandLine::ForCurrentProcess()->HasSwitch(
While this is technically correct, registering the scheme always as a display
isolated is not going to harm security. I doubt it will bring perf hit either,
so might as well do it all the time. This way you don't need to test for it when
the flag is passed.

https://codereview.chromium.org/105723002/diff/110001/chrome/renderer/content...
File chrome/renderer/content_settings_observer.cc (right):

https://codereview.chromium.org/105723002/diff/110001/chrome/renderer/content...
chrome/renderer/content_settings_observer.cc:656: if
(EqualsASCII(origin.protocol(), chrome::kDomDistillerScheme))
See the other comment in host_content_settings_map.cc.

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

https://codereview.chromium.org/105723002/diff/110001/components/dom_distille...
components/dom_distiller/content/dom_distiller_viewer_source.h:27: int
render_view_id,
Any reason you are changing this to render_view_id? We are moving the codebase
to user RenderFrame instead of RenderView.

https://codereview.chromium.org/105723002/diff/110001/content/browser/rendere...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/105723002/diff/110001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.cc:1008:
switches::kEnableDomDistiller,
This shouldn't be here, as the chrome layer can add extra switches in
ChromeContentBrowserClient::AppendExtraCommandLineSwitches.

https://codereview.chromium.org/105723002/diff/110001/content/public/browser/...
File content/public/browser/url_data_source.cc (right):

https://codereview.chromium.org/105723002/diff/110001/content/public/browser/...
content/public/browser/url_data_source.cc:50:
request->url().SchemeIs(chrome::kDomDistillerScheme) ||
nit: alphabetical order

https://codereview.chromium.org/105723002/diff/110001/content/public/common/c...
File content/public/common/content_switches.h (right):

https://codereview.chromium.org/105723002/diff/110001/content/public/common/c...
content/public/common/content_switches.h:130: CONTENT_EXPORT extern const char
kEnableDomDistiller[];
Why is this part of content switches? This is not meant to be part of content,
is it?

[nyquist, 2014-01-22 00:47:57]

Addressed all comments until now.

Still looking into:
1) Writing a browsertest
2) Removing the last few bits from //content

https://codereview.chromium.org/105723002/diff/110001/chrome/browser/content_...
File chrome/browser/content_settings/host_content_settings_map.cc (right):

https://codereview.chromium.org/105723002/diff/110001/chrome/browser/content_...
chrome/browser/content_settings/host_content_settings_map.cc:557:
primary_url.SchemeIs(chrome::kDomDistillerScheme) ||
On 2014/01/16 23:39:12, nasko wrote:
> I'm not sure if we need to exempt the scheme from content settings. If I
> understand the feature correctly, we will strip down the DOM to the simplest
> form and display that. This is not Chrome internal content, rather untrusted
> data from the web, we don't need to exempt this content from content settings.
> 
> nit: please keep this in alphabetical order, since most other places where the
> schemes appear do it.

Removed.

https://codereview.chromium.org/105723002/diff/110001/chrome/renderer/chrome_...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/105723002/diff/110001/chrome/renderer/chrome_...
chrome/renderer/chrome_content_renderer_client.cc:311: if
(CommandLine::ForCurrentProcess()->HasSwitch(
On 2014/01/16 23:39:12, nasko wrote:
> While this is technically correct, registering the scheme always as a display
> isolated is not going to harm security. I doubt it will bring perf hit either,
> so might as well do it all the time. This way you don't need to test for it
when
> the flag is passed.

Done.

https://codereview.chromium.org/105723002/diff/110001/chrome/renderer/content...
File chrome/renderer/content_settings_observer.cc (right):

https://codereview.chromium.org/105723002/diff/110001/chrome/renderer/content...
chrome/renderer/content_settings_observer.cc:656: if
(EqualsASCII(origin.protocol(), chrome::kDomDistillerScheme))
On 2014/01/16 23:39:12, nasko wrote:
> See the other comment in host_content_settings_map.cc.

Removed.

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

https://codereview.chromium.org/105723002/diff/110001/components/dom_distille...
components/dom_distiller/content/dom_distiller_viewer_source.h:27: int
render_view_id,
On 2014/01/16 23:39:12, nasko wrote:
> Any reason you are changing this to render_view_id? We are moving the codebase
> to user RenderFrame instead of RenderView.

Changed back. No good reason by the way...

https://codereview.chromium.org/105723002/diff/110001/content/browser/rendere...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/105723002/diff/110001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.cc:1008:
switches::kEnableDomDistiller,
On 2014/01/16 23:39:12, nasko wrote:
> This shouldn't be here, as the chrome layer can add extra switches in
> ChromeContentBrowserClient::AppendExtraCommandLineSwitches.

Removed.

https://codereview.chromium.org/105723002/diff/110001/content/public/browser/...
File content/public/browser/url_data_source.cc (right):

https://codereview.chromium.org/105723002/diff/110001/content/public/browser/...
content/public/browser/url_data_source.cc:50:
request->url().SchemeIs(chrome::kDomDistillerScheme) ||
On 2014/01/16 23:39:12, nasko wrote:
> nit: alphabetical order

We override this in the data source, so removed this.

https://codereview.chromium.org/105723002/diff/110001/content/public/common/c...
File content/public/common/content_switches.h (right):

https://codereview.chromium.org/105723002/diff/110001/content/public/common/c...
content/public/common/content_switches.h:130: CONTENT_EXPORT extern const char
kEnableDomDistiller[];
On 2014/01/16 23:39:12, nasko wrote:
> Why is this part of content switches? This is not meant to be part of content,
> is it?

Removed.

[nyquist, 2014-01-22 20:06:56]

nasko: PTAL.

Only missing part is removing the last few bits from //content.

[nasko, 2014-01-23 16:37:03]

Just a couple of comments. 
Once we find how to remove the last bits from content/ it should be good to go.

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

https://codereview.chromium.org/105723002/diff/500001/chrome/browser/dom_dist...
chrome/browser/dom_distiller/dom_distiller_viewer_source_browsertest.cc:86:
content::URLDataSource::Add(browser()->profile(),
If you explicitly register the data source, then you aren't testing whether your
flag works or not. Is this intentional?

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

https://codereview.chromium.org/105723002/diff/500001/components/dom_distille...
components/dom_distiller/content/dom_distiller_viewer_source.cc:45: const {
style: const shouldn't be on its own line.

[nyquist, 2014-01-24 03:03:07]

jam: PTAL
nasko: PTAL

Changes since patch set 12:
- Made code for working with new scheme in view-source and URL->URLDataSource
mapping generic (not dependent on the scheme itself).
- Moved scheme declaration and definition from //content to //chrome
- Added TODO for scheme registration
- Removed override of CSP in source (it defaults to what was currently returned)
and fixed MIME-type to text/plain.

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

https://codereview.chromium.org/105723002/diff/500001/chrome/browser/dom_dist...
chrome/browser/dom_distiller/dom_distiller_viewer_source_browsertest.cc:86:
content::URLDataSource::Add(browser()->profile(),
On 2014/01/23 16:37:04, nasko wrote:
> If you explicitly register the data source, then you aren't testing whether
your
> flag works or not. Is this intentional?

We haven't decided where we want to register the data source yet in the normal
code, so currently it is being registered when the service is created. That
doesn't normally happen (without say going to the debug page
chrome://dom-distiller), so temporarily adding it here.

I will add a comment to clean this up when we have it registered in a more
always-called-if-the-flag-is-there way.

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

https://codereview.chromium.org/105723002/diff/500001/components/dom_distille...
components/dom_distiller/content/dom_distiller_viewer_source.cc:45: const {
On 2014/01/23 16:37:04, nasko wrote:
> style: const shouldn't be on its own line.

This is being done by our clang-format I am afraid.

[nasko, 2014-01-24 17:04:56]

Thanks for persisting through all of my comments. This looks much better!
LGTM

[bengr, 2014-01-24 17:10:39]

https://codereview.chromium.org/105723002/diff/1010001/content/browser/browse...
File content/browser/browser_url_handler_impl.cc (right):

https://codereview.chromium.org/105723002/diff/1010001/content/browser/browse...
content/browser/browser_url_handler_impl.cc:46: for
(std::vector<std::string>::const_iterator it =
for (size_t i = 0; i < additional_schemes.size(); ++i) {
  if (url->SchemeIs(additional_schemes[i].c_str()))...

[jam, 2014-01-24 22:07:54]

content lgtm with these changes, thanks

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

https://codereview.chromium.org/105723002/diff/500001/components/dom_distille...
components/dom_distiller/content/dom_distiller_viewer_source.cc:45: const {
On 2014/01/24 03:03:07, nyquist wrote:
> On 2014/01/23 16:37:04, nasko wrote:
> > style: const shouldn't be on its own line.
> 
> This is being done by our clang-format I am afraid.

seems like until clang-format is fixed, this should be fixed manually before
upload

https://codereview.chromium.org/105723002/diff/1290001/components/dom_distill...
File components/dom_distiller/webui/dom_distiller_handler.cc (right):

https://codereview.chromium.org/105723002/diff/1290001/components/dom_distill...
components/dom_distiller/webui/dom_distiller_handler.cc:60: content::Referrer(),
content::PAGE_TRANSITION_GENERATED,
nit: indentation

https://codereview.chromium.org/105723002/diff/1290001/content/browser/browse...
File content/browser/browser_url_handler_impl.cc (right):

https://codereview.chromium.org/105723002/diff/1290001/content/browser/browse...
content/browser/browser_url_handler_impl.cc:44:
GetContentClient()->browser()->GetAdditionalSchemesForViewSource(
actually, now that I think about it some more, it seems reasonable that we call
GetAdditionalWebUISchemes here and then we don't need
GetAdditionalSchemesForViewSource at all. then you can create a vector and stick
allowed_sub_schemes in them and also the result of GetAdditionalWebUISchemes, so
that this small for loop below doesn't need to be duplicated as well

[Nico, 2014-01-24 22:16:34]

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

https://codereview.chromium.org/105723002/diff/500001/components/dom_distille...
components/dom_distiller/content/dom_distiller_viewer_source.cc:45: const {
On 2014/01/24 22:07:54, jam wrote:
> On 2014/01/24 03:03:07, nyquist wrote:
> > On 2014/01/23 16:37:04, nasko wrote:
> > > style: const shouldn't be on its own line.
> > 
> > This is being done by our clang-format I am afraid.
> 
> seems like until clang-format is fixed, this should be fixed manually before
> upload

How'd you format this?

clang-format is used widely google-internally by now, and most aspects of it
have been discussed at length. Most of its formatting discussions are
intentional by now, not by accident. This doesn't look terrible to me fwiw. If
this agrees with google style, do you think it's bad enough that clang-format
should make chromium style deviate from google style in this respect? In
general, I thought we try to keep chromium style relatively similar to google
style. (There are a few differences, but this seems really minor.)

[jam, 2014-01-24 22:29:24]

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

https://codereview.chromium.org/105723002/diff/500001/components/dom_distille...
components/dom_distiller/content/dom_distiller_viewer_source.cc:45: const {
On 2014/01/24 22:16:35, Nico wrote:
> On 2014/01/24 22:07:54, jam wrote:
> > On 2014/01/24 03:03:07, nyquist wrote:
> > > On 2014/01/23 16:37:04, nasko wrote:
> > > > style: const shouldn't be on its own line.
> > > 
> > > This is being done by our clang-format I am afraid.
> > 
> > seems like until clang-format is fixed, this should be fixed manually before
> > upload
> 
> How'd you format this?
> 
> clang-format is used widely google-internally by now, and most aspects of it
> have been discussed at length. Most of its formatting discussions are
> intentional by now, not by accident. This doesn't look terrible to me fwiw. If
> this agrees with google style, do you think it's bad enough that clang-format
> should make chromium style deviate from google style in this respect? In
> general, I thought we try to keep chromium style relatively similar to google
> style. (There are a few differences, but this seems really minor.)

Oh, I was under the impression that clang-format sometimes does things that
don't confirm. I completely agree that we should follow all the parts of the
google style guide everywhere since not doing so is a slippery slope.

I had looked in the google style guide and it didn't say anything about this
particular case. by convention, I know we didn't start a line with "const {" or
"OVERRIDE;" in chrome.

I wonder in cases like this where the google style guide doesn't say one thing
or the other, should we just go with what clang-format says, even if it goes
against the conventions we've used?

[nyquist, 2014-01-25 00:40:38]

https://codereview.chromium.org/105723002/diff/1290001/components/dom_distill...
File components/dom_distiller/webui/dom_distiller_handler.cc (right):

https://codereview.chromium.org/105723002/diff/1290001/components/dom_distill...
components/dom_distiller/webui/dom_distiller_handler.cc:60: content::Referrer(),
content::PAGE_TRANSITION_GENERATED,
On 2014/01/24 22:07:55, jam wrote:
> nit: indentation

Done.

https://codereview.chromium.org/105723002/diff/1290001/content/browser/browse...
File content/browser/browser_url_handler_impl.cc (right):

https://codereview.chromium.org/105723002/diff/1290001/content/browser/browse...
content/browser/browser_url_handler_impl.cc:44:
GetContentClient()->browser()->GetAdditionalSchemesForViewSource(
On 2014/01/24 22:07:55, jam wrote:
> actually, now that I think about it some more, it seems reasonable that we
call
> GetAdditionalWebUISchemes here and then we don't need
> GetAdditionalSchemesForViewSource at all. then you can create a vector and
stick
> allowed_sub_schemes in them and also the result of GetAdditionalWebUISchemes,
so
> that this small for loop below doesn't need to be duplicated as well

Done.

One note: this adds chrome::kChromeSearchScheme to the list of schemes for which
view-source is allowed.

[commit-bot: I haz the power, 2014-01-27 00:24:24]

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

[commit-bot: I haz the power, 2014-01-27 01:53:53]

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

[commit-bot: I haz the power, 2014-01-27 02:08:01]

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

[commit-bot: I haz the power, 2014-01-27 02:59:49]

Change committed as 247187