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 49 matching lines @@
     } else {
       DrawAutofillEntry(canvas, i, line_rect);
     }
   }
 }
 
 void AutofillPopupViewViews::InvalidateRow(size_t row) {
   SchedulePaintInRect(controller_->layout_model().GetRowBounds(row));
 }
 
+/**
+* Autofill entries in ltr.
+*
+* ............................................................................
+* . ICON | HTTP WARNING MESSAGE VALUE                                | LABEL .
+* ............................................................................
+* . OTHER AUTOFILL ENTRY VALUE |                                LABEL | ICON .
+* ............................................................................
+*
+* Autofill entries in rtl.
+*
+* ............................................................................
+* . LABEL |                                HTTP WARNING MESSAGE VALUE | ICON .
+* ............................................................................
+* . ICON | LABEL                                | OTHER AUTOFILL ENTRY VALUE .
+* ............................................................................

[Mathieu, 2016/11/28 15:01:35]

Thanks, I would add a note in this comment for the next person wanting to modify
that code, indicating a test URL that would trigger the HTTP warning entry (you
can use a goo.gl short link if it's too long).

[lshang, 2016/11/29 10:44:30]

Done.

+*/
 void AutofillPopupViewViews::DrawAutofillEntry(gfx::Canvas* canvas,
                                                int index,
                                                const gfx::Rect& entry_rect) {
   canvas->FillRect(entry_rect, controller_->GetBackgroundColorForRow(index));
 
+  const bool is_http_warning =
+      (controller_->GetSuggestionAt(index).frontend_id ==
+       POPUP_ITEM_ID_HTTP_NOT_SECURE_WARNING_MESSAGE);
   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);
-  canvas->DrawStringRectWithFlags(
-      controller_->GetElidedValueAt(index),
-      controller_->layout_model().GetValueFontListForRow(index),
-      controller_->layout_model().GetValueFontColorForRow(index), value_rect,
-      text_align);
 
-  // Use this to figure out where all the other Autofill items should be placed.
-  int x_align_left =
-      is_rtl ? AutofillPopupLayoutModel::kEndPadding
-             : entry_rect.right() - AutofillPopupLayoutModel::kEndPadding;
+  int icon_x_align_left = 0;

[Mathieu, 2016/11/28 15:01:35]

would change to icon_x_align_start, so that it's clearer in the context of RTL
(this naming style is used on Android). What do you think?

[lshang, 2016/11/29 10:44:30]

Not for this case I think. RTL or LTR decides where to draw the element(the
position of the element in the whole gfx::Rect), in either mode
|icon_x_align_start| is the left align point of the icon to place it, so this is
a consistent definition.

For other cases like paddings, I agree that start/end is better than left/right
since they will be flipped around in different modes and definition of
left/right will change.

+  if ((is_http_warning && !is_rtl) || (!is_http_warning && is_rtl))
+    icon_x_align_left = value_rect.x();
+  else
+    icon_x_align_left = value_rect.right();
 
   // 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;
 
-    x_align_left += is_rtl ? 0 : -image.width();
+    if (icon_x_align_left == value_rect.right())
+      icon_x_align_left -= image.width();
 
-    canvas->DrawImageInt(image, x_align_left, icon_y);
+    canvas->DrawImageInt(image, icon_x_align_left, icon_y);
 
-    x_align_left += is_rtl
-                        ? image.width() + AutofillPopupLayoutModel::kIconPadding
-                        : -AutofillPopupLayoutModel::kIconPadding;
+    if (is_http_warning) {

[Mathieu, 2016/11/28 15:01:35]

I would add a comment to the reader such as

// An icon was drawn; adjust the |icon_x_align_start| value for the next element

[lshang, 2016/11/29 10:44:30]

Done.

+      icon_x_align_left +=
+          is_rtl ? -AutofillPopupLayoutModel::kHttpWarningIconPadding
+                 : image.width() +
+                       AutofillPopupLayoutModel::kHttpWarningIconPadding;
+    } else {
+      icon_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());
-  if (!is_rtl)
-    x_align_left -= label_width;
+  // Draw the value text
+  const int value_width = gfx::GetStringWidth(
+      controller_->GetElidedValueAt(index),
+      controller_->layout_model().GetValueFontListForRow(index));
+  int value_x_align_left = icon_x_align_left;
+
+  if (is_http_warning)

[Mathieu, 2016/11/28 15:01:35]

I would add {} here

[lshang, 2016/11/29 10:44:30]

Done.

+    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_->GetElidedLabelAt(index),
-      controller_->layout_model().GetLabelFontList(), kLabelTextColor,
-      gfx::Rect(x_align_left, entry_rect.y(), label_width, entry_rect.height()),
+      controller_->GetElidedValueAt(index),
+      controller_->layout_model().GetValueFontListForRow(index),
+      controller_->layout_model().GetValueFontColorForRow(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 = icon_x_align_left;
+
+    if (is_http_warning)

[Mathieu, 2016/11/28 15:01:35]

I would add {} around the blocks

[lshang, 2016/11/29 10:44:30]

Done.

+      label_x_align_left =
+          is_rtl ? value_rect.x() : value_rect.right() - label_width;
+    else
+      label_x_align_left += is_rtl ? 0 : -label_width;
+
+    canvas->DrawStringRectWithFlags(
+        controller_->GetElidedLabelAt(index),
+        controller_->layout_model().GetLabelFontListForRow(index),
+        kLabelTextColor, gfx::Rect(label_x_align_left, entry_rect.y(),
+                                   label_width, entry_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