ContentSettings API should not interact with <webview>

ContentSettings API should not interact with <webview>

The extension contentSettings API (http://developer.chrome.com/extensions/contentSettings.html) was affecting the behavior of <webview> content. This CL isolates <webview> and other <*view> from this API.

BUG=320109
Test=Try an extension that uses the content settings API and verify that <webview> content is not impacted.

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

[markusheintz_, 2013-11-25 18:04:41]

LGTM

https://codereview.chromium.org/78303005/diff/80001/chrome/browser/chrome_con...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/78303005/diff/80001/chrome/browser/chrome_con...
chrome/browser/chrome_content_browser_client.cc:975: &rules);
nit: I guess this fits into the line above.

https://codereview.chromium.org/78303005/diff/80001/chrome/test/data/extensio...
File chrome/test/data/extensions/platform_apps/web_view/shim/main.js (right):

https://codereview.chromium.org/78303005/diff/80001/chrome/test/data/extensio...
chrome/test/data/extensions/platform_apps/web_view/shim/main.js:1172: 'injected
into webview.');
Same as below.

https://codereview.chromium.org/78303005/diff/80001/chrome/test/data/extensio...
chrome/test/data/extensions/platform_apps/web_view/shim/main.js:1209: 'injected
into webview.');
nit: The log string doesn't sound like a correct phrase. Please double check.

[Fady Samuel, 2013-11-25 21:54:01]

+jam@ for chrome/browser*, chrome/renderer/* OWNER approval.

https://codereview.chromium.org/78303005/diff/80001/chrome/browser/chrome_con...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/78303005/diff/80001/chrome/browser/chrome_con...
chrome/browser/chrome_content_browser_client.cc:975: &rules);
On 2013/11/25 18:04:41, markusheintz_ wrote:
> nit: I guess this fits into the line above.

Done.

https://codereview.chromium.org/78303005/diff/80001/chrome/test/data/extensio...
File chrome/test/data/extensions/platform_apps/web_view/shim/main.js (right):

https://codereview.chromium.org/78303005/diff/80001/chrome/test/data/extensio...
chrome/test/data/extensions/platform_apps/web_view/shim/main.js:1172: 'injected
into webview.');
On 2013/11/25 18:04:41, markusheintz_ wrote:
> Same as below.

Done.

https://codereview.chromium.org/78303005/diff/80001/chrome/test/data/extensio...
chrome/test/data/extensions/platform_apps/web_view/shim/main.js:1209: 'injected
into webview.');
On 2013/11/25 18:04:41, markusheintz_ wrote:
> nit: The log string doesn't sound like a correct phrase. Please double check.

Done.

[jam, 2013-11-25 22:42:35]

also, jochen would be a better src\chrome owners since he wrote a lot of the
content settings code

https://codereview.chromium.org/78303005/diff/110001/content/browser/renderer...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://codereview.chromium.org/78303005/diff/110001/content/browser/renderer...
content/browser/renderer_host/render_view_host_impl.cc:1379:
GetContentClient()->browser()->RenderViewReady(this);
this duplicates WebContentsObserver::RenderViewReady, please use that instead of
adding another method to ContentBrowserClient. The latter is a last-resort place
to put embedder callbacks when there isn't context

[jam, 2013-11-25 22:51:53]

https://codereview.chromium.org/78303005/diff/110001/content/browser/renderer...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://codereview.chromium.org/78303005/diff/110001/content/browser/renderer...
content/browser/renderer_host/render_view_host_impl.cc:1379:
GetContentClient()->browser()->RenderViewReady(this);
On 2013/11/25 22:42:35, jam wrote:
> this duplicates WebContentsObserver::RenderViewReady, please use that instead
of
> adding another method to ContentBrowserClient. The latter is a last-resort
place
> to put embedder callbacks when there isn't context 

to expand, you can put this on TabSpecificContentSettings::RenderViewReady

[jochen (gone - plz use gerrit), 2013-11-26 13:51:24]

Isn't a webview a per-process thing anyway?

I'm worried that we're sending a potentially large database of content settings
several times to the same process.

[markusheintz_, 2013-11-26 13:54:37]

On 2013/11/26 13:51:24, jochen wrote:
> Isn't a webview a per-process thing anyway?
> 
> I'm worried that we're sending a potentially large database of content
settings
> several times to the same process.

This CL is about NOT sending content settings to a webview.

See
https://codereview.chromium.org/78303005/diff/110001/chrome/browser/chrome_co...
:950

[jochen (gone - plz use gerrit), 2013-11-26 13:57:39]

On 2013/11/26 13:54:37, markusheintz_ wrote:
> On 2013/11/26 13:51:24, jochen wrote:
> > Isn't a webview a per-process thing anyway?
> > 
> > I'm worried that we're sending a potentially large database of content
> settings
> > several times to the same process.
> 
> This CL is about NOT sending content settings to a webview.

Sorry for being unclear. Why are we switching to sending the content settings
per render view as opposed to once per process. My impression was that a
<webview> can only be hosted in a plugin guest renderer (so it would be enough
to just not send content settings to such a process).

[Fady Samuel, 2013-11-26 15:58:08]

On 2013/11/26 13:57:39, jochen wrote:
> On 2013/11/26 13:54:37, markusheintz_ wrote:
> > On 2013/11/26 13:51:24, jochen wrote:
> > > Isn't a webview a per-process thing anyway?
> > > 
> > > I'm worried that we're sending a potentially large database of content
> > settings
> > > several times to the same process.
> > 
> > This CL is about NOT sending content settings to a webview.
> 
> Sorry for being unclear. Why are we switching to sending the content settings
> per render view as opposed to once per process. My impression was that a
> <webview> can only be hosted in a plugin guest renderer (so it would be enough
> to just not send content settings to such a process).

Multiple <webview>s may host content in a single guest renderer. Generally we
isolate
guest content from one another. Is there any reason why we can't disable images
in one
webview but enable them in another, for example?

[Fady Samuel, 2013-11-26 15:59:01]

On 2013/11/26 15:58:08, Fady Samuel wrote:
> On 2013/11/26 13:57:39, jochen wrote:
> > On 2013/11/26 13:54:37, markusheintz_ wrote:
> > > On 2013/11/26 13:51:24, jochen wrote:
> > > > Isn't a webview a per-process thing anyway?
> > > > 
> > > > I'm worried that we're sending a potentially large database of content
> > > settings
> > > > several times to the same process.
> > > 
> > > This CL is about NOT sending content settings to a webview.
> > 
> > Sorry for being unclear. Why are we switching to sending the content
settings
> > per render view as opposed to once per process. My impression was that a
> > <webview> can only be hosted in a plugin guest renderer (so it would be
enough
> > to just not send content settings to such a process).
> 
> Multiple <webview>s may host content in a single guest renderer. Generally we
> isolate
> guest content from one another. Is there any reason why we can't disable
images
> in one
> webview but enable them in another, for example?


Sorry, I should mention that we may want a content settings API in the future
for <webview>, that's isolated in behavior on a per-webview basis.

[markusheintz_, 2013-11-26 16:08:53]

On 2013/11/26 15:59:01, Fady Samuel wrote:
> On 2013/11/26 15:58:08, Fady Samuel wrote:
> > On 2013/11/26 13:57:39, jochen wrote:
> > > On 2013/11/26 13:54:37, markusheintz_ wrote:
> > > > On 2013/11/26 13:51:24, jochen wrote:
> > > > > Isn't a webview a per-process thing anyway?
> > > > > 
> > > > > I'm worried that we're sending a potentially large database of content
> > > > settings
> > > > > several times to the same process.
> > > > 
> > > > This CL is about NOT sending content settings to a webview.
> > > 
> > > Sorry for being unclear. Why are we switching to sending the content
> settings
> > > per render view as opposed to once per process. My impression was that a
> > > <webview> can only be hosted in a plugin guest renderer (so it would be
> enough
> > > to just not send content settings to such a process).
> > 
> > Multiple <webview>s may host content in a single guest renderer. Generally
we
> > isolate
> > guest content from one another. Is there any reason why we can't disable
> images
> > in one
> > webview but enable them in another, for example?
> 
> 
> Sorry, I should mention that we may want a content settings API in the future
> for <webview>, that's isolated in behavior on a per-webview basis.

Sorry content settings API is very ambiguous for me :)
Is this content settings API meant to be provided to the embedder of the
webview?

[Fady Samuel, 2013-11-26 16:11:53]

On 2013/11/26 16:08:53, markusheintz_ wrote:
> On 2013/11/26 15:59:01, Fady Samuel wrote:
> > On 2013/11/26 15:58:08, Fady Samuel wrote:
> > > On 2013/11/26 13:57:39, jochen wrote:
> > > > On 2013/11/26 13:54:37, markusheintz_ wrote:
> > > > > On 2013/11/26 13:51:24, jochen wrote:
> > > > > > Isn't a webview a per-process thing anyway?
> > > > > > 
> > > > > > I'm worried that we're sending a potentially large database of
content
> > > > > settings
> > > > > > several times to the same process.
> > > > > 
> > > > > This CL is about NOT sending content settings to a webview.
> > > > 
> > > > Sorry for being unclear. Why are we switching to sending the content
> > settings
> > > > per render view as opposed to once per process. My impression was that a
> > > > <webview> can only be hosted in a plugin guest renderer (so it would be
> > enough
> > > > to just not send content settings to such a process).
> > > 
> > > Multiple <webview>s may host content in a single guest renderer. Generally
> we
> > > isolate
> > > guest content from one another. Is there any reason why we can't disable
> > images
> > > in one
> > > webview but enable them in another, for example?
> > 
> > 
> > Sorry, I should mention that we may want a content settings API in the
future
> > for <webview>, that's isolated in behavior on a per-webview basis.
> 
> Sorry content settings API is very ambiguous for me :)
> Is this content settings API meant to be provided to the embedder of the
> webview?

Yes, the embedder would decide the webview's content settings.

[markusheintz_, 2013-11-26 16:23:26]

On 2013/11/26 16:11:53, Fady Samuel wrote:
> On 2013/11/26 16:08:53, markusheintz_ wrote:
> > On 2013/11/26 15:59:01, Fady Samuel wrote:
> > > On 2013/11/26 15:58:08, Fady Samuel wrote:
> > > > On 2013/11/26 13:57:39, jochen wrote:
> > > > > On 2013/11/26 13:54:37, markusheintz_ wrote:
> > > > > > On 2013/11/26 13:51:24, jochen wrote:
> > > > > > > Isn't a webview a per-process thing anyway?
> > > > > > > 
> > > > > > > I'm worried that we're sending a potentially large database of
> content
> > > > > > settings
> > > > > > > several times to the same process.
> > > > > > 
> > > > > > This CL is about NOT sending content settings to a webview.
> > > > > 
> > > > > Sorry for being unclear. Why are we switching to sending the content
> > > settings
> > > > > per render view as opposed to once per process. My impression was that
a
> > > > > <webview> can only be hosted in a plugin guest renderer (so it would
be
> > > enough
> > > > > to just not send content settings to such a process).
> > > > 
> > > > Multiple <webview>s may host content in a single guest renderer.
Generally
> > we
> > > > isolate
> > > > guest content from one another. Is there any reason why we can't disable
> > > images
> > > > in one
> > > > webview but enable them in another, for example?
> > > 
> > > 
> > > Sorry, I should mention that we may want a content settings API in the
> future
> > > for <webview>, that's isolated in behavior on a per-webview basis.
> > 
> > Sorry content settings API is very ambiguous for me :)
> > Is this content settings API meant to be provided to the embedder of the
> > webview?
> 
> Yes, the embedder would decide the webview's content settings.

Great! Could you please share any design docs with me and cc me on any issues
regarding this API.
This would help me to keep track of the various content settings and permissions
parts in Chrome :) Thanks.

[Fady Samuel, 2013-11-26 16:40:54]

PTAL jochen@, I've addressed jam@'s concern. I've removed
ContentBrowserClient::RenderViewReady.

[jam, 2013-11-26 17:14:40]

It sounds like right now content settings don't apply to webview. If I'm
understanding you correctly, you're saying that it might in the future?

If this is so, then as Jochen says why is this switching to be per RV instead of
per process now?

[Fady Samuel, 2013-11-26 19:59:35]

On 2013/11/26 17:14:40, jam wrote:
> It sounds like right now content settings don't apply to webview. If I'm
> understanding you correctly, you're saying that it might in the future?
> 
> If this is so, then as Jochen says why is this switching to be per RV instead
of
> per process now?

I'm currently investigating this path: Never sending content settings to guest
processes.

It seems that swapped out RenderViews are getting content settings applied.

This breaks <webview>.contentWindow as contentWindow is a proxy "swapped out"
RenderView to the guest RenderView.

I'm investigating why this is happening now.

[Fady Samuel, 2013-11-26 22:53:17]

PTAL jochen@. I've simplified this patch greatly. Chrome App processes, and
<webview> guest processes do not get content settings injected now.

[jochen (gone - plz use gerrit), 2013-11-27 14:29:29]

I wonder whether it isn't enough to add kSwappedOutScheme to
ContentSettingsObserver::IsWhitelistedForContentSettings?

[Fady Samuel, 2013-11-27 14:34:14]

On 2013/11/27 14:29:29, jochen wrote:
> I wonder whether it isn't enough to add kSwappedOutScheme to
> ContentSettingsObserver::IsWhitelistedForContentSettings?

I spoke to Charlie Reis about that and he was hesitant about that approach.
Adding him to the review now.

[Charlie Reis, 2013-11-27 17:35:54]

https://codereview.chromium.org/78303005/diff/170001/chrome/browser/chrome_co...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/78303005/diff/170001/chrome/browser/chrome_co...
chrome/browser/chrome_content_browser_client.cc:1277: (!extension ||
!extension->is_platform_app());
This condition is really hard to read.  Please pull the ! out of the parentheses
to make it clearer that you mean "not a platform app".  Also worth adding a
comment saying "Don't inject content setting rules into guests and platform
apps."

I admit that I'm not very familiar with content settings.  Is it intentional to
never inject them into platform apps?  I think you mentioned that we used to
inject them but they were ignored on the renderer side?  Maybe it's worth
mentioning that in the comment.

https://codereview.chromium.org/78303005/diff/170001/chrome/browser/chrome_co...
chrome/browser/chrome_content_browser_client.cc:1283: new
ChromeViewMsg_SetContentSettingRules(rules));
SiteInstanceGotProcess can be called multiple times for the same
RenderProcessHost (e.g., if we're sharing a process), while RenderProcessCreated
is only called once per RenderProcessHost.  Is that ok for this message?

https://codereview.chromium.org/78303005/diff/170001/chrome/renderer/content_...
File chrome/renderer/content_settings_observer.cc (left):

https://codereview.chromium.org/78303005/diff/170001/chrome/renderer/content_...
chrome/renderer/content_settings_observer.cc:64: NOTREACHED();
Why is this changing?

[jochen (gone - plz use gerrit), 2013-11-27 17:55:41]

Charlie, what do you think about just whitelisting swappedout://?

That's what we do for everything else, so I'd rather extend that mechanism
instead of introducing a new special casing

[jochen (gone - plz use gerrit), 2013-11-27 17:55:43]

Charlie, what do you think about just whitelisting swappedout://?

That's what we do for everything else, so I'd rather extend that mechanism
instead of introducing a new special casing

[Charlie Reis, 2013-11-27 18:11:32]

On 2013/11/27 17:55:43, jochen wrote:
> Charlie, what do you think about just whitelisting swappedout://?
> 
> That's what we do for everything else, so I'd rather extend that mechanism
> instead of introducing a new special casing

I don't think swappedout:// should be visible outside the content module. 
(Seems like a mistake to have it public in url_constants.h.) It's also going
away as soon as we can get RenderFrameProxies implemented.

If there's a way to do it within the content module, I might be open to the
idea.  I'm not sure of the specifics of why most app RenderViews don't get
content settings but swappedout app RenderViews do.

[jochen (gone - plz use gerrit), 2013-11-27 20:11:20]

On 2013/11/27 18:11:32, creis wrote:
> On 2013/11/27 17:55:43, jochen wrote:
> > Charlie, what do you think about just whitelisting swappedout://?
> > 
> > That's what we do for everything else, so I'd rather extend that mechanism
> > instead of introducing a new special casing
> 
> I don't think swappedout:// should be visible outside the content module. 
> (Seems like a mistake to have it public in url_constants.h.) It's also going
> away as soon as we can get RenderFrameProxies implemented.

We'll still need a way for the embedder to decide whether or not to initialize
javascript in this frame or not.

Since the embedder sees the URL, I don't really see the point in keeping it
secret. I think it's similar to kUnreachableWebData which we also expose (and
whitelist).

> 
> If there's a way to do it within the content module, I might be open to the
> idea.  I'm not sure of the specifics of why most app RenderViews don't get
> content settings but swappedout app RenderViews do.

The whitelisting is done here
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/co....
The app is whitelisted based on the main frame URL being on a
chrome-extension:// scheme.

We could add a getter to RenderView something like is_proxy() and use that to
whitelist?

[jochen (gone - plz use gerrit), 2013-11-27 21:27:02]

So after some back and forth, what about this:

we add a content API RenderView::IsBrowserPluginProxy() or similar that returns
true for a swapped out RV that forwards to a <webview>

There should be a comment that makes it clear why it's needed and that it should
be removed once we don't use swappedout:// anymore but RenderFrameProxies.

This API can be used in ContentSettingsObserver::IsWhitelistedForContentSettings
to whitelist those RVs.

wdyt?

[Charlie Reis, 2013-11-27 21:46:22]

On 2013/11/27 21:27:02, jochen wrote:
> So after some back and forth, what about this:
> 
> we add a content API RenderView::IsBrowserPluginProxy() or similar that
returns
> true for a swapped out RV that forwards to a <webview>
> 
> There should be a comment that makes it clear why it's needed and that it
should
> be removed once we don't use swappedout:// anymore but RenderFrameProxies.
> 
> This API can be used in
ContentSettingsObserver::IsWhitelistedForContentSettings
> to whitelist those RVs.
> 
> wdyt?

I'm not sure I see a better approach, so I'm ok with it.  This will make sure
that scripting works in the embedder's swapped out RenderView, whether the
embedder is an app or not.

Please do make it clear why this is needed, and that it's a temporary measure
until swapped out RenderViews go away.  (In general, the concept of a RenderView
"proxy" shouldn't be exposed outside content.)

[jam, 2013-11-27 22:05:56]

On 2013/11/27 21:46:22, creis wrote:
> On 2013/11/27 21:27:02, jochen wrote:
> > So after some back and forth, what about this:
> > 
> > we add a content API RenderView::IsBrowserPluginProxy() or similar that
> returns
> > true for a swapped out RV that forwards to a <webview>
> > 
> > There should be a comment that makes it clear why it's needed and that it
> should
> > be removed once we don't use swappedout:// anymore but RenderFrameProxies.
> > 
> > This API can be used in
> ContentSettingsObserver::IsWhitelistedForContentSettings
> > to whitelist those RVs.
> > 
> > wdyt?
> 
> I'm not sure I see a better approach, so I'm ok with it.  This will make sure
> that scripting works in the embedder's swapped out RenderView, whether the
> embedder is an app or not.
> 
> Please do make it clear why this is needed, and that it's a temporary measure
> until swapped out RenderViews go away.  (In general, the concept of a
RenderView
> "proxy" shouldn't be exposed outside content.)

In general, we only add Content API because there's no other way to do
something.

In this case, patchset 8 seems to accomplish this task without requiring new
APIs. Why add a new one?

[jochen (gone - plz use gerrit), 2013-11-28 15:52:44]

On 2013/11/27 22:05:56, jam wrote:
> On 2013/11/27 21:46:22, creis wrote:
> > On 2013/11/27 21:27:02, jochen wrote:
> > > So after some back and forth, what about this:
> > > 
> > > we add a content API RenderView::IsBrowserPluginProxy() or similar that
> > returns
> > > true for a swapped out RV that forwards to a <webview>
> > > 
> > > There should be a comment that makes it clear why it's needed and that it
> > should
> > > be removed once we don't use swappedout:// anymore but RenderFrameProxies.
> > > 
> > > This API can be used in
> > ContentSettingsObserver::IsWhitelistedForContentSettings
> > > to whitelist those RVs.
> > > 
> > > wdyt?
> > 
> > I'm not sure I see a better approach, so I'm ok with it.  This will make
sure
> > that scripting works in the embedder's swapped out RenderView, whether the
> > embedder is an app or not.
> > 
> > Please do make it clear why this is needed, and that it's a temporary
measure
> > until swapped out RenderViews go away.  (In general, the concept of a
> RenderView
> > "proxy" shouldn't be exposed outside content.)
> 
> In general, we only add Content API because there's no other way to do
> something.
> 
> In this case, patchset 8 seems to accomplish this task without requiring new
> APIs. Why add a new one?

No, patchset #8 doesn't solve the problem (the swapped out render view doesn't
get the right settings in all cases), but it entirely breaks content settings
for platform apps (returning DEFAULT is incorrect, we need to at least transfer
the default settings in any case).

The most correct solution would be to implement a WebPermissionClient inside of
content that whitelists all the special URL content knows about (for example
kUnreachableWebDataURL) and only forwards the rest to chrome.

This is however a rather large change and will require an API change as well.

[jochen (gone - plz use gerrit), 2013-11-28 22:23:29]

I added some specific comments

https://codereview.chromium.org/78303005/diff/170001/chrome/browser/chrome_co...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/78303005/diff/170001/chrome/browser/chrome_co...
chrome/browser/chrome_content_browser_client.cc:1276: bool inject_content_rules
= !site_instance->GetProcess()->IsGuest() &&
from the description, the bug happens in platform app processes, so this
shouldn't be needed

https://codereview.chromium.org/78303005/diff/170001/chrome/browser/chrome_co...
chrome/browser/chrome_content_browser_client.cc:1277: (!extension ||
!extension->is_platform_app());
what happens for other processes that contain a browser plugin?

https://codereview.chromium.org/78303005/diff/170001/chrome/renderer/content_...
File chrome/renderer/content_settings_observer.cc (right):

https://codereview.chromium.org/78303005/diff/170001/chrome/renderer/content_...
chrome/renderer/content_settings_observer.cc:64: return CONTENT_SETTING_DEFAULT;
DEFAULT is just a placeholder value, we should never return it

We need at least the default rules in each process

[jam, 2013-12-02 17:22:10]

On 2013/11/28 15:52:44, jochen wrote:
> On 2013/11/27 22:05:56, jam wrote:
> > On 2013/11/27 21:46:22, creis wrote:
> > > On 2013/11/27 21:27:02, jochen wrote:
> > > > So after some back and forth, what about this:
> > > > 
> > > > we add a content API RenderView::IsBrowserPluginProxy() or similar that
> > > returns
> > > > true for a swapped out RV that forwards to a <webview>
> > > > 
> > > > There should be a comment that makes it clear why it's needed and that
it
> > > should
> > > > be removed once we don't use swappedout:// anymore but
RenderFrameProxies.
> > > > 
> > > > This API can be used in
> > > ContentSettingsObserver::IsWhitelistedForContentSettings
> > > > to whitelist those RVs.
> > > > 
> > > > wdyt?
> > > 
> > > I'm not sure I see a better approach, so I'm ok with it.  This will make
> sure
> > > that scripting works in the embedder's swapped out RenderView, whether the
> > > embedder is an app or not.
> > > 
> > > Please do make it clear why this is needed, and that it's a temporary
> measure
> > > until swapped out RenderViews go away.  (In general, the concept of a
> > RenderView
> > > "proxy" shouldn't be exposed outside content.)
> > 
> > In general, we only add Content API because there's no other way to do
> > something.
> > 
> > In this case, patchset 8 seems to accomplish this task without requiring new
> > APIs. Why add a new one?
> 
> No, patchset #8 doesn't solve the problem (the swapped out render view doesn't
> get the right settings in all cases), but it entirely breaks content settings
> for platform apps (returning DEFAULT is incorrect, we need to at least
transfer
> the default settings in any case).
> 
> The most correct solution would be to implement a WebPermissionClient inside
of
> content that whitelists all the special URL content knows about (for example
> kUnreachableWebDataURL) and only forwards the rest to chrome.

WebPermissionClient was created so that chrome could handle content setting
specific tasks directly from blink, without having to go through content layer.
Can we make chrome handle this itself, for example by looking only at specific
schemes that it knows about?

I don't understand your second last paragraph (as I'm not that familiar with
content settings). what does transfer default settings mean?

> 
> This is however a rather large change and will require an API change as well.

[jochen (gone - plz use gerrit), 2013-12-02 18:05:48]

On 2013/12/02 17:22:10, jam wrote:
> On 2013/11/28 15:52:44, jochen wrote:
> > On 2013/11/27 22:05:56, jam wrote:
> > > On 2013/11/27 21:46:22, creis wrote:
> > > > On 2013/11/27 21:27:02, jochen wrote:
> > > > > So after some back and forth, what about this:
> > > > > 
> > > > > we add a content API RenderView::IsBrowserPluginProxy() or similar
that
> > > > returns
> > > > > true for a swapped out RV that forwards to a <webview>
> > > > > 
> > > > > There should be a comment that makes it clear why it's needed and that
> it
> > > > should
> > > > > be removed once we don't use swappedout:// anymore but
> RenderFrameProxies.
> > > > > 
> > > > > This API can be used in
> > > > ContentSettingsObserver::IsWhitelistedForContentSettings
> > > > > to whitelist those RVs.
> > > > > 
> > > > > wdyt?
> > > > 
> > > > I'm not sure I see a better approach, so I'm ok with it.  This will make
> > sure
> > > > that scripting works in the embedder's swapped out RenderView, whether
the
> > > > embedder is an app or not.
> > > > 
> > > > Please do make it clear why this is needed, and that it's a temporary
> > measure
> > > > until swapped out RenderViews go away.  (In general, the concept of a
> > > RenderView
> > > > "proxy" shouldn't be exposed outside content.)
> > > 
> > > In general, we only add Content API because there's no other way to do
> > > something.
> > > 
> > > In this case, patchset 8 seems to accomplish this task without requiring
new
> > > APIs. Why add a new one?
> > 
> > No, patchset #8 doesn't solve the problem (the swapped out render view
doesn't
> > get the right settings in all cases), but it entirely breaks content
settings
> > for platform apps (returning DEFAULT is incorrect, we need to at least
> transfer
> > the default settings in any case).
> > 
> > The most correct solution would be to implement a WebPermissionClient inside
> of
> > content that whitelists all the special URL content knows about (for example
> > kUnreachableWebDataURL) and only forwards the rest to chrome.
> 
> WebPermissionClient was created so that chrome could handle content setting
> specific tasks directly from blink, without having to go through content
layer.
> Can we make chrome handle this itself, for example by looking only at specific
> schemes that it knows about?

That's possible, however, less secure: if we add a scheme in chrome and forget
about updating the content settings observer, we would grant privileges to sites
the user hasn't configured.


> 
> I don't understand your second last paragraph (as I'm not that familiar with
> content settings). what does transfer default settings mean?

We present content settings to users as a default choice + exceptions.

Internally, there are just exceptions with the default setting being mapped to a
catch-all rule.

We need to transfer at least the catch-all rule (which is applied to unknown
schemes as well, so we can't forget about a scheme)


> 
> > 
> > This is however a rather large change and will require an API change as
well.

[Fady Samuel, 2013-12-03 00:24:04]

Hi Jochen,

I believe I've addressed all comments. Please take a look at
GuestView::GetDefaultContentSettingRules. I'm not entirely sure if I got the
default content setting rules correctly here. Thanks!

[jochen (gone - plz use gerrit), 2013-12-03 08:55:20]

mostly looks good

https://codereview.chromium.org/78303005/diff/190001/chrome/browser/chrome_co...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/78303005/diff/190001/chrome/browser/chrome_co...
chrome/browser/chrome_content_browser_client.cc:1276:
RendererContentSettingRules rules;
can we keep this block in RenderProcessHostCreated?

https://codereview.chromium.org/78303005/diff/190001/chrome/browser/guestview...
File chrome/browser/guestview/guestview.cc (right):

https://codereview.chromium.org/78303005/diff/190001/chrome/browser/guestview...
chrome/browser/guestview/guestview.cc:131: false));
please add incongito as parameter instead of creating the rules twice

https://codereview.chromium.org/78303005/diff/190001/chrome/renderer/content_...
File chrome/renderer/content_settings_observer.cc (right):

https://codereview.chromium.org/78303005/diff/190001/chrome/renderer/content_...
chrome/renderer/content_settings_observer.cc:369: if
(EqualsASCII(origin.protocol(), GURL(content::kSwappedOutURL).scheme())) {
nit. no { }

https://codereview.chromium.org/78303005/diff/190001/content/browser/renderer...
File content/browser/renderer_host/render_view_host_delegate.h (right):

https://codereview.chromium.org/78303005/diff/190001/content/browser/renderer...
content/browser/renderer_host/render_view_host_delegate.h:456: virtual bool
IsGuest() const;
why is this on RVHD? I would expect this on RPH?

[Fady Samuel, 2013-12-03 15:31:26]

PTAL Jochen.

https://codereview.chromium.org/78303005/diff/190001/chrome/browser/chrome_co...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/78303005/diff/190001/chrome/browser/chrome_co...
chrome/browser/chrome_content_browser_client.cc:1276:
RendererContentSettingRules rules;
On 2013/12/03 08:55:21, jochen wrote:
> can we keep this block in RenderProcessHostCreated?

Done.

https://codereview.chromium.org/78303005/diff/190001/chrome/browser/guestview...
File chrome/browser/guestview/guestview.cc (right):

https://codereview.chromium.org/78303005/diff/190001/chrome/browser/guestview...
chrome/browser/guestview/guestview.cc:131: false));
On 2013/12/03 08:55:21, jochen wrote:
> please add incongito as parameter instead of creating the rules twice

Done.

https://codereview.chromium.org/78303005/diff/190001/chrome/renderer/content_...
File chrome/renderer/content_settings_observer.cc (right):

https://codereview.chromium.org/78303005/diff/190001/chrome/renderer/content_...
chrome/renderer/content_settings_observer.cc:369: if
(EqualsASCII(origin.protocol(), GURL(content::kSwappedOutURL).scheme())) {
On 2013/12/03 08:55:21, jochen wrote:
> nit. no { }

Done.

https://codereview.chromium.org/78303005/diff/190001/content/browser/renderer...
File content/browser/renderer_host/render_view_host_delegate.h (right):

https://codereview.chromium.org/78303005/diff/190001/content/browser/renderer...
content/browser/renderer_host/render_view_host_delegate.h:456: virtual bool
IsGuest() const;
On 2013/12/03 08:55:21, jochen wrote:
> why is this on RVHD? I would expect this on RPH?

Oops, this is unnecessary code I added while trying different solutions and I
forgot to revert it. Thanks.

[jochen (gone - plz use gerrit), 2013-12-03 15:35:06]

lgtm from my side with comments addressed

https://codereview.chromium.org/78303005/diff/210001/chrome/browser/chrome_co...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/78303005/diff/210001/chrome/browser/chrome_co...
chrome/browser/chrome_content_browser_client.cc:947:
GuestView::GetDefaultContentSettingRules(&rules, false /* incognito */);
this should be just profile->IsOffTheRecord();

https://codereview.chromium.org/78303005/diff/210001/chrome/renderer/content_...
File chrome/renderer/content_settings_observer.cc (right):

https://codereview.chromium.org/78303005/diff/210001/chrome/renderer/content_...
chrome/renderer/content_settings_observer.cc:369: if
(EqualsASCII(origin.protocol(), GURL(content::kSwappedOutURL).scheme()))
doesn't document_url == GURL(content::kSwappedOutURL) work?

[Fady Samuel, 2013-12-03 15:56:26]

Charlie, are you OK with this change as a short term solution until swapped out
RenderViews go away? I've added a TODO.

https://codereview.chromium.org/78303005/diff/210001/chrome/browser/chrome_co...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/78303005/diff/210001/chrome/browser/chrome_co...
chrome/browser/chrome_content_browser_client.cc:947:
GuestView::GetDefaultContentSettingRules(&rules, false /* incognito */);
On 2013/12/03 15:35:07, jochen wrote:
> this should be just profile->IsOffTheRecord();

Done.

https://codereview.chromium.org/78303005/diff/210001/chrome/renderer/content_...
File chrome/renderer/content_settings_observer.cc (right):

https://codereview.chromium.org/78303005/diff/210001/chrome/renderer/content_...
chrome/renderer/content_settings_observer.cc:369: if
(EqualsASCII(origin.protocol(), GURL(content::kSwappedOutURL).scheme()))
On 2013/12/03 15:35:07, jochen wrote:
> doesn't document_url == GURL(content::kSwappedOutURL) work?

It does. Changed.

[Charlie Reis, 2013-12-03 18:36:04]

Ok, I suppose we can expose swappedout:// until we get RenderFrameProxy in
place.  LGTM.

https://codereview.chromium.org/78303005/diff/230001/chrome/browser/chrome_co...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/78303005/diff/230001/chrome/browser/chrome_co...
chrome/browser/chrome_content_browser_client.cc:1284: 
nit: No need for churn here.

[Fady Samuel, 2013-12-03 18:50:55]

I've addressed Charlie's comment. John, since you looked at this CL earlier,
could you please sign off on this? I believe all comments have been addressed.
Thanks!

https://codereview.chromium.org/78303005/diff/230001/chrome/browser/chrome_co...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/78303005/diff/230001/chrome/browser/chrome_co...
chrome/browser/chrome_content_browser_client.cc:1284: 
On 2013/12/03 18:36:04, creis wrote:
> nit: No need for churn here.

Done.

[jam, 2013-12-03 18:57:19]

lgtm

[commit-bot: I haz the power, 2013-12-03 18:58:01]

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

[commit-bot: I haz the power, 2013-12-03 19:11:49]

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

[commit-bot: I haz the power, 2013-12-03 23:38:14]

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

[commit-bot: I haz the power, 2013-12-04 03:01:30]

Change committed as 238534