Take script URLs into account when applying script content settings.

Take script URLs into account when applying script content settings.

Transmit script content settings to the renderer. Use the script URL as the secondary URL
for the content setting rules.

BUG=90840
TEST=ChromeRenderViewTest.ContentSettings(Allow|Block)Scripts


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=109005

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=109036

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=109213

[marja, 2011-11-02 09:16:01]

Hi Jochen, this is the CL I was talking about.

[jochen (gone - plz use gerrit), 2011-11-02 09:25:05]

http://codereview.chromium.org/8409006/diff/8001/chrome/renderer/chrome_rende...
File chrome/renderer/chrome_render_view_observer.cc (right):

http://codereview.chromium.org/8409006/diff/8001/chrome/renderer/chrome_rende...
chrome/renderer/chrome_render_view_observer.cc:420: WebFrame* frame, bool
enabled_per_settings,
nit. each parameter on its own line

http://codereview.chromium.org/8409006/diff/8001/chrome/renderer/content_sett...
File chrome/renderer/content_settings_observer.cc (right):

http://codereview.chromium.org/8409006/diff/8001/chrome/renderer/content_sett...
chrome/renderer/content_settings_observer.cc:266:
AllowContentType(CONTENT_SETTINGS_TYPE_JAVASCRIPT)) {
can you also update this to take frame->top and frame into account?

[Bernhard Bauer, 2011-11-02 09:57:14]

http://codereview.chromium.org/8409006/diff/8001/chrome/renderer/chrome_rende...
File chrome/renderer/chrome_render_process_observer.h (right):

http://codereview.chromium.org/8409006/diff/8001/chrome/renderer/chrome_rende...
chrome/renderer/chrome_render_process_observer.h:79: ContentSettingsForOneType
content_setting_rules_[CONTENT_SETTINGS_NUM_TYPES];
I actually want to get rid of indexing content settings by their type, as it
creates overhead for adding more content types. We should have a struct which
contains only content settings pushed to the renderer (similar to
RendererPreferences), and maybe we could merge the content setting rules into
that struct?

http://codereview.chromium.org/8409006/diff/8001/chrome/renderer/content_sett...
File chrome/renderer/content_settings_observer.cc (right):

http://codereview.chromium.org/8409006/diff/8001/chrome/renderer/content_sett...
chrome/renderer/content_settings_observer.cc:266:
AllowContentType(CONTENT_SETTINGS_TYPE_JAVASCRIPT)) {
On 2011/11/02 09:25:05, jochen wrote:
> can you also update this to take frame->top and frame into account?

How are they going to interact?

[jochen (gone - plz use gerrit), 2011-11-02 10:40:48]

http://codereview.chromium.org/8409006/diff/8001/chrome/renderer/content_sett...
File chrome/renderer/content_settings_observer.cc (right):

http://codereview.chromium.org/8409006/diff/8001/chrome/renderer/content_sett...
chrome/renderer/content_settings_observer.cc:266:
AllowContentType(CONTENT_SETTINGS_TYPE_JAVASCRIPT)) {
On 2011/11/02 09:57:14, Bernhard Bauer wrote:
> On 2011/11/02 09:25:05, jochen wrote:
> > can you also update this to take frame->top and frame into account?
> 
> How are they going to interact?


allowScripts checks whether a given security origin is allowed to execute
scripts, ...FromSource controls whether a script is loaded from the network

[Bernhard Bauer, 2011-11-02 10:51:18]

On 2011/11/02 10:40:48, jochen wrote:
>
http://codereview.chromium.org/8409006/diff/8001/chrome/renderer/content_sett...
> File chrome/renderer/content_settings_observer.cc (right):
> 
>
http://codereview.chromium.org/8409006/diff/8001/chrome/renderer/content_sett...
> chrome/renderer/content_settings_observer.cc:266:
> AllowContentType(CONTENT_SETTINGS_TYPE_JAVASCRIPT)) {
> On 2011/11/02 09:57:14, Bernhard Bauer wrote:
> > On 2011/11/02 09:25:05, jochen wrote:
> > > can you also update this to take frame->top and frame into account?
> > 
> > How are they going to interact?
> 
> 
> allowScripts checks whether a given security origin is allowed to execute
> scripts, ...FromSource controls whether a script is loaded from the network

I'm not happy that this overloads the meaning of the secondary URL for
Javascript, but I can't think of an example right now where this would really be
a problem. Let's just go with it and see if trouble comes up.

[marja, 2011-11-02 14:46:35]

Thanks for comments!

http://codereview.chromium.org/8409006/diff/8001/chrome/renderer/chrome_rende...
File chrome/renderer/chrome_render_process_observer.h (right):

http://codereview.chromium.org/8409006/diff/8001/chrome/renderer/chrome_rende...
chrome/renderer/chrome_render_process_observer.h:79: ContentSettingsForOneType
content_setting_rules_[CONTENT_SETTINGS_NUM_TYPES];
On 2011/11/02 09:57:14, Bernhard Bauer wrote:
> I actually want to get rid of indexing content settings by their type, as it
> creates overhead for adding more content types. We should have a struct which
> contains only content settings pushed to the renderer (similar to
> RendererPreferences), and maybe we could merge the content setting rules into
> that struct?

Done: Added struct RendererContentSettingRules.

http://codereview.chromium.org/8409006/diff/8001/chrome/renderer/chrome_rende...
File chrome/renderer/chrome_render_view_observer.cc (right):

http://codereview.chromium.org/8409006/diff/8001/chrome/renderer/chrome_rende...
chrome/renderer/chrome_render_view_observer.cc:420: WebFrame* frame, bool
enabled_per_settings,
On 2011/11/02 09:25:05, jochen wrote:
> nit. each parameter on its own line

Done.

http://codereview.chromium.org/8409006/diff/8001/chrome/renderer/content_sett...
File chrome/renderer/content_settings_observer.cc (right):

http://codereview.chromium.org/8409006/diff/8001/chrome/renderer/content_sett...
chrome/renderer/content_settings_observer.cc:266:
AllowContentType(CONTENT_SETTINGS_TYPE_JAVASCRIPT)) {
On 2011/11/02 09:25:05, jochen wrote:
> can you also update this to take frame->top and frame into account?

Using the top document's security origin as primary URL and the document's
security origin as secondary URL, right?

[jochen (gone - plz use gerrit), 2011-11-02 19:19:00]

http://codereview.chromium.org/8409006/diff/8001/chrome/renderer/content_sett...
File chrome/renderer/content_settings_observer.cc (right):

http://codereview.chromium.org/8409006/diff/8001/chrome/renderer/content_sett...
chrome/renderer/content_settings_observer.cc:266:
AllowContentType(CONTENT_SETTINGS_TYPE_JAVASCRIPT)) {
right

[marja, 2011-11-02 22:16:01]

http://codereview.chromium.org/8409006/diff/8001/chrome/renderer/content_sett...
File chrome/renderer/content_settings_observer.cc (right):

http://codereview.chromium.org/8409006/diff/8001/chrome/renderer/content_sett...
chrome/renderer/content_settings_observer.cc:266:
AllowContentType(CONTENT_SETTINGS_TYPE_JAVASCRIPT)) {
On 2011/11/02 19:19:00, jochen wrote:
> right

Done, in that case.

[jochen (gone - plz use gerrit), 2011-11-03 15:55:09]

http://codereview.chromium.org/8409006/diff/14001/chrome/renderer/content_set...
File chrome/renderer/content_settings_observer.cc (right):

http://codereview.chromium.org/8409006/diff/14001/chrome/renderer/content_set...
chrome/renderer/content_settings_observer.cc:73: return CONTENT_SETTING_DEFAULT;
what if the default is to block all images?

http://codereview.chromium.org/8409006/diff/14001/chrome/renderer/content_set...
chrome/renderer/content_settings_observer.cc:236: enabled_per_settings) {
nit. doesn't this fit into one line?

[marja, 2011-11-03 16:28:39]

http://codereview.chromium.org/8409006/diff/14001/chrome/renderer/content_set...
File chrome/renderer/content_settings_observer.cc (right):

http://codereview.chromium.org/8409006/diff/14001/chrome/renderer/content_set...
chrome/renderer/content_settings_observer.cc:73: return CONTENT_SETTING_DEFAULT;
On 2011/11/03 15:55:09, jochen wrote:
> what if the default is to block all images?

Ahh, I should've put NOTREACHED() here but I forgot. The last rule is supposed
the be the default rule (*, *). Added the NOTREACHED().

http://codereview.chromium.org/8409006/diff/14001/chrome/renderer/content_set...
chrome/renderer/content_settings_observer.cc:236: enabled_per_settings) {
On 2011/11/03 15:55:09, jochen wrote:
> nit. doesn't this fit into one line?

Done.

[jochen (gone - plz use gerrit), 2011-11-03 16:31:15]

lgtm assuming trybot happiness

[Bernhard Bauer, 2011-11-03 22:18:38]

http://codereview.chromium.org/8409006/diff/12025/chrome/browser/content_sett...
File chrome/browser/content_settings/content_settings_utils.cc (right):

http://codereview.chromium.org/8409006/diff/12025/chrome/browser/content_sett...
chrome/browser/content_settings/content_settings_utils.cc:206: sender->Send(new
ChromeViewMsg_SetContentSettingRules(rules));
Could you change this method so that it populates a RendererContentSettingRules
struct from the HostContentSettingsMap, and have the |Send| method in clients
instead?

http://codereview.chromium.org/8409006/diff/12025/chrome/renderer/content_set...
File chrome/renderer/content_settings_observer.cc (right):

http://codereview.chromium.org/8409006/diff/12025/chrome/renderer/content_set...
chrome/renderer/content_settings_observer.cc:239: allow =
(GetContentSettingFromRules(
Nit: opening parenthesis is unnecessary.

[marja, 2011-11-07 09:32:48]

http://codereview.chromium.org/8409006/diff/12025/chrome/browser/content_sett...
File chrome/browser/content_settings/content_settings_utils.cc (right):

http://codereview.chromium.org/8409006/diff/12025/chrome/browser/content_sett...
chrome/browser/content_settings/content_settings_utils.cc:206: sender->Send(new
ChromeViewMsg_SetContentSettingRules(rules));
On 2011/11/03 22:18:38, Bernhard Bauer wrote:
> Could you change this method so that it populates a
RendererContentSettingRules
> struct from the HostContentSettingsMap, and have the |Send| method in clients
> instead?

Done.

http://codereview.chromium.org/8409006/diff/12025/chrome/renderer/content_set...
File chrome/renderer/content_settings_observer.cc (right):

http://codereview.chromium.org/8409006/diff/12025/chrome/renderer/content_set...
chrome/renderer/content_settings_observer.cc:239: allow =
(GetContentSettingFromRules(
On 2011/11/03 22:18:38, Bernhard Bauer wrote:
> Nit: opening parenthesis is unnecessary.

Done.

[Bernhard Bauer, 2011-11-08 09:32:52]

LGTM, with an additional cleanup suggestion:

http://codereview.chromium.org/8409006/diff/24001/chrome/renderer/content_set...
File chrome/renderer/content_settings_observer.h (right):

http://codereview.chromium.org/8409006/diff/24001/chrome/renderer/content_set...
chrome/renderer/content_settings_observer.h:104: // Stores if loading of scripts
and plugins is allowed.
This comment is not accurate anymore. In fact, I think the whole ContentSettings
struct isn't needed anymore! :) You can remove it in another CL though if you
want.

[marja, 2011-11-08 09:39:35]

Thanks for the review!

http://codereview.chromium.org/8409006/diff/24001/chrome/renderer/content_set...
File chrome/renderer/content_settings_observer.h (right):

http://codereview.chromium.org/8409006/diff/24001/chrome/renderer/content_set...
chrome/renderer/content_settings_observer.h:104: // Stores if loading of scripts
and plugins is allowed.
On 2011/11/08 09:32:52, Bernhard Bauer wrote:
> This comment is not accurate anymore. In fact, I think the whole
ContentSettings
> struct isn't needed anymore! :) You can remove it in another CL though if you
> want.

Ahh, true, I'll create another CL for removing this and other unnecessary things
from ContentSettingsObserver.

[commit-bot: I haz the power, 2011-11-08 09:39:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/marja@chromium.org/8409006/24001

[commit-bot: I haz the power, 2011-11-08 10:49:52]

Change committed as 109005

[commit-bot: I haz the power, 2011-11-08 15:03:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/marja@chromium.org/8409006/24001

[commit-bot: I haz the power, 2011-11-08 16:09:24]

Change committed as 109036

[commit-bot: I haz the power, 2011-11-09 09:07:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/marja@chromium.org/8409006/24001

[commit-bot: I haz the power, 2011-11-09 10:02:47]

Change committed as 109213