Update Content Settings Bubbles

Peter: Please review model, test, and WIN changes.
Elliot: Please review GTK changes, referencing model changes.
Nico: Please review MAC changes, referencing model changes.

Remove horizontal divider in plug-in content setting bubble (WIN,GTK)
Add separator after blocked geolocation domain list.
Refactor model and layout for simplicity (WIN,GTK,MAC)
 clear_link, info_link, load_plugins_link_title are now just custom_link.
 Make proper cookies subclass of ContentSettingTitleLinkAndCustomModel
Update unit test to reflect changes and test slightly more.

NOTE: Removes an extra vertical space that was between the popups list and the radio group (I can add more space here if desired).

NOTE: I would have liked to refactor some more mac code, but that would require modifying the .xibs, which is tough without my own mac dev box... I'll file a bug on this later.

NOTE: I addressed all of Peter's earlier comments, except using an enabled bit field. This change supersedes 5592004 and 5563003.

BUG=http://code.google.com/p/chromium/issues/detail?id=62188
TEST=Check layout and functionality of content settings bubbles, especially around verticals spacing, horizontal separators, and the buttons "Run all plug-ins this time", "Clear these settings for future visits" (Geolocation), and "Show cookies and other site data".

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

[msw, 2010-12-07 03:28:58]

Please review my TENTATIVE change to guide me...
Corresponding changes required for Linux&Mac layout.

http://codereview.chromium.org/5564007/diff/1/chrome/browser/content_setting_...
File chrome/browser/content_setting_bubble_model.cc (right):

http://codereview.chromium.org/5564007/diff/1/chrome/browser/content_setting_...
chrome/browser/content_setting_bubble_model.cc:309:
settings->set_load_plugins_link_enabled(false);
If we want, I can rename
TabSpecificContentSettings::set_load_plugins_link_enabled()

[Peter Kasting, 2010-12-07 17:53:00]

http://codereview.chromium.org/5564007/diff/1/chrome/browser/content_setting_...
File chrome/browser/content_setting_bubble_model.cc (right):

http://codereview.chromium.org/5564007/diff/1/chrome/browser/content_setting_...
chrome/browser/content_setting_bubble_model.cc:149: DCHECK(content_type() ==
CONTENT_SETTINGS_TYPE_COOKIES);
Nit: Now that we've combined all these, it seems wrong to me for the base class
to handle the cookie case and overrides the others.  I would prefer to also make
the cookie case be a lightweight subclass that overrides this function.

http://codereview.chromium.org/5564007/diff/1/chrome/browser/content_setting_...
chrome/browser/content_setting_bubble_model.cc:309:
settings->set_load_plugins_link_enabled(false);
On 2010/12/07 03:28:58, msw wrote:
> If we want, I can rename
> TabSpecificContentSettings::set_load_plugins_link_enabled()

No, I think that'd be wrong.  From that class' perspective the link to load
plugins is distinct from the links that do other things, so we'd want the
concepts to remain separate.

The only change that would make sense to me is to convert from a bool to a
bitfield, e.g. "info_link_enabled = CONTENT_SETTINGS_TYPE_PLUGINS |
CONTENT_SETTINGS_TYPE_GEOLOCATION".  But I'm not sure that buys much in today's
world.

http://codereview.chromium.org/5564007/diff/1/chrome/browser/content_setting_...
File chrome/browser/content_setting_bubble_model.h (right):

http://codereview.chromium.org/5564007/diff/1/chrome/browser/content_setting_...
chrome/browser/content_setting_bubble_model.h:73: bool info_link_enabled;
Nit: I think "info_link" is too specific of a name if we're making this thing
into a generic, do-anything link.

Perhaps "custom_link"?

I think we should also move |manage_link| below that link, to reflect the
visible order within the bubble (and similarly reorder all the model functions
having to do with |manage_link|).

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

http://codereview.chromium.org/5564007/diff/1/chrome/browser/ui/views/content...
chrome/browser/ui/views/content_setting_bubble_contents.cc:215:
layout->AddPaddingRow(0, kRelatedControlVerticalSpacing);
Seems like we need to be a little more careful in how we add padding rows. 
Something like this, perhaps:

bool bubble_empty = true;
if (!bubble_content.title.empty()) {
  ...
  layout->AddView(title_label);
  bubble_empty = false;
}

...
if (!plugins.empty()) {
  if (!bubble_empty)
    layout->AddPaddingRow(0, kRelatedControlVerticalSpacing);
  for (...) {
    ...
    layout->AddView(new views::Label(name));
    bubble_empty = false;
  }
}

etc., carried throughout the function.

This way, we can add padding rows and separator lines iff they're actually
separating from something.

[Nico, 2010-12-09 06:41:46]

http://codereview.chromium.org/5564007/diff/14001/chrome/browser/ui/cocoa/con...
File chrome/browser/ui/cocoa/content_setting_bubble_cocoa.mm (right):

http://codereview.chromium.org/5564007/diff/14001/chrome/browser/ui/cocoa/con...
chrome/browser/ui/cocoa/content_setting_bubble_cocoa.mm:333: control =
button.release();
This is wrong – please read
http://developer.apple.com/library/mac/#documentation/Cocoa/Conceptual/Memory...
(in short: you probably want to make |control| a scoped_nsobject and reset it to
the button, so that the refcount of the button is decremented by one)

http://codereview.chromium.org/5564007/diff/14001/chrome/browser/ui/cocoa/con...
chrome/browser/ui/cocoa/content_setting_bubble_cocoa.mm:337:
LabelWithFrame(base::SysUTF8ToNSString(content.custom_link), frame));
LabelWithFrame() returns an autoreleased object.

[Nico, 2010-12-09 06:44:39]

Also, all CL descriptions should have a BUG= and a TEST= line (and there
probably should be a bug for this change if there isn't yet), see
http://codereview.chromium.org/all?limit=100 for examples (though many get the
TEST= line wrong. It's primarily meant for instructions for QA guys, so it's
better to write 'TEST=click button A, observe that B happens' instead of
'TEST=unittests').

[Elliot Glaysher, 2010-12-09 17:32:27]

Gtk UI changes LGTM.

[Peter Kasting, 2010-12-09 17:56:34]

Since you're abandoning them, you should go back to changes 5592004 and 5563003
and close them with the "(x)" image by the issue title.  (Anyone can do this, I
just want you to do it :) )

LGTM with nits

http://codereview.chromium.org/5564007/diff/14001/chrome/browser/content_sett...
File chrome/browser/content_setting_bubble_model.cc (right):

http://codereview.chromium.org/5564007/diff/14001/chrome/browser/content_sett...
chrome/browser/content_setting_bubble_model.cc:155:
ContentSettingSingleRadioGroup(TabContents* tab_contents, Profile* profile,
Nit: While you're here, can you fix the parameter wrapping to be one per line a
la the ContentSettingTitleLinkAndCustomModel above?

http://codereview.chromium.org/5564007/diff/14001/chrome/browser/content_sett...
chrome/browser/content_setting_bubble_model.cc:284:
ContentSettingCookiesBubbleModel(TabContents* tab_contents, Profile* profile,
Nit: |profile| goes on its own line.

http://codereview.chromium.org/5564007/diff/14001/chrome/browser/content_sett...
chrome/browser/content_setting_bubble_model.cc:288: DCHECK_EQ(content_type,
CONTENT_SETTINGS_TYPE_COOKIES);
Nit: DCHECK_EQ(EXPECTED, ACTUAL);

http://codereview.chromium.org/5564007/diff/14001/chrome/browser/content_sett...
chrome/browser/content_setting_bubble_model.cc:315: if (tab_contents()) {
Nit: While here, remove extra {}

http://codereview.chromium.org/5564007/diff/14001/chrome/browser/content_sett...
chrome/browser/content_setting_bubble_model.cc:362: virtual void
OnCustomLinkClicked() OVERRIDE {}
Nit: No need for this since the base class provides an empty body.

http://codereview.chromium.org/5564007/diff/14001/chrome/browser/content_sett...
chrome/browser/content_setting_bubble_model.cc:410:
set_custom_link_enabled(false);
Nit: You can remove this line and the conditional arm below it, since a disabled
empty link is the default constructed state.

http://codereview.chromium.org/5564007/diff/14001/chrome/browser/content_sett...
chrome/browser/content_setting_bubble_model.cc:467: if (tab_contents) {
Nit: This whole conditional could be reduced to just:

  set_custom_link_enabled(!tab_contents || tab_contents->
      GetTabSpecificContentSettings()->load_plugins_link_enabled());

http://codereview.chromium.org/5564007/diff/14001/chrome/browser/content_sett...
File chrome/browser/content_setting_bubble_model_unittest.cc (right):

http://codereview.chromium.org/5564007/diff/14001/chrome/browser/content_sett...
chrome/browser/content_setting_bubble_model_unittest.cc:33:
EXPECT_EQ(std::string(), bubble_content.title);
Nit: Use EXPECT_TRUE(bubble_content.title.empty()); here and in all the similar
places below (boo to whomever wrote this unit test)

http://codereview.chromium.org/5564007/diff/14001/chrome/browser/content_sett...
chrome/browser/content_setting_bubble_model_unittest.cc:34: EXPECT_EQ(0U,
bubble_content.radio_group.radio_items.size());
Nit: These can both be changed to EXPECT_TRUE(xxx.empty()) also

http://codereview.chromium.org/5564007/diff/14001/chrome/browser/content_sett...
chrome/browser/content_setting_bubble_model_unittest.cc:37: if
(expect_clear_link || expect_reload_hint) {
Nit: This conditional can just become:

  EXPECT_NE(expect_clear_link || expect_reload_hint,
            bubble_content.custom_link.empty());

[msw, 2010-12-10 00:34:18]

I've addressed all the CR comments, fixed up the tests, and fixed a couple minor
bugs I found in my own testing.
Please review my updated changes.

[Nico, 2010-12-10 00:41:42]

Mac parts LG

http://codereview.chromium.org/5564007/diff/28001/chrome/browser/ui/cocoa/con...
File chrome/browser/ui/cocoa/content_setting_bubble_cocoa.mm (right):

http://codereview.chromium.org/5564007/diff/28001/chrome/browser/ui/cocoa/con...
chrome/browser/ui/cocoa/content_setting_bubble_cocoa.mm:326:
control.reset(static_cast<NSControl*>(button));
NSButton is derived from NSControl, you shouldn't need a cast here

http://codereview.chromium.org/5564007/diff/28001/chrome/browser/ui/cocoa/con...
chrome/browser/ui/cocoa/content_setting_bubble_cocoa.mm:333: } else {
Why is this branch required now but wasn't previously? Was the text just
missing?

http://codereview.chromium.org/5564007/diff/28001/chrome/browser/ui/cocoa/con...
chrome/browser/ui/cocoa/content_setting_bubble_cocoa.mm:337:
control.reset(static_cast<NSControl*>([customText retain]));
Same here. Since you don't call NSTextField-specific methods on customText, you
can probably do

control.reset([LabelWithFrame(base::SysUTF8ToNSString(content.custom_link),
frame) retrain]);
SetControlSize(control.get(), NSSmallControlSize);

[Peter Kasting, 2010-12-10 00:57:24]

LGTM

http://codereview.chromium.org/5564007/diff/28001/chrome/browser/content_sett...
File chrome/browser/content_setting_bubble_model.cc (right):

http://codereview.chromium.org/5564007/diff/28001/chrome/browser/content_sett...
chrome/browser/content_setting_bubble_model.cc:309:
ContentSettingPluginBubbleModel(TabContents* tab_contents, Profile* profile,
Nit: Can you fix the arg wrapping here too?

http://codereview.chromium.org/5564007/diff/28001/chrome/browser/content_sett...
chrome/browser/content_setting_bubble_model.cc:315:
set_custom_link_enabled(!settings || settings->load_plugins_link_enabled());
How about we just do:

  set_custom_link_enabled(tab_contents && tab_contents->
      GetTabSpecificContentSettings()->load_plugins_link_enabled());

...because even if |tab_contents| can be NULL it makes no sense to have a link
in that case...

This would also mean you could DCHECK(tab_contents()) in OnCustomLinkClicked(),
instead of conditionalizing it.

http://codereview.chromium.org/5564007/diff/28001/chrome/browser/content_sett...
chrome/browser/content_setting_bubble_model.cc:333:
ContentSettingPopupBubbleModel(TabContents* tab_contents, Profile* profile,
Nit: And fix arg wrapping here

[msw, 2010-12-10 03:32:05]

I addressed your additional CR feedback.
These changes look good locally and ran through git try clean.
If they still LGTYall, help me land my first change!
Mike

http://codereview.chromium.org/5564007/diff/28001/chrome/browser/content_sett...
File chrome/browser/content_setting_bubble_model.cc (right):

http://codereview.chromium.org/5564007/diff/28001/chrome/browser/content_sett...
chrome/browser/content_setting_bubble_model.cc:309:
ContentSettingPluginBubbleModel(TabContents* tab_contents, Profile* profile,
On 2010/12/10 00:57:24, Peter Kasting wrote:
> Nit: Can you fix the arg wrapping here too?

Done, also caught ContentSettingTitleAndLinkModel and ContentSettingBubbleModel.

http://codereview.chromium.org/5564007/diff/28001/chrome/browser/content_sett...
chrome/browser/content_setting_bubble_model.cc:315:
set_custom_link_enabled(!settings || settings->load_plugins_link_enabled());
On 2010/12/10 00:57:24, Peter Kasting wrote:
> How about we just do:
> 
>   set_custom_link_enabled(tab_contents && tab_contents->
>       GetTabSpecificContentSettings()->load_plugins_link_enabled());
> 
> ...because even if |tab_contents| can be NULL it makes no sense to have a link
> in that case...
> 
> This would also mean you could DCHECK(tab_contents()) in
OnCustomLinkClicked(),
> instead of conditionalizing it.

Done, also refactored OnCustomLinkClicked() slightly.

http://codereview.chromium.org/5564007/diff/28001/chrome/browser/content_sett...
chrome/browser/content_setting_bubble_model.cc:333:
ContentSettingPopupBubbleModel(TabContents* tab_contents, Profile* profile,
On 2010/12/10 00:57:24, Peter Kasting wrote:
> Nit: And fix arg wrapping here

Done.

http://codereview.chromium.org/5564007/diff/28001/chrome/browser/ui/cocoa/con...
File chrome/browser/ui/cocoa/content_setting_bubble_cocoa.mm (right):

http://codereview.chromium.org/5564007/diff/28001/chrome/browser/ui/cocoa/con...
chrome/browser/ui/cocoa/content_setting_bubble_cocoa.mm:326:
control.reset(static_cast<NSControl*>(button));
On 2010/12/10 00:41:42, Nico wrote:
> NSButton is derived from NSControl, you shouldn't need a cast here

Done.

http://codereview.chromium.org/5564007/diff/28001/chrome/browser/ui/cocoa/con...
chrome/browser/ui/cocoa/content_setting_bubble_cocoa.mm:333: } else {
Previously this text was delivered as an extra geolocation domain list title...
I revised that strange behavior to favor an explicit string and text field.

On 2010/12/10 00:41:42, Nico wrote:
> Why is this branch required now but wasn't previously? Was the text just
> missing?

http://codereview.chromium.org/5564007/diff/28001/chrome/browser/ui/cocoa/con...
chrome/browser/ui/cocoa/content_setting_bubble_cocoa.mm:337:
control.reset(static_cast<NSControl*>([customText retain]));
On 2010/12/10 00:41:42, Nico wrote:
> Same here. Since you don't call NSTextField-specific methods on customText,
you
> can probably do
> 
> control.reset([LabelWithFrame(base::SysUTF8ToNSString(content.custom_link),
> frame) retrain]);
> SetControlSize(control.get(), NSSmallControlSize);

Done.