Uses child views in Autofill Popup so we can trigger

chrome/browser/ui/autofill/autofill_popup_controller_impl.cc

Index: chrome/browser/ui/autofill/autofill_popup_controller_impl.cc
diff --git a/chrome/browser/ui/autofill/autofill_popup_controller_impl.cc b/chrome/browser/ui/autofill/autofill_popup_controller_impl.cc
index 487248fba6fdda38fb06cfa215aecb70cb0b4acf..884d85a606c48ab433e957649fe78971f25e4c1e 100644
--- a/chrome/browser/ui/autofill/autofill_popup_controller_impl.cc
+++ b/chrome/browser/ui/autofill/autofill_popup_controller_impl.cc
@@ -10,6 +10,7 @@
 #include "base/command_line.h"
 #include "base/logging.h"
 #include "base/macros.h"
+#include "base/optional.h"
 #include "base/strings/utf_string_conversions.h"
 #include "build/build_config.h"
 #include "chrome/browser/ui/autofill/autofill_popup_view.h"
@@ -25,12 +26,6 @@
 using base::WeakPtr;
 
 namespace autofill {
-namespace {
-
-// Used to indicate that no line is currently selected by the user.
-const int kNoSelection = -1;
-
-}  // namespace
 
 // static
 WeakPtr<AutofillPopupControllerImpl> AutofillPopupControllerImpl::GetOrCreate(
@@ -114,9 +109,12 @@ void AutofillPopupControllerImpl::Show(
 #endif
 
   if (just_created) {
-    ShowView();
+    view_->Show();
   } else {
-    UpdateBoundsAndRedrawPopup();
+    if (selected_line_ && *selected_line_ >= suggestions_.size()) {

[Evan Stade, 2017/03/18 00:47:03]

nit: no curlies

[csashi, 2017/03/18 02:25:09]

Done.

+      selected_line_.reset();
+    }
+    OnSuggestionsChanged();
   }
 
   controller_common_->RegisterKeyPressCallback();
@@ -153,7 +151,7 @@ void AutofillPopupControllerImpl::UpdateDataListValues(
 
      // The popup contents have changed, so either update the bounds or hide it.
     if (HasSuggestions())
-      UpdateBoundsAndRedrawPopup();
+      OnSuggestionsChanged();
     else
       Hide();
 
@@ -185,7 +183,7 @@ void AutofillPopupControllerImpl::UpdateDataListValues(
     elided_labels_[i] = labels[i];
   }
 
-  UpdateBoundsAndRedrawPopup();
+  OnSuggestionsChanged();
   DCHECK_EQ(suggestions_.size(), elided_values_.size());
   DCHECK_EQ(suggestions_.size(), elided_labels_.size());
 }
@@ -220,7 +218,7 @@ bool AutofillPopupControllerImpl::HandleKeyPressEvent(
     case ui::VKEY_PRIOR:  // Page up.
       // Set no line and then select the next line in case the first line is not
       // selectable.
-      SetSelectedLine(kNoSelection);
+      SetSelectedLine(base::nullopt);
       SelectNextLine();
       return true;
     case ui::VKEY_NEXT:  // Page down.
@@ -245,7 +243,7 @@ bool AutofillPopupControllerImpl::HandleKeyPressEvent(
   }
 }
 
-void AutofillPopupControllerImpl::UpdateBoundsAndRedrawPopup() {
+void AutofillPopupControllerImpl::OnSuggestionsChanged() {
 #if !defined(OS_ANDROID)
   // TODO(csharp): Since UpdatePopupBounds can change the position of the popup,
   // the popup could end up jumping from above the element to below it.
@@ -255,7 +253,7 @@ void AutofillPopupControllerImpl::UpdateBoundsAndRedrawPopup() {
 #endif
 
   // Platform-specific draw call.
-  view_->UpdateBoundsAndRedrawPopup();
+  view_->OnSuggestionsChanged();
 }
 
 void AutofillPopupControllerImpl::SetSelectionAtPoint(const gfx::Point& point) {
@@ -263,21 +261,20 @@ void AutofillPopupControllerImpl::SetSelectionAtPoint(const gfx::Point& point) {
 }
 
 bool AutofillPopupControllerImpl::AcceptSelectedLine() {
-  if (selected_line_ == kNoSelection)
+  if (!selected_line_)
     return false;
 
-  DCHECK_GE(selected_line_, 0);
-  DCHECK_LT(selected_line_, static_cast<int>(GetLineCount()));
+  DCHECK_LT(*selected_line_, GetLineCount());
 
-  if (!CanAccept(suggestions_[selected_line_].frontend_id))
+  if (!CanAccept(suggestions_[*selected_line_].frontend_id))
     return false;
 
-  AcceptSuggestion(selected_line_);
+  AcceptSuggestion(*selected_line_);
   return true;
 }
 
 void AutofillPopupControllerImpl::SelectionCleared() {
-  SetSelectedLine(kNoSelection);
+  SetSelectedLine(base::nullopt);
 }
 
 void AutofillPopupControllerImpl::AcceptSuggestion(size_t index) {
@@ -362,11 +359,11 @@ bool AutofillPopupControllerImpl::RemoveSuggestion(int list_index) {
   elided_values_.erase(elided_values_.begin() + list_index);
   elided_labels_.erase(elided_labels_.begin() + list_index);
 
-  SetSelectedLine(kNoSelection);
+  SetSelectedLine(base::nullopt);
 
   if (HasSuggestions()) {
     delegate_->ClearPreviewedForm();
-    UpdateBoundsAndRedrawPopup();
+    OnSuggestionsChanged();
   } else {
     Hide();
   }
@@ -376,12 +373,12 @@ bool AutofillPopupControllerImpl::RemoveSuggestion(int list_index) {
 
 ui::NativeTheme::ColorId
 AutofillPopupControllerImpl::GetBackgroundColorIDForRow(int index) const {
-  return index == selected_line_ ?
-    ui::NativeTheme::kColorId_ResultsTableHoveredBackground :
-    ui::NativeTheme::kColorId_ResultsTableNormalBackground;
+  return selected_line_ && index == static_cast<int>(*selected_line_)

[Evan Stade, 2017/03/18 00:47:03]

since some things are of type size_t and some are int, we have a few annoying
casts. I don't think we should be using size_t --- I used to think it was a good
idea to use it for things that shouldn't be negative, but that's explicitly
mentioned in the style guide as the wrong reason to use size_t. Therefore
everything should probably be int.

[csashi, 2017/03/18 02:25:09]

Acknowledged. There are many places where we are using size_t to refer to a row
(examples: GetSuggestionAt, GetElidedValueAt). There are some places where we
are using int (examples: GetBackgroundColorIDForRow, RemoveSuggestion). When you
say "everything should probably be int", I assumed you wanted every place where
we are using size_t to mean an int that is >= 0.

[Evan Stade, 2017/03/18 02:37:48]

Yea, when changing things from size_t to int they will still be non-negative.

[csashi, 2017/03/18 03:51:37]

Acknowledged.

+             ? ui::NativeTheme::kColorId_ResultsTableHoveredBackground
+             : ui::NativeTheme::kColorId_ResultsTableNormalBackground;
 }
 
-int AutofillPopupControllerImpl::selected_line() const {
+base::Optional<size_t> AutofillPopupControllerImpl::selected_line() const {
   return selected_line_;
 }
 
@@ -390,68 +387,70 @@ const AutofillPopupLayoutModel& AutofillPopupControllerImpl::layout_model()
   return layout_model_;
 }
 
-void AutofillPopupControllerImpl::SetSelectedLine(int selected_line) {
+void AutofillPopupControllerImpl::SetSelectedLine(
+    base::Optional<size_t> selected_line) {
   if (selected_line_ == selected_line)
     return;
 
-  if (selected_line_ != kNoSelection &&
-      static_cast<size_t>(selected_line_) < suggestions_.size())
-    InvalidateRow(selected_line_);
-
-  if (selected_line != kNoSelection) {
-    InvalidateRow(selected_line);
-
-    if (!CanAccept(suggestions_[selected_line].frontend_id))
-      selected_line = kNoSelection;
+  if (selected_line) {
+    DCHECK_LT(*selected_line, suggestions_.size());
+    if (!CanAccept(suggestions_[*selected_line].frontend_id))
+      selected_line = base::nullopt;
   }
 
+  auto previous_selected_line(selected_line_);
   selected_line_ = selected_line;
+  view_->OnSelectedRowChanged(previous_selected_line, selected_line_);
 
-  if (selected_line_ != kNoSelection) {
-    delegate_->DidSelectSuggestion(suggestions_[selected_line_].value,
-                                   suggestions_[selected_line_].frontend_id);
+  if (selected_line_) {
+    delegate_->DidSelectSuggestion(suggestions_[*selected_line_].value,
+                                   suggestions_[*selected_line_].frontend_id);
   } else {
     delegate_->ClearPreviewedForm();
   }
 }
 
 void AutofillPopupControllerImpl::SelectNextLine() {
-  int new_selected_line = selected_line_ + 1;
+  size_t new_selected_line = selected_line_ ? *selected_line_ + 1 : 0;
 
   // Skip over any lines that can't be selected.
-  while (static_cast<size_t>(new_selected_line) < GetLineCount() &&
+  while (new_selected_line < GetLineCount() &&
          !CanAccept(suggestions_[new_selected_line].frontend_id)) {
     ++new_selected_line;
   }
 
-  if (new_selected_line >= static_cast<int>(GetLineCount()))
+  if (new_selected_line >= GetLineCount())
     new_selected_line = 0;
 
   SetSelectedLine(new_selected_line);
 }
 
 void AutofillPopupControllerImpl::SelectPreviousLine() {
-  int new_selected_line = selected_line_ - 1;
-
-  // Skip over any lines that can't be selected.
-  while (new_selected_line > kNoSelection &&
-         !CanAccept(GetSuggestionAt(new_selected_line).frontend_id)) {
-    --new_selected_line;
-  }
+  int new_selected_line;

[Evan Stade, 2017/03/18 00:47:03]

nit: int new_selected_line = selected_line_.value_or(0) - 1;

(this is both shorter than what you have and it doesn't break when the last line
is unselectable, which is often the case)

[csashi, 2017/03/18 02:25:09]

Done. Did you mean selected_line_.value_or(GetLineCount()) - 1?

+  if (selected_line_) {
+    new_selected_line = *selected_line_ - 1;
+
+    // Skip over any lines that can't be selected.
+    while (new_selected_line >= 0 &&
+           !CanAccept(GetSuggestionAt(new_selected_line).frontend_id)) {
+      --new_selected_line;
+    }
 
-  if (new_selected_line <= kNoSelection)
+    if (new_selected_line < 0)
+      new_selected_line = GetLineCount() - 1;
+  } else {
     new_selected_line = GetLineCount() - 1;
+  }
 
   SetSelectedLine(new_selected_line);
 }
 
 bool AutofillPopupControllerImpl::RemoveSelectedLine() {
-  if (selected_line_ == kNoSelection)
+  if (!selected_line_)
     return false;
 
-  DCHECK_GE(selected_line_, 0);
-  DCHECK_LT(selected_line_, static_cast<int>(GetLineCount()));
-  return RemoveSuggestion(selected_line_);
+  DCHECK_LT(*selected_line_, GetLineCount());
+  return RemoveSuggestion(*selected_line_);
 }
 
 bool AutofillPopupControllerImpl::CanAccept(int id) {
@@ -482,16 +481,6 @@ void AutofillPopupControllerImpl::SetValues(
   }
 }
 
-void AutofillPopupControllerImpl::ShowView() {
-  view_->Show();
-}
-
-void AutofillPopupControllerImpl::InvalidateRow(size_t row) {
-  DCHECK(0 <= row);
-  DCHECK(row < suggestions_.size());
-  view_->InvalidateRow(row);
-}
-
 WeakPtr<AutofillPopupControllerImpl> AutofillPopupControllerImpl::GetWeakPtr() {
   return weak_ptr_factory_.GetWeakPtr();
 }
@@ -531,7 +520,7 @@ void AutofillPopupControllerImpl::ClearState() {
   elided_values_.clear();
   elided_labels_.clear();
 
-  selected_line_ = kNoSelection;
+  selected_line_.reset();
 }
 
 }  // namespace autofill