Reintroduce Combobox::SetSelectedIndex Optimization

Bug: 1132244
Change-Id: I0486856cffc8c567d70dc2d4cb3a9aa14ad44214
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2482031Reviewed-by: Wei Li <weili@chromium.org>
Commit-Queue: Elaine Chien <elainec@chromium.org>
Cr-Commit-Position: refs/heads/master@{#820356}

ui/views/controls/combobox/combobox.cc

@@ -214,7 +214,7 @@ class Combobox::ComboboxMenuModel : public ui::MenuModel {
   }
 
   void ActivatedAt(int index) override {
-    owner_->selected_index_ = index;
+    owner_->SetSelectedIndex(index);
     owner_->OnPerformAction();
   }
 
@@ -279,7 +279,8 @@ const gfx::FontList& Combobox::GetFontList() const {
 }
 
 void Combobox::SetSelectedIndex(int index) {
-  // TODO(http://crbug.com/1132465): No-op when selected_index_ == index.
+  if (selected_index_ == index)
+    return;
   selected_index_ = index;
   if (size_to_largest_label_) {
     OnPropertyChanged(&selected_index_, kPropertyEffectsPaint);
@@ -316,7 +317,7 @@ void Combobox::SetModel(ui::ComboboxModel* model) {
   if (model_) {
     menu_model_ = std::make_unique<ComboboxMenuModel>(this, model_);
     observer_.Add(model_);
-    selected_index_ = model_->GetDefaultIndex();
+    SetSelectedIndex(model_->GetDefaultIndex());
     OnComboboxModelChanged(model_);
   }
 }
@@ -430,7 +431,7 @@ bool Combobox::OnKeyPressed(const ui::KeyEvent& e) {
   DCHECK_GE(selected_index_, 0);
   DCHECK_LT(selected_index_, GetModel()->GetItemCount());
   if (selected_index_ < 0 || selected_index_ > GetModel()->GetItemCount())
-    selected_index_ = 0;
+    SetSelectedIndex(0);
 
   bool show_menu = false;
   int new_index = kNoSelection;
@@ -490,7 +491,7 @@ bool Combobox::OnKeyPressed(const ui::KeyEvent& e) {
     ShowDropDownMenu(ui::MENU_SOURCE_KEYBOARD);
   } else if (new_index != selected_index_ && new_index != kNoSelection) {
     DCHECK(!GetModel()->IsItemSeparatorAt(new_index));
-    selected_index_ = new_index;
+    SetSelectedIndex(new_index);
     OnPerformAction();
   }
 
@@ -561,7 +562,7 @@ void Combobox::OnComboboxModelChanged(ui::ComboboxModel* model) {
   if (selected_index_ >= model_->GetItemCount() ||
       model_->GetItemCount() == 0 ||
       model_->IsItemSeparatorAt(selected_index_)) {
-    selected_index_ = model_->GetDefaultIndex();
+    SetSelectedIndex(model_->GetDefaultIndex());
   }
 
   content_size_ = GetContentSize();
@@ -615,10 +616,10 @@ void Combobox::PaintIconAndText(gfx::Canvas* canvas) {
 
   // Draw the text.
   SkColor text_color = GetTextColorForEnableState(*this, GetEnabled());
-  DCHECK_GE(selected_index_, 0);
-  DCHECK_LT(selected_index_, GetModel()->GetItemCount());
-  if (selected_index_ < 0 || selected_index_ > GetModel()->GetItemCount())
-    selected_index_ = 0;
+  if (selected_index_ < 0 || selected_index_ > GetModel()->GetItemCount()) {
+    NOTREACHED();
+    SetSelectedIndex(0);
+  }
   base::string16 text = GetModel()->GetItemAt(selected_index_);
 
   int disclosure_arrow_offset = width() - kComboboxArrowContainerWidth;

ui/views/controls/combobox/combobox_unittest.cc

@@ -29,6 +29,7 @@
 #include "ui/events/keycodes/keyboard_codes.h"
 #include "ui/events/test/event_generator.h"
 #include "ui/events/types/event_type.h"
+#include "ui/gfx/text_utils.h"
 #include "ui/views/style/platform_style.h"
 #include "ui/views/test/ax_event_counter.h"
 #include "ui/views/test/combobox_test_api.h"
@@ -686,6 +687,35 @@ TEST_F(ComboboxTest, ConsumingPressKeyEvents) {
   }
 }
 
+// Test that ensures that the combobox is resized correctly when selecting
+// between indices of different label lengths.
+TEST_F(ComboboxTest, ContentSizeUpdateOnSetSelectedIndex) {
+  const gfx::FontList& font_list =
+      style::GetFont(Combobox::kDefaultComboboxTextContext,
+                     Combobox::kDefaultComboboxTextStyle);
+  InitCombobox(nullptr);
+  combobox_->SetSizeToLargestLabel(false);
+  test_api_->PerformActionAt(1);
+  EXPECT_EQ(gfx::GetStringWidth(model_->GetItemAt(1), font_list),
+            test_api_->content_size().width());
+  combobox_->SetSelectedIndex(1);
+  EXPECT_EQ(gfx::GetStringWidth(model_->GetItemAt(1), font_list),
+            test_api_->content_size().width());
+
+  // Avoid selected_index_ == index optimization and start with index 1 selected
+  // to test resizing from a an index with a shorter label to an index with a
+  // longer label.
+  combobox_->SetSelectedIndex(0);
+  combobox_->SetSelectedIndex(1);
+
+  test_api_->PerformActionAt(0);
+  EXPECT_EQ(gfx::GetStringWidth(model_->GetItemAt(0), font_list),
+            test_api_->content_size().width());
+  combobox_->SetSelectedIndex(0);
+  EXPECT_EQ(gfx::GetStringWidth(model_->GetItemAt(0), font_list),
+            test_api_->content_size().width());
+}
+
 TEST_F(ComboboxTest, ContentWidth) {
   std::vector<std::string> values;
   VectorComboboxModel model(&values);