[Autofill] Move functions from the AutofillPopupController to AutofillPopupLayoutModel

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 3902ab93b843bc17d68e875687c71530009cf515..5abf69e56fe501f79d1bbc89bc5e659174f162ec 100644
--- a/chrome/browser/ui/autofill/autofill_popup_controller_impl.cc
+++ b/chrome/browser/ui/autofill/autofill_popup_controller_impl.cc
@@ -13,18 +13,11 @@
 #include "base/strings/utf_string_conversions.h"
 #include "build/build_config.h"
 #include "chrome/browser/ui/autofill/autofill_popup_view.h"
-#include "chrome/browser/ui/autofill/popup_constants.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 "components/autofill/core/common/autofill_util.h"
 #include "content/public/browser/native_web_keyboard_event.h"
-#include "grit/components_scaled_resources.h"
-#include "ui/base/resource/resource_bundle.h"
 #include "ui/events/event.h"
-#include "ui/gfx/geometry/rect_conversions.h"
-#include "ui/gfx/geometry/vector2d.h"
-#include "ui/gfx/screen.h"
 #include "ui/gfx/text_elider.h"
 #include "ui/gfx/text_utils.h"
 
@@ -36,40 +29,12 @@ namespace {
 // Used to indicate that no line is currently selected by the user.
 const int kNoSelection = -1;
 
-// The vertical height of each row in pixels.
-const size_t kRowHeight = 24;
-
-// The vertical height of a separator in pixels.
-const size_t kSeparatorHeight = 1;
 
 #if !defined(OS_ANDROID)
 // Size difference between name and label in pixels.
 const int kLabelFontSizeDelta = -2;
-
-const size_t kNamePadding = AutofillPopupView::kNamePadding;
-const size_t kIconPadding = AutofillPopupView::kIconPadding;
-const size_t kEndPadding = AutofillPopupView::kEndPadding;
 #endif
 
-struct DataResource {
-  const char* name;
-  int id;
-};
-
-const DataResource kDataResources[] = {
-  { "americanExpressCC", IDR_AUTOFILL_CC_AMEX },
-  { "dinersCC", IDR_AUTOFILL_CC_GENERIC },
-  { "discoverCC", IDR_AUTOFILL_CC_DISCOVER },
-  { "genericCC", IDR_AUTOFILL_CC_GENERIC },
-  { "jcbCC", IDR_AUTOFILL_CC_GENERIC },
-  { "masterCardCC", IDR_AUTOFILL_CC_MASTERCARD },
-  { "visaCC", IDR_AUTOFILL_CC_VISA },
-#if defined(OS_ANDROID)
-  { "scanCreditCardIcon", IDR_AUTOFILL_CC_SCAN_NEW },
-  { "settings", IDR_AUTOFILL_SETTINGS },
-#endif
-};
-
 }  // namespace
 
 // static
@@ -135,41 +100,7 @@ void AutofillPopupControllerImpl::Show(
   DCHECK_EQ(suggestions_.size(), elided_values_.size());
   DCHECK_EQ(suggestions_.size(), elided_labels_.size());
 
-#if !defined(OS_ANDROID)
-  // Android displays the long text with ellipsis using the view attributes.
-
-  UpdatePopupBounds();
-  int popup_width = popup_bounds().width();
-
-  // Elide the name and label strings so that the popup fits in the available
-  // space.
-  for (size_t i = 0; i < suggestions_.size(); ++i) {
-    int value_width =
-        gfx::GetStringWidth(suggestions_[i].value, GetValueFontListForRow(i));
-    int label_width =
-        gfx::GetStringWidth(suggestions_[i].label, GetLabelFontList());
-    int total_text_length = value_width + label_width;
-
-    // The line can have no strings if it represents a UI element, such as
-    // a separator line.
-    if (total_text_length == 0)
-      continue;
-
-    int available_width = popup_width - RowWidthWithoutText(i);
-
-    // Each field receives space in proportion to its length.
-    int value_size = available_width * value_width / total_text_length;
-    elided_values_[i] = gfx::ElideText(suggestions_[i].value,
-                                       GetValueFontListForRow(i),
-                                       value_size, gfx::ELIDE_TAIL);
-
-    int label_size = available_width * label_width / total_text_length;
-    elided_labels_[i] = gfx::ElideText(suggestions_[i].label,
-                                       GetLabelFontList(),
-                                       label_size, gfx::ELIDE_TAIL);
-  }
-#endif
-
+  bool just_created = false;
   if (!view_) {
     view_ = AutofillPopupView::Create(this);
 
@@ -179,7 +110,23 @@ void AutofillPopupControllerImpl::Show(
       Hide();
       return;
     }
+    just_created = true;
+  }
+
+#if !defined(OS_ANDROID)
+  // Android displays the long text with ellipsis using the view attributes.
+
+  view_->UpdatePopupBounds();
+
+  // 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, view_->GetAvailableWidthForRow(i, has_label));
+  }
+#endif
 
+  if (just_created) {
     ShowView();
   } else {
     UpdateBoundsAndRedrawPopup();
@@ -317,14 +264,15 @@ void AutofillPopupControllerImpl::UpdateBoundsAndRedrawPopup() {
   // 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.
-  UpdatePopupBounds();
+  view_->UpdatePopupBounds();
 #endif
 
+  // Platform-specific draw call.
   view_->UpdateBoundsAndRedrawPopup();
 }
 
 void AutofillPopupControllerImpl::SetSelectionAtPoint(const gfx::Point& point) {
-  SetSelectedLine(LineFromY(point.y()));
+  SetSelectedLine(view_->LineFromY(point.y()));

[Evan Stade, 2016/01/13 04:30:27]

so in Model-View-Controller (or Model-View-Presenter, which is actually more
like what Chrome uses, although it uses the MVC nomenclature), the controller
owns or at least has access to the model. The model (here termed "helper")
shouldn't be owned by the view, rather the view should ask the controller (aka
delegate_ in this case) for the model. Then you wouldn't need all these new
thunks on AutofillPopupView.

[Mathieu, 2016/01/13 17:04:27]

I think we should move towards having the PopupController just holding on to the
Suggestions (what I call the model) and dealing with user actions (select,
remove, etc.).

The ViewHelper I've added here is holding the view's state: popup bounds, incl.
width, height, location of the view. As such I believe the view should own its
internal state and have the ViewHelper as a member.

I would move towards enforcing that AutofillPopupView is the interface through
which PopupController can tell it to redraw, or question which line corresponds
to point Y for example. AutofillPopupViewDelegate can be how the view can get
the suggestions (model) from the controller, or tell the controller something
happened on the view.

Further refactors would go towards the goal of view implementations having a
pointer to the PopupViewDelegate instead of the controller.

Accessors like popup_bounds shouldn't be on the controller either (to be
refactored as well).

So in essence, I think the goal of this refactor is to have the view own more of
its internal state, so I would suggest leaving the ViewHelper as a member of the
view.

[Evan Stade, 2016/01/13 18:18:56]

To me it's a code smell when you have to add a lot of pass-throughs. Right now
the call chain is

view -> controller (SetSelectedLineAtPoint)
controller -> view (LineFromY)
view -> view helper (LineFromY)

One way to fix this would be to change SetSelectedLineAtPoint to SetSelectedLine
(by index). If you can fix all the thunks this way, groovy.

 }
 
 bool AutofillPopupControllerImpl::AcceptSelectedLine() {
@@ -351,43 +299,12 @@ void AutofillPopupControllerImpl::AcceptSuggestion(size_t index) {
                                  index);
 }
 
-int AutofillPopupControllerImpl::GetIconResourceID(
-    const base::string16& resource_name) const {
-  int result = -1;
-  for (size_t i = 0; i < arraysize(kDataResources); ++i) {
-    if (resource_name == base::ASCIIToUTF16(kDataResources[i].name)) {
-      result = kDataResources[i].id;
-      break;
-    }
-  }
-
-#if defined(OS_ANDROID)
-  if (result == IDR_AUTOFILL_CC_SCAN_NEW && IsKeyboardAccessoryEnabled())
-    result = IDR_AUTOFILL_CC_SCAN_NEW_KEYBOARD_ACCESSORY;
-#endif  // OS_ANDROID
-
-  return result;
-}
-
 bool AutofillPopupControllerImpl::IsWarning(size_t index) const {
   return suggestions_[index].frontend_id == POPUP_ITEM_ID_WARNING_MESSAGE;
 }
 
-gfx::Rect AutofillPopupControllerImpl::GetRowBounds(size_t index) {
-  int top = kPopupBorderThickness;
-  for (size_t i = 0; i < index; ++i) {
-    top += GetRowHeightFromId(suggestions_[i].frontend_id);
-  }
-
-  return gfx::Rect(
-      kPopupBorderThickness,
-      top,
-      popup_bounds_.width() - 2 * kPopupBorderThickness,
-      GetRowHeightFromId(suggestions_[index].frontend_id));
-}
-
-const gfx::Rect& AutofillPopupControllerImpl::popup_bounds() const {
-  return popup_bounds_;
+gfx::Rect AutofillPopupControllerImpl::popup_bounds() const {
+  return view_->GetPopupBounds();
 }
 
 content::WebContents* AutofillPopupControllerImpl::web_contents() {
@@ -406,6 +323,21 @@ bool AutofillPopupControllerImpl::IsRTL() const {
   return controller_common_->is_rtl();
 }
 
+const std::vector<autofill::Suggestion>
+AutofillPopupControllerImpl::GetSuggestions() {
+  return suggestions_;
+}
+
+#if !defined(OS_ANDROID)
+int AutofillPopupControllerImpl::GetElidedValueWidthForRow(size_t row) {
+  return gfx::GetStringWidth(GetElidedValueAt(row), value_font_list_);
+}
+
+int AutofillPopupControllerImpl::GetElidedLabelWidthForRow(size_t row) {
+  return gfx::GetStringWidth(GetElidedValueAt(row), label_font_list_);
+}
+#endif
+
 size_t AutofillPopupControllerImpl::GetLineCount() const {
   return suggestions_.size();
 }
@@ -542,27 +474,6 @@ bool AutofillPopupControllerImpl::RemoveSelectedLine() {
   return RemoveSuggestion(selected_line_);
 }
 
-int AutofillPopupControllerImpl::LineFromY(int y) {
-  int current_height = kPopupBorderThickness;
-
-  for (size_t i = 0; i < suggestions_.size(); ++i) {
-    current_height += GetRowHeightFromId(suggestions_[i].frontend_id);
-
-    if (y <= current_height)
-      return i;
-  }
-
-  // The y value goes beyond the popup so stop the selection at the last line.
-  return GetLineCount() - 1;
-}
-
-int AutofillPopupControllerImpl::GetRowHeightFromId(int identifier) const {
-  if (identifier == POPUP_ITEM_ID_SEPARATOR)
-    return kSeparatorHeight;
-
-  return kRowHeight;
-}
-
 bool AutofillPopupControllerImpl::CanAccept(int id) {
   return id != POPUP_ITEM_ID_SEPARATOR && id != POPUP_ITEM_ID_WARNING_MESSAGE &&
          id != POPUP_ITEM_ID_TITLE;
@@ -600,72 +511,40 @@ void AutofillPopupControllerImpl::InvalidateRow(size_t row) {
   view_->InvalidateRow(row);
 }
 
-#if !defined(OS_ANDROID)
-int AutofillPopupControllerImpl::GetDesiredPopupWidth() const {
-  int popup_width = controller_common_->RoundedElementBounds().width();
-  for (size_t i = 0; i < GetLineCount(); ++i) {
-    int row_size =
-        gfx::GetStringWidth(GetElidedValueAt(i), value_font_list_) +
-        gfx::GetStringWidth(GetElidedLabelAt(i), label_font_list_) +
-        RowWidthWithoutText(i);
-
-    popup_width = std::max(popup_width, row_size);
-  }
-
-  return popup_width;
-}
-
-int AutofillPopupControllerImpl::GetDesiredPopupHeight() const {
-  int popup_height = 2 * kPopupBorderThickness;
-
-  for (size_t i = 0; i < suggestions_.size(); ++i) {
-    popup_height += GetRowHeightFromId(suggestions_[i].frontend_id);
-  }
-
-  return popup_height;
-}
-
-int AutofillPopupControllerImpl::RowWidthWithoutText(int row) const {
-  int row_size = kEndPadding;
-
-  if (!elided_labels_[row].empty())
-    row_size += kNamePadding;
-
-  // Add the Autofill icon size, if required.
-  const base::string16& icon = suggestions_[row].icon;
-  if (!icon.empty()) {
-    int icon_width = ui::ResourceBundle::GetSharedInstance().GetImageNamed(
-        GetIconResourceID(icon)).Width();
-    row_size += icon_width + kIconPadding;
-  }
-
-  // Add the padding at the end.
-  row_size += kEndPadding;
-
-  // Add room for the popup border.
-  row_size += 2 * kPopupBorderThickness;
-
-  return row_size;
+WeakPtr<AutofillPopupControllerImpl> AutofillPopupControllerImpl::GetWeakPtr() {
+  return weak_ptr_factory_.GetWeakPtr();
 }
 
-void AutofillPopupControllerImpl::UpdatePopupBounds() {
-  int popup_width = GetDesiredPopupWidth();
-  int popup_height = GetDesiredPopupHeight();
+#if !defined(OS_ANDROID)
+void AutofillPopupControllerImpl::ElideValueAndLabelForRow(
+    size_t row,
+    int available_width) {
+  int value_width =
+      gfx::GetStringWidth(suggestions_[row].value, GetValueFontListForRow(row));
+  int label_width =
+      gfx::GetStringWidth(suggestions_[row].label, GetLabelFontList());
+  int total_text_length = value_width + label_width;
+
+  // The line can have no strings if it represents a UI element, such as
+  // a separator line.
+  if (total_text_length == 0)
+    return;
 
-  popup_bounds_ = controller_common_->GetPopupBounds(popup_width, popup_height);
-}
-#endif  // !defined(OS_ANDROID)
+  // Each field receives space in proportion to its length.
+  int value_size = available_width * value_width / total_text_length;
+  elided_values_[row] =
+      gfx::ElideText(suggestions_[row].value, GetValueFontListForRow(row),
+                     value_size, gfx::ELIDE_TAIL);
 
-WeakPtr<AutofillPopupControllerImpl> AutofillPopupControllerImpl::GetWeakPtr() {
-  return weak_ptr_factory_.GetWeakPtr();
+  int label_size = available_width * label_width / total_text_length;
+  elided_labels_[row] = gfx::ElideText(
+      suggestions_[row].label, GetLabelFontList(), label_size, gfx::ELIDE_TAIL);
 }
+#endif
 
 void AutofillPopupControllerImpl::ClearState() {
   // Don't clear view_, because otherwise the popup will have to get regenerated
   // and this will cause flickering.
-
-  popup_bounds_ = gfx::Rect();
-
   suggestions_.clear();
   elided_values_.clear();
   elided_labels_.clear();