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..f6448210135253e895a7bd9c00365e59f30da6db 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,9 @@ void AutofillPopupControllerImpl::Show(
 #endif
 
   if (just_created) {
-    ShowView();
+    view_->Show();
   } else {
-    UpdateBoundsAndRedrawPopup();
+    OnSuggestionsChanged();
   }
 
   controller_common_->RegisterKeyPressCallback();
@@ -153,7 +148,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 +180,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 +215,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 +240,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 +250,7 @@ void AutofillPopupControllerImpl::UpdateBoundsAndRedrawPopup() {
 #endif
 
   // Platform-specific draw call.
-  view_->UpdateBoundsAndRedrawPopup();
+  view_->OnSuggestionsChanged();
 }
 
 void AutofillPopupControllerImpl::SetSelectionAtPoint(const gfx::Point& point) {
@@ -263,21 +258,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_.value(), GetLineCount());
 
-  if (!CanAccept(suggestions_[selected_line_].frontend_id))
+  if (!CanAccept(suggestions_[selected_line_.value()].frontend_id))

[Evan Stade, 2017/03/16 17:39:00]

*selected_line_

[csashi, 2017/03/16 22:42:05]

Done.

     return false;
 
-  AcceptSuggestion(selected_line_);
+  AcceptSuggestion(selected_line_.value());
   return true;
 }
 
 void AutofillPopupControllerImpl::SelectionCleared() {
-  SetSelectedLine(kNoSelection);
+  SetSelectedLine(base::nullopt);
 }
 
 void AutofillPopupControllerImpl::AcceptSuggestion(size_t index) {
@@ -362,11 +356,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 +370,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 index == static_cast<int>(selected_line_.value())
+             ? 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 +384,71 @@ 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.value(), suggestions_.size());
+    if (!CanAccept(suggestions_[selected_line.value()].frontend_id))
+      selected_line = base::nullopt;
   }
 
+  view_->OnSelectedRowChanged(selected_line_, selected_line);

[Evan Stade, 2017/03/16 17:39:00]

calling this before you change the value of selected_line_ is a little weird
because it means |current_selected_line| will not match
controller_->selected_line().

You might be able to get by without even adding any params to this function,
assuming you call it after updating selected_line_. I'm not sure if scheduling a
paint in just the selected and unselected lines actually paints less than if you
repainted the whole popup.

[csashi, 2017/03/16 22:42:05]

Done.
Perhaps - but seems better if the view can decide this - no?

[Evan Stade, 2017/03/17 14:11:54]

Maybe, but then again in this CL the view is still deciding to make its
implementation more elaborate. I'd envision:

AutofillPopupViewViews::OnSelectedRowChanged() {
  SchedulePaint();
  if (controller_->selected_line())
    child_at(*controller_->selected_line())->NotifyAccessibilityEvent(
        ui::AX_EVENT_SELECTION, true);
}

[csashi, 2017/03/17 16:30:35]

Assuming SchedulePaintInRect on 2 rectangles is not faster than SchedulePaint,
this is clearly simpler.

We are currently using
https://developer.apple.com/reference/uikit/uiview/1622587-setneedsdisplayinrect
to redraw specific rectangles. I could change the rect parameter that we pass in
https://codesearch.chromium.org/chromium/src/chrome/browser/ui/cocoa/autofill...
to the rect for the whole frame
(https://developer.apple.com/reference/appkit/nsview/1483458-initwithframe) - I
expect this will have a similar effect as changing SchedulePaintInRect to
SchedulePaint, but I don't have a way to build on Mac to verify this. Can you
confirm that using the rect for the whole frame on Mac is a safe change?

[Evan Stade, 2017/03/17 19:08:54]

I'm only talking about the Views implementation right now, not cocoa. I'm pretty
sure that in Views, schedulepaintinrect winds up repainting the whole view (to
my surprise, but when I tried to fix that it got real complicated real fast).

[csashi, 2017/03/17 21:55:21]

Do you mean you want to have two function signatures for OnSelectedRowChanged? -
one that takes the previous_row (and the current_row) and one for Views
implementation?
Both Cocoa and Views implementation implement the interface defined in
chrome/browser/ui/autofill/autofill_popup_view.h. view_ is an implementation of
that interface.

[csashi, 2017/03/17 22:52:26]

I  kept the function signature to pass both previous and current selection, but
simplified Views implementation.

+
   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()].value,
+        suggestions_[selected_line_.value()].frontend_id);
   } else {
     delegate_->ClearPreviewedForm();
   }
 }
 
 void AutofillPopupControllerImpl::SelectNextLine() {
-  int new_selected_line = selected_line_ + 1;
+  size_t new_selected_line = selected_line_ ? selected_line_.value() + 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;
+  if (selected_line_) {
+    new_selected_line = selected_line_.value() - 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_.value(), GetLineCount());
+  return RemoveSuggestion(selected_line_.value());
 }
 
 bool AutofillPopupControllerImpl::CanAccept(int id) {
@@ -482,16 +479,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 +518,7 @@ void AutofillPopupControllerImpl::ClearState() {
   elided_values_.clear();
   elided_labels_.clear();
 
-  selected_line_ = kNoSelection;
+  selected_line_.reset();
 }
 
 }  // namespace autofill