Send a message to the renderers when content settings change.

Send a message to the renderers when content settings change.

BUG=32719
TEST=none

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

[Nico, 2010-02-01 07:02:42]

Could something like this work, or is the approach completely off?

(Ignore the changes to preferences_window_controller.mm and Preferences.xib.
These are git artifacts; I'll revert 'em before checking in if this goes
anywhere)

[darin (slow to review), 2010-02-01 07:12:54]

http://codereview.chromium.org/551225/diff/3009/2012
File chrome/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/551225/diff/3009/2012#newcode571
chrome/browser/tab_contents/tab_contents.cc:571: GetURL().host(), content_type);
GetURL() returns NavigationEntry::virtual_url().  I think we actually want
NavigationEntry::url().  we want to operate on the actual URL of the page.

http://codereview.chromium.org/551225/diff/3009/2012#newcode2722
chrome/browser/tab_contents/tab_contents.cc:2722: std::string host =
GetURL().host();
ditto

[Nico, 2010-02-01 07:19:58]

http://codereview.chromium.org/551225/diff/3009/2012
File chrome/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/551225/diff/3009/2012#newcode571
chrome/browser/tab_contents/tab_contents.cc:571: GetURL().host(), content_type);
On 2010/02/01 07:12:54, darin wrote:
> GetURL() returns NavigationEntry::virtual_url().  I think we actually want
> NavigationEntry::url().  we want to operate on the actual URL of the page.

Done.

http://codereview.chromium.org/551225/diff/3009/2012#newcode2722
chrome/browser/tab_contents/tab_contents.cc:2722: std::string host =
GetURL().host();
On 2010/02/01 07:12:54, darin wrote:
> ditto

Done.

[darin (slow to review), 2010-02-01 07:47:42]

more detailed review this time.  mostly just style nits.

http://codereview.chromium.org/551225/diff/5002/6003
File chrome/browser/host_content_settings_map.cc (right):

http://codereview.chromium.org/551225/diff/5002/6003#newcode130
chrome/browser/host_content_settings_map.cc:130: NotifyObservers("");
nit: use std::string() instead of "" to save on generated code

http://codereview.chromium.org/551225/diff/5002/6003#newcode159
chrome/browser/host_content_settings_map.cc:159: }
maybe this would be a good case for an "else" statement?  that way you would
only need one call to NotifyObservers(host) at the very end of this file. 
returns in the middle of a function sometimes make the function harder to read.

http://codereview.chromium.org/551225/diff/5002/6003#newcode207
chrome/browser/host_content_settings_map.cc:207: NotifyObservers("");
ditto

http://codereview.chromium.org/551225/diff/5002/6003#newcode237
chrome/browser/host_content_settings_map.cc:237: NotifyObservers("");
ditto

http://codereview.chromium.org/551225/diff/5002/6004
File chrome/browser/host_content_settings_map.h (right):

http://codereview.chromium.org/551225/diff/5002/6004#newcode30
chrome/browser/host_content_settings_map.h:30: ContentSettingsDetails(const
std::string host) : host_(host) {}
nit: change parameter to "const std::string& host"

http://codereview.chromium.org/551225/diff/5002/6004#newcode34
chrome/browser/host_content_settings_map.h:34: std::string host_;
for structs with public members, trailing underscore naming should not be used. 
but perhaps you should just make this a class w/ only a public host() method? 
do you need the host to be modified after construction of the
ContentSettingsDetails object?

http://codereview.chromium.org/551225/diff/5002/6005
File chrome/browser/renderer_host/render_view_host.cc (right):

http://codereview.chromium.org/551225/diff/5002/6005#newcode1767
chrome/browser/renderer_host/render_view_host.cc:1767: Send(new
ViewMsg_SetContentSettingsForCurrentHost(
it looks like you should be able to kill this new line w/o exceeding the 80
column limit

http://codereview.chromium.org/551225/diff/5002/6007
File chrome/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/551225/diff/5002/6007#newcode573
chrome/browser/tab_contents/tab_contents.cc:573: return
profile()->GetHostContentSettingsMap()->GetContentSetting(
doesn't GetContentSetting return a ContentSetting value?  i think it would be
best to compare this return value against a ContentSetting value.

[Nico, 2010-02-01 17:00:49]

Thanks.

http://codereview.chromium.org/551225/diff/5002/6003
File chrome/browser/host_content_settings_map.cc (right):

http://codereview.chromium.org/551225/diff/5002/6003#newcode130
chrome/browser/host_content_settings_map.cc:130: NotifyObservers("");
On 2010/02/01 07:47:42, darin wrote:
> nit: use std::string() instead of "" to save on generated code 

Done.

http://codereview.chromium.org/551225/diff/5002/6003#newcode159
chrome/browser/host_content_settings_map.cc:159: }
On 2010/02/01 07:47:42, darin wrote:
> maybe this would be a good case for an "else" statement?  that way you would
> only need one call to NotifyObservers(host) at the very end of this file. 
> returns in the middle of a function sometimes make the function harder to
read.

Done.

http://codereview.chromium.org/551225/diff/5002/6003#newcode207
chrome/browser/host_content_settings_map.cc:207: NotifyObservers("");
On 2010/02/01 07:47:42, darin wrote:
> ditto

Done.

http://codereview.chromium.org/551225/diff/5002/6003#newcode237
chrome/browser/host_content_settings_map.cc:237: NotifyObservers("");
On 2010/02/01 07:47:42, darin wrote:
> ditto

Done.

http://codereview.chromium.org/551225/diff/5002/6004
File chrome/browser/host_content_settings_map.h (right):

http://codereview.chromium.org/551225/diff/5002/6004#newcode30
chrome/browser/host_content_settings_map.h:30: ContentSettingsDetails(const
std::string host) : host_(host) {}
On 2010/02/01 07:47:42, darin wrote:
> nit: change parameter to "const std::string& host"

Done.

http://codereview.chromium.org/551225/diff/5002/6004#newcode34
chrome/browser/host_content_settings_map.h:34: std::string host_;
On 2010/02/01 07:47:42, darin wrote:
> for structs with public members, trailing underscore naming should not be
used. 
> but perhaps you should just make this a class w/ only a public host() method? 
> do you need the host to be modified after construction of the
> ContentSettingsDetails object?

No. Done.

http://codereview.chromium.org/551225/diff/5002/6005
File chrome/browser/renderer_host/render_view_host.cc (right):

http://codereview.chromium.org/551225/diff/5002/6005#newcode1767
chrome/browser/renderer_host/render_view_host.cc:1767: Send(new
ViewMsg_SetContentSettingsForCurrentHost(
On 2010/02/01 07:47:42, darin wrote:
> it looks like you should be able to kill this new line w/o exceeding the 80
> column limit

Done.

http://codereview.chromium.org/551225/diff/5002/6007
File chrome/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/551225/diff/5002/6007#newcode573
chrome/browser/tab_contents/tab_contents.cc:573: return
profile()->GetHostContentSettingsMap()->GetContentSetting(
On 2010/02/01 07:47:42, darin wrote:
> doesn't GetContentSetting return a ContentSetting value?  i think it would be
> best to compare this return value against a ContentSetting value.

*blush*

Done.

[Peter Kasting, 2010-02-01 18:24:06]

http://codereview.chromium.org/551225/diff/3011/3015
File chrome/browser/host_content_settings_map.h (right):

http://codereview.chromium.org/551225/diff/3011/3015#newcode28
chrome/browser/host_content_settings_map.h:28: // Details for the
CONTENT_SETTINGS_CHANGED notification.
Nit: It's not totally clear from here what listeners will be using the "host"
field for and thus whether, if the user does something that clears three hosts
at once, this class should prefer to send three notifications that specify
hosts, or one that doesn't.  Maybe you can write a comment with some insight on
that?

http://codereview.chromium.org/551225/diff/3011/3015#newcode32
chrome/browser/host_content_settings_map.h:32: // The host whose settings have
changed. Empty if all hosts are affected (
Nit: Maybe "all hosts" should be "many hosts"?  Also, paren on next line.

http://codereview.chromium.org/551225/diff/3011/3015#newcode35
chrome/browser/host_content_settings_map.h:35: private:
Nit: Blank line before this?

http://codereview.chromium.org/551225/diff/3011/3015#newcode115
chrome/browser/host_content_settings_map.h:115: void
SetSettingsFromDictionary(const DictionaryValue* dictionary,
Nit: I think the old name was clearer.  "Set" sounds like we're modifying the
dictionary.

http://codereview.chromium.org/551225/diff/3011/3016
File chrome/browser/renderer_host/render_view_host.cc (right):

http://codereview.chromium.org/551225/diff/3011/3016#newcode1767
chrome/browser/renderer_host/render_view_host.cc:1767: Send(new
ViewMsg_SetContentSettingsForCurrentHost(host, settings));
Curiosity: Is the renderer side of this IPC message already hooked up?

http://codereview.chromium.org/551225/diff/3011/3018
File chrome/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/551225/diff/3011/3018#newcode573
chrome/browser/tab_contents/tab_contents.cc:573: return
profile()->GetHostContentSettingsMap()->GetContentSetting(
This is wrong.  This function needs to return whether content of the given type
has actually been blocked on the current page, not whether it should be blocked.

You're not prepared to implement this function in this changelist.

http://codereview.chromium.org/551225/diff/3011/3018#newcode2723
chrome/browser/tab_contents/tab_contents.cc:2723:
Details<HostContentSettingsMap::ContentSettingsDetails> settings_details(
Nit: Might be fractionally more readable to wrap before settings_details?

[Nico, 2010-02-01 22:00:27]

I can't see the mac unit_test failure on my local box. Maybe it's only visible
in release builds. I'm compiling one right now.

http://codereview.chromium.org/551225/diff/3011/3015
File chrome/browser/host_content_settings_map.h (right):

http://codereview.chromium.org/551225/diff/3011/3015#newcode28
chrome/browser/host_content_settings_map.h:28: // Details for the
CONTENT_SETTINGS_CHANGED notification.
On 2010/02/01 18:24:06, Peter Kasting wrote:
> Nit: It's not totally clear from here what listeners will be using the "host"
> field for and thus whether, if the user does something that clears three hosts
> at once, this class should prefer to send three notifications that specify
> hosts, or one that doesn't.  Maybe you can write a comment with some insight
on
> that?

Done, I think.

http://codereview.chromium.org/551225/diff/3011/3015#newcode32
chrome/browser/host_content_settings_map.h:32: // The host whose settings have
changed. Empty if all hosts are affected (
On 2010/02/01 18:24:06, Peter Kasting wrote:
> Nit: Maybe "all hosts" should be "many hosts"?  Also, paren on next line.

Done.

http://codereview.chromium.org/551225/diff/3011/3015#newcode35
chrome/browser/host_content_settings_map.h:35: private:
On 2010/02/01 18:24:06, Peter Kasting wrote:
> Nit: Blank line before this?

Done.

http://codereview.chromium.org/551225/diff/3011/3015#newcode115
chrome/browser/host_content_settings_map.h:115: void
SetSettingsFromDictionary(const DictionaryValue* dictionary,
On 2010/02/01 18:24:06, Peter Kasting wrote:
> Nit: I think the old name was clearer.  "Set" sounds like we're modifying the
> dictionary.

Hm, I disliked the old name because it started with "Get" but modified |this|.

Changed it back anyway, done.

http://codereview.chromium.org/551225/diff/3011/3016
File chrome/browser/renderer_host/render_view_host.cc (right):

http://codereview.chromium.org/551225/diff/3011/3016#newcode1767
chrome/browser/renderer_host/render_view_host.cc:1767: Send(new
ViewMsg_SetContentSettingsForCurrentHost(host, settings));
On 2010/02/01 18:24:06, Peter Kasting wrote:
> Curiosity: Is the renderer side of this IPC message already hooked up?

Yes, that was in my CL from friday (
http://codereview.chromium.org/549218/diff/1012/10 )

http://codereview.chromium.org/551225/diff/3011/3018
File chrome/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/551225/diff/3011/3018#newcode573
chrome/browser/tab_contents/tab_contents.cc:573: return
profile()->GetHostContentSettingsMap()->GetContentSetting(
On 2010/02/01 18:24:06, Peter Kasting wrote:
> This is wrong.  This function needs to return whether content of the given
type
> has actually been blocked on the current page, not whether it should be
blocked.
> 
> You're not prepared to implement this function in this changelist.

Ooh, I see. The renderer is going to send an IPC message when it blocks
something. Reverted this to the TODO.

(Curiosity: Will changing the "block" setting cause a page reload? Or does
webkit have a way to unblock images/scripts/plugins on a page without a complete
reload?)

http://codereview.chromium.org/551225/diff/3011/3018#newcode2723
chrome/browser/tab_contents/tab_contents.cc:2723:
Details<HostContentSettingsMap::ContentSettingsDetails> settings_details(
On 2010/02/01 18:24:06, Peter Kasting wrote:
> Nit: Might be fractionally more readable to wrap before settings_details?

Done.

[Peter Kasting, 2010-02-01 22:17:23]

LGTM

[Nico, 2010-02-02 01:02:56]

On 2010/02/01 22:17:23, Peter Kasting wrote:
> LGTM

In between 80% stuff, I think I managed to track down what turned the mac bot
red. I added an explicit test for that (and for some other stuff). Hopefully,
the trybots now turn green.

Maybe you want to take another look.

[Peter Kasting, 2010-02-02 01:11:47]

LGTM

http://codereview.chromium.org/551225/diff/11009/10004
File chrome/browser/host_content_settings_map.cc (right):

http://codereview.chromium.org/551225/diff/11009/10004#newcode196
chrome/browser/host_content_settings_map.cc:196: bool found =
all_settings_dictionary->GetDictionaryWithoutPathExpansion(
Nit: Line >80 characters?

http://codereview.chromium.org/551225/diff/11009/10006
File chrome/browser/host_content_settings_map_unittest.cc (right):

http://codereview.chromium.org/551225/diff/11009/10006#newcode7
chrome/browser/host_content_settings_map_unittest.cc:7: #include
"chrome/common/notification_observer.h"
Nit: The observer and type #includes aren't necessary (the other two #includes
pull them in)

http://codereview.chromium.org/551225/diff/11009/10006#newcode51
chrome/browser/host_content_settings_map_unittest.cc:51: private:
Nit: Blank line before this?

[Nico, 2010-02-02 01:19:00]

http://codereview.chromium.org/551225/diff/11009/10004
File chrome/browser/host_content_settings_map.cc (right):

http://codereview.chromium.org/551225/diff/11009/10004#newcode196
chrome/browser/host_content_settings_map.cc:196: bool found =
all_settings_dictionary->GetDictionaryWithoutPathExpansion(
On 2010/02/02 01:11:47, Peter Kasting wrote:
> Nit: Line >80 characters?

Done.

http://codereview.chromium.org/551225/diff/11009/10006
File chrome/browser/host_content_settings_map_unittest.cc (right):

http://codereview.chromium.org/551225/diff/11009/10006#newcode7
chrome/browser/host_content_settings_map_unittest.cc:7: #include
"chrome/common/notification_observer.h"
On 2010/02/02 01:11:47, Peter Kasting wrote:
> Nit: The observer and type #includes aren't necessary (the other two #includes
> pull them in)

Done.

http://codereview.chromium.org/551225/diff/11009/10006#newcode51
chrome/browser/host_content_settings_map_unittest.cc:51: private:
On 2010/02/02 01:11:47, Peter Kasting wrote:
> Nit: Blank line before this?

Done. Sorry about doing this wrong twice.