AutofillPopupController clarifications + simplifications.

chrome/browser/ui/autofill/autofill_popup_controller_impl.h

Index: chrome/browser/ui/autofill/autofill_popup_controller_impl.h
diff --git a/chrome/browser/ui/autofill/autofill_popup_controller_impl.h b/chrome/browser/ui/autofill/autofill_popup_controller_impl.h
index c1ef8769128b5814229323b3d69a066e28705ee5..1c6bbd379de57397db298e1ddf4037dcd4584b17 100644
--- a/chrome/browser/ui/autofill/autofill_popup_controller_impl.h
+++ b/chrome/browser/ui/autofill/autofill_popup_controller_impl.h
@@ -35,10 +35,10 @@ class AutofillPopupControllerImpl : public AutofillPopupController,
       const gfx::Rect& element_bounds);
 
   // Shows the popup, or updates the existing popup with the given values.
-  void Show(const std::vector<string16>& autofill_values,
-            const std::vector<string16>& autofill_labels,
-            const std::vector<string16>& autofill_icons,
-            const std::vector<int>& autofill_unique_ids);
+  void Show(const std::vector<string16>& labels,
+            const std::vector<string16>& sub_labels,

[Ilya Sherman, 2012/12/20 04:34:52]

I'd rather prefer you keep the names "values" and "labels" for now, to be
consistent with the names used in the previous implementation; or else rename
the variables there as well.  Moreover, the notion a "label" -- referring to
what you call a sublabel here -- is used elsewhere in the Autofill code as well;
so it would be good to rename those at the same time also.

[Evan Stade, 2012/12/20 20:01:25]

I don't think it's a worthwhile use of time to update the old implementation.
in the context of the UI, "label" and "sub label" make more sense. What is a
"value" to the autofill backend can be used as a "label" in the UI.

I think it's outside of the scope of this CL to update all of autofill, and this
CL is already a yak shave compared to my real goal of implementing payments UI.
But I'll give you my opinion on autofill's variable naming: the term "label"
probably shouldn't be used in the backend at all because it implies something
about the presentation. sub_value or minor_value or differentiating_value or
something would be better. Also, value is fairly generic and non-descriptive, so
it probably should be replaced with something more specific.

[Ilya Sherman, 2012/12/20 21:57:42]

"value" is the value that gets filled into the active form field (with the
exception of phone numbers, which are exceptional in the Autofill code in
several ways).  "label" is the at-a-glance summary of the profile.  These terms
are currently used consistently throughout the Autofill code: in the
AutofillManager class, in the chrome://settings handler, in the AutofillAgent
code (which is deprecated), and in the newer UI code prior to this CL.  This CL
changes the semantics of "label", but in only one of these places, which
introduces inconsistency: sometimes a "label" is the profile summary; sometimes
it's the value that will be filled into the field.

In short, please don't overload the semantics of "label".  Either choose a
different term to describe the UI, or just don't rename "value" and "label" as
part of this CL.

[Evan Stade, 2012/12/20 23:03:59]

renamed to name and subtext.

+            const std::vector<string16>& icons,
+            const std::vector<int>& identifiers);
 
   // Hides the popup and destroys the controller. This also invalidates
   // |delegate_|. Virtual for testing.
@@ -56,31 +56,28 @@ class AutofillPopupControllerImpl : public AutofillPopupController,
   // AutofillPopupController implementation.
   virtual void ViewDestroyed() OVERRIDE;
   virtual void UpdateBoundsAndRedrawPopup() OVERRIDE;
-  virtual void SetSelectedPosition(int x, int y) OVERRIDE;
-  virtual bool AcceptAutofillSuggestion(const string16& value,
-                                        int unique_id,
-                                        unsigned index) OVERRIDE;
-  virtual void AcceptSelectedPosition(int x, int y) OVERRIDE;
-  virtual void ClearSelectedLine() OVERRIDE;
+  virtual void MouseHovered(int x, int y) OVERRIDE;
+  virtual void MouseClicked(int x, int y) OVERRIDE;
+  virtual void MouseLeftPopup() OVERRIDE;
+  virtual void AcceptSuggestion(size_t index) OVERRIDE;
   virtual int GetIconResourceID(const string16& resource_name) OVERRIDE;
-  virtual bool CanDelete(int id) OVERRIDE;
+  virtual bool CanDelete(size_t index) OVERRIDE;
 #if !defined(OS_ANDROID)
   virtual int GetPopupRequiredWidth() OVERRIDE;
   virtual int GetPopupRequiredHeight() OVERRIDE;
 #endif
-  virtual int GetRowHeightFromId(int unique_id) OVERRIDE;
-  virtual gfx::Rect GetRectForRow(size_t row, int width) OVERRIDE;
+  virtual gfx::Rect GetRowBounds(size_t row) OVERRIDE;
   virtual void SetPopupBounds(const gfx::Rect& bounds) OVERRIDE;
   virtual const gfx::Rect& popup_bounds() const OVERRIDE;
   virtual gfx::NativeView container_view() const OVERRIDE;
   virtual const gfx::Rect& element_bounds() const OVERRIDE;
-  virtual const std::vector<string16>& autofill_values() const OVERRIDE;
-  virtual const std::vector<string16>& autofill_labels() const OVERRIDE;
-  virtual const std::vector<string16>& autofill_icons() const OVERRIDE;
-  virtual const std::vector<int>& autofill_unique_ids() const OVERRIDE;
+  virtual const std::vector<string16>& labels() const OVERRIDE;
+  virtual const std::vector<string16>& sub_labels() const OVERRIDE;
+  virtual const std::vector<string16>& icons() const OVERRIDE;
+  virtual const std::vector<int>& identifiers() const OVERRIDE;
 #if !defined(OS_ANDROID)
   virtual const gfx::Font& label_font() const OVERRIDE;
-  virtual const gfx::Font& value_font() const OVERRIDE;
+  virtual const gfx::Font& sub_label_font() const OVERRIDE;
 #endif
   virtual int selected_line() const OVERRIDE;
   virtual bool delete_icon_hovered() const OVERRIDE;
@@ -111,6 +108,9 @@ class AutofillPopupControllerImpl : public AutofillPopupController,
   // Convert a y-coordinate to the closest line.
   int LineFromY(int y);
 
+  // Returns the height of a row depending on its type.
+  int GetRowHeightFromId(int unique_id);

[Ilya Sherman, 2012/12/20 04:34:52]

nit: |unique_id| -> |identifier|?

[Evan Stade, 2012/12/20 20:01:25]

Done, here and everywhere.

+
   // Returns true if the given |x| and |y| coordinates refer to a point that
   // hits the delete icon in the current selected line.
   bool DeleteIconIsUnder(int x, int y);
@@ -139,15 +139,15 @@ class AutofillPopupControllerImpl : public AutofillPopupController,
   gfx::Rect popup_bounds_;
 
   // The current Autofill query values.
-  std::vector<string16> autofill_values_;
-  std::vector<string16> autofill_labels_;
-  std::vector<string16> autofill_icons_;
-  std::vector<int> autofill_unique_ids_;
+  std::vector<string16> labels_;
+  std::vector<string16> sub_labels_;
+  std::vector<string16> icons_;
+  std::vector<int> identifiers_;
 
 #if !defined(OS_ANDROID)
   // The fonts for the popup text.
-  gfx::Font value_font_;
   gfx::Font label_font_;
+  gfx::Font sub_label_font_;
 #endif
 
   // The line that is currently selected by the user.