AutofillPopupController clarifications + simplifications.

chrome/browser/ui/autofill/autofill_popup_controller_impl.h

 // 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.
 
 #ifndef CHROME_BROWSER_UI_AUTOFILL_AUTOFILL_POPUP_CONTROLLER_IMPL_H_
 #define CHROME_BROWSER_UI_AUTOFILL_AUTOFILL_POPUP_CONTROLLER_IMPL_H_
 
 #include "base/gtest_prod_util.h"
 #include "base/string16.h"
 #include "chrome/browser/ui/autofill/autofill_popup_controller.h"
@@ skipping 17 matching lines @@
   // Creates a new |AutofillPopupControllerImpl|, or reuses |previous| if
   // the construction arguments are the same. |previous| may be invalidated by
   // this call.
   static AutofillPopupControllerImpl* GetOrCreate(
       AutofillPopupControllerImpl* previous,
       AutofillPopupDelegate* delegate,
       gfx::NativeView container_view,
       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.
   virtual void Hide();
 
  protected:
   FRIEND_TEST_ALL_PREFIXES(AutofillExternalDelegateBrowserTest,
                            CloseWidgetAndNoLeaking);
 
   AutofillPopupControllerImpl(AutofillPopupDelegate* delegate,
                               gfx::NativeView container_view,
                               const gfx::Rect& element_bounds);
   virtual ~AutofillPopupControllerImpl();
 
   // 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;
 
   // KeyboardListener implementation.
   virtual bool HandleKeyPressEvent(
       const content::NativeWebKeyboardEvent& event) OVERRIDE;
 
   // Like Hide(), but doesn't invalidate |delegate_| (the delegate will still
   // be informed of destruction).
@@ skipping 10 matching lines @@
 
   // The user has choosen the selected line.
   bool AcceptSelectedLine();
 
   // The user has removed a suggestion.
   bool RemoveSelectedLine();
 
   // 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);
 
   // Returns true if the given id refers to an element that can be accepted.
   bool CanAccept(int id);
 
   // Returns true if the popup still has non-options entries to show the user.
   bool HasAutofillEntries();
 
   AutofillPopupView* view() { return view_; }
 
   // |view_| pass throughs (virtual for testing).
   virtual void ShowView();
   virtual void InvalidateRow(size_t row);
 
  private:
   AutofillPopupView* view_;  // Weak reference.
   AutofillPopupDelegate* delegate_;  // Weak reference.
   gfx::NativeView container_view_;  // Weak reference.
 
   // The bounds of the text element that is the focus of the Autofill.
   const gfx::Rect element_bounds_;
 
   // The bounds of the Autofill popup.
   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.
   // |kNoSelection| indicates that no line is currently selected.
   int selected_line_;
 
   // Used to indicate if the delete icon within a row is currently selected.
   bool delete_icon_hovered_;
 
   // True if |HideInternal| has already been called.
   bool is_hiding_;
 };
 
 #endif  // CHROME_BROWSER_UI_AUTOFILL_AUTOFILL_POPUP_CONTROLLER_IMPL_H_