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

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 "chrome/browser/ui/autofill/autofill_popup_controller.h"
 #include "chrome/browser/ui/autofill/autofill_popup_layout_model.h"
 #include "chrome/browser/ui/autofill/popup_constants.h"
 #include "components/autofill/core/browser/popup_item_ids.h"
@@ skipping 40 matching lines @@
 
   canvas->DrawColor(kPopupBackground);
   OnPaintBorder(canvas);
 
   for (size_t i = 0; i < controller_->GetLineCount(); ++i) {
     gfx::Rect line_rect = controller_->layout_model().GetRowBounds(i);
 
     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);
     }
   }
 }
 
 void AutofillPopupViewViews::InvalidateRow(size_t row) {
   SchedulePaintInRect(controller_->layout_model().GetRowBounds(row));
 }
 
@@ skipping 31 matching lines @@
     x_align_left += is_rtl ? 0 : -image->width();
 
     canvas->DrawImageInt(*image, x_align_left, icon_y);
 
     x_align_left +=
         is_rtl ? image->width() + AutofillPopupLayoutModel::kIconPadding
                : -AutofillPopupLayoutModel::kIconPadding;
   }
 
   // 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 =
       views::Widget::GetTopLevelWidgetForNativeView(
           controller->container_view());
 
   // 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