Adding device selection menus to the content setting bubble

chrome/browser/ui/views/content_setting_bubble_contents.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/views/content_setting_bubble_contents.h"
 
 #include <algorithm>
 #include <set>
 #include <string>
 #include <vector>
 
+#include "base/bind.h"
 #include "base/utf_string_conversions.h"
 #include "chrome/browser/content_settings/host_content_settings_map.h"
 #include "chrome/browser/plugins/plugin_finder.h"
 #include "chrome/browser/plugins/plugin_metadata.h"
+#include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/chrome_style.h"
 #include "chrome/browser/ui/content_settings/content_setting_bubble_model.h"
+#include "chrome/browser/ui/content_settings/content_setting_media_menu_model.h"
 #include "chrome/browser/ui/views/browser_dialogs.h"
 #include "content/public/browser/notification_source.h"
 #include "content/public/browser/notification_types.h"
 #include "content/public/browser/plugin_service.h"
 #include "grit/generated_resources.h"
 #include "ui/base/l10n/l10n_util.h"
+#include "ui/base/models/simple_menu_model.h"
+#include "ui/views/controls/button/menu_button.h"
 #include "ui/views/controls/button/radio_button.h"
 #include "ui/views/controls/button/text_button.h"
 #include "ui/views/controls/image_view.h"
 #include "ui/views/controls/label.h"
 #include "ui/views/controls/link.h"
+#include "ui/views/controls/menu/menu.h"
+#include "ui/views/controls/menu/menu_model_adapter.h"
+#include "ui/views/controls/menu/menu_runner.h"
 #include "ui/views/controls/separator.h"
 #include "ui/views/layout/grid_layout.h"
 #include "ui/views/layout/layout_constants.h"
 
 #if defined(USE_AURA)
 #include "ui/base/cursor/cursor.h"
 #endif
 
+namespace {

[Peter Kasting, 2013/02/06 22:11:18]

Nit: Add blank line below

[no longer working on chromium, 2013/02/07 16:25:19]

Done.

 // If we don't clamp the maximum width, then very long URLs and titles can make
 // the bubble arbitrarily wide.
 const int kMaxContentsWidth = 500;
 
 // When we have multiline labels, we should set a minimum width lest we get very
 // narrow bubbles with lots of line-wrapping.
 const int kMinMultiLineContentsWidth = 250;
 
+// Color of the border of media menu button.
+const SkColor kBorderLightColor = SkColorSetRGB(0xeb, 0xeb, 0xeb);
+
+// The width of the media menu button.
+const int kMediaMenuButtonWidth = 300;

[Peter Kasting, 2013/02/06 22:11:18]

This seems arbitrary.  How was this decided?  Should this instead be calculated
based on the bubble's current width or max contents width, or based on the
longest string in any of the menus, or something?

[no longer working on chromium, 2013/02/07 16:25:19]

The width of the button is decided based on how the UI looks like, I changed it
to 150 after turning how the UI looks.

We can't set this value based on the calculation, since the length of the
strings in the menus can be varying. For example, some device has a short name
like Default, while others might have a pretty long name.

With this fixed width, when the string is longer than it, it will shortcut the
string to something like
Microphone (SB ARen...

[Peter Kasting, 2013/02/07 23:55:04]

But that's the point: "how the UI looks" is almost certainly taking into account
the factors I just mentioned.

I suspect if the bubble width doubled, you'd want the value here to change. 
That in turn suggests that your value here should be based on such other
constants, not simply hardcoded directly.  For example, by using some of the
existing views layout constants, you could specify that the menu should have a
standard padding amount at the edges.

(The goal is to minimize the number of distinct constants, which tend to
fragment our UI appearance and lead to less modifiable UI when we want to update
everything's appearance.)

If you can't figure out how to avoid a constant here, post screenshots and I
will give it a shot.

[no longer working on chromium, 2013/02/08 12:32:39]

We actually don't allow the users to resize the bubble. And even if the bubble
width doubles, we still want to keep the buttons the same size, since the button
should always look like button, and doubling the width of the button might look
pretty odd. So adapting the size of button to the size of bubble might not give
us a good UI. Neither we want to adapt to the length of the menu strings since
they can vary a lot.
I agree, but I hope it is fine to set a preferred size for the buttons here
given the size of the bubble won't be changed.
Sure, I am sending you photos on look it looks.

[Peter Kasting, 2013/02/09 01:46:34]

I'm actually worried about not adapting to the length of the menu strings.  It
seems from your photos like we could have real problems with long strings
getting truncated.

What if we did something like the following:

The menu widths are the max width of all strings in both menus, except:
* We never allow the width to be shorter than some minimum value, say 100 px,
except subject to:
* We also never allow the menu to get long enough to extend past the right-hand
padding on the bubble.

This would make sure the menus were even-length, stayed inside the bubble,
didn't look broken when containing very short or long strings, but still avoided
eliding device names as much as possible.

WDYT?

[no longer working on chromium, 2013/02/09 14:53:51]

Thanks for ideas. After thinking a bit, I still think we prefer to having the
truncated string rather than a long button. If you look at
chrome://settings/content media, you will see we are trying to implement similar
menus in the bubble.

Actually, the truncated string is not a problem, since users can still see some
key words from the truncated strings. And users can always get the whole strings
by clicking the menus.

One more thing, we have two menus, and the length of strings won't be the same,
if one is long, while the other is short, adapt the width of the menus to the
long one will make the short one filled with plenty of blank.

Also, when users plug in new devices, I am not sure if we want the size of the
bubbon changes correspondingly.

[Peter Kasting, 2013/02/09 17:42:46]

What if the names of the devices are all identical for the first big chunk?
It seems graphically nicer to have the menu button width match the menu dropdown
width (when possible) than not.
That doesn't seem like a bad thing to me.  I don't think this will look bad.
That's actually a good point: how do you handle device insertion/removal today? 
Do you re-read the device list each time the user opens the menu?  Or just when
the bubble is shown?

+
+}
+
 using content::PluginService;
 using content::WebContents;
 
 class ContentSettingBubbleContents::Favicon : public views::ImageView {
  public:
   Favicon(const gfx::Image& image,
           ContentSettingBubbleContents* parent,
           views::Link* link);
   virtual ~Favicon();
 
@@ skipping 37 matching lines @@
 #if defined(USE_AURA)
   return ui::kCursorHand;
 #elif defined(OS_WIN)
   static HCURSOR g_hand_cursor = LoadCursor(NULL, IDC_HAND);
   return g_hand_cursor;
 #endif
 }
 
 ContentSettingBubbleContents::ContentSettingBubbleContents(
     ContentSettingBubbleModel* content_setting_bubble_model,
+    Profile* profile,
     WebContents* web_contents,
     views::View* anchor_view,
     views::BubbleBorder::ArrowLocation arrow_location)
     : BubbleDelegateView(anchor_view, arrow_location),
       content_setting_bubble_model_(content_setting_bubble_model),
+      profile_(profile),
       web_contents_(web_contents),
       custom_link_(NULL),
       manage_link_(NULL),
       close_button_(NULL) {
+  DCHECK(profile);
   // Compensate for built-in vertical padding in the anchor view's image.
   set_anchor_insets(gfx::Insets(5, 0, 5, 0));
 
   registrar_.Add(this, content::NOTIFICATION_WEB_CONTENTS_DESTROYED,
                  content::Source<WebContents>(web_contents));
 }
 
 ContentSettingBubbleContents::~ContentSettingBubbleContents() {
+  // Free any MediaMenuParts objects left over.

[Peter Kasting, 2013/02/06 22:11:18]

Nit: Comment unnecessary.

[no longer working on chromium, 2013/02/07 16:25:19]

Done.

+  for (MediaMenuPartsMap::const_iterator it = media_menus_.begin();
+      it != media_menus_.end(); ++it) {
+    delete it->second;

[Peter Kasting, 2013/02/06 22:11:18]

Nit: I believe STLDeleteValues() does what you want.

[no longer working on chromium, 2013/02/07 16:25:19]

Done.

+  }
 }
 
 gfx::Size ContentSettingBubbleContents::GetPreferredSize() {
   gfx::Size preferred_size(views::View::GetPreferredSize());
   int preferred_width =
       (!content_setting_bubble_model_->bubble_content().domain_lists.empty() &&
        (kMinMultiLineContentsWidth > preferred_size.width())) ?
       kMinMultiLineContentsWidth : preferred_size.width();
   preferred_size.set_width(std::min(preferred_width, kMaxContentsWidth));
   return preferred_size;
 }
 
+void ContentSettingBubbleContents::UpdateMenuLabel(
+    content::MediaStreamType type,
+    const std::string& label) {
+  MediaMenuPartsMap::const_iterator it = media_menus_.begin();

[Peter Kasting, 2013/02/06 22:11:18]

Nit: Declare this in the loop declaration.

You can still assert that we find the desired item if you want, by changing
"break;" to "return;" and the final DCHECK to a NOTREACHED.

[no longer working on chromium, 2013/02/07 16:25:19]

Done.

+  for (; it != media_menus_.end(); ++it) {
+    if (it->second->type == type) {
+      it->first->SetText(UTF8ToUTF16(label));
+      break;
+    }
+  }
+  DCHECK(it != media_menus_.end());
+}
+
 void ContentSettingBubbleContents::Init() {
   using views::GridLayout;
 
   GridLayout* layout = new views::GridLayout(this);
   SetLayoutManager(layout);
 
   const int kSingleColumnSetId = 0;
   views::ColumnSet* column_set = layout->AddColumnSet(kSingleColumnSetId);
   column_set->AddColumn(GridLayout::LEADING, GridLayout::FILL, 1,
                         GridLayout::USE_PREF, 0, 0);
@@ skipping 77 matching lines @@
       layout->StartRow(0, indented_kSingleColumnSetId);
       layout->AddView(radio);
       bubble_content_empty = false;
     }
     DCHECK(!radio_group_.empty());
     // Now that the buttons have been added to the view hierarchy, it's safe
     // to call SetChecked() on them.
     radio_group_[radio_group.default_item]->SetChecked(true);
   }
 
+  // Layout code for the media device menus.
+  if (content_setting_bubble_model_->content_type() ==
+      CONTENT_SETTINGS_TYPE_MEDIASTREAM) {
+    const int kMediaMenuColumnSetId = 2;
+    views::ColumnSet* menu_column_set =
+        layout->AddColumnSet(kMediaMenuColumnSetId);
+    menu_column_set->AddColumn(GridLayout::LEADING, GridLayout::FILL, 0,
+                               GridLayout::USE_PREF, 0, 0);
+    menu_column_set->AddPaddingColumn(
+        0, views::kRelatedControlHorizontalSpacing);
+    menu_column_set->AddColumn(GridLayout::LEADING, GridLayout::FILL, 1,
+                               GridLayout::USE_PREF, 0, 0);
+
+    for (ContentSettingBubbleModel::MediaMenuMap::const_iterator i(
+         bubble_content.media_menus.begin());
+         i != bubble_content.media_menus.end(); ++i) {
+      if (!bubble_content_empty)
+        layout->AddPaddingRow(0, views::kRelatedControlVerticalSpacing);
+      layout->StartRow(0, kMediaMenuColumnSetId);
+
+      views::Label* menu_label = new views::Label(
+                  UTF8ToUTF16(i->second.label));

[Peter Kasting, 2013/02/06 22:11:18]

Nit: Bizarre indenting (all over this section; did you use tabs instead of
spaces?)

[no longer working on chromium, 2013/02/07 16:25:19]

likely, I just got my windows working, so the VS editor is still using tab. I
will get rid of all of them.

+      menu_label->SetHorizontalAlignment(gfx::ALIGN_LEFT);
+          gfx::Size menu_label_size = menu_label->GetPreferredSize();
+      views::MenuButton* menu_button =
+          new views::MenuButton(
+                  NULL, UTF8ToUTF16((i->second.selected_device.name)), this, false);

[Peter Kasting, 2013/02/06 22:11:18]

Nit: 80 columns (due to indenting)

[no longer working on chromium, 2013/02/07 16:25:19]

Done.

+      menu_button->set_alignment(views::TextButton::ALIGN_LEFT);
+      menu_button->set_border(views::Border::CreateSolidSidedBorder(
+                  1, 0, 1, 0, kBorderLightColor));
+      menu_button->SetSize(gfx::Size(kMediaMenuButtonWidth,
+                                             menu_label_size.height()));
+          menu_button->SetVisible(true);
+
+      layout->AddView(menu_label);
+      layout->AddView(menu_button);
+       // Store the menu view to the map.

[Peter Kasting, 2013/02/06 22:11:18]

Nit: This comment adds nothing.

[no longer working on chromium, 2013/02/07 16:25:19]

Done.

+      MediaMenuParts* menu_view = new MediaMenuParts(i->first);
+      menu_view->menu_model.reset(new ContentSettingMediaMenuModel(
+          profile_,
+          i->second.default_device.type,
+          content_setting_bubble_model_.get(),
+          base::Bind(&ContentSettingBubbleContents::UpdateMenuLabel,
+                     base::Unretained(this))));
+      media_menus_[menu_button] = menu_view;
+      bubble_content_empty = false;
+    }
+  }
+
   gfx::Font domain_font =
       views::Label().font().DeriveFont(0, gfx::Font::BOLD);
   for (std::vector<ContentSettingBubbleModel::DomainList>::const_iterator i(
        bubble_content.domain_lists.begin());
        i != bubble_content.domain_lists.end(); ++i) {
     layout->StartRow(0, kSingleColumnSetId);
     views::Label* section_title = new views::Label(UTF8ToUTF16(i->title));
     section_title->SetMultiLine(true);
     section_title->SetHorizontalAlignment(gfx::ALIGN_LEFT);
     layout->AddView(section_title, 1, 1, GridLayout::FILL, GridLayout::LEADING);
@@ skipping 75 matching lines @@
     // CAREFUL: Showing the settings window activates it, which deactivates the
     // info bubble, which causes it to close, which deletes us.
     return;
   }
 
   PopupLinks::const_iterator i(popup_links_.find(source));
   DCHECK(i != popup_links_.end());
   content_setting_bubble_model_->OnPopupClicked(i->second);
 }
 
+void ContentSettingBubbleContents::OnMenuButtonClicked(
+    views::View* source,
+    const gfx::Point& point) {
+  MediaMenuPartsMap::iterator i(media_menus_.find(
+      static_cast<views::MenuButton*>(source)));
+  DCHECK(i != media_menus_.end());
+
+  views::MenuModelAdapter menu_model_adapter(i->second->menu_model.get());
+  menu_runner_.reset(new views::MenuRunner(menu_model_adapter.CreateMenu()));
+
+  gfx::Point screen_location;
+  views::View::ConvertPointToScreen(i->first, &screen_location);
+  if (menu_runner_->RunMenuAt(source->GetWidget(),
+                              i->first,
+                              gfx::Rect(screen_location, i->first->size()),
+                              views::MenuItemView::TOPRIGHT,
+                              views::MenuRunner::HAS_MNEMONICS) ==
+          views::MenuRunner::MENU_DELETED)
+    return;

[Peter Kasting, 2013/02/06 22:11:18]

We return here anyway.  There's no need to explicitly do so.

[no longer working on chromium, 2013/02/07 16:25:19]

We need it to get rid of a warning which complains the returned value from
RunMenuAt is ignored. 

[Peter Kasting, 2013/02/07 23:55:04]

There's an explicit way to note that you don't care about the return value. 
Look at other callers of RunMenuAt().

[no longer working on chromium, 2013/02/08 12:32:39]

Done.

+}
+
 void ContentSettingBubbleContents::Observe(
     int type,
     const content::NotificationSource& source,
     const content::NotificationDetails& details) {
   DCHECK_EQ(content::NOTIFICATION_WEB_CONTENTS_DESTROYED, type);
   DCHECK(source == content::Source<WebContents>(web_contents_));
   web_contents_ = NULL;
 }
+
+ContentSettingBubbleContents::MediaMenuParts::MediaMenuParts(
+    content::MediaStreamType type)
+    : type(type) {}
+
+ContentSettingBubbleContents::MediaMenuParts::~MediaMenuParts() {}