Http Bad: Put icon on the left of http warning message on Views

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

Index: chrome/browser/ui/views/autofill/autofill_popup_view_views.cc
diff --git a/chrome/browser/ui/views/autofill/autofill_popup_view_views.cc b/chrome/browser/ui/views/autofill/autofill_popup_view_views.cc
index b3ce678f84b03b47d62cb491a4ef1fa3a95eec14..6d592ea761f9b6fa1c74e4f0627cf28e821223c4 100644
--- a/chrome/browser/ui/views/autofill/autofill_popup_view_views.cc
+++ b/chrome/browser/ui/views/autofill/autofill_popup_view_views.cc
@@ -58,6 +58,9 @@ void AutofillPopupViewViews::OnPaint(gfx::Canvas* canvas) {
     if (controller_->GetSuggestionAt(i).frontend_id ==
         POPUP_ITEM_ID_SEPARATOR) {
       canvas->FillRect(line_rect, kLabelTextColor);
+    } else if (controller_->GetSuggestionAt(i).frontend_id ==
+               POPUP_ITEM_ID_HTTP_NOT_SECURE_WARNING_MESSAGE) {
+      DrawAutofillHttpWarningEntry(canvas, i, line_rect);
     } else {
       DrawAutofillEntry(canvas, i, line_rect);
     }
@@ -109,19 +112,85 @@ void AutofillPopupViewViews::DrawAutofillEntry(gfx::Canvas* canvas,
   }
 
   // Draw the label text.
-  const int label_width =
-      gfx::GetStringWidth(controller_->GetElidedLabelAt(index),
-                          controller_->layout_model().GetLabelFontList());
+  const int label_width = gfx::GetStringWidth(
+      controller_->GetElidedLabelAt(index),
+      controller_->layout_model().GetLabelFontListForRow(index));
   if (!is_rtl)
     x_align_left -= label_width;
-
   canvas->DrawStringRectWithFlags(
       controller_->GetElidedLabelAt(index),
-      controller_->layout_model().GetLabelFontList(), kLabelTextColor,
+      controller_->layout_model().GetLabelFontListForRow(index),
+      kLabelTextColor,
       gfx::Rect(x_align_left, entry_rect.y(), label_width, entry_rect.height()),
       text_align);
 }
 
+void AutofillPopupViewViews::DrawAutofillHttpWarningEntry(

[Mathieu, 2016/11/22 14:36:45]

While I do like a clean separation and appreciate the motivation, there is too
much repetition here for my taste, sorry. It makes the code harder to maintain
(what if there is a bug, and the person fixing it forgets to update this
method?).

From a quick comparison it looks like the code is 90% similar to
DrawAutofillEntry so it shouldn't be too bad to have them part of the same
method. 

Another point: Now that it's becoming non trivial, I think we could be better at
documenting what this does, with ASCII art, similar to what PaymentRequest does,
here:
https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr...

[lshang, 2016/11/27 04:59:13]

Done. Drawing the ASCII art is interesting.

+    gfx::Canvas* canvas,
+    int index,
+    const gfx::Rect& entry_rect) {
+  canvas->FillRect(entry_rect, controller_->GetBackgroundColorForRow(index));
+
+  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);
+
+  int x_align_left = is_rtl ? value_rect.right() : value_rect.x();
+
+  // Draw the Autofill icon, if one exists
+  ui::ResourceBundle& rb = ui::ResourceBundle::GetSharedInstance();
+  int row_height = controller_->layout_model().GetRowBounds(index).height();
+  if (!controller_->GetSuggestionAt(index).icon.empty()) {
+    int icon = controller_->layout_model().GetIconResourceID(
+        controller_->GetSuggestionAt(index).icon);
+    DCHECK_NE(-1, icon);
+    const gfx::ImageSkia* image = rb.GetImageSkiaNamed(icon);
+    int icon_y = entry_rect.y() + (row_height - image->height()) / 2;
+
+    x_align_left += is_rtl ? -image->width() : 0;
+
+    canvas->DrawImageInt(*image, value_rect.x(), icon_y);
+
+    // Use this to figure out where all the other Autofill items should be
+    // placed.
+    x_align_left += is_rtl
+                        ? -AutofillPopupLayoutModel::kHttpWarningIconPadding
+                        : image->width() +
+                              AutofillPopupLayoutModel::kHttpWarningIconPadding;
+  }
+
+  // Draw the value text.
+  const int value_width = gfx::GetStringWidth(
+      controller_->GetElidedValueAt(index),
+      controller_->layout_model().GetValueFontListForRow(index));
+  if (is_rtl)
+    x_align_left -= value_width;
+  canvas->DrawStringRectWithFlags(
+      controller_->GetElidedValueAt(index),
+      controller_->layout_model().GetValueFontListForRow(index),
+      controller_->layout_model().GetValueFontColorForRow(index),
+      gfx::Rect(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()) {

[Mathieu, 2016/11/22 14:36:45]

you can add this if condition in the main DrawAutofillEntry, no?

[lshang, 2016/11/27 04:59:13]

Done.

+    const int label_width = gfx::GetStringWidth(
+        controller_->GetElidedLabelAt(index),
+        controller_->layout_model().GetLabelFontListForRow(index));
+
+    x_align_left = is_rtl ? value_rect.x() : value_rect.right() - label_width;
+
+    canvas->DrawStringRectWithFlags(
+        controller_->GetElidedLabelAt(index),
+        controller_->layout_model().GetLabelFontListForRow(index),
+        kLabelTextColor, gfx::Rect(x_align_left, value_rect.y(), label_width,
+                                   value_rect.height()),
+        text_align);
+  }
+}
+
 AutofillPopupView* AutofillPopupView::Create(
     AutofillPopupController* controller) {
   views::Widget* observing_widget =