Uses child views in Autofill Popup so we can trigger

chrome/browser/ui/autofill/autofill_popup_controller_impl.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/autofill/autofill_popup_controller_impl.h"
 
 #include <algorithm>
 #include <utility>
 
 #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"
 #include "components/autofill/core/browser/autofill_popup_delegate.h"
 #include "components/autofill/core/browser/popup_item_ids.h"
 #include "components/autofill/core/browser/suggestion.h"
 #include "content/public/browser/native_web_keyboard_event.h"
 #include "ui/events/event.h"
 #include "ui/gfx/canvas.h"
 #include "ui/gfx/text_elider.h"
 #include "ui/gfx/text_utils.h"
 
 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(
     WeakPtr<AutofillPopupControllerImpl> previous,
     WeakPtr<AutofillPopupDelegate> delegate,
     content::WebContents* web_contents,
     gfx::NativeView container_view,
     const gfx::RectF& element_bounds,
     base::i18n::TextDirection text_direction) {
   if (previous.get() && previous->web_contents() == web_contents &&
@@ skipping 63 matching lines @@
   // Elide the name and label strings so that the popup fits in the available
   // space.
   for (size_t i = 0; i < suggestions_.size(); ++i) {
     bool has_label = !suggestions_[i].label.empty();
     ElideValueAndLabelForRow(
         i, layout_model_.GetAvailableWidthForRow(i, has_label));
   }
 #endif
 
   if (just_created) {
-    ShowView();
+    view_->Show();
   } else {
-    UpdateBoundsAndRedrawPopup();
+    OnSuggestionsChanged();
   }
 
   controller_common_->RegisterKeyPressCallback();
   delegate_->OnPopupShown();
 
   DCHECK_EQ(suggestions_.size(), elided_values_.size());
   DCHECK_EQ(suggestions_.size(), elided_labels_.size());
 }
 
 void AutofillPopupControllerImpl::UpdateDataListValues(
@@ skipping 16 matching lines @@
   if (values.empty()) {
     if (!suggestions_.empty() &&
         suggestions_[0].frontend_id == POPUP_ITEM_ID_SEPARATOR) {
       suggestions_.erase(suggestions_.begin());
       elided_values_.erase(elided_values_.begin());
       elided_labels_.erase(elided_labels_.begin());
     }
 
      // The popup contents have changed, so either update the bounds or hide it.
     if (HasSuggestions())
-      UpdateBoundsAndRedrawPopup();
+      OnSuggestionsChanged();
     else
       Hide();
 
     return;
   }
 
   // Add a separator if there are any other values.
   if (!suggestions_.empty() &&
       suggestions_[0].frontend_id != POPUP_ITEM_ID_SEPARATOR) {
     suggestions_.insert(suggestions_.begin(), autofill::Suggestion());
@@ skipping 11 matching lines @@
   for (size_t i = 0; i < values.size(); i++) {
     suggestions_[i].value = values[i];
     suggestions_[i].label = labels[i];
     suggestions_[i].frontend_id = POPUP_ITEM_ID_DATALIST_ENTRY;
 
     // TODO(brettw) it looks like these should be elided.
     elided_values_[i] = values[i];
     elided_labels_[i] = labels[i];
   }
 
-  UpdateBoundsAndRedrawPopup();
+  OnSuggestionsChanged();
   DCHECK_EQ(suggestions_.size(), elided_values_.size());
   DCHECK_EQ(suggestions_.size(), elided_labels_.size());
 }
 
 void AutofillPopupControllerImpl::Hide() {
   controller_common_->RemoveKeyPressCallback();
   if (delegate_)
     delegate_->OnPopupHidden();
 
   if (view_)
@@ skipping 14 matching lines @@
   switch (event.windowsKeyCode) {
     case ui::VKEY_UP:
       SelectPreviousLine();
       return true;
     case ui::VKEY_DOWN:
       SelectNextLine();
       return true;
     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.
       SetSelectedLine(GetLineCount() - 1);
       return true;
     case ui::VKEY_ESCAPE:
       Hide();
       return true;
     case ui::VKEY_DELETE:
       return (event.modifiers() & content::NativeWebKeyboardEvent::ShiftKey) &&
              RemoveSelectedLine();
     case ui::VKEY_TAB:
       // A tab press should cause the selected line to be accepted, but still
       // return false so the tab key press propagates and changes the cursor
       // location.
       AcceptSelectedLine();
       return false;
     case ui::VKEY_RETURN:
       return AcceptSelectedLine();
     default:
       return false;
   }
 }
 
-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.
   // It is unclear if it is better to keep the popup where it was, or if it
   // should try and move to its desired position.
   layout_model_.UpdatePopupBounds();
 #endif
 
   // Platform-specific draw call.
-  view_->UpdateBoundsAndRedrawPopup();
+  view_->OnSuggestionsChanged();
 }
 
 void AutofillPopupControllerImpl::SetSelectionAtPoint(const gfx::Point& point) {
   SetSelectedLine(layout_model_.LineFromY(point.y()));
 }
 
 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) {
   const autofill::Suggestion& suggestion = suggestions_[index];
   delegate_->DidAcceptSuggestion(suggestion.value, suggestion.frontend_id,
                                  index);
 }
 
 gfx::Rect AutofillPopupControllerImpl::popup_bounds() const {
   return layout_model_.popup_bounds();
@@ skipping 64 matching lines @@
   if (!delegate_->RemoveSuggestion(suggestions_[list_index].value,
                                    suggestions_[list_index].frontend_id)) {
     return false;
   }
 
   // Remove the deleted element.
   suggestions_.erase(suggestions_.begin() + 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();
   }
 
   return true;
 }
 
 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_;
 }
 
 const AutofillPopupLayoutModel& AutofillPopupControllerImpl::layout_model()
     const {
   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) {
+    DCHECK_LT(selected_line.value(), suggestions_.size());
+    if (!CanAccept(suggestions_[selected_line.value()].frontend_id))
+      selected_line = base::nullopt;
+  }
 
-  if (selected_line != kNoSelection) {
-    InvalidateRow(selected_line);
-
-    if (!CanAccept(suggestions_[selected_line].frontend_id))
-      selected_line = kNoSelection;
-  }
+  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;
+  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 > kNoSelection &&
-         !CanAccept(GetSuggestionAt(new_selected_line).frontend_id)) {
-    --new_selected_line;
+    // 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 < 0)
+      new_selected_line = GetLineCount() - 1;
+  } else {
+    new_selected_line = GetLineCount() - 1;
   }
 
-  if (new_selected_line <= kNoSelection)
-    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) {
   return id != POPUP_ITEM_ID_SEPARATOR &&
          id != POPUP_ITEM_ID_INSECURE_CONTEXT_PAYMENT_DISABLED_MESSAGE &&
          id != POPUP_ITEM_ID_TITLE;
 }
 
 bool AutofillPopupControllerImpl::HasSuggestions() {
   if (suggestions_.empty())
@@ skipping 10 matching lines @@
     const std::vector<autofill::Suggestion>& suggestions) {
   suggestions_ = suggestions;
   elided_values_.resize(suggestions.size());
   elided_labels_.resize(suggestions.size());
   for (size_t i = 0; i < suggestions.size(); i++) {
     elided_values_[i] = suggestions[i].value;
     elided_labels_[i] = suggestions[i].label;
   }
 }
 
-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();
 }
 
 #if !defined(OS_ANDROID)
 void AutofillPopupControllerImpl::ElideValueAndLabelForRow(
     size_t row,
     int available_width) {
   int value_width = gfx::GetStringWidth(
       suggestions_[row].value, layout_model_.GetValueFontListForRow(row));
@@ skipping 19 matching lines @@
 }
 #endif
 
 void AutofillPopupControllerImpl::ClearState() {
   // Don't clear view_, because otherwise the popup will have to get regenerated
   // and this will cause flickering.
   suggestions_.clear();
   elided_values_.clear();
   elided_labels_.clear();
 
-  selected_line_ = kNoSelection;
+  selected_line_.reset();
 }
 
 }  // namespace autofill