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

chrome/browser/ui/views/autofill/autofill_popup_view_views.cc

 // 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.
 
 #include "chrome/browser/ui/views/autofill/autofill_popup_view_views.h"
 
 #include "base/optional.h"
 #include "chrome/browser/ui/autofill/autofill_popup_controller.h"
 #include "chrome/browser/ui/autofill/autofill_popup_layout_model.h"
 #include "chrome/grit/generated_resources.h"
@@ skipping 124 matching lines @@
 * . LABEL |                                HTTP WARNING MESSAGE VALUE | ICON .
 * ............................................................................
 * . ICON | LABEL                                | OTHER AUTOFILL ENTRY VALUE .
 * ............................................................................
 *
 * Anyone who wants to modify the code below, remember to make sure that HTTP
 * warning entry displays right. To trigger the warning message entry, enable
 * #mark-non-secure-as flag as "display form warning", go to goo.gl/CEIjc6 with
 * stored autofill info and check for credit card or password forms.
 */
 void AutofillPopupViewViews::DrawAutofillEntry(gfx::Canvas* canvas,

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

This entire function with all its directionality conditionals is almost totally
unreadable. It's not your fault, but every additional change makes it even more
complicated and is nefariously difficult to verify correctness for. Is it not
possible to do all the calculations in an LTR world and then permute the
coordinates for canvas calls at the last second? I'm thinking of promoting the
views::View::GetMirrored* functions to some sort of utility that you can call
for arbitrary rects.

(The bigger task of actually using views instead of this crazy bespoke layout
would be nice but is much harder.)

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

The initial change to this function was more straight forward:
https://codereview.chromium.org/2971783002/diff/60001/chrome/browser/ui/views...

And code in this patchset is result of some "simplification" we come up with.

Since we have tight deadline for  the project which includes "Show all
paswords", what I suggest is to revert this file to the initial modification +
adding TODO for myself. This way other steps of the project will not be blocked
on this CL.

We'll have to do something about this code either way, because our mocks include
significant changes in how password suggestion is displayed, and this one
definitely can't be implemented without simplifying this function. 

[Mathieu, 2017/07/13 14:16:04]

I agree with Evan, we need to rework this function with views.

I think we should work together on a refactor of this dropdown that will make
this much nicer in the M62 timeframe (and make your future password work much
easier). The password generation popup has done it this way.

For now there are not many changes to this file, so I wouldn't block this for
M61, but it's still weird to land this a few days before branch. 

                                                int index,
                                                const gfx::Rect& entry_rect) {
   canvas->FillRect(
       entry_rect,
       GetNativeTheme()->GetSystemColor(
           controller_->GetBackgroundColorIDForRow(index)));
 
-  const bool is_http_warning =
-      (controller_->GetSuggestionAt(index).frontend_id ==
-       POPUP_ITEM_ID_HTTP_NOT_SECURE_WARNING_MESSAGE);
+  int current_row_frontend_id = controller_->GetSuggestionAt(index).frontend_id;

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

nit: const

also, to shorten the variable name I think it's ok to get rid of "current_row_"

   const bool is_rtl = controller_->IsRTL();
   const int text_align =
       is_rtl ? gfx::Canvas::TEXT_ALIGN_RIGHT : gfx::Canvas::TEXT_ALIGN_LEFT;
   gfx::Rect value_rect = entry_rect;
   value_rect.Inset(AutofillPopupLayoutModel::kEndPadding, 0);
+  const bool icon_in_front_of_text =
+      (current_row_frontend_id ==
+           POPUP_ITEM_ID_HTTP_NOT_SECURE_WARNING_MESSAGE ||
+       current_row_frontend_id == POPUP_ITEM_ID_ALL_SAVED_PASSWORDS_ENTRY);
 
-  // If the icon is on the right of the rect, no matter in RTL or LTR mode.
-  bool icon_on_the_right = is_http_warning == is_rtl;
-  int x_align_left = icon_on_the_right ? value_rect.right() : value_rect.x();
+  int x_align_left = is_rtl ? value_rect.x() : value_rect.right();
+  // Draw the Autofill icon, if one exists
+  if (!controller_->GetSuggestionAt(index).icon.empty()) {
+    int row_height = controller_->layout_model().GetRowBounds(index).height();
+    // If the icon is on the right of the rect, no matter in RTL or LTR mode.
+    bool icon_on_the_right = icon_in_front_of_text == is_rtl;

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

nit: const

+    x_align_left = icon_on_the_right ? value_rect.right() : value_rect.x();
 
-  // Draw the Autofill icon, if one exists
-  int row_height = controller_->layout_model().GetRowBounds(index).height();
-  if (!controller_->GetSuggestionAt(index).icon.empty()) {
     const gfx::ImageSkia image =
         controller_->layout_model().GetIconImage(index);
     int icon_y = entry_rect.y() + (row_height - image.height()) / 2;
 
     int icon_x_align_left =
         icon_on_the_right ? x_align_left - image.width() : x_align_left;
 
     canvas->DrawImageInt(image, icon_x_align_left, icon_y);
 
+    const int width_icon_padding =
+        image.width() +
+        (icon_in_front_of_text
+             ? AutofillPopupLayoutModel::kPaddingBetweenLeftSideIconAndText
+             : AutofillPopupLayoutModel::kIconPadding);
+
     // An icon was drawn; adjust the |x_align_left| value for the next element.
-    if (is_http_warning) {
-      x_align_left =
-          icon_x_align_left +
-          (is_rtl ? -AutofillPopupLayoutModel::kHttpWarningIconPadding
-                  : image.width() +
-                        AutofillPopupLayoutModel::kHttpWarningIconPadding);
-    } else {
-      x_align_left =
-          icon_x_align_left +
-          (is_rtl ? image.width() + AutofillPopupLayoutModel::kIconPadding
-                  : -AutofillPopupLayoutModel::kIconPadding);
-    }
+    x_align_left = x_align_left + (icon_on_the_right ? -width_icon_padding
+                                                     : width_icon_padding);
   }
 
-  // Draw the value text
+  // Draw the value text.
   const int value_width = gfx::GetStringWidth(
       controller_->GetElidedValueAt(index),
       controller_->layout_model().GetValueFontListForRow(index));
   int value_x_align_left = x_align_left;
 
-  if (is_http_warning) {
+  if (icon_in_front_of_text) {
     value_x_align_left += is_rtl ? -value_width : 0;
   } else {
     value_x_align_left =
         is_rtl ? value_rect.right() - value_width : value_rect.x();
   }
 
   canvas->DrawStringRectWithFlags(
       controller_->GetElidedValueAt(index),
       controller_->layout_model().GetValueFontListForRow(index),
       GetNativeTheme()->GetSystemColor(
           controller_->layout_model().GetValueFontColorIDForRow(index)),
       gfx::Rect(value_x_align_left, value_rect.y(), value_width,
                 value_rect.height()),
       text_align);
 
   // Draw the label text, if one exists.
   if (!controller_->GetSuggestionAt(index).label.empty()) {
     const int label_width = gfx::GetStringWidth(
         controller_->GetElidedLabelAt(index),
         controller_->layout_model().GetLabelFontListForRow(index));
     int label_x_align_left = x_align_left;
 
-    if (is_http_warning) {
+    if (icon_in_front_of_text) {
       label_x_align_left =
           is_rtl ? value_rect.x() : value_rect.right() - label_width;
     } else {
       label_x_align_left += is_rtl ? 0 : -label_width;
     }
 
-    // TODO(crbug.com/678033):Add a GetLabelFontColorForRow function similar to
-    // GetValueFontColorForRow so that the cocoa impl could use it too
+    // TODO(crbug.com/678033): Add a GetLabelFontColorForRow function similar to
+    // GetValueFontColorForRow so that the cocoa impl could use it too.
     canvas->DrawStringRectWithFlags(
         controller_->GetElidedLabelAt(index),
         controller_->layout_model().GetLabelFontListForRow(index),
         GetNativeTheme()->GetSystemColor(
             ui::NativeTheme::kColorId_ResultsTableNormalDimmedText),
         gfx::Rect(label_x_align_left, entry_rect.y(), label_width,
                   entry_rect.height()),
         text_align);
   }
 }
@@ skipping 20 matching lines @@
 
   // If the top level widget can't be found, cancel the popup since we can't
   // fully set it up.
   if (!observing_widget)
     return NULL;
 
   return new AutofillPopupViewViews(controller, observing_widget);
 }
 
 }  // namespace autofill