Disable content-settings GTK-UI for managed content-settings-types

Disable content-settings GTK-UI for managed content-settings-types

BUG=63176
TEST=none

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

[markusheintz_, 2010-11-16 15:58:04]

Hey Nico please review my CL. Thanks!

[Nico, 2010-11-16 16:00:56]

Overall direction looks good to me. Let me know when you're ready for
lower-level comments.

[markusheintz, 2010-11-16 16:06:46]

Great Thanks!

On Tue, Nov 16, 2010 at 5:00 PM, <thakis@chromium.org> wrote:

> Overall direction looks good to me. Let me know when you're ready for
> lower-level comments.
>
>
> http://codereview.chromium.org/5078002/
>



-- 
Markus

[markusheintz_, 2010-11-16 18:19:50]

Please move ahead and review my CL.

Thanks!

On 2010/11/16 16:06:46, markusheintz wrote:
> Great Thanks!
> 
> On Tue, Nov 16, 2010 at 5:00 PM, <mailto:thakis@chromium.org> wrote:
> 
> > Overall direction looks good to me. Let me know when you're ready for
> > lower-level comments.
> >
> >
> > http://codereview.chromium.org/5078002/
> >
> 
> 
> 
> -- 
> Markus

[Nico, 2010-11-16 18:39:11]

erg: fyi

http://codereview.chromium.org/5078002/diff/8001/chrome/browser/gtk/options/c...
File chrome/browser/gtk/options/content_filter_page_gtk.cc (right):

http://codereview.chromium.org/5078002/diff/8001/chrome/browser/gtk/options/c...
chrome/browser/gtk/options/content_filter_page_gtk.cc:147: UpdateButtonsState();
Since you call UpdateButtonsState() _after_ the g_signal_connects for "toggled"
above, will the gtk_toggle_button_set_active() calls in UpdatebuttonsState() end
up calling OnAllowToggled() and hence always replace CONTENT_SETTING_DEFAULT
with CONTENT_SETTING_ASK (or whatever the default is)? Put differently, if you
a) start chrome with a new profile
b) open the prefs file in a text editor and
c) open the content settings dialog and then quit chrome
, will the text in the prefs file change from b after quitting chrome?

http://codereview.chromium.org/5078002/diff/8001/chrome/browser/gtk/options/c...
chrome/browser/gtk/options/content_filter_page_gtk.cc:158: // Get
default_setting
.

http://codereview.chromium.org/5078002/diff/8001/chrome/browser/gtk/options/c...
chrome/browser/gtk/options/content_filter_page_gtk.cc:181: // Disable the UI if
the default-content-setting is managed.
s/-/ /g

http://codereview.chromium.org/5078002/diff/8001/chrome/browser/gtk/options/c...
chrome/browser/gtk/options/content_filter_page_gtk.cc:186: }
bool sensitive =
profile()->GetHostContentSettingsMap()->IsDefaultContentSettingManaged(content_type_);

http://codereview.chromium.org/5078002/diff/8001/chrome/browser/gtk/options/c...
chrome/browser/gtk/options/content_filter_page_gtk.cc:282: if
(CONTENT_SETTINGS_TYPE_DEFAULT == details->type() ||
details->type() == CONTENT_SETTINGS_TYPE_DEFAULT

http://codereview.chromium.org/5078002/diff/8001/chrome/browser/gtk/options/c...
File chrome/browser/gtk/options/content_filter_page_gtk.h (right):

http://codereview.chromium.org/5078002/diff/8001/chrome/browser/gtk/options/c...
chrome/browser/gtk/options/content_filter_page_gtk.h:31: // notifications.
This comment can be removed. If you want to keep it, just say

"// To handle CONTENT_SETTINGS_CHANGED notifications."

http://codereview.chromium.org/5078002/diff/8001/chrome/browser/gtk/options/c...
chrome/browser/gtk/options/content_filter_page_gtk.h:37: virtual void
UpdateButtonsState();
Add a comment when this might be called

http://codereview.chromium.org/5078002/diff/8001/chrome/browser/gtk/options/c...
File chrome/browser/gtk/options/cookie_filter_page_gtk.cc (right):

http://codereview.chromium.org/5078002/diff/8001/chrome/browser/gtk/options/c...
chrome/browser/gtk/options/cookie_filter_page_gtk.cc:128: UpdateUiState();
same question as in the other file

http://codereview.chromium.org/5078002/diff/8001/chrome/browser/gtk/options/c...
chrome/browser/gtk/options/cookie_filter_page_gtk.cc:176: // If the
CONTENT_SETTING_TYPE_COOKIES is set to a managed setting the toggel
toggle

http://codereview.chromium.org/5078002/diff/8001/chrome/browser/gtk/options/c...
chrome/browser/gtk/options/cookie_filter_page_gtk.cc:177: // is meightbe change
programatically and not by a user action. We ignore the
I think you want to say "If CONTENT_SETTING_TYPE_COOKIES is managed, it can be
toggled programmatically and not by a user action."

http://codereview.chromium.org/5078002/diff/8001/chrome/browser/gtk/options/c...
chrome/browser/gtk/options/cookie_filter_page_gtk.cc:178: //
OnCookiesAllowToggled in this case to avoid overwriting the user defined
|OnCookiesAllowToggled()| call

'user-defined" can be written with a dash

http://codereview.chromium.org/5078002/diff/8001/chrome/browser/gtk/options/c...
chrome/browser/gtk/options/cookie_filter_page_gtk.cc:179: // setting.
As far as I understand, this can be called programmatically even if
C_S_T_COOKIES is _not_managed (this is basically my question in the other file).

http://codereview.chromium.org/5078002/diff/8001/chrome/browser/gtk/options/c...
File chrome/browser/gtk/options/cookie_filter_page_gtk.h (right):

http://codereview.chromium.org/5078002/diff/8001/chrome/browser/gtk/options/c...
chrome/browser/gtk/options/cookie_filter_page_gtk.h:71: // Managed
default-cookies-setting.
%s/-/ /g for all files

[markusheintz_, 2010-11-17 11:21:15]

Sry I had to upload 2 patch sets, since I forgot to change some comments. So for
everything related to content_filter_page_gtk.h see the lates patchset other
wise the one before.

http://codereview.chromium.org/5078002/diff/8001/chrome/browser/gtk/options/c...
File chrome/browser/gtk/options/content_filter_page_gtk.cc (right):

http://codereview.chromium.org/5078002/diff/8001/chrome/browser/gtk/options/c...
chrome/browser/gtk/options/content_filter_page_gtk.cc:147: UpdateButtonsState();
OnAllowToggle checks a guard. This means if a content setting is modified by an
action other than a user action the guard flag ignore_toggle_ is set and
OnAllowToggle will ignore the toggle change.

On 2010/11/16 18:39:12, Nico wrote:
> Since you call UpdateButtonsState() _after_ the g_signal_connects for
"toggled"
> above, will the gtk_toggle_button_set_active() calls in UpdatebuttonsState()
end
> up calling OnAllowToggled() and hence always replace CONTENT_SETTING_DEFAULT
> with CONTENT_SETTING_ASK (or whatever the default is)? Put differently, if you
> a) start chrome with a new profile
> b) open the prefs file in a text editor and
> c) open the content settings dialog and then quit chrome
> , will the text in the prefs file change from b after quitting chrome?

http://codereview.chromium.org/5078002/diff/8001/chrome/browser/gtk/options/c...
chrome/browser/gtk/options/content_filter_page_gtk.cc:158: // Get
default_setting
On 2010/11/16 18:39:12, Nico wrote:
> .

Done.

http://codereview.chromium.org/5078002/diff/8001/chrome/browser/gtk/options/c...
chrome/browser/gtk/options/content_filter_page_gtk.cc:181: // Disable the UI if
the default-content-setting is managed.
On 2010/11/16 18:39:12, Nico wrote:
> s/-/ /g

Are you sure? For consistency I would need to change all over the place. I also
use the same naming scheme in other files.

http://codereview.chromium.org/5078002/diff/8001/chrome/browser/gtk/options/c...
chrome/browser/gtk/options/content_filter_page_gtk.cc:186: }
On 2010/11/16 18:39:12, Nico wrote:
> bool sensitive =
>
profile()->GetHostContentSettingsMap()->IsDefaultContentSettingManaged(content_type_);

UUps :-) DONE!

http://codereview.chromium.org/5078002/diff/8001/chrome/browser/gtk/options/c...
chrome/browser/gtk/options/content_filter_page_gtk.cc:282: if
(CONTENT_SETTINGS_TYPE_DEFAULT == details->type() ||
On 2010/11/16 18:39:12, Nico wrote:
> details->type() == CONTENT_SETTINGS_TYPE_DEFAULT

Done. (For the line below as well)

http://codereview.chromium.org/5078002/diff/8001/chrome/browser/gtk/options/c...
File chrome/browser/gtk/options/content_filter_page_gtk.h (right):

http://codereview.chromium.org/5078002/diff/8001/chrome/browser/gtk/options/c...
chrome/browser/gtk/options/content_filter_page_gtk.h:31: // notifications.
deleted

On 2010/11/16 18:39:12, Nico wrote:
> This comment can be removed. If you want to keep it, just say
> 
> "// To handle CONTENT_SETTINGS_CHANGED notifications."

http://codereview.chromium.org/5078002/diff/8001/chrome/browser/gtk/options/c...
chrome/browser/gtk/options/content_filter_page_gtk.h:37: virtual void
UpdateButtonsState();
On 2010/11/16 18:39:12, Nico wrote:
> Add a comment when this might be called

Done.

http://codereview.chromium.org/5078002/diff/8001/chrome/browser/gtk/options/c...
File chrome/browser/gtk/options/cookie_filter_page_gtk.cc (right):

http://codereview.chromium.org/5078002/diff/8001/chrome/browser/gtk/options/c...
chrome/browser/gtk/options/cookie_filter_page_gtk.cc:128: UpdateUiState();
On 2010/11/16 18:39:12, Nico wrote:
> same question as in the other file

Reply in other file.
I added a guard here as well.

http://codereview.chromium.org/5078002/diff/8001/chrome/browser/gtk/options/c...
chrome/browser/gtk/options/cookie_filter_page_gtk.cc:176: // If the
CONTENT_SETTING_TYPE_COOKIES is set to a managed setting the toggel
On 2010/11/16 18:39:12, Nico wrote:
> toggle

Done.

http://codereview.chromium.org/5078002/diff/8001/chrome/browser/gtk/options/c...
chrome/browser/gtk/options/cookie_filter_page_gtk.cc:177: // is meightbe change
programatically and not by a user action. We ignore the
removed code

On 2010/11/16 18:39:12, Nico wrote:
> I think you want to say "If CONTENT_SETTING_TYPE_COOKIES is managed, it can be
> toggled programmatically and not by a user action."

http://codereview.chromium.org/5078002/diff/8001/chrome/browser/gtk/options/c...
chrome/browser/gtk/options/cookie_filter_page_gtk.cc:178: //
OnCookiesAllowToggled in this case to avoid overwriting the user defined
removed code/comment
On 2010/11/16 18:39:12, Nico wrote:
> |OnCookiesAllowToggled()| call
> 
> 'user-defined" can be written with a dash

http://codereview.chromium.org/5078002/diff/8001/chrome/browser/gtk/options/c...
chrome/browser/gtk/options/cookie_filter_page_gtk.cc:179: // setting.
On 2010/11/16 18:39:12, Nico wrote:
> As far as I understand, this can be called programmatically even if
> C_S_T_COOKIES is _not_managed (this is basically my question in the other
file).

added a guard. reply see other file.

http://codereview.chromium.org/5078002/diff/8001/chrome/browser/gtk/options/c...
File chrome/browser/gtk/options/cookie_filter_page_gtk.h (right):

http://codereview.chromium.org/5078002/diff/8001/chrome/browser/gtk/options/c...
chrome/browser/gtk/options/cookie_filter_page_gtk.h:71: // Managed
default-cookies-setting.
On 2010/11/16 18:39:12, Nico wrote:
> %s/-/ /g for all files

NO! please, please, please

[Nico, 2010-11-17 14:14:23]

LG with comments addressed.

http://codereview.chromium.org/5078002/diff/8001/chrome/browser/gtk/options/c...
File chrome/browser/gtk/options/content_filter_page_gtk.cc (right):

http://codereview.chromium.org/5078002/diff/8001/chrome/browser/gtk/options/c...
chrome/browser/gtk/options/content_filter_page_gtk.cc:181: // Disable the UI if
the default-content-setting is managed.
On 2010/11/17 11:21:16, markusheintz_ wrote:
> On 2010/11/16 18:39:12, Nico wrote:
> > s/-/ /g
> 
> Are you sure? For consistency I would need to change all over the place. I
also
> use the same naming scheme in other files.

theinn:src thakis$ git gs "content settings" | wc -l
      84
theinn:src thakis$ git gs "content-settings" | wc -l
      11

Seems less work to consistently remove the dashes than to consistently add them
:-)

http://codereview.chromium.org/5078002/diff/21001/chrome/browser/gtk/options/...
chrome/browser/gtk/options/content_filter_page_gtk.cc:202: content_type_)) {
Is this necessary, or is this just here for cleanliness?

http://codereview.chromium.org/5078002/diff/21001/chrome/browser/gtk/options/...
File chrome/browser/gtk/options/cookie_filter_page_gtk.cc (right):

http://codereview.chromium.org/5078002/diff/21001/chrome/browser/gtk/options/...
chrome/browser/gtk/options/cookie_filter_page_gtk.cc:60: ignore_toggle_ = true;
You might want to use base/auto_reset in this function, so that the member is
reset correctly in the future when someone adds a return in the middle of the
function.

[markusheintz_, 2010-11-17 16:43:50]

Thanks.

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

http://codereview.chromium.org/5078002/diff/21001/chrome/browser/gtk/options/...
chrome/browser/gtk/options/content_filter_page_gtk.cc:202: return;
On 2010/11/17 14:14:28, Nico wrote:
> Is this necessary, or is this just here for cleanliness?

The same approach is already used in cookie_filter_page_gtk.cc. This is
basically where I copied the idea from. I'd like to avoid unnecessary method
calls. If the radio_button that is toggled off triggers the "OnAllowToggled"
method the method set's the content_setting to it's current value. This is not a
big deal, but it also triggers a CONTENT_SETTINGS_CHANGE event and causes more
unnecessary method calls. I'd like to avoid this, especially since the same code
path is executed again afterwards just to change to setting to the new value.

http://codereview.chromium.org/5078002/diff/21001/chrome/browser/gtk/options/...
File chrome/browser/gtk/options/cookie_filter_page_gtk.cc (right):

http://codereview.chromium.org/5078002/diff/21001/chrome/browser/gtk/options/...
chrome/browser/gtk/options/cookie_filter_page_gtk.cc:60: ignore_toggle_ = true;
On 2010/11/17 14:14:28, Nico wrote:
> You might want to use base/auto_reset in this function, so that the member is
> reset correctly in the future when someone adds a return in the middle of the
> function.

Great suggestion. Thanks. DONE.