Disable the radio button of a content settings bubble if the content setting for the current site i…

Disable the radio buttons of a content settings bubble if the corresponding content setting for the current site is managed.


BUG=98029
TEST=Set a content settings e.g. plugins to block. Visit a website that uses e.g. plugins and open the content settings bubble. The radio buttons should be disabled.

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

[markusheintz_, 2012-06-08 11:05:02]

Please review this CL. Thanks.

[tfarina, 2012-06-08 21:21:48]

Hi Markus,

This LGTM.

But my approval won't help here, because I'm not listed in
chrome/browser/ui/OWNERS and thus it won't pass through CQ. You will probably
need approval from Peter (pkasting) or Scott (sky). And there is no
chrome/browser/ui/views/OWNERS (which should probably have the same owners of
chrome/browser/ui).

http://codereview.chromium.org/10537071/diff/1/chrome/browser/ui/views/conten...
File chrome/browser/ui/views/content_setting_bubble_contents.cc (right):

http://codereview.chromium.org/10537071/diff/1/chrome/browser/ui/views/conten...
chrome/browser/ui/views/content_setting_bubble_contents.cc:211: if
(!bubble_content.radio_group_enabled)
this seems logically correct. would be worth to do this unconditionally though?

outside the loop:
const bool is_enabled = bubble_content.radio_group_enable;

inside the loop:
radio->SetEnabled(is_enabled);

Just a suggestion, up to you of course.

[markusheintz_, 2012-06-11 13:25:03]

Thanks for the review.

@Peter: Could you review this CL please? Thanks a lot.

http://codereview.chromium.org/10537071/diff/1/chrome/browser/ui/views/conten...
File chrome/browser/ui/views/content_setting_bubble_contents.cc (right):

http://codereview.chromium.org/10537071/diff/1/chrome/browser/ui/views/conten...
chrome/browser/ui/views/content_setting_bubble_contents.cc:211: if
(!bubble_content.radio_group_enabled)
On 2012/06/08 21:21:48, tfarina wrote:
> this seems logically correct. would be worth to do this unconditionally
though?
> 
> outside the loop:
> const bool is_enabled = bubble_content.radio_group_enable;
> 
> inside the loop:
> radio->SetEnabled(is_enabled);
> 
> Just a suggestion, up to you of course.

Thanks for pointing this out.

I guess I can even do:
  radio->SetEnabled(bubble_content.radio_group_enable);

Then I even save the local variable.

[markusheintz_, 2012-06-11 13:27:44]

sky to cc
Sorry for the spam I had a wrong reviews list.

@Peter: Could you please review this CL? Thanks a lot.

[Peter Kasting, 2012-06-11 17:57:28]

Showing a control but disabling it is kind of poor UX.  The user doesn't know
why you're disabling the control.  Ideally there'd be some obvious reason that
could give more information.  Less ideally, the control would simply be absent
entirely rather than disabled.

Have you checked with the UI leads on how this case should behave?

[Peter Kasting, 2012-06-11 17:58:39]

(LGTM on the change itself if this is really the UX we want)

[markusheintz_, 2012-06-12 17:59:08]

On 2012/06/11 17:58:39, Peter Kasting wrote:
> (LGTM on the change itself if this is really the UX we want)

Thanks a lot Peter for the review. I totally agree with you. I will add an icon
or banner like we do it on the settings page for settings that are managed via
policy. It's just not finished yet.

The reason why I want to commit this now is that doing nothing and allowing the
user to change the radio buttons is worse then disabling them.

I won't commit this CL before I hear back form you. 

Thanks.

[Peter Kasting, 2012-06-12 18:01:04]

OK, go ahead.

[commit-bot: I haz the power, 2012-06-12 19:04:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/markusheintz@chromium.org/10537071/4001

[commit-bot: I haz the power, 2012-06-12 20:20:43]

Change committed as 141724