Allow default desktop content settings to be managed via policy

Allow default desktop content settings and default geolocation settings to be managed via policy. (Support for the Cocoa UI will be added via a separate CL)

BUG=63190, 63187, 63180
TEST=none

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

[markusheintz_, 2010-11-30 14:57:39]

Pls review my CL please. Thanks :)

[jochen (gone - plz use gerrit), 2010-11-30 15:32:49]

what about cocoa and dom UI?

http://codereview.chromium.org/5398001/diff/4001/chrome/browser/notifications...
File chrome/browser/notifications/desktop_notification_service.h (right):

http://codereview.chromium.org/5398001/diff/4001/chrome/browser/notifications...
chrome/browser/notifications/desktop_notification_service.h:96: bool
IsDefaultContentSettingManaged();
can this be const?

[markusheintz_, 2010-11-30 15:45:43]

I forgot the DOM-UI.

Could I fix that in a separate CL? I already got another CL for the Geolocation
settings that depends on this one. I could fix the DOMUI for both ContentTypes
(notifications and location) with on CL then.

http://codereview.chromium.org/5398001/diff/4001/chrome/browser/notifications...
File chrome/browser/notifications/desktop_notification_service.h (right):

http://codereview.chromium.org/5398001/diff/4001/chrome/browser/notifications...
chrome/browser/notifications/desktop_notification_service.h:96: bool
IsDefaultContentSettingManaged();
On 2010/11/30 15:32:49, jochen wrote:
> can this be const?

Absolutely.

[markusheintz_, 2010-11-30 15:49:27]

Since cocoa UI changes are also missing I'll add them and the DOM_UI stuff here.

On 2010/11/30 15:45:43, markusheintz_ wrote:
> I forgot the DOM-UI.
> 
> Could I fix that in a separate CL? I already got another CL for the
Geolocation
> settings that depends on this one. I could fix the DOMUI for both ContentTypes
> (notifications and location) with on CL then.
> 
>
http://codereview.chromium.org/5398001/diff/4001/chrome/browser/notifications...
> File chrome/browser/notifications/desktop_notification_service.h (right):
> 
>
http://codereview.chromium.org/5398001/diff/4001/chrome/browser/notifications...
> chrome/browser/notifications/desktop_notification_service.h:96: bool
> IsDefaultContentSettingManaged();
> On 2010/11/30 15:32:49, jochen wrote:
> > can this be const?
> 
> Absolutely.

[markusheintz_, 2010-12-06 09:32:20]

Added DOM-UI stuff. MAC is still missing but I'd like to do that in a separate
CL.

Pls review. Thanks!


On 2010/11/30 15:49:27, markusheintz_ wrote:
> Since cocoa UI changes are also missing I'll add them and the DOM_UI stuff
here.
> 
> On 2010/11/30 15:45:43, markusheintz_ wrote:
> > I forgot the DOM-UI.
> > 
> > Could I fix that in a separate CL? I already got another CL for the
> Geolocation
> > settings that depends on this one. I could fix the DOMUI for both
ContentTypes
> > (notifications and location) with on CL then.
> > 
> >
>
http://codereview.chromium.org/5398001/diff/4001/chrome/browser/notifications...
> > File chrome/browser/notifications/desktop_notification_service.h (right):
> > 
> >
>
http://codereview.chromium.org/5398001/diff/4001/chrome/browser/notifications...
> > chrome/browser/notifications/desktop_notification_service.h:96: bool
> > IsDefaultContentSettingManaged();
> > On 2010/11/30 15:32:49, jochen wrote:
> > > can this be const?
> > 
> > Absolutely.

[markusheintz_, 2010-12-06 12:44:14]

FYI the trybots are ok. The test that fails is actually disabled due to bug
http://crbug.com/65462. (or see 
chrome/browser/web_resource/web_resource_service_unittest.cc:97)



On 2010/12/06 09:32:20, markusheintz_ wrote:
> Added DOM-UI stuff. MAC is still missing but I'd like to do that in a separate
> CL.
> 
> Pls review. Thanks!
> 
> 
> On 2010/11/30 15:49:27, markusheintz_ wrote:
> > Since cocoa UI changes are also missing I'll add them and the DOM_UI stuff
> here.
> > 
> > On 2010/11/30 15:45:43, markusheintz_ wrote:
> > > I forgot the DOM-UI.
> > > 
> > > Could I fix that in a separate CL? I already got another CL for the
> > Geolocation
> > > settings that depends on this one. I could fix the DOMUI for both
> ContentTypes
> > > (notifications and location) with on CL then.
> > > 
> > >
> >
>
http://codereview.chromium.org/5398001/diff/4001/chrome/browser/notifications...
> > > File chrome/browser/notifications/desktop_notification_service.h (right):
> > > 
> > >
> >
>
http://codereview.chromium.org/5398001/diff/4001/chrome/browser/notifications...
> > > chrome/browser/notifications/desktop_notification_service.h:96: bool
> > > IsDefaultContentSettingManaged();
> > > On 2010/11/30 15:32:49, jochen wrote:
> > > > can this be const?
> > > 
> > > Absolutely.

[jochen (gone - plz use gerrit), 2010-12-07 09:43:52]

http://codereview.chromium.org/5398001/diff/29001/chrome/browser/dom_ui/optio...
File chrome/browser/dom_ui/options/content_settings_handler.h (right):

http://codereview.chromium.org/5398001/diff/29001/chrome/browser/dom_ui/optio...
chrome/browser/dom_ui/options/content_settings_handler.h:99: bool
GetDefaultSettingManagedFromModel(ContentSettingsType type);
GetManagedDefaultSettingFromModel? Also, please add a comment what this does

http://codereview.chromium.org/5398001/diff/29001/chrome/browser/geolocation/...
File chrome/browser/geolocation/geolocation_content_settings_map.cc (right):

http://codereview.chromium.org/5398001/diff/29001/chrome/browser/geolocation/...
chrome/browser/geolocation/geolocation_content_settings_map.cc:24: #include
"chrome/common/content_settings_types.h"
why did you need this?

http://codereview.chromium.org/5398001/diff/29001/chrome/browser/geolocation/...
chrome/browser/geolocation/geolocation_content_settings_map.cc:63: return
profile_->GetPrefs()->IsManagedPreference(
you should also watch for PROFILE_DESTROYED

http://codereview.chromium.org/5398001/diff/29001/chrome/browser/geolocation/...
chrome/browser/geolocation/geolocation_content_settings_map.cc:193: if
(NotificationType::PREF_CHANGED == type) {
hum, why are you doing this here instead of sending the notification when
changing the setting? this looks as if it would interact badly with chrome sync

http://codereview.chromium.org/5398001/diff/29001/chrome/browser/geolocation/...
File chrome/browser/geolocation/geolocation_content_settings_map.h (right):

http://codereview.chromium.org/5398001/diff/29001/chrome/browser/geolocation/...
chrome/browser/geolocation/geolocation_content_settings_map.h:20: #include
"chrome/browser/content_settings/content_settings_details.h"
a forward declaration should be enough

http://codereview.chromium.org/5398001/diff/29001/chrome/browser/geolocation/...
chrome/browser/geolocation/geolocation_content_settings_map.h:46: // Returns
whether the content setting is managed (set by a policy).
True if ...

http://codereview.chromium.org/5398001/diff/29001/chrome/browser/geolocation/...
chrome/browser/geolocation/geolocation_content_settings_map.h:115: //
comment?

http://codereview.chromium.org/5398001/diff/29001/chrome/browser/gtk/options/...
File chrome/browser/gtk/options/content_filter_page_gtk.cc (right):

http://codereview.chromium.org/5398001/diff/29001/chrome/browser/gtk/options/...
chrome/browser/gtk/options/content_filter_page_gtk.cc:270: 
why the newline?

http://codereview.chromium.org/5398001/diff/29001/chrome/browser/resources/op...
File chrome/browser/resources/options/content_settings.js (right):

http://codereview.chromium.org/5398001/diff/29001/chrome/browser/resources/op...
chrome/browser/resources/options/content_settings.js:85: for (var i=0,
len=radios.length; i<len; ++i) {
spaces around operators, and i++

http://codereview.chromium.org/5398001/diff/29001/chrome/browser/resources/op...
chrome/browser/resources/options/content_settings.js:88: 
no newline please

http://codereview.chromium.org/5398001/diff/29001/chrome/browser/ui/views/opt...
File chrome/browser/ui/views/options/content_filter_page_view.cc (right):

http://codereview.chromium.org/5398001/diff/29001/chrome/browser/ui/views/opt...
chrome/browser/ui/views/options/content_filter_page_view.cc:165: if
(content_type_ == CONTENT_SETTINGS_TYPE_GEOLOCATION) {
you forgot geolocation?

[markusheintz_, 2010-12-07 11:46:15]

I'm watching for PROFILE_DESTROYED notification now. In case the profile gets
destroyed I set it to NULL.

The GetYYY methods do not check if the profile is null, this seems kind of
dangerous for me, but you can find the similar pattern in the
desktop_notification_service. So I just wanted to double check with you in case
I miss something. Otherwise I'll cache the pref values in the
geolocation_content_settings_map and serve the last know value in case the
profile get's destroyed.

http://codereview.chromium.org/5398001/diff/29001/chrome/browser/dom_ui/optio...
File chrome/browser/dom_ui/options/content_settings_handler.h (right):

http://codereview.chromium.org/5398001/diff/29001/chrome/browser/dom_ui/optio...
chrome/browser/dom_ui/options/content_settings_handler.h:99: bool
GetDefaultSettingManagedFromModel(ContentSettingsType type);
On 2010/12/07 09:43:52, jochen wrote:
> GetManagedDefaultSettingFromModel? Also, please add a comment what this does

Done.

http://codereview.chromium.org/5398001/diff/29001/chrome/browser/geolocation/...
File chrome/browser/geolocation/geolocation_content_settings_map.cc (right):

http://codereview.chromium.org/5398001/diff/29001/chrome/browser/geolocation/...
chrome/browser/geolocation/geolocation_content_settings_map.cc:24: #include
"chrome/common/content_settings_types.h"
On 2010/12/07 09:43:52, jochen wrote:
> why did you need this?

removed

http://codereview.chromium.org/5398001/diff/29001/chrome/browser/geolocation/...
chrome/browser/geolocation/geolocation_content_settings_map.cc:63: return
profile_->GetPrefs()->IsManagedPreference(
On 2010/12/07 09:43:52, jochen wrote:
> you should also watch for PROFILE_DESTROYED

Done.

http://codereview.chromium.org/5398001/diff/29001/chrome/browser/geolocation/...
chrome/browser/geolocation/geolocation_content_settings_map.cc:193: if
(NotificationType::PREF_CHANGED == type) {
On 2010/12/07 09:43:52, jochen wrote:
> hum, why are you doing this here instead of sending the notification when
> changing the setting? this looks as if it would interact badly with chrome
sync

Because I must listen for PREF_CHANGED events that are triggered through the
policy changes. When a managed value changes then a PREF_CHANGED notification is
sent. If I don't listen for them here I miss when policies get updated.

What's the problem her with sync?

http://codereview.chromium.org/5398001/diff/29001/chrome/browser/geolocation/...
File chrome/browser/geolocation/geolocation_content_settings_map.h (right):

http://codereview.chromium.org/5398001/diff/29001/chrome/browser/geolocation/...
chrome/browser/geolocation/geolocation_content_settings_map.h:20: #include
"chrome/browser/content_settings/content_settings_details.h"
On 2010/12/07 09:43:52, jochen wrote:
> a forward declaration should be enough

Done.

http://codereview.chromium.org/5398001/diff/29001/chrome/browser/geolocation/...
chrome/browser/geolocation/geolocation_content_settings_map.h:46: // Returns
whether the content setting is managed (set by a policy).
On 2010/12/07 09:43:52, jochen wrote:
> True if ...

True if the content setting is managed.

http://codereview.chromium.org/5398001/diff/29001/chrome/browser/geolocation/...
chrome/browser/geolocation/geolocation_content_settings_map.h:115: //
On 2010/12/07 09:43:52, jochen wrote:
> comment?

sry.

Added a comment.

http://codereview.chromium.org/5398001/diff/29001/chrome/browser/gtk/options/...
File chrome/browser/gtk/options/content_filter_page_gtk.cc (right):

http://codereview.chromium.org/5398001/diff/29001/chrome/browser/gtk/options/...
chrome/browser/gtk/options/content_filter_page_gtk.cc:270: 
On 2010/12/07 09:43:52, jochen wrote:
> why the newline?

sry. removed the newline. looks like an accident.

http://codereview.chromium.org/5398001/diff/29001/chrome/browser/resources/op...
File chrome/browser/resources/options/content_settings.js (right):

http://codereview.chromium.org/5398001/diff/29001/chrome/browser/resources/op...
chrome/browser/resources/options/content_settings.js:85: for (var i=0,
len=radios.length; i<len; ++i) {
On 2010/12/07 09:43:52, jochen wrote:
> spaces around operators, and i++

Done.

http://codereview.chromium.org/5398001/diff/29001/chrome/browser/resources/op...
chrome/browser/resources/options/content_settings.js:88: 
On 2010/12/07 09:43:52, jochen wrote:
> no newline please

Done.

http://codereview.chromium.org/5398001/diff/29001/chrome/browser/ui/views/opt...
File chrome/browser/ui/views/options/content_filter_page_view.cc (right):

http://codereview.chromium.org/5398001/diff/29001/chrome/browser/ui/views/opt...
chrome/browser/ui/views/options/content_filter_page_view.cc:165: if
(content_type_ == CONTENT_SETTINGS_TYPE_GEOLOCATION) {
On 2010/12/07 09:43:52, jochen wrote:
> you forgot geolocation?

Done.

[jochen (gone - plz use gerrit), 2010-12-07 18:55:55]

http://codereview.chromium.org/5398001/diff/39001/chrome/browser/dom_ui/optio...
File chrome/browser/dom_ui/options/content_settings_handler.h (right):

http://codereview.chromium.org/5398001/diff/39001/chrome/browser/dom_ui/optio...
chrome/browser/dom_ui/options/content_settings_handler.h:99: // Returns whether
the default setting for the given content settings type
True if...

http://codereview.chromium.org/5398001/diff/39001/chrome/browser/geolocation/...
File chrome/browser/geolocation/geolocation_content_settings_map.cc (right):

http://codereview.chromium.org/5398001/diff/39001/chrome/browser/geolocation/...
chrome/browser/geolocation/geolocation_content_settings_map.cc:57: const
PrefService* prefs = profile_->GetPrefs();
you should make sure that profile_ actually exists (here and below)

http://codereview.chromium.org/5398001/diff/39001/chrome/browser/geolocation/...
chrome/browser/geolocation/geolocation_content_settings_map.cc:199:
NotifyObservers(ContentSettingsDetails(
Since this map always reads the settings out of the pref service, I think it's
ok to always send the notification here.

You could also consider to inline NotifyObservers since it's only used once here

http://codereview.chromium.org/5398001/diff/39001/chrome/browser/geolocation/...
File chrome/browser/geolocation/geolocation_content_settings_map.h (right):

http://codereview.chromium.org/5398001/diff/39001/chrome/browser/geolocation/...
chrome/browser/geolocation/geolocation_content_settings_map.h:47: // Returns
whether the content setting is managed (set by a policy).
True if...

http://codereview.chromium.org/5398001/diff/39001/chrome/browser/geolocation/...
chrome/browser/geolocation/geolocation_content_settings_map.h:92: virtual void
Observe(NotificationType type,
why is this not private?

http://codereview.chromium.org/5398001/diff/39001/chrome/browser/geolocation/...
chrome/browser/geolocation/geolocation_content_settings_map.h:120: 
no empty line

[markusheintz_, 2010-12-08 14:41:39]

http://codereview.chromium.org/5398001/diff/39001/chrome/browser/dom_ui/optio...
File chrome/browser/dom_ui/options/content_settings_handler.h (right):

http://codereview.chromium.org/5398001/diff/39001/chrome/browser/dom_ui/optio...
chrome/browser/dom_ui/options/content_settings_handler.h:99: // Returns whether
the default setting for the given content settings type
On 2010/12/07 18:55:55, jochen wrote:
> True if...

Done.

http://codereview.chromium.org/5398001/diff/39001/chrome/browser/geolocation/...
File chrome/browser/geolocation/geolocation_content_settings_map.cc (right):

http://codereview.chromium.org/5398001/diff/39001/chrome/browser/geolocation/...
chrome/browser/geolocation/geolocation_content_settings_map.cc:57: const
PrefService* prefs = profile_->GetPrefs();
On 2010/12/07 18:55:55, jochen wrote:
> you should make sure that profile_ actually exists (here and below)

Done.

http://codereview.chromium.org/5398001/diff/39001/chrome/browser/geolocation/...
chrome/browser/geolocation/geolocation_content_settings_map.cc:199:
NotifyObservers(ContentSettingsDetails(
On 2010/12/07 18:55:55, jochen wrote:
> Since this map always reads the settings out of the pref service, I think it's
> ok to always send the notification here.
> 
> You could also consider to inline NotifyObservers since it's only used once
here

If you don't mind I'd like to keep it separate.

http://codereview.chromium.org/5398001/diff/39001/chrome/browser/geolocation/...
File chrome/browser/geolocation/geolocation_content_settings_map.h (right):

http://codereview.chromium.org/5398001/diff/39001/chrome/browser/geolocation/...
chrome/browser/geolocation/geolocation_content_settings_map.h:47: // Returns
whether the content setting is managed (set by a policy).
On 2010/12/07 18:55:55, jochen wrote:
> True if...

Done.

http://codereview.chromium.org/5398001/diff/39001/chrome/browser/geolocation/...
chrome/browser/geolocation/geolocation_content_settings_map.h:92: virtual void
Observe(NotificationType type,
On 2010/12/07 18:55:55, jochen wrote:
> why is this not private?

Maybe I get something wrong, but if this is a Observer, then probably someone
else is calling the Observe method. So it shouldn't be private.

http://codereview.chromium.org/5398001/diff/39001/chrome/browser/geolocation/...
chrome/browser/geolocation/geolocation_content_settings_map.h:120: 
On 2010/12/07 18:55:55, jochen wrote:
> no empty line

removed

[jochen (gone - plz use gerrit), 2010-12-08 14:57:43]

http://codereview.chromium.org/5398001/diff/44001/chrome/browser/geolocation/...
File chrome/browser/geolocation/geolocation_content_settings_map.cc (right):

http://codereview.chromium.org/5398001/diff/44001/chrome/browser/geolocation/...
chrome/browser/geolocation/geolocation_content_settings_map.cc:206: void
GeolocationContentSettingsMap::NotifyObservers(
please add a unit test that the notifications are send correctly.

http://codereview.chromium.org/5398001/diff/44001/chrome/browser/geolocation/...
File chrome/browser/geolocation/geolocation_content_settings_map.h (right):

http://codereview.chromium.org/5398001/diff/44001/chrome/browser/geolocation/...
chrome/browser/geolocation/geolocation_content_settings_map.h:122: bool
updating_preferences_;
in this map, you don't need this, since you only send the notifications from the
observer anyway

[markusheintz_, 2010-12-09 14:27:01]

http://codereview.chromium.org/5398001/diff/44001/chrome/browser/geolocation/...
File chrome/browser/geolocation/geolocation_content_settings_map.cc (right):

http://codereview.chromium.org/5398001/diff/44001/chrome/browser/geolocation/...
chrome/browser/geolocation/geolocation_content_settings_map.cc:206: void
GeolocationContentSettingsMap::NotifyObservers(
On 2010/12/08 14:57:43, jochen wrote:
> please add a unit test that the notifications are send correctly.

Done.

http://codereview.chromium.org/5398001/diff/44001/chrome/browser/geolocation/...
File chrome/browser/geolocation/geolocation_content_settings_map.h (right):

http://codereview.chromium.org/5398001/diff/44001/chrome/browser/geolocation/...
chrome/browser/geolocation/geolocation_content_settings_map.h:122: bool
updating_preferences_;
On 2010/12/08 14:57:43, jochen wrote:
> in this map, you don't need this, since you only send the notifications from
the
> observer anyway

removed again

[jochen (gone - plz use gerrit), 2010-12-09 14:36:28]

http://codereview.chromium.org/5398001/diff/50001/chrome/browser/geolocation/...
File chrome/browser/geolocation/geolocation_content_settings_map.cc (right):

http://codereview.chromium.org/5398001/diff/50001/chrome/browser/geolocation/...
chrome/browser/geolocation/geolocation_content_settings_map.cc:84: // If the
profile is destroyed (and set to NULL) return true.
true -> CONTENT_SETTING_BLOCK

http://codereview.chromium.org/5398001/diff/50001/chrome/browser/geolocation/...
chrome/browser/geolocation/geolocation_content_settings_map.cc:183:
requesting_origin_settings_dictionary->SetWithoutPathExpansion(
no indent

http://codereview.chromium.org/5398001/diff/50001/chrome/browser/geolocation/...
chrome/browser/geolocation/geolocation_content_settings_map.cc:200:
NotificationType::CONTENT_SETTINGS_CHANGED,
hum, now that I see this (and ran into a similar bug this morning): this doesn't
work. The receiver will try to cast Source<> to a host content settings map.
You'll need a separate notification type

http://codereview.chromium.org/5398001/diff/50001/chrome/browser/geolocation/...
chrome/browser/geolocation/geolocation_content_settings_map.cc:209: if
(NotificationType::PREF_CHANGED == type) {
type == NotificationType::PREF_CHANGED (see coding style)

http://codereview.chromium.org/5398001/diff/50001/chrome/browser/geolocation/...
File chrome/browser/geolocation/geolocation_content_settings_map_unittest.cc
(right):

http://codereview.chromium.org/5398001/diff/50001/chrome/browser/geolocation/...
chrome/browser/geolocation/geolocation_content_settings_map_unittest.cc:40: //
last_notifier->GetContentSettings(GURL("http://random-hostname.com/"));
why is this commented out?

[markusheintz_, 2010-12-15 12:15:58]

sry for the long delay, but my cl is unblocked now. I addressed all comments.
PLS check if it is ok to submit after the trybot pass green.

http://codereview.chromium.org/5398001/diff/50001/chrome/browser/geolocation/...
File chrome/browser/geolocation/geolocation_content_settings_map.cc (right):

http://codereview.chromium.org/5398001/diff/50001/chrome/browser/geolocation/...
chrome/browser/geolocation/geolocation_content_settings_map.cc:84: // If the
profile is destroyed (and set to NULL) return true.
On 2010/12/09 14:36:28, jochen wrote:
> true -> CONTENT_SETTING_BLOCK

Done.

http://codereview.chromium.org/5398001/diff/50001/chrome/browser/geolocation/...
chrome/browser/geolocation/geolocation_content_settings_map.cc:183:
requesting_origin_settings_dictionary->SetWithoutPathExpansion(
On 2010/12/09 14:36:28, jochen wrote:
> no indent

Done.

http://codereview.chromium.org/5398001/diff/50001/chrome/browser/geolocation/...
chrome/browser/geolocation/geolocation_content_settings_map.cc:200:
NotificationType::CONTENT_SETTINGS_CHANGED,
On 2010/12/09 14:36:28, jochen wrote:
> hum, now that I see this (and ran into a similar bug this morning): this
doesn't
> work. The receiver will try to cast Source<> to a host content settings map.
> You'll need a separate notification type

Done.

http://codereview.chromium.org/5398001/diff/50001/chrome/browser/geolocation/...
chrome/browser/geolocation/geolocation_content_settings_map.cc:209: if
(NotificationType::PREF_CHANGED == type) {
On 2010/12/09 14:36:28, jochen wrote:
> type == NotificationType::PREF_CHANGED (see coding style)

Done.

http://codereview.chromium.org/5398001/diff/50001/chrome/browser/geolocation/...
File chrome/browser/geolocation/geolocation_content_settings_map_unittest.cc
(right):

http://codereview.chromium.org/5398001/diff/50001/chrome/browser/geolocation/...
chrome/browser/geolocation/geolocation_content_settings_map_unittest.cc:40: //
last_notifier->GetContentSettings(GURL("http://random-hostname.com/"));
On 2010/12/09 14:36:28, jochen wrote:
> why is this commented out?

Done.

[jochen (gone - plz use gerrit), 2010-12-15 14:02:11]

adding danno for the policy stuff. Doesn't this require some update to the
templates?

Otherwise the CL is nearly done, just make it compile on windows :)

http://codereview.chromium.org/5398001/diff/61001/chrome/browser/geolocation/...
File chrome/browser/geolocation/geolocation_content_settings_map.h (right):

http://codereview.chromium.org/5398001/diff/61001/chrome/browser/geolocation/...
chrome/browser/geolocation/geolocation_content_settings_map.h:107:
~GeolocationContentSettingsMap();
destructor should come before other methods. also, please group the static
methods together

http://codereview.chromium.org/5398001/diff/61001/chrome/browser/geolocation/...
File chrome/browser/geolocation/geolocation_content_settings_map_unittest.cc
(right):

http://codereview.chromium.org/5398001/diff/61001/chrome/browser/geolocation/...
chrome/browser/geolocation/geolocation_content_settings_map_unittest.cc:40:
last_notifier->GetContentSettings(GURL("http://random-hostname.com/"));
i think that doesn't work

[markusheintz_, 2010-12-15 14:08:35]

The policy template will be updated with a separate CL.

http://codereview.chromium.org/5398001/diff/61001/chrome/browser/geolocation/...
File chrome/browser/geolocation/geolocation_content_settings_map.h (right):

http://codereview.chromium.org/5398001/diff/61001/chrome/browser/geolocation/...
chrome/browser/geolocation/geolocation_content_settings_map.h:107:
~GeolocationContentSettingsMap();
On 2010/12/15 14:02:11, jochen wrote:
> destructor should come before other methods. also, please group the static
> methods together

Done.

http://codereview.chromium.org/5398001/diff/61001/chrome/browser/geolocation/...
File chrome/browser/geolocation/geolocation_content_settings_map_unittest.cc
(right):

http://codereview.chromium.org/5398001/diff/61001/chrome/browser/geolocation/...
chrome/browser/geolocation/geolocation_content_settings_map_unittest.cc:40:
last_notifier->GetContentSettings(GURL("http://random-hostname.com/"));
On 2010/12/15 14:02:11, jochen wrote:
> i think that doesn't work

Already fixed.

[markusheintz_, 2010-12-15 14:20:00]

FYI: The Windows try bot failure is already fixed, but not updated here.
On 2010/12/15 14:08:35, markusheintz_ wrote:
> The policy template will be updated with a separate CL.
> 
>
http://codereview.chromium.org/5398001/diff/61001/chrome/browser/geolocation/...
> File chrome/browser/geolocation/geolocation_content_settings_map.h (right):
> 
>
http://codereview.chromium.org/5398001/diff/61001/chrome/browser/geolocation/...
> chrome/browser/geolocation/geolocation_content_settings_map.h:107:
> ~GeolocationContentSettingsMap();
> On 2010/12/15 14:02:11, jochen wrote:
> > destructor should come before other methods. also, please group the static
> > methods together
> 
> Done.
> 
>
http://codereview.chromium.org/5398001/diff/61001/chrome/browser/geolocation/...
> File chrome/browser/geolocation/geolocation_content_settings_map_unittest.cc
> (right):
> 
>
http://codereview.chromium.org/5398001/diff/61001/chrome/browser/geolocation/...
> chrome/browser/geolocation/geolocation_content_settings_map_unittest.cc:40:
> last_notifier->GetContentSettings(GURL("http://random-hostname.com/"));
> On 2010/12/15 14:02:11, jochen wrote:
> > i think that doesn't work
> 
> Already fixed.

[jochen (gone - plz use gerrit), 2010-12-15 15:05:51]

LGTM with nits adressed

http://codereview.chromium.org/5398001/diff/74001/chrome/browser/geolocation/...
File chrome/browser/geolocation/geolocation_content_settings_map.h (right):

http://codereview.chromium.org/5398001/diff/74001/chrome/browser/geolocation/...
chrome/browser/geolocation/geolocation_content_settings_map.h:107:
~GeolocationContentSettingsMap();
destructor is back at wrong position?

http://codereview.chromium.org/5398001/diff/74001/chrome/browser/geolocation/...
File chrome/browser/geolocation/geolocation_content_settings_map_unittest.cc
(right):

http://codereview.chromium.org/5398001/diff/74001/chrome/browser/geolocation/...
chrome/browser/geolocation/geolocation_content_settings_map_unittest.cc:55: } 
// namespace
please move back to end of file

[markusheintz_, 2010-12-15 15:24:08]

http://codereview.chromium.org/5398001/diff/74001/chrome/browser/geolocation/...
File chrome/browser/geolocation/geolocation_content_settings_map.h (right):

http://codereview.chromium.org/5398001/diff/74001/chrome/browser/geolocation/...
chrome/browser/geolocation/geolocation_content_settings_map.h:107:
~GeolocationContentSettingsMap();
On 2010/12/15 15:05:52, jochen wrote:
> destructor is back at wrong position?

sry wired thing it was correct in my client.

http://codereview.chromium.org/5398001/diff/74001/chrome/browser/geolocation/...
File chrome/browser/geolocation/geolocation_content_settings_map_unittest.cc
(right):

http://codereview.chromium.org/5398001/diff/74001/chrome/browser/geolocation/...
chrome/browser/geolocation/geolocation_content_settings_map_unittest.cc:55: } 
// namespace
On 2010/12/15 15:05:52, jochen wrote:
> please move back to end of file

Done.

[Nico, 2010-12-15 15:30:04]

LG

Please update the CL description to say that this includes UI changes for domui,
views, gtk, but not mac.

http://codereview.chromium.org/5398001/diff/70002/chrome/browser/gtk/options/...
File chrome/browser/gtk/options/content_filter_page_gtk.cc (right):

http://codereview.chromium.org/5398001/diff/70002/chrome/browser/gtk/options/...
chrome/browser/gtk/options/content_filter_page_gtk.cc:300:
ContentSettingsPattern(),
indent needs one more space

[danno, 2010-12-15 15:54:06]

http://codereview.chromium.org/5398001/diff/70002/chrome/browser/gtk/options/...
File chrome/browser/gtk/options/content_filter_page_gtk.cc (right):

http://codereview.chromium.org/5398001/diff/70002/chrome/browser/gtk/options/...
chrome/browser/gtk/options/content_filter_page_gtk.cc:299:
ContentSettingsDetails content_settings_details(
nit: remove extra leading space here.

http://codereview.chromium.org/5398001/diff/70002/chrome/common/policy_consta...
File chrome/common/policy_constants.h (right):

http://codereview.chromium.org/5398001/diff/70002/chrome/common/policy_consta...
chrome/common/policy_constants.h:63: extern const char
kDefaultGeolocationSetting[];
The precedent is to add the policy template changes at the same time as adding
the constants. It shouldn't be very much work, I think it would be good to do
so.

[markusheintz_, 2010-12-15 18:07:20]

[+ bauerb as reviewer]

@Nico: I added the cocoa UI changes to this CL.

@danno: I updated the policy template and will finish the TODOs in the grd file
befor I commit the CL.

@bauerb: I prepared a cute little CL for you that got now merge into this blob.
You are of course welcome to review the entire CL but you actually only need to
have a look at the cocoa UI changes. THANKS

@jochen: I hope I did not revert any of my fixes. Will check that carefully
before committing. Sry for the broken patch set.

http://codereview.chromium.org/5398001/diff/70002/chrome/browser/gtk/options/...
File chrome/browser/gtk/options/content_filter_page_gtk.cc (right):

http://codereview.chromium.org/5398001/diff/70002/chrome/browser/gtk/options/...
chrome/browser/gtk/options/content_filter_page_gtk.cc:299:
ContentSettingsDetails content_settings_details(
On 2010/12/15 15:54:07, danno wrote:
> nit: remove extra leading space here.

Done.

http://codereview.chromium.org/5398001/diff/70002/chrome/browser/gtk/options/...
chrome/browser/gtk/options/content_filter_page_gtk.cc:300:
ContentSettingsPattern(),
On 2010/12/15 15:30:05, Nico wrote:
> indent needs one more space 
not needed anymore after removing the extra space.

http://codereview.chromium.org/5398001/diff/70002/chrome/common/policy_consta...
File chrome/common/policy_constants.h (right):

http://codereview.chromium.org/5398001/diff/70002/chrome/common/policy_consta...
chrome/common/policy_constants.h:63: extern const char
kDefaultGeolocationSetting[];
On 2010/12/15 15:54:07, danno wrote:
> The precedent is to add the policy template changes at the same time as adding
> the constants. It shouldn't be very much work, I think it would be good to do
> so. 

I'll update the policy template.

[danno, 2010-12-16 13:56:41]

http://codereview.chromium.org/5398001/diff/102001/chrome/app/policy/policy_t...
File chrome/app/policy/policy_templates.grd (right):

http://codereview.chromium.org/5398001/diff/102001/chrome/app/policy/policy_t...
chrome/app/policy/policy_templates.grd:380: Allows you to set whether websites
are allowed to track the physical location. Tracking the physical location can
be allowed by default, denied by default or the user can be asked everytime a
website requests the pysical location.
nit: insteaad of "the physical location" how about "users' physical location".
Also 3x below.

http://codereview.chromium.org/5398001/diff/102001/chrome/app/policy/policy_t...
File chrome/app/policy/policy_templates.json (right):

http://codereview.chromium.org/5398001/diff/102001/chrome/app/policy/policy_t...
chrome/app/policy/policy_templates.json:520: 'supported_on': ['chrome.*:8-'],
10-

http://codereview.chromium.org/5398001/diff/102001/chrome/app/policy/policy_t...
chrome/app/policy/policy_templates.json:534: 'supported_on': ['chrome.*:8-'],
10-

http://codereview.chromium.org/5398001/diff/102001/chrome/common/policy_const...
File chrome/common/policy_constants.h (right):

http://codereview.chromium.org/5398001/diff/102001/chrome/common/policy_const...
chrome/common/policy_constants.h:57: extern const char kDefaultCookiesSetting[];
There doesn't seem to be anything in the policy JSON for
Cookies/Image/JavaScript. Are they not supported yet? If not, they should be
removed.

[markusheintz_, 2010-12-16 17:29:30]

http://codereview.chromium.org/5398001/diff/102001/chrome/app/policy/policy_t...
File chrome/app/policy/policy_templates.grd (right):

http://codereview.chromium.org/5398001/diff/102001/chrome/app/policy/policy_t...
chrome/app/policy/policy_templates.grd:380: Allows you to set whether websites
are allowed to track the physical location. Tracking the physical location can
be allowed by default, denied by default or the user can be asked everytime a
website requests the pysical location.
On 2010/12/16 13:56:42, danno wrote:
> nit: insteaad of "the physical location" how about "users' physical location".
> Also 3x below.

Like that. Done.

http://codereview.chromium.org/5398001/diff/102001/chrome/app/policy/policy_t...
File chrome/app/policy/policy_templates.json (right):

http://codereview.chromium.org/5398001/diff/102001/chrome/app/policy/policy_t...
chrome/app/policy/policy_templates.json:520: 'supported_on': ['chrome.*:8-'],
On 2010/12/16 13:56:42, danno wrote:
> 10-

Done.

http://codereview.chromium.org/5398001/diff/102001/chrome/app/policy/policy_t...
chrome/app/policy/policy_templates.json:534: 'supported_on': ['chrome.*:8-'],
On 2010/12/16 13:56:42, danno wrote:
> 10-

Done.

http://codereview.chromium.org/5398001/diff/102001/chrome/common/policy_const...
File chrome/common/policy_constants.h (right):

http://codereview.chromium.org/5398001/diff/102001/chrome/common/policy_const...
chrome/common/policy_constants.h:57: extern const char kDefaultCookiesSetting[];
On 2010/12/16 13:56:42, danno wrote:
> There doesn't seem to be anything in the policy JSON for
> Cookies/Image/JavaScript. Are they not supported yet? If not, they should be
> removed.

Added these policies to the policy template as well.

[Bernhard Bauer, 2010-12-16 17:39:06]

Cocoa stuff LGMT with a nit, but please update the description to include the
XIB changes as well.

http://codereview.chromium.org/5398001/diff/106001/chrome/browser/ui/cocoa/co...
File chrome/browser/ui/cocoa/content_settings_dialog_controller.h (right):

http://codereview.chromium.org/5398001/diff/106001/chrome/browser/ui/cocoa/co...
chrome/browser/ui/cocoa/content_settings_dialog_controller.h:103:
@property(nonatomic, readonly) BOOL geolocationSettingsManaged;
Nit: Please change the order so that it's consistent with the indexes above.

[Nico, 2010-12-16 17:50:35]

SLG

What Bernhard said, please add a "xib changes: bound $A to $B with
NegateBoolean" section to the CL description

http://codereview.chromium.org/5398001/diff/106001/chrome/browser/ui/cocoa/co...
File chrome/browser/ui/cocoa/content_settings_dialog_controller.mm (right):

http://codereview.chromium.org/5398001/diff/106001/chrome/browser/ui/cocoa/co...
chrome/browser/ui/cocoa/content_settings_dialog_controller.mm:646: 
no newline

[danno, 2010-12-17 13:50:04]

Policy stuff LGTM with nit.

http://codereview.chromium.org/5398001/diff/106001/chrome/app/policy/policy_t...
File chrome/app/policy/policy_templates.grd (right):

http://codereview.chromium.org/5398001/diff/106001/chrome/app/policy/policy_t...
chrome/app/policy/policy_templates.grd:358: Content Settings allow you to
specify how contents of specific a type like for example Cookies, Images or
JavaScript is handled.
nit: a type (for example Cookies, Images or JavaScript) is handled.

[markusheintz_, 2010-12-17 13:54:13]

http://codereview.chromium.org/5398001/diff/106001/chrome/app/policy/policy_t...
File chrome/app/policy/policy_templates.grd (right):

http://codereview.chromium.org/5398001/diff/106001/chrome/app/policy/policy_t...
chrome/app/policy/policy_templates.grd:358: Content Settings allow you to
specify how contents of specific a type like for example Cookies, Images or
JavaScript is handled.
On 2010/12/17 13:50:05, danno wrote:
> nit: a type (for example Cookies, Images or JavaScript) is handled.

Done.

[markusheintz_, 2010-12-20 11:51:34]

The Mac build bot failed and the content_settings_dialog_controller.mm is to
blame. I removed the Cocoa UI changes and will commit them in a separate CL
after fixing the problem. Sorry for removing that part of the CL again.

On 2010/12/17 13:54:13, markusheintz_ wrote:
>
http://codereview.chromium.org/5398001/diff/106001/chrome/app/policy/policy_t...
> File chrome/app/policy/policy_templates.grd (right):
> 
>
http://codereview.chromium.org/5398001/diff/106001/chrome/app/policy/policy_t...
> chrome/app/policy/policy_templates.grd:358: Content Settings allow you to
> specify how contents of specific a type like for example Cookies, Images or
> JavaScript is handled.
> On 2010/12/17 13:50:05, danno wrote:
> > nit: a type (for example Cookies, Images or JavaScript) is handled.
> 
> Done.

[Bernhard Bauer, 2010-12-20 17:23:46]

What was the probem?

On Mon, Dec 20, 2010 at 03:51,  <markusheintz@chromium.org> wrote:
> The Mac build bot failed and the content_settings_dialog_controller.mm is to
> blame. I removed the Cocoa UI changes and will commit them in a separate CL
> after fixing the problem. Sorry for removing that part of the CL again.
>
> On 2010/12/17 13:54:13, markusheintz_ wrote:
>
>
http://codereview.chromium.org/5398001/diff/106001/chrome/app/policy/policy_t...
>>
>> File chrome/app/policy/policy_templates.grd (right):
>
>
>
http://codereview.chromium.org/5398001/diff/106001/chrome/app/policy/policy_t...
>>
>> chrome/app/policy/policy_templates.grd:358: Content Settings allow you to
>> specify how contents of specific a type like for example Cookies, Images
>> or
>> JavaScript is handled.
>> On 2010/12/17 13:50:05, danno wrote:
>> > nit: a type (for example Cookies, Images or JavaScript) is handled.
>
>> Done.
>
>
>
> http://codereview.chromium.org/5398001/
>