Skeleton for showing "Show all saved passwords row" for Linux/CrOs/Windows platforms

chrome/browser/ui/autofill/autofill_popup_layout_model.cc

Index: chrome/browser/ui/autofill/autofill_popup_layout_model.cc
diff --git a/chrome/browser/ui/autofill/autofill_popup_layout_model.cc b/chrome/browser/ui/autofill/autofill_popup_layout_model.cc
index 385d472d25dcc03c910ac86ecd1555f3c2568b6d..0df04db066b4b512de88c8f59e06aa4f249c4948 100644
--- a/chrome/browser/ui/autofill/autofill_popup_layout_model.cc
+++ b/chrome/browser/ui/autofill/autofill_popup_layout_model.cc
@@ -26,6 +26,7 @@
 #include "ui/gfx/paint_vector_icon.h"
 
 #if !defined(OS_ANDROID)
+#include "chrome/app/vector_icons/vector_icons.h"
 #include "components/toolbar/vector_icons.h"  // nogncheck
 #endif
 
@@ -121,19 +122,26 @@ int AutofillPopupLayoutModel::GetDesiredPopupWidth() const {
 int AutofillPopupLayoutModel::RowWidthWithoutText(int row,
                                                   bool with_label) const {
   std::vector<autofill::Suggestion> suggestions = delegate_->GetSuggestions();
-  bool is_warning_message = (suggestions[row].frontend_id ==
-                             POPUP_ITEM_ID_HTTP_NOT_SECURE_WARNING_MESSAGE);
+  bool is_row_with_left_handside_icon =

[Evan Stade, 2017/07/12 17:53:52]

nit: const bool

also, "lefthand side" would translate to "lefthand_side" in c_hacker_syntax, but
to be more technically accurate in the context of a possible RTL layout I think
you'd call this something like "icon_positioned_at_start".

[melandory, 2017/07/12 23:13:47]

Done.

+      ((suggestions[row].frontend_id ==
+        POPUP_ITEM_ID_HTTP_NOT_SECURE_WARNING_MESSAGE) ||
+       (suggestions[row].frontend_id ==
+        POPUP_ITEM_ID_ALL_SAVED_PASSWORDS_ENTRY));
 
   int row_size = kEndPadding;

[Evan Stade, 2017/07/12 17:53:51]

imo this would be easier to follow if you just started with 

int row_size = 2 * (kEndPadding + kPopupBorderThickness);

instead of adding things little by little.

[melandory, 2017/07/12 23:13:47]

Done.

 
-  if (with_label)
-    row_size += is_warning_message ? kHttpWarningNamePadding : kNamePadding;
+  if (with_label) {

[Evan Stade, 2017/07/12 17:53:52]

it's surprising to me there can be rows with no label. After some digging I
realize it's because "label" in this case is the subtext after the "value" aka
"name".

Can we improve the variable names to make this more apparent? 
"RowWidthWithoutText" is especially confusing to pair with "with_label".

Maybe 

RowWidthWithoutText(int row, bool has_subtext)

[melandory, 2017/07/12 23:13:47]

Done.

+    row_size +=
+        is_row_with_left_handside_icon ? kHttpWarningNamePadding : kNamePadding;

[vasilii, 2017/07/12 13:20:13]

Can we also rename |kHttpWarningNamePadding|?

[Evan Stade, 2017/07/12 17:53:51]

technically this is still only used for http warnings, right? Because the
show-all-passwords row doesn't have subtext. I would change the condition back
to just checking if it's a warning message instead of 
is_row_with_left_handside_icon.

[melandory, 2017/07/12 23:13:47]

Done.

+  }
 
   // Add the Autofill icon size, if required.
   const base::string16& icon = suggestions[row].icon;
   if (!icon.empty()) {
-    row_size += GetIconImage(row).width() +
-                (is_warning_message ? kHttpWarningIconPadding : kIconPadding);
+    row_size +=
+        GetIconImage(row).width() + (is_row_with_left_handside_icon
+                                         ? kPaddingBetweenLeftSideIconAndText
+                                         : kIconPadding);
   }
 
   // Add the padding at the end.
@@ -180,6 +188,7 @@ const gfx::FontList& AutofillPopupLayoutModel::GetValueFontListForRow(
     case POPUP_ITEM_ID_HTTP_NOT_SECURE_WARNING_MESSAGE:
     case POPUP_ITEM_ID_TITLE:
     case POPUP_ITEM_ID_PASSWORD_ENTRY:
+    case POPUP_ITEM_ID_ALL_SAVED_PASSWORDS_ENTRY:
       return normal_font_list_;
     case POPUP_ITEM_ID_AUTOCOMPLETE_ENTRY:
     case POPUP_ITEM_ID_DATALIST_ENTRY:
@@ -230,6 +239,11 @@ gfx::ImageSkia AutofillPopupLayoutModel::GetIconImage(size_t index) const {
                                  kHttpWarningIconWidth, gfx::kGoogleRed700);
   }
 
+  if (icon_str == base::ASCIIToUTF16("showAllSavedPasswords")) {
+    return gfx::CreateVectorIcon(kShowAllSavedPasswordsIcon,
+                                 kHttpWarningIconWidth, gfx::kChromeIconGrey);

[Evan Stade, 2017/07/12 17:53:52]

nit: kHttpWarningIconWidth is one I'd rename.

[melandory, 2017/07/12 23:13:47]

Done.

+  }
+
   // For other suggestion entries, get icon from PNG files.
   int icon_id = GetIconResourceID(icon_str);
   DCHECK_NE(-1, icon_id);