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. 

This CL contains the functionality of the following 2 CLs, hopefully without performance
regressions:
http://codereview.chromium.org/8409006
http://codereview.chromium.org/8498007

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


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

[marja, 2011-11-11 12:30:09]

And this is the CL I e-mailed about.

[Bernhard Bauer, 2011-11-11 13:00:52]

It would be helpful to have a comparison of the old CL (i.e.
http://codereview.chromium.org/8409006 + http://codereview.chromium.org/8498007)
vs. this one. Could you upload the old version as a patch set?

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

http://codereview.chromium.org/8538004/diff/1001/chrome/renderer/content_sett...
chrome/renderer/content_settings_observer.cc:70: return rules[0].setting;
Can we have a DCHECK here that this rule applies?

http://codereview.chromium.org/8538004/diff/1001/chrome/renderer/content_sett...
chrome/renderer/content_settings_observer.cc:231: if (rules.size() == 1) {
Can you use |GetContentSettingFromRules| here?

[marja, 2011-11-11 13:13:56]

On 2011/11/11 13:00:52, Bernhard Bauer wrote:
> It would be helpful to have a comparison of the old CL (i.e.
> http://codereview.chromium.org/8409006 +
http://codereview.chromium.org/8498007)
> vs. this one. Could you upload the old version as a patch set?

Patch set 3.

[marja, 2011-11-11 14:25:09]

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

http://codereview.chromium.org/8538004/diff/1001/chrome/renderer/content_sett...
chrome/renderer/content_settings_observer.cc:70: return rules[0].setting;
On 2011/11/11 13:00:53, Bernhard Bauer wrote:
> Can we have a DCHECK here that this rule applies?

Done. (Checked that it's a wildcard-wildcard rule.)

http://codereview.chromium.org/8538004/diff/1001/chrome/renderer/content_sett...
chrome/renderer/content_settings_observer.cc:231: if (rules.size() == 1) {
On 2011/11/11 13:00:53, Bernhard Bauer wrote:
> Can you use |GetContentSettingFromRules| here?

Done. (Seems to not kill the perf if I modify the func to take a WebFrame*.)

[Bernhard Bauer, 2011-11-15 09:29:35]

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

http://codereview.chromium.org/8538004/diff/14001/chrome/renderer/content_set...
chrome/renderer/content_settings_observer.cc:58: return frame->document().url();
Ooh, is it on purpose that we use |frame->document()| here, and
|frame->top()->document()| above?

http://codereview.chromium.org/8538004/diff/14001/chrome/renderer/content_set...
chrome/renderer/content_settings_observer.cc:62: inline ContentSetting
GetContentSettingFromRules(
Why do you explicitly add an "inline" here? Are you sure that a) the compiler
won't inline it by itself, and b) not inlining it will kill performance?

[marja, 2011-11-15 11:49:58]

Hmm, it seems that the cache was a good idea after all; the page cyclers run
quicker if there are multiple content setting rules. (The default setting is
probably to run them with only the default rule, so this won't show up in the
automated tests afaik.) So I added the cache back.

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

http://codereview.chromium.org/8538004/diff/14001/chrome/renderer/content_set...
chrome/renderer/content_settings_observer.cc:58: return frame->document().url();
On 2011/11/15 09:29:35, Bernhard Bauer wrote:
> Ooh, is it on purpose that we use |frame->document()| here, and
> |frame->top()->document()| above?

Hmm, I did it on purpose but I might have misinterpreted the discussion starting
from http://codereview.chromium.org/7831075#msg29 . I changed this to be
frame->top()->document().url(), too.

http://codereview.chromium.org/8538004/diff/14001/chrome/renderer/content_set...
chrome/renderer/content_settings_observer.cc:62: inline ContentSetting
GetContentSettingFromRules(
On 2011/11/15 09:29:35, Bernhard Bauer wrote:
> Why do you explicitly add an "inline" here? Are you sure that a) the compiler
> won't inline it by itself, and b) not inlining it will kill performance?

Just because it seemed to be better in some of my experiments at some point.
However, now that I added the cache back, it doesn't make a difference any more,
so I removed it.

[Bernhard Bauer, 2011-11-16 09:26:43]

LGTM.

[commit-bot: I haz the power, 2011-11-16 09:31:59]

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

[commit-bot: I haz the power, 2011-11-16 09:32:04]

Can't apply patch for file chrome/browser/chrome_content_browser_client.cc.
While running patch -p1 --forward --force;
patching file chrome/browser/chrome_content_browser_client.cc
Hunk #1 FAILED at 15.
Hunk #2 succeeded at 320 (offset 3 lines).
1 out of 2 hunks FAILED -- saving rejects to file
chrome/browser/chrome_content_browser_client.cc.rej

[commit-bot: I haz the power, 2011-11-16 09:46:47]

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

[commit-bot: I haz the power, 2011-11-16 11:57:01]

Change committed as 110286