Delegating the "are images allowed" decision to renderer.

Delegating the "are images allowed" decision to renderer.

This enables making the decision based on both image url and the page url. E.g., blocking third-party images.

BUG=81179
TEST=RenderViewTest.ImagesBlockedByDefault, RenderViewTest.ImagesAllowedByDefault


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

[marja, 2011-09-09 10:41:52]

Hi Jochen,

Is this a sane approach to delegating the "images allowed" decision to
HostContentSettingsMap? Is it overkill to send one IPC per image?

[marja, 2011-09-13 14:01:48]

Hi again,

Now transmitting the image settings to the renderer. Does this look like it's
going to the right direction?

(This cl requires http://codereview.chromium.org/7885006/ .)

[jochen (gone - plz use gerrit), 2011-09-13 14:46:23]

On 2011/09/13 14:01:48, marja wrote:
> Hi again,
> 
> Now transmitting the image settings to the renderer. Does this look like it's
> going to the right direction?
> 
> (This cl requires http://codereview.chromium.org/7885006/ .)

two comments at this stage:

- this code doesn't send updates if the content settings in the browser change
- this code takes the main frame/frame URLs. Maybe it should take the main
frame/img URL instead? That would, ofcourse require changes to webkit

[marja, 2011-09-13 15:09:51]

On 2011/09/13 14:46:23, jochen wrote:
> - this code doesn't send updates if the content settings in the browser change

Hmm, it sends them in TabSpecificContentSettings::Observe(), isn't that ok?
(Tested quickly, seems to work.)

[jochen (gone - plz use gerrit), 2011-09-13 15:16:12]

On 2011/09/13 15:09:51, marja wrote:
> On 2011/09/13 14:46:23, jochen wrote:
> > - this code doesn't send updates if the content settings in the browser
change
> 
> Hmm, it sends them in TabSpecificContentSettings::Observe(), isn't that ok?
> (Tested quickly, seems to work.)

oh, you're right

then my comment would be that the initial set shouldn't be transfered on each
navigation, but only when a new renderer is started.

[Bernhard Bauer, 2011-09-13 17:38:11]

On 2011/09/09 10:41:52, marja wrote:
> Hi Jochen,
> 
> Is this a sane approach to delegating the "images allowed" decision to
> HostContentSettingsMap? Is it overkill to send one IPC per image?

That is what I'm asking myself as well. It seems definitely a lot easier to just
do an IPC per image.

If we're concerned about the sync IPC overhead, we could land it as an
experiment and see if performance on the page cycler degrades, for example.

http://codereview.chromium.org/7831075/diff/5001/chrome/browser/content_setti...
File chrome/browser/content_settings/host_content_settings_map.cc (right):

http://codereview.chromium.org/7831075/diff/5001/chrome/browser/content_setti...
chrome/browser/content_settings/host_content_settings_map.cc:404: //
ContentSettingPattern precedence relation.
Uhhh… is this going to work? The precedence relation wasn't defined to be a
total relation, so there might be some strangeness if we start to sort things by
it.

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

http://codereview.chromium.org/7831075/diff/5001/chrome/renderer/content_sett...
chrome/renderer/content_settings_observer.cc:208: return false; // Uninitialized
document.
Does this change the behavior?

http://codereview.chromium.org/7831075/diff/5001/chrome/renderer/content_sett...
chrome/renderer/content_settings_observer.cc:217: return (it->c !=
CONTENT_SETTING_BLOCK);
This simply returns |false| when the setting is BLOCK, without notifying the
browser about it (which we used to do below with DidBlockContentType).

http://codereview.chromium.org/7831075/diff/5001/chrome/renderer/content_sett...
File chrome/renderer/content_settings_observer.h (right):

http://codereview.chromium.org/7831075/diff/5001/chrome/renderer/content_sett...
chrome/renderer/content_settings_observer.h:91: static ContentSettingRules
image_setting_rules_;
I'd really like to avoid adding even more static initializers (see
http://neugierig.org/software/chromium/notes/2011/08/static-initializers.html
for an example why this is bad).

[marja, 2011-09-14 14:34:24]

On 2011/09/13 15:16:12, jochen wrote:
> then my comment would be that the initial set shouldn't be transfered on each
> navigation, but only when a new renderer is started.

Ok. Should that be done by 1) having TabSpecificContentSettings send the image
settings only when RenderViewCreated is called the first time, or 2) sending the
image settings in BrowserRenderProcessHost when it's created, or 3) some other
way?

[jochen (gone - plz use gerrit), 2011-09-14 18:39:36]

I think (2) is the way to go

On Wed, Sep 14, 2011 at 4:34 PM, <marja@chromium.org> wrote:

> On 2011/09/13 15:16:12, jochen wrote:
>
>> then my comment would be that the initial set shouldn't be transfered on
>> each
>> navigation, but only when a new renderer is started.
>>
>
> Ok. Should that be done by 1) having TabSpecificContentSettings send the
> image
> settings only when RenderViewCreated is called the first time, or 2)
> sending the
> image settings in BrowserRenderProcessHost when it's created, or 3) some
> other
> way?
>
>
>
>
http://codereview.chromium.**org/7831075/<http://codereview.chromium.org/7831...
>

[marja, 2011-09-15 12:00:11]

> > Ok. Should that be done by 1) having TabSpecificContentSettings
> > send the image
> > settings only when RenderViewCreated is called the first time, or
> > 2) sending the
> > image settings in BrowserRenderProcessHost when it's created, or
> > 3) some other way?

> I think (2) is the way to go

Oops, I thought it would work, but it won't, since BrowserRenderProcessHost is
in content/ and HostContentSettingsMap etc. is in chrome/.

[marja, 2011-09-15 12:02:00]

Thanks for the comments!

http://codereview.chromium.org/7831075/diff/5001/chrome/browser/content_setti...
File chrome/browser/content_settings/host_content_settings_map.cc (right):

http://codereview.chromium.org/7831075/diff/5001/chrome/browser/content_setti...
chrome/browser/content_settings/host_content_settings_map.cc:404: //
ContentSettingPattern precedence relation.
On 2011/09/13 17:38:11, Bernhard Bauer wrote:
> Uhhh… is this going to work? The precedence relation wasn't defined to be a
> total relation, so there might be some strangeness if we start to sort things
by
> it.

Hmm, what would go wrong? Sorting should be ok even if both a < b and b < a
return false, then a and b would just be considered equal and appear in the
result next to each other in arbitrary order. It seems that "a < b == b < a ==
false" occurs when a and b are ContentSettingsPatterns that are in the
"identity" relation with each other, but not when a is a subcase of b, or when
they are unrelated, or when they overlap. "a < b == b > a == true" could result
in some weirdness, but afaics it doesn't happen.

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

http://codereview.chromium.org/7831075/diff/5001/chrome/renderer/content_sett...
chrome/renderer/content_settings_observer.cc:208: return false; // Uninitialized
document.
On 2011/09/13 17:38:11, Bernhard Bauer wrote:
> Does this change the behavior?

I moved the IsWhiteListedForContentSettings up, that seemed the right thing to
do at least. What to do with uninitialized documents: 1) return false 2) return
true 3) return the default setting (which is the last element of the image
settings)?

http://codereview.chromium.org/7831075/diff/5001/chrome/renderer/content_sett...
chrome/renderer/content_settings_observer.cc:217: return (it->c !=
CONTENT_SETTING_BLOCK);
On 2011/09/13 17:38:11, Bernhard Bauer wrote:
> This simply returns |false| when the setting is BLOCK, without notifying the
> browser about it (which we used to do below with DidBlockContentType).

Ahh, true! I added the DidBlockContentType here and in the "return false" case
above.

http://codereview.chromium.org/7831075/diff/5001/chrome/renderer/content_sett...
File chrome/renderer/content_settings_observer.h (right):

http://codereview.chromium.org/7831075/diff/5001/chrome/renderer/content_sett...
chrome/renderer/content_settings_observer.h:91: static ContentSettingRules
image_setting_rules_;
On 2011/09/13 17:38:11, Bernhard Bauer wrote:
> I'd really like to avoid adding even more static initializers (see
> http://neugierig.org/software/chromium/notes/2011/08/static-initializers.html
> for an example why this is bad).

I made ChromeContentRendererClient own the image_setting_rules, and
ContentSettingsObserver just to have a pointer to it. Not sure whether that's
the most elegant solution.

[Bernhard Bauer, 2011-09-15 12:52:19]

http://codereview.chromium.org/7831075/diff/5001/chrome/browser/content_setti...
File chrome/browser/content_settings/host_content_settings_map.cc (right):

http://codereview.chromium.org/7831075/diff/5001/chrome/browser/content_setti...
chrome/browser/content_settings/host_content_settings_map.cc:404: //
ContentSettingPattern precedence relation.
On 2011/09/15 12:02:00, marja wrote:
> On 2011/09/13 17:38:11, Bernhard Bauer wrote:
> > Uhhh… is this going to work? The precedence relation wasn't defined to be a
> > total relation, so there might be some strangeness if we start to sort
things
> by
> > it.
> 
> Hmm, what would go wrong? Sorting should be ok even if both a < b and b < a
> return false, then a and b would just be considered equal and appear in the
> result next to each other in arbitrary order. It seems that "a < b == b < a ==
> false" occurs when a and b are ContentSettingsPatterns that are in the
> "identity" relation with each other, but not when a is a subcase of b, or when
> they are unrelated, or when they overlap. "a < b == b > a == true" could
result
> in some weirdness, but afaics it doesn't happen.

Hm, I guess the most important property is transitivity, and we should have
that.

This method is still very similar to GetSettingsForOneType. We should look into
unifying them (and the types used! Or can you tell me the difference between
content_settings::ProviderInterface::Rules, ContentSettingRules and
SettingsForOneType from the top of your head?)

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

http://codereview.chromium.org/7831075/diff/9002/chrome/renderer/content_sett...
chrome/renderer/content_settings_observer.cc:217: bool allow = (it->c !=
CONTENT_SETTING_BLOCK);
You already have a call to DidBlockContentType below. You could move |allow| out
of this scope, break and then return it below.

http://codereview.chromium.org/7831075/diff/9002/chrome/renderer/content_sett...
File chrome/renderer/content_settings_observer.h (right):

http://codereview.chromium.org/7831075/diff/9002/chrome/renderer/content_sett...
chrome/renderer/content_settings_observer.h:88: // Stores the rules for image
content settings. Not owned.
Can you explain who owns them? And maybe make the pointer const?

[marja, 2011-09-15 16:00:03]

Hi,

I did some refactoring according to bauerb's comments. I'm not at all sure I
didn't break anything in the process, so I'll have a better look at all this
tomorrow...

http://codereview.chromium.org/7831075/diff/5001/chrome/browser/content_setti...
File chrome/browser/content_settings/host_content_settings_map.cc (right):

http://codereview.chromium.org/7831075/diff/5001/chrome/browser/content_setti...
chrome/browser/content_settings/host_content_settings_map.cc:404: //
ContentSettingPattern precedence relation.
On 2011/09/15 12:52:19, Bernhard Bauer wrote:
> This method is still very similar to GetSettingsForOneType. We should look
into
> unifying them (and the types used! Or can you tell me the difference between
> content_settings::ProviderInterface::Rules, ContentSettingRules and
> SettingsForOneType from the top of your head?)

markusheintz had some diabolic-soul-merchandise going on, and first I didn't
want to meddle in. But I meddled in after all, so now there is only
ContentSettingsForOneType and it's defined in chrome/common/content_settings.h.
I also removed this function and added a sort function as a parameter to
GetSettingsForOneType.

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

http://codereview.chromium.org/7831075/diff/9002/chrome/renderer/content_sett...
chrome/renderer/content_settings_observer.cc:217: bool allow = (it->c !=
CONTENT_SETTING_BLOCK);
On 2011/09/15 12:52:19, Bernhard Bauer wrote:
> You already have a call to DidBlockContentType below. You could move |allow|
out
> of this scope, break and then return it below.

Done.

http://codereview.chromium.org/7831075/diff/9002/chrome/renderer/content_sett...
File chrome/renderer/content_settings_observer.h (right):

http://codereview.chromium.org/7831075/diff/9002/chrome/renderer/content_sett...
chrome/renderer/content_settings_observer.h:88: // Stores the rules for image
content settings. Not owned.
On 2011/09/15 12:52:19, Bernhard Bauer wrote:
> Can you explain who owns them? And maybe make the pointer const?

Done.

[Bernhard Bauer, 2011-09-16 09:16:48]

http://codereview.chromium.org/7831075/diff/11003/chrome/browser/content_sett...
File chrome/browser/content_settings/host_content_settings_map.cc (right):

http://codereview.chromium.org/7831075/diff/11003/chrome/browser/content_sett...
chrome/browser/content_settings/host_content_settings_map.cc:352:
ContentSettingsForOneType* settings)
Nit: I think the |const| belongs on this line?

[marja, 2011-09-19 11:53:40]

I moved the "send initial image settings" code from TabSpecificContentSettings
to ChromeContentBrowserClient.

Does this approach look ok now? There are no tests yet, I'll add them next (if
this looks ok).

http://codereview.chromium.org/7831075/diff/11003/chrome/browser/content_sett...
File chrome/browser/content_settings/host_content_settings_map.cc (right):

http://codereview.chromium.org/7831075/diff/11003/chrome/browser/content_sett...
chrome/browser/content_settings/host_content_settings_map.cc:352:
ContentSettingsForOneType* settings)
On 2011/09/16 09:16:48, Bernhard Bauer wrote:
> Nit: I think the |const| belongs on this line?

Done.

[marja, 2011-09-20 13:40:12]

Hi,

I added some tests (unit test for HostContentSettingMap, browser test for
ContentSettingsObserver).

Does the HostContentSettingsMap::GetSettingsForOneType look ok? It is used by
the UI which needs the settings ordered alphabetically, as well as the
send-rules-to-renderer code which needs the settings ordered from specific to
generic. Now it's semi-ugly that the sorting function (which it takes as a
param) sorts ProviderInterface::Rules, and thus the user of
HostContentSettingsMap needs to know about ProviderInterface. I could change it
so that the sorting function sorts ContentSettingsPatternSourceTuples, but that
would add a conversion step to GetSettingsForOneType(). Should I do that?

[marja, 2011-10-17 12:38:28]

Hi Jochen & Bernhard,

I rebased this on top of current trunk. The refactoring I was working on
(HostContentSettingsMap, ProviderInterface, OriginIdentifierValueMap) is finally
done, and now this CL is a bit simpler. Could you have another look?

[Bernhard Bauer, 2011-10-17 13:13:19]

Woohoo!

http://codereview.chromium.org/7831075/diff/67001/chrome/browser/content_sett...
File chrome/browser/content_settings/host_content_settings_map.h (left):

http://codereview.chromium.org/7831075/diff/67001/chrome/browser/content_sett...
chrome/browser/content_settings/host_content_settings_map.h:125: // to be
specified, the |resource_identifier| must be non-empty.
Wait, we're changing the comment without changing the implementation of the
method. Does that mean that the current behavior is already to return all
settings for an incognito HCSM? Does that in turn mean that the content settings
UI is broken?

http://codereview.chromium.org/7831075/diff/67001/chrome/common/content_setti...
File chrome/common/content_settings.h (right):

http://codereview.chromium.org/7831075/diff/67001/chrome/common/content_setti...
chrome/common/content_settings.h:48: // I really want my soul back, so I really
will change this ASAP.
If we're moving the definition, now might be a good time to change it to a
struct?

http://codereview.chromium.org/7831075/diff/67001/chrome/common/render_messag...
File chrome/common/render_messages.h (right):

http://codereview.chromium.org/7831075/diff/67001/chrome/common/render_messag...
chrome/common/render_messages.h:108: struct ParamTraits<ContentSettingsPattern>
{
I think you could do this via the |IPC_STRUCT_TRAITS_| macros like below
(directly serializing the parts of the parsed pattern instead of stringifying
and re-parsing it).

http://codereview.chromium.org/7831075/diff/67001/chrome/renderer/chrome_cont...
File chrome/renderer/chrome_content_renderer_client.h (right):

http://codereview.chromium.org/7831075/diff/67001/chrome/renderer/chrome_cont...
chrome/renderer/chrome_content_renderer_client.h:144: ContentSettingsForOneType
image_setting_rules_;
The content client should be the glue between the content layer and the chrome
embedder, and carry no state itself. If you want to store one copy of the image
setting rules, you can store it them in the ChromeRenderProcessObserver.

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

http://codereview.chromium.org/7831075/diff/67001/chrome/renderer/content_set...
chrome/renderer/content_settings_observer.cc:211:
frame->top()->document().securityOrigin().isEmpty()) {
Nit: If you move this check into the |if (enabled_per_settings)| below, you can
remove this call to |DidBlockContentType|.

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

http://codereview.chromium.org/7831075/diff/67001/chrome/renderer/content_set...
chrome/renderer/content_settings_observer.h:89: static ContentSettings
default_settings_;
If we're storing image settings globally, we could store the default settings in
the same place. Could you add a TODO for that?

[marja, 2011-10-17 15:10:11]

Thanks for comments, I'll do the fixes tomorrow...

http://codereview.chromium.org/7831075/diff/67001/chrome/browser/content_sett...
File chrome/browser/content_settings/host_content_settings_map.h (left):

http://codereview.chromium.org/7831075/diff/67001/chrome/browser/content_sett...
chrome/browser/content_settings/host_content_settings_map.h:125: // to be
specified, the |resource_identifier| must be non-empty.
On 2011/10/17 13:13:19, Bernhard Bauer wrote:
> Wait, we're changing the comment without changing the implementation of the
> method. Does that mean that the current behavior is already to return all
> settings for an incognito HCSM? Does that in turn mean that the content
settings
> UI is broken?

The "If the map was created for the incognito profile" part is outdated (after
my CL 7982028). It will return settings for incognito + normal mode in this
case. The UI is not broken, since it will filter the settings based on the
"incognito" bool.

[Bernhard Bauer, 2011-10-17 15:14:12]

http://codereview.chromium.org/7831075/diff/67001/chrome/browser/content_sett...
File chrome/browser/content_settings/host_content_settings_map.h (left):

http://codereview.chromium.org/7831075/diff/67001/chrome/browser/content_sett...
chrome/browser/content_settings/host_content_settings_map.h:125: // to be
specified, the |resource_identifier| must be non-empty.
On 2011/10/17 15:10:11, marja wrote:
> On 2011/10/17 13:13:19, Bernhard Bauer wrote:
> > Wait, we're changing the comment without changing the implementation of the
> > method. Does that mean that the current behavior is already to return all
> > settings for an incognito HCSM? Does that in turn mean that the content
> settings
> > UI is broken?
> 
> The "If the map was created for the incognito profile" part is outdated (after
> my CL 7982028). It will return settings for incognito + normal mode in this
> case. The UI is not broken, since it will filter the settings based on the
> "incognito" bool.

Ok, just checking (cs.chromium.org seems to be a bit behind).

[marja, 2011-10-18 12:23:02]

http://codereview.chromium.org/7831075/diff/67001/chrome/common/content_setti...
File chrome/common/content_settings.h (right):

http://codereview.chromium.org/7831075/diff/67001/chrome/common/content_setti...
chrome/common/content_settings.h:48: // I really want my soul back, so I really
will change this ASAP.
On 2011/10/17 13:13:19, Bernhard Bauer wrote:
> If we're moving the definition, now might be a good time to change it to a
> struct?

Done.

http://codereview.chromium.org/7831075/diff/67001/chrome/common/render_messag...
File chrome/common/render_messages.h (right):

http://codereview.chromium.org/7831075/diff/67001/chrome/common/render_messag...
chrome/common/render_messages.h:108: struct ParamTraits<ContentSettingsPattern>
{
On 2011/10/17 13:13:19, Bernhard Bauer wrote:
> I think you could do this via the |IPC_STRUCT_TRAITS_| macros like below
> (directly serializing the parts of the parsed pattern instead of stringifying
> and re-parsing it). 

Done. But, I had to make ContentSettingsPattern::PatternParts, parts_ and
is_valid_ public, to be able to do this. Is that ok? It also exposes the
internal structure of ContentSettingsPattern, whereas the previous solution
assumed that they can be converted to strings and back without losing
information. Not sure which is more elegant.

http://codereview.chromium.org/7831075/diff/67001/chrome/renderer/chrome_cont...
File chrome/renderer/chrome_content_renderer_client.h (right):

http://codereview.chromium.org/7831075/diff/67001/chrome/renderer/chrome_cont...
chrome/renderer/chrome_content_renderer_client.h:144: ContentSettingsForOneType
image_setting_rules_;
On 2011/10/17 13:13:19, Bernhard Bauer wrote:
> The content client should be the glue between the content layer and the chrome
> embedder, and carry no state itself. If you want to store one copy of the
image
> setting rules, you can store it them in the ChromeRenderProcessObserver.

Done. But, the ChromeRenderProcessObserver is not created in the tests, so I
added a ha.. err, indirection here: ChromeContentRendererClient holds a pointer
to the image setting rules. Normally ChromeRenderProcessObserver sets it to
point to its member, but also the tests can set it.

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

http://codereview.chromium.org/7831075/diff/67001/chrome/renderer/content_set...
chrome/renderer/content_settings_observer.cc:211:
frame->top()->document().securityOrigin().isEmpty()) {
On 2011/10/17 13:13:19, Bernhard Bauer wrote:
> Nit: If you move this check into the |if (enabled_per_settings)| below, you
can
> remove this call to |DidBlockContentType|.

Done.

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

http://codereview.chromium.org/7831075/diff/67001/chrome/renderer/content_set...
chrome/renderer/content_settings_observer.h:89: static ContentSettings
default_settings_;
On 2011/10/17 13:13:19, Bernhard Bauer wrote:
> If we're storing image settings globally, we could store the default settings
in
> the same place. Could you add a TODO for that?

Done.

[Bernhard Bauer, 2011-10-18 13:14:31]

http://codereview.chromium.org/7831075/diff/67001/chrome/common/render_messag...
File chrome/common/render_messages.h (right):

http://codereview.chromium.org/7831075/diff/67001/chrome/common/render_messag...
chrome/common/render_messages.h:108: struct ParamTraits<ContentSettingsPattern>
{
On 2011/10/18 12:23:02, marja wrote:
> On 2011/10/17 13:13:19, Bernhard Bauer wrote:
> > I think you could do this via the |IPC_STRUCT_TRAITS_| macros like below
> > (directly serializing the parts of the parsed pattern instead of
stringifying
> > and re-parsing it). 
> 
> Done. But, I had to make ContentSettingsPattern::PatternParts, parts_ and
> is_valid_ public, to be able to do this. Is that ok? It also exposes the
> internal structure of ContentSettingsPattern, whereas the previous solution
> assumed that they can be converted to strings and back without losing
> information. Not sure which is more elegant.

Making ContentSettingsPattern::PatternParts itself public is fine. |parts_| and
|is_valid_| shouldn't be public. I mainly want to use the macros for
ContentSettingsPattern::PatternParts (to avoid repetition), so you could declare
ParamTraits<ContentSettingsPattern> here and then serialize the PatternParts in
the implementation (declaring IPC::ParamTraits<ContentSettingsPattern> a
friend)?

http://codereview.chromium.org/7831075/diff/80003/chrome/renderer/chrome_cont...
File chrome/renderer/chrome_content_renderer_client.h (right):

http://codereview.chromium.org/7831075/diff/80003/chrome/renderer/chrome_cont...
chrome/renderer/chrome_content_renderer_client.h:154: // creating a
|ChromeRenderProcessObserver|.
So, if there is no ChromeRenderProcessObserver, it's not clear who owns this
pointer :(

Do we actually need a non-empty set of image setting rules in tests? In the unit
test for ContentSettingsObserver it gets its rules separately.

Hm, thinking about it, I'm pretty sure we don't, as the vector in RenderViewTest
is never modified. So, we could make SetImageSettingRules a method on
ContentSettingsObserver, and only call it from ChromeContentRendererClient if
there is a ChromeRenderProcessObserver, and in ContentSettingsObserver just
return the default if it is NULL.

[marja, 2011-10-19 16:56:35]

This should be up to date with trunk again; could you have another look?

http://codereview.chromium.org/7831075/diff/67001/chrome/common/render_messag...
File chrome/common/render_messages.h (right):

http://codereview.chromium.org/7831075/diff/67001/chrome/common/render_messag...
chrome/common/render_messages.h:108: struct ParamTraits<ContentSettingsPattern>
{
On 2011/10/18 13:14:31, Bernhard Bauer wrote:
> On 2011/10/18 12:23:02, marja wrote:
> > On 2011/10/17 13:13:19, Bernhard Bauer wrote:
> > > I think you could do this via the |IPC_STRUCT_TRAITS_| macros like below
> > > (directly serializing the parts of the parsed pattern instead of
> stringifying
> > > and re-parsing it). 
> > 
> > Done. But, I had to make ContentSettingsPattern::PatternParts, parts_ and
> > is_valid_ public, to be able to do this. Is that ok? It also exposes the
> > internal structure of ContentSettingsPattern, whereas the previous solution
> > assumed that they can be converted to strings and back without losing
> > information. Not sure which is more elegant.
> 
> Making ContentSettingsPattern::PatternParts itself public is fine. |parts_|
and
> |is_valid_| shouldn't be public. I mainly want to use the macros for
> ContentSettingsPattern::PatternParts (to avoid repetition), so you could
declare
> ParamTraits<ContentSettingsPattern> here and then serialize the PatternParts
in
> the implementation (declaring IPC::ParamTraits<ContentSettingsPattern> a
> friend)?

(This got resolved by your CL.)

http://codereview.chromium.org/7831075/diff/80003/chrome/renderer/chrome_cont...
File chrome/renderer/chrome_content_renderer_client.h (right):

http://codereview.chromium.org/7831075/diff/80003/chrome/renderer/chrome_cont...
chrome/renderer/chrome_content_renderer_client.h:154: // creating a
|ChromeRenderProcessObserver|.
On 2011/10/18 13:14:31, Bernhard Bauer wrote:
> So, if there is no ChromeRenderProcessObserver, it's not clear who owns this
> pointer :(
> 
> Do we actually need a non-empty set of image setting rules in tests? In the
unit
> test for ContentSettingsObserver it gets its rules separately.
> 
> Hm, thinking about it, I'm pretty sure we don't, as the vector in
RenderViewTest
> is never modified. So, we could make SetImageSettingRules a method on
> ContentSettingsObserver, and only call it from ChromeContentRendererClient if
> there is a ChromeRenderProcessObserver, and in ContentSettingsObserver just
> return the default if it is NULL.

It was used by ChromeRenderViewTest.Images(Allowed|Blocked)ByDefault. However,
what you said makes sense, so I did that. And the test calls
ContentSettingsObserver::SetImageSettingRules.

[marja, 2011-10-20 08:46:29]

I also updated chrome/common/extensions/docs/contentSettings.html now.

[Bernhard Bauer, 2011-10-20 09:21:53]

http://codereview.chromium.org/7831075/diff/84016/chrome/browser/content_sett...
File chrome/browser/content_settings/host_content_settings_map.h (right):

http://codereview.chromium.org/7831075/diff/84016/chrome/browser/content_sett...
chrome/browser/content_settings/host_content_settings_map.h:120: // mapped to
their actual settings, in the order the settings are applied.
Nit: I guess technically only one rule applies? So it should be someting like
"precedence order".

http://codereview.chromium.org/7831075/diff/84016/chrome/common/content_setti...
File chrome/common/content_settings.h (right):

http://codereview.chromium.org/7831075/diff/84016/chrome/common/content_setti...
chrome/common/content_settings.h:31: struct ContentSettings {
While you're at it, could you out-of-line these methods? Thanks!

http://codereview.chromium.org/7831075/diff/84016/chrome/common/extensions/do...
File chrome/common/extensions/docs/contentSettings.html (right):

http://codereview.chromium.org/7831075/diff/84016/chrome/common/extensions/do...
chrome/common/extensions/docs/contentSettings.html:450: <dd>Whether to show
images. One of<br><var>allow</var>: Show images,<br><var>block</var>: Don't show
images. Default is <var>allow</var>.<br>The primary URL is the main-frame URL.
The secondary URL is the URL of the image.</dd>
Documentation is auto-generated from
chrome/common/extensions/api/extension_api.json. You should update that file
instead, then run chrome/common/extensions/docs/build/build.sh.

http://codereview.chromium.org/7831075/diff/84016/chrome/common/render_messag...
File chrome/common/render_messages.h (right):

http://codereview.chromium.org/7831075/diff/84016/chrome/common/render_messag...
chrome/common/render_messages.h:172:
IPC_STRUCT_TRAITS_BEGIN(ContentSettingPatternSource)
Nit: Keep these declarations is alphabetical order? I.e. this would come after
ContentSettingsPattern::PatternParts.

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

http://codereview.chromium.org/7831075/diff/84016/chrome/renderer/content_set...
chrome/renderer/content_settings_observer.cc:209: bool allow = false;
I think we may want to default to allow?

http://codereview.chromium.org/7831075/diff/84016/chrome/renderer/content_set...
chrome/renderer/content_settings_observer.cc:210: GURL
top_url(frame->top()->document().securityOrigin().toString());
Argh, I just noticed that per http://crbug.com/100353 we return an empty
security origin for file: URLs. I think that would mean that all images would be
blocked on file: pages? :-/

Could you manually check that? If they are indeed blocked, we should use
|document.url()| instead and I'm gonna yell at Jochen.

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

http://codereview.chromium.org/7831075/diff/84016/chrome/renderer/content_set...
chrome/renderer/content_settings_observer.h:38: // |ContentSettingObserver| does
not take the ownership of the
Nit: Can you add a comment that state that callers should make sure the passed
|ContentSettingsForOneType*| outlives this object?

OTOH, stating that the method does *not* take ownership is not something we
usually document (we only document if it does).

[marja, 2011-10-20 11:44:22]

Thanks for comments again!

http://codereview.chromium.org/7831075/diff/84016/chrome/browser/content_sett...
File chrome/browser/content_settings/host_content_settings_map.h (right):

http://codereview.chromium.org/7831075/diff/84016/chrome/browser/content_sett...
chrome/browser/content_settings/host_content_settings_map.h:120: // mapped to
their actual settings, in the order the settings are applied.
On 2011/10/20 09:21:53, Bernhard Bauer wrote:
> Nit: I guess technically only one rule applies? So it should be someting like
> "precedence order".

Done.

http://codereview.chromium.org/7831075/diff/84016/chrome/common/content_setti...
File chrome/common/content_settings.h (right):

http://codereview.chromium.org/7831075/diff/84016/chrome/common/content_setti...
chrome/common/content_settings.h:31: struct ContentSettings {
On 2011/10/20 09:21:53, Bernhard Bauer wrote:
> While you're at it, could you out-of-line these methods? Thanks!

Done.

http://codereview.chromium.org/7831075/diff/84016/chrome/common/extensions/do...
File chrome/common/extensions/docs/contentSettings.html (right):

http://codereview.chromium.org/7831075/diff/84016/chrome/common/extensions/do...
chrome/common/extensions/docs/contentSettings.html:450: <dd>Whether to show
images. One of<br><var>allow</var>: Show images,<br><var>block</var>: Don't show
images. Default is <var>allow</var>.<br>The primary URL is the main-frame URL.
The secondary URL is the URL of the image.</dd>
On 2011/10/20 09:21:53, Bernhard Bauer wrote:
> Documentation is auto-generated from
> chrome/common/extensions/api/extension_api.json. You should update that file
> instead, then run chrome/common/extensions/docs/build/build.sh. 

Done.

http://codereview.chromium.org/7831075/diff/84016/chrome/common/render_messag...
File chrome/common/render_messages.h (right):

http://codereview.chromium.org/7831075/diff/84016/chrome/common/render_messag...
chrome/common/render_messages.h:172:
IPC_STRUCT_TRAITS_BEGIN(ContentSettingPatternSource)
On 2011/10/20 09:21:53, Bernhard Bauer wrote:
> Nit: Keep these declarations is alphabetical order? I.e. this would come after
> ContentSettingsPattern::PatternParts.

Done.

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

http://codereview.chromium.org/7831075/diff/84016/chrome/renderer/content_set...
chrome/renderer/content_settings_observer.cc:209: bool allow = false;
On 2011/10/20 09:21:53, Bernhard Bauer wrote:
> I think we may want to default to allow?

Shouldn't we default to enabled_per_settings instead?

http://codereview.chromium.org/7831075/diff/84016/chrome/renderer/content_set...
chrome/renderer/content_settings_observer.cc:210: GURL
top_url(frame->top()->document().securityOrigin().toString());
On 2011/10/20 09:21:53, Bernhard Bauer wrote:
> Argh, I just noticed that per http://crbug.com/100353 we return an empty
> security origin for file: URLs. I think that would mean that all images would
be
> blocked on file: pages? :-/
> 
> Could you manually check that? If they are indeed blocked, we should use
> |document.url()| instead and I'm gonna yell at Jochen.

There is the gotcha that "a unique security origins is not the same as an empty
security origin". So even if
GURL(frame->top()->document().securityOrigin().toString()) is "" (it's unique),
frame->top()->document().securityOrigin().isEmpty() is false.

I also checked manually, and the images get displayed correctly for a document
loaded by the URL file:///somewhere.

It's a bit ugly though that frame->top()->document().securityOrigin().toString()
is "null" in that case, and GURL("null") gets converted to GURL(""). So, this
only works by a coincidence.

Unluckily, WebSecurityOrigin doesn't expose isUnique(). 

I changed this so that it handles the "null" case separately. Does it look less
ugly?

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

http://codereview.chromium.org/7831075/diff/84016/chrome/renderer/content_set...
chrome/renderer/content_settings_observer.h:38: // |ContentSettingObserver| does
not take the ownership of the
On 2011/10/20 09:21:53, Bernhard Bauer wrote:
> Nit: Can you add a comment that state that callers should make sure the passed
> |ContentSettingsForOneType*| outlives this object?
> 
> OTOH, stating that the method does *not* take ownership is not something we
> usually document (we only document if it does).

Done.

[Bernhard Bauer, 2011-10-20 11:58:24]

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

http://codereview.chromium.org/7831075/diff/84016/chrome/renderer/content_set...
chrome/renderer/content_settings_observer.cc:209: bool allow = false;
On 2011/10/20 11:44:22, marja wrote:
> On 2011/10/20 09:21:53, Bernhard Bauer wrote:
> > I think we may want to default to allow?
> 
> Shouldn't we default to enabled_per_settings instead?

OK (although in practice |enabled_per_settings| is going to be true unless
someone turns off images for the whole browser, and I'm not even sure that's
possible).

http://codereview.chromium.org/7831075/diff/84016/chrome/renderer/content_set...
chrome/renderer/content_settings_observer.cc:210: GURL
top_url(frame->top()->document().securityOrigin().toString());
On 2011/10/20 11:44:22, marja wrote:
> On 2011/10/20 09:21:53, Bernhard Bauer wrote:
> > Argh, I just noticed that per http://crbug.com/100353 we return an empty
> > security origin for file: URLs. I think that would mean that all images
would
> be
> > blocked on file: pages? :-/
> > 
> > Could you manually check that? If they are indeed blocked, we should use
> > |document.url()| instead and I'm gonna yell at Jochen.
> 
> There is the gotcha that "a unique security origins is not the same as an
empty
> security origin". So even if
> GURL(frame->top()->document().securityOrigin().toString()) is "" (it's
unique),
> frame->top()->document().securityOrigin().isEmpty() is false.
> 
> I also checked manually, and the images get displayed correctly for a document
> loaded by the URL file:///somewhere.
> 
> It's a bit ugly though that
frame->top()->document().securityOrigin().toString()
> is "null" in that case, and GURL("null") gets converted to GURL(""). So, this
> only works by a coincidence.

Plus, it means that file:/// content setting patterns aren't going to apply.

I still think that using |document.url()| is the right thing to do here, as it
preserves the most information. We might want to sync with Jochen though.

> Unluckily, WebSecurityOrigin doesn't expose isUnique(). 
> 
> I changed this so that it handles the "null" case separately. Does it look
less
> ugly?

[marja, 2011-10-20 14:41:15]

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

http://codereview.chromium.org/7831075/diff/84016/chrome/renderer/content_set...
chrome/renderer/content_settings_observer.cc:209: bool allow = false;
On 2011/10/20 11:58:24, Bernhard Bauer wrote:
> On 2011/10/20 11:44:22, marja wrote:
> > On 2011/10/20 09:21:53, Bernhard Bauer wrote:
> > > I think we may want to default to allow?
> > 
> > Shouldn't we default to enabled_per_settings instead?
> 
> OK (although in practice |enabled_per_settings| is going to be true unless
> someone turns off images for the whole browser, and I'm not even sure that's
> possible).

Done.

http://codereview.chromium.org/7831075/diff/84016/chrome/renderer/content_set...
chrome/renderer/content_settings_observer.cc:210: GURL
top_url(frame->top()->document().securityOrigin().toString());
On 2011/10/20 11:58:24, Bernhard Bauer wrote:
> On 2011/10/20 11:44:22, marja wrote:
> > On 2011/10/20 09:21:53, Bernhard Bauer wrote:
> > > Argh, I just noticed that per http://crbug.com/100353 we return an empty
> > > security origin for file: URLs. I think that would mean that all images
> would
> > be
> > > blocked on file: pages? :-/
> > > 
> > > Could you manually check that? If they are indeed blocked, we should use
> > > |document.url()| instead and I'm gonna yell at Jochen.
> > 
> > There is the gotcha that "a unique security origins is not the same as an
> empty
> > security origin". So even if
> > GURL(frame->top()->document().securityOrigin().toString()) is "" (it's
> unique),
> > frame->top()->document().securityOrigin().isEmpty() is false.
> > 
> > I also checked manually, and the images get displayed correctly for a
document
> > loaded by the URL file:///somewhere.
> > 
> > It's a bit ugly though that
> frame->top()->document().securityOrigin().toString()
> > is "null" in that case, and GURL("null") gets converted to GURL(""). So,
this
> > only works by a coincidence.
> 
> Plus, it means that file:/// content setting patterns aren't going to apply.
> 
> I still think that using |document.url()| is the right thing to do here, as it
> preserves the most information. We might want to sync with Jochen though.
> 
> > Unluckily, WebSecurityOrigin doesn't expose isUnique(). 
> > 
> > I changed this so that it handles the "null" case separately. Does it look
> less
> > ugly?
> 

I changed this to be frame->document().url() and pinged Jochen.

[jochen (gone - plz use gerrit), 2011-10-20 15:32:15]

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

http://codereview.chromium.org/7831075/diff/84016/chrome/renderer/content_set...
chrome/renderer/content_settings_observer.cc:210: GURL
top_url(frame->top()->document().securityOrigin().toString());
On 2011/10/20 14:41:15, marja wrote:
> On 2011/10/20 11:58:24, Bernhard Bauer wrote:
> > On 2011/10/20 11:44:22, marja wrote:
> > > On 2011/10/20 09:21:53, Bernhard Bauer wrote:
> > > > Argh, I just noticed that per http://crbug.com/100353 we return an empty
> > > > security origin for file: URLs. I think that would mean that all images
> > would
> > > be
> > > > blocked on file: pages? :-/
> > > > 
> > > > Could you manually check that? If they are indeed blocked, we should use
> > > > |document.url()| instead and I'm gonna yell at Jochen.
> > > 
> > > There is the gotcha that "a unique security origins is not the same as an
> > empty
> > > security origin". So even if
> > > GURL(frame->top()->document().securityOrigin().toString()) is "" (it's
> > unique),
> > > frame->top()->document().securityOrigin().isEmpty() is false.
> > > 
> > > I also checked manually, and the images get displayed correctly for a
> document
> > > loaded by the URL file:///somewhere.
> > > 
> > > It's a bit ugly though that
> > frame->top()->document().securityOrigin().toString()
> > > is "null" in that case, and GURL("null") gets converted to GURL(""). So,
> this
> > > only works by a coincidence.
> > 
> > Plus, it means that file:/// content setting patterns aren't going to apply.
> > 
> > I still think that using |document.url()| is the right thing to do here, as
it
> > preserves the most information. We might want to sync with Jochen though.
> > 
> > > Unluckily, WebSecurityOrigin doesn't expose isUnique(). 
> > > 
> > > I changed this so that it handles the "null" case separately. Does it look
> > less
> > > ugly?
> > 
> 
> I changed this to be frame->document().url() and pinged Jochen.

I'd prefer if we'd stick to the security origin. Otherwise, a page could open an
about:blank frame and then display images in it.

What about falling back to document.url() if the security origin is empty  and
document.url is a file url?

[Bernhard Bauer, 2011-10-20 15:37:09]

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

http://codereview.chromium.org/7831075/diff/84016/chrome/renderer/content_set...
chrome/renderer/content_settings_observer.cc:210: GURL
top_url(frame->top()->document().securityOrigin().toString());
On 2011/10/20 15:32:15, jochen wrote:
> On 2011/10/20 14:41:15, marja wrote:
> > On 2011/10/20 11:58:24, Bernhard Bauer wrote:
> > > On 2011/10/20 11:44:22, marja wrote:
> > > > On 2011/10/20 09:21:53, Bernhard Bauer wrote:
> > > > > Argh, I just noticed that per http://crbug.com/100353 we return an
empty
> > > > > security origin for file: URLs. I think that would mean that all
images
> > > would
> > > > be
> > > > > blocked on file: pages? :-/
> > > > > 
> > > > > Could you manually check that? If they are indeed blocked, we should
use
> > > > > |document.url()| instead and I'm gonna yell at Jochen.
> > > > 
> > > > There is the gotcha that "a unique security origins is not the same as
an
> > > empty
> > > > security origin". So even if
> > > > GURL(frame->top()->document().securityOrigin().toString()) is "" (it's
> > > unique),
> > > > frame->top()->document().securityOrigin().isEmpty() is false.
> > > > 
> > > > I also checked manually, and the images get displayed correctly for a
> > document
> > > > loaded by the URL file:///somewhere.
> > > > 
> > > > It's a bit ugly though that
> > > frame->top()->document().securityOrigin().toString()
> > > > is "null" in that case, and GURL("null") gets converted to GURL(""). So,
> > this
> > > > only works by a coincidence.
> > > 
> > > Plus, it means that file:/// content setting patterns aren't going to
apply.
> > > 
> > > I still think that using |document.url()| is the right thing to do here,
as
> it
> > > preserves the most information. We might want to sync with Jochen though.
> > > 
> > > > Unluckily, WebSecurityOrigin doesn't expose isUnique(). 
> > > > 
> > > > I changed this so that it handles the "null" case separately. Does it
look
> > > less
> > > > ugly?
> > > 
> > 
> > I changed this to be frame->document().url() and pinged Jochen.
> 
> I'd prefer if we'd stick to the security origin. Otherwise, a page could open
an
> about:blank frame and then display images in it.

How would that work if we use the main frame URL?

> What about falling back to document.url() if the security origin is empty  and
> document.url is a file url?

[jochen (gone - plz use gerrit), 2011-10-20 19:01:21]

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

http://codereview.chromium.org/7831075/diff/84016/chrome/renderer/content_set...
chrome/renderer/content_settings_observer.cc:210: GURL
top_url(frame->top()->document().securityOrigin().toString());
sorry for being unclear, I meant opening the frame in a new window.

[marja, 2011-10-21 09:52:57]

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

http://codereview.chromium.org/7831075/diff/84016/chrome/renderer/content_set...
chrome/renderer/content_settings_observer.cc:210: GURL
top_url(frame->top()->document().securityOrigin().toString());
On 2011/10/20 15:32:15, jochen wrote:
> What about falling back to document.url() if the security origin is empty  and
> document.url is a file url?

I uploaded a version which falls back to document.url() if 
frame->top()->document().securityOrigin() is unique (*). Bernhard, WDYT?

(*) It won't be empty, only unique, but I guess you meant that.

[Bernhard Bauer, 2011-10-21 10:10:10]

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

http://codereview.chromium.org/7831075/diff/90002/chrome/renderer/content_set...
chrome/renderer/content_settings_observer.cc:216: if (top_origin == "null" &&
page_url.SchemeIsFile())
Are there other cases where we might want to use the URL instead? 
http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/third_party/WebKit/So...
says that the SecurityOrigin could be unique for sandboxed iframes as well.

Also, we might want to pull this out into a general method (for example, to use
it for plug-in content settings as well).

[Bernhard Bauer, 2011-10-21 10:10:28]

Oh, and LGTM.

[marja, 2011-10-21 10:25:05]

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

http://codereview.chromium.org/7831075/diff/90002/chrome/renderer/content_set...
chrome/renderer/content_settings_observer.cc:216: if (top_origin == "null" &&
page_url.SchemeIsFile())
On 2011/10/21 10:10:10, Bernhard Bauer wrote:
> Are there other cases where we might want to use the URL instead? 
>
http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/third_party/WebKit/So...
> says that the SecurityOrigin could be unique for sandboxed iframes as well.
> 
> Also, we might want to pull this out into a general method (for example, to
use
> it for plug-in content settings as well).

How about this version: use the doc url always when the top origin url is
unique? (I'd like to leave out from this CL the other Allow* funcs using this
func.) Still lgty?

[Bernhard Bauer, 2011-10-21 10:57:35]

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

http://codereview.chromium.org/7831075/diff/90002/chrome/renderer/content_set...
chrome/renderer/content_settings_observer.cc:216: if (top_origin == "null" &&
page_url.SchemeIsFile())
On 2011/10/21 10:25:05, marja wrote:
> On 2011/10/21 10:10:10, Bernhard Bauer wrote:
> > Are there other cases where we might want to use the URL instead? 
> >
>
http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/third_party/WebKit/So...
> > says that the SecurityOrigin could be unique for sandboxed iframes as well.
> > 
> > Also, we might want to pull this out into a general method (for example, to
> use
> > it for plug-in content settings as well).
> 
> How about this version: use the doc url always when the top origin url is
> unique? (I'd like to leave out from this CL the other Allow* funcs using this
> func.) Still lgty?

I guess both versions serve the file URL use case, which is what I was concerned
about, so LGTM. I don't know enough about all the other edge cases to judge how
we want to treat them.

[commit-bot: I haz the power, 2011-10-27 12:01:17]

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

[commit-bot: I haz the power, 2011-10-27 12:01:56]

Presubmit check for 7831075-99001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Warnings **
Found lines longer than 80 characters (first 5 shown).
  chrome/browser/ui/webui/options/content_settings_handler.cc, line 484, 81
chars

** Presubmit ERRORS **
Missing LGTM from an OWNER for: chrome/common/extensions/api/extension_api.json

Presubmit checks took 3.1s to calculate.

[Finnur, 2011-10-27 12:33:25]

extension_api.json LGTM.

On 2011/10/27 12:01:56, I haz the power (commit-bot) wrote:
> Presubmit check for 7831075-99001 failed and returned exit status 1.
> 
> Running presubmit commit checks ...
> 
> ** Presubmit Warnings **
> Found lines longer than 80 characters (first 5 shown).
>   chrome/browser/ui/webui/options/content_settings_handler.cc, line 484, 81
> chars
> 
> ** Presubmit ERRORS **
> Missing LGTM from an OWNER for:
chrome/common/extensions/api/extension_api.json
> 
> Presubmit checks took 3.1s to calculate.

[commit-bot: I haz the power, 2011-10-27 12:34:45]

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

[commit-bot: I haz the power, 2011-10-27 13:43:44]

Change committed as 107562