Combobox: Rename styles to STYLE_NORMAL and STYLE_ACTION and modify behaviors

ui/views/controls/combobox/combobox.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 "ui/views/controls/combobox/combobox.h"
 
 #include "base/logging.h"
 #include "base/strings/utf_string_conversions.h"
 #include "grit/ui_resources.h"
 #include "ui/base/accessibility/accessible_view_state.h"
@@ skipping 260 matching lines @@
   return rb.GetFontList(ui::ResourceBundle::BaseFont);
 }
 
 void Combobox::SetStyle(Style style) {
   if (style_ == style)
     return;
 
   style_ = style;
 
   UpdateBorder();
+  UpdateFromModel();
   PreferredSizeChanged();
 }
 
 void Combobox::ModelChanged() {
   selected_index_ = std::min(0, model_->GetItemCount());
   UpdateFromModel();
   PreferredSizeChanged();
 }
 
 void Combobox::SetSelectedIndex(int index) {
@@ skipping 60 matching lines @@
   return false;
 }
 
 bool Combobox::IsCommandEnabled(int id) const {
   return model()->IsItemEnabledAt(MenuCommandToIndex(id));
 }
 
 void Combobox::ExecuteCommand(int id) {
   selected_index_ = MenuCommandToIndex(id);
   OnSelectionChanged();
+  if (style_ == STYLE_NOTIFY_ON_CLICK)
+    HandleClickEvent(CLICK_EVENT_SOURCE_MENU_ITEM);
 }
 
 bool Combobox::GetAccelerator(int id, ui::Accelerator* accel) {
   return false;
 }
 
 int Combobox::GetRowCount() {
   return model()->GetItemCount();
 }
 
@@ skipping 91 matching lines @@
         text_button_->SetState(Button::STATE_PRESSED);
       } else {
         return false;
       }
       break;
 
     // Click the button only when the button style mode.
     case ui::VKEY_RETURN:
       if (style_ != STYLE_NOTIFY_ON_CLICK)
         return false;
-      HandleClickEvent();
+      HandleClickEvent(CLICK_EVENT_SOURCE_KEYBOARD);
       break;
 
     default:
       return false;
   }
 
   if (show_menu) {
     UpdateFromModel();
     ShowDropDownMenu(ui::MENU_SOURCE_KEYBOARD);
   } else if (new_index != selected_index_ && new_index != kNoSelection) {
     DCHECK(!model()->IsItemSeparatorAt(new_index));
     selected_index_ = new_index;
     OnSelectionChanged();
   }
 
   return true;
 }
 
 bool Combobox::OnKeyReleased(const ui::KeyEvent& e) {
   if (style_ != STYLE_NOTIFY_ON_CLICK)
     return false;  // crbug.com/127520
 
-  if (e.key_code() == ui::VKEY_SPACE)
-    HandleClickEvent();
+  if (e.key_code() == ui::VKEY_SPACE && style_ == STYLE_NOTIFY_ON_CLICK)
+    HandleClickEvent(CLICK_EVENT_SOURCE_KEYBOARD);
 
   return false;
 }
 
 void Combobox::OnPaint(gfx::Canvas* canvas) {
   switch (style_) {
     case STYLE_SHOW_DROP_DOWN_ON_CLICK: {
       OnPaintBackground(canvas);
       PaintText(canvas);
       OnPaintBorder(canvas);
@@ skipping 32 matching lines @@
 
 void Combobox::OnComboboxModelChanged(ui::ComboboxModel* model) {
   DCHECK_EQ(model, model_);
   ModelChanged();
 }
 
 void Combobox::ButtonPressed(Button* sender, const ui::Event& event) {
   RequestFocus();
 
   if (sender == text_button_) {
-    HandleClickEvent();
+    HandleClickEvent(CLICK_EVENT_SOURCE_MOUSE);
   } else {
     DCHECK_EQ(arrow_button_, sender);
     // TODO(hajimehoshi): Fix the problem that the arrow button blinks when
     // cliking this while the dropdown menu is opened.
     const base::TimeDelta delta = base::Time::Now() - closed_time_;
     if (delta.InMilliseconds() <= kMinimumMsBetweenButtonClicks)
       return;
 
     ui::MenuSourceType source_type = ui::MENU_SOURCE_MOUSE;
     if (event.IsKeyEvent())
       source_type = ui::MENU_SOURCE_KEYBOARD;
     else if (event.IsGestureEvent() || event.IsTouchEvent())
       source_type = ui::MENU_SOURCE_TOUCH;
     ShowDropDownMenu(source_type);
   }
 }
 
 void Combobox::UpdateFromModel() {
-  int max_width = 0;
   const gfx::FontList& font_list = Combobox::GetFontList();
 
   MenuItemView* menu = new MenuItemView(this);
   // MenuRunner owns |menu|.
   dropdown_list_menu_runner_.reset(new MenuRunner(menu));
 
   int num_items = model()->GetItemCount();
+  int width = 0;
   for (int i = 0; i < num_items; ++i) {
     if (model()->IsItemSeparatorAt(i)) {
       menu->AppendSeparator();
       continue;
     }
 
     base::string16 text = model()->GetItemAt(i);
 
     // Inserting the Unicode formatting characters if necessary so that the
     // text is displayed correctly in right-to-left UIs.
     base::i18n::AdjustStringForLocaleDirection(&text);
 
     menu->AppendMenuItem(i + kFirstMenuItemId, text, MenuItemView::NORMAL);
-    max_width = std::max(max_width, gfx::GetStringWidth(text, font_list));
+    if (style_ != STYLE_NOTIFY_ON_CLICK || i == 0)

[sky, 2014/01/23 15:40:25]

Based on your description I think this should use selected_index_ and not 0.

[hajimehoshi, 2014/01/24 04:10:56]

Done.

+      width = std::max(width, gfx::GetStringWidth(text, font_list));
   }
 
-  content_size_.SetSize(max_width, font_list.GetHeight());
+  content_size_.SetSize(width, font_list.GetHeight());
 }
 
 void Combobox::UpdateBorder() {
   FocusableBorder* border = new FocusableBorder();
   if (style_ == STYLE_NOTIFY_ON_CLICK)
     border->SetInsets(8, 13, 8, 13);
   if (invalid_)
     border->SetColor(kWarningColor);
   set_border(border);
 }
@@ skipping 11 matching lines @@
   int x = insets.left();
   int y = insets.top();
   int text_height = height() - insets.height();
   SkColor text_color = GetNativeTheme()->GetSystemColor(
       ui::NativeTheme::kColorId_LabelEnabledColor);
 
   DCHECK_GE(selected_index_, 0);
   DCHECK_LT(selected_index_, model()->GetItemCount());
   if (selected_index_ < 0 || selected_index_ > model()->GetItemCount())
     selected_index_ = 0;
-  base::string16 text = model()->GetItemAt(selected_index_);
+  base::string16 text;
+  if (style_ == STYLE_SHOW_DROP_DOWN_ON_CLICK)

[sky, 2014/01/23 15:40:25]

This shouldn't be needed. If a consumer wants to force the selected index to 0,
it should do that, not combobox.

[hajimehoshi, 2014/01/24 04:10:56]

Done.

+    text = model()->GetItemAt(selected_index_);
+  else
+    text = model()->GetItemAt(0);
 
   int disclosure_arrow_offset = width() - disclosure_arrow_->width() -
       GetDisclosureArrowLeftPadding() - GetDisclosureArrowRightPadding();
 
   const gfx::FontList& font_list = Combobox::GetFontList();
   int text_width = gfx::GetStringWidth(text, font_list);
   if ((text_width + insets.width()) > disclosure_arrow_offset)
     text_width = disclosure_arrow_offset - insets.width();
 
   gfx::Rect text_bounds(x, y, text_width, text_height);
@@ skipping 146 matching lines @@
   switch (style_) {
     case STYLE_SHOW_DROP_DOWN_ON_CLICK:
       return kDisclosureArrowRightPadding;
     case STYLE_NOTIFY_ON_CLICK:
       return kDisclosureArrowButtonRightPadding;
   }
   NOTREACHED();
   return 0;
 }
 
-void Combobox::HandleClickEvent() {
+void Combobox::HandleClickEvent(ClickEventSource source) {
   if (style_ != STYLE_NOTIFY_ON_CLICK)
     return;
 
+  if (source != CLICK_EVENT_SOURCE_MENU_ITEM)

[sky, 2014/01/23 15:40:25]

Same comment as above. Your code should change selection, not combobox.

[hajimehoshi, 2014/01/24 04:10:56]

Done.

+    selected_index_ = 0;
+
   if (listener_)
     listener_->OnComboboxTextButtonClicked(this);
 }
 
 }  // namespace views