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

chrome/browser/ui/autofill/autofill_popup_layout_model.cc

 // Copyright (c) 2016 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_layout_model.h"
 
 #include <algorithm>
 
 #include "base/macros.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/ui/autofill/autofill_popup_view.h"
 #include "chrome/browser/ui/autofill/popup_constants.h"
 #include "components/autofill/core/browser/autofill_experiments.h"
 #include "components/autofill/core/browser/credit_card.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 "components/grit/components_scaled_resources.h"
 #include "third_party/skia/include/core/SkColor.h"
 #include "ui/base/resource/resource_bundle.h"
 #include "ui/gfx/color_palette.h"
 #include "ui/gfx/color_utils.h"
 #include "ui/gfx/font_list.h"
 #include "ui/gfx/geometry/rect_conversions.h"
 #include "ui/gfx/image/image_skia.h"
 #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
 
 namespace autofill {
 
 namespace {
 
 // The vertical height of each row in pixels.
 const size_t kRowHeight = 24;
 
@@ skipping 75 matching lines @@
 
     popup_width = std::max(popup_width, row_size);
   }
 
   return popup_width;
 }
 
 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.
   row_size += kEndPadding;
 
   // Add room for the popup border.
   row_size += 2 * kPopupBorderThickness;
 
   return row_size;
 }
@@ skipping 26 matching lines @@
     case POPUP_ITEM_ID_INSECURE_CONTEXT_PAYMENT_DISABLED_MESSAGE:
     case POPUP_ITEM_ID_CLEAR_FORM:
     case POPUP_ITEM_ID_CREDIT_CARD_SIGNIN_PROMO:
     case POPUP_ITEM_ID_AUTOFILL_OPTIONS:
     case POPUP_ITEM_ID_CREATE_HINT:
     case POPUP_ITEM_ID_SCAN_CREDIT_CARD:
     case POPUP_ITEM_ID_SEPARATOR:
     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:
     case POPUP_ITEM_ID_USERNAME_ENTRY:
       return bold_font_list_;
   }
   NOTREACHED();
   return normal_font_list_;
 }
 
@@ skipping 13 matching lines @@
   switch (suggestions[index].frontend_id) {
     case POPUP_ITEM_ID_HTTP_NOT_SECURE_WARNING_MESSAGE:
       return ui::NativeTheme::kColorId_AlertSeverityHigh;
     case POPUP_ITEM_ID_INSECURE_CONTEXT_PAYMENT_DISABLED_MESSAGE:
       return ui::NativeTheme::kColorId_ResultsTableNormalDimmedText;
     default:
       return ui::NativeTheme::kColorId_ResultsTableNormalText;
   }
 }
 
 gfx::ImageSkia AutofillPopupLayoutModel::GetIconImage(size_t index) const {

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

side nit: I believe this interface should return a gfx::Image

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

will address tomorrow, code search doesn't work for me now and I prefer to use
it in order to find what to use instead of CreateVectorIcon in order to get
Image instead of ImageSkia.

[Evan Stade, 2017/07/13 16:48:43]

By "side nit" I intended to convey it wasn't important to address as part of
this patch but maybe instead in a later cleanup. In the case of CreateVectorIcon
you would have to wrap in gfx::Image() which is no big win or loss. But for the
resource bundle images, you'd use GetNativeImageNamed() which I believe avoids a
conversion from skia image to NSImage for Cocoa.

[melandory, 2017/07/25 15:08:49]

Could you suggest what method should I use in order to create gfx::Image

   std::vector<autofill::Suggestion> suggestions = delegate_->GetSuggestions();
   const base::string16& icon_str = suggestions[index].icon;
 
   // For http warning message, get icon images from VectorIcon, which is the
   // same as security indicator icons in location bar.
   if (suggestions[index].frontend_id ==
       POPUP_ITEM_ID_HTTP_NOT_SECURE_WARNING_MESSAGE) {

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

nit: why this check? aren't the string comparisons (admittedly quite icky)
sufficient?

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

Done.

     if (icon_str == base::ASCIIToUTF16("httpWarning")) {
       return gfx::CreateVectorIcon(toolbar::kHttpIcon, kHttpWarningIconWidth,
                                    gfx::kChromeIconGrey);
     }
     DCHECK_EQ(icon_str, base::ASCIIToUTF16("httpsInvalid"));
     return gfx::CreateVectorIcon(toolbar::kHttpsInvalidIcon,
                                  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);
   return *ui::ResourceBundle::GetSharedInstance().GetImageSkiaNamed(icon_id);
 }
 #endif

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

nit: can you add what condition this is the end of

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

Done.

 
 int AutofillPopupLayoutModel::LineFromY(int y) const {
   std::vector<autofill::Suggestion> suggestions = delegate_->GetSuggestions();
   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;
@@ skipping 55 matching lines @@
 bool AutofillPopupLayoutModel::IsIconAtStart(int frontend_id) const {
   return frontend_id == POPUP_ITEM_ID_HTTP_NOT_SECURE_WARNING_MESSAGE ||
       (is_credit_card_popup_ && IsIconInCreditCardPopupAtStart());
 }
 
 unsigned int AutofillPopupLayoutModel::GetMargin() const {
   return GetPopupMargin();
 }
 
 }  // namespace autofill