Add Icon Support for New Autofill Gtk UI

chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc

Index: chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc
diff --git a/chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc b/chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc
index 017bef00a6f8a12595439aba0577accea2d92aad..3c674d0173b0f9a04cd03fbd87de2c17b190759d 100644
--- a/chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc
+++ b/chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc
@@ -10,6 +10,7 @@
 #include "base/utf_string_conversions.h"
 #include "chrome/browser/autofill/autofill_external_delegate.h"
 #include "chrome/browser/ui/gtk/gtk_util.h"
+#include "chrome/browser/ui/gtk/gtk_theme_service.h"
 #include "content/public/browser/render_view_host.h"
 #include "content/public/browser/web_contents.h"
 #include "ui/base/gtk/gtk_compat.h"
@@ -23,31 +24,45 @@ const GdkColor kBorderColor = GDK_COLOR_RGB(0xc7, 0xca, 0xce);
 const GdkColor kHoveredBackgroundColor = GDK_COLOR_RGB(0x0CD, 0xCD, 0xCD);
 const GdkColor kTextColor = GDK_COLOR_RGB(0x00, 0x00, 0x00);
 
+// The vertical height of each row.

[Ilya Sherman, 2012/05/11 22:12:17]

nit: "in pixels"

[csharp, 2012/05/14 14:19:03]

Done.

+const int kRowHeight = 24;
+
 // The amount of minimum padding between the Autofill value and label in pixels.
 const int kMiddlePadding = 10;
 
+// The amount of padding between the label and icon or edge and icon in pixels.
+const int kIconPadding = 5;
+
 // We have a 1 pixel border around the entire results popup.
 const int kBorderThickness = 1;
 
+// Width of the icons.

[Ilya Sherman, 2012/05/11 22:12:17]

nit: "in pixels"

[csharp, 2012/05/14 14:19:03]

Done.

+const int kIconWidth = 25;
+
+// Height of the icons

[Ilya Sherman, 2012/05/11 22:12:17]

nit: "in pixels"

[csharp, 2012/05/14 14:19:03]

Done.

+const int kIconHeight = 16;
+
 gfx::Rect GetWindowRect(GdkWindow* window) {
   return gfx::Rect(gdk_window_get_width(window),
                    gdk_window_get_height(window));
 }
 
 // Returns the rectangle containing the item at position |index| in the popup.
-gfx::Rect GetRectForRow(size_t index, int width, int height) {
-  return gfx::Rect(0, (index * height), width, height);
+gfx::Rect GetRectForRow(size_t index, int width) {
+  return gfx::Rect(0, (index * kRowHeight), width, kRowHeight);
 }
 
 }  // namespace
 
 AutofillPopupViewGtk::AutofillPopupViewGtk(
     content::WebContents* web_contents,
+    Profile* profile,
     AutofillExternalDelegate* external_delegate,
     GtkWidget* parent)
     : AutofillPopupView(web_contents, external_delegate),
       parent_(parent),
       window_(gtk_window_new(GTK_WINDOW_POPUP)),
+      theme_service_(GtkThemeService::GetFrom(profile)),

[Ilya Sherman, 2012/05/11 22:12:17]

nit: How about passing the GtkThemeService directly, rather than the profile?

[csharp, 2012/05/14 14:19:03]

Done.

       render_view_host_(web_contents->GetRenderViewHost()) {
   CHECK(parent != NULL);
   gtk_window_set_resizable(GTK_WINDOW(window_), FALSE);
@@ -69,8 +84,6 @@ AutofillPopupViewGtk::AutofillPopupViewGtk(
 
   // Cache the layout so we don't have to create it for every expose.
   layout_ = gtk_widget_create_pango_layout(window_, NULL);
-
-  row_height_ = font_.GetHeight();
 }
 
 AutofillPopupViewGtk::~AutofillPopupViewGtk() {
@@ -100,8 +113,7 @@ void AutofillPopupViewGtk::HideInternal() {
 }
 
 void AutofillPopupViewGtk::InvalidateRow(size_t row) {
-  GdkRectangle row_rect = GetRectForRow(
-      row, bounds_.width(), row_height_).ToGdkRectangle();
+  GdkRectangle row_rect = GetRectForRow(row, bounds_.width()).ToGdkRectangle();
   GdkWindow* gdk_window = gtk_widget_get_window(window_);
   gdk_window_invalidate_rect(gdk_window, &row_rect, FALSE);
 }
@@ -110,7 +122,7 @@ void AutofillPopupViewGtk::ResizePopup() {
   gtk_widget_set_size_request(
       window_,
       GetPopupRequiredWidth(),
-      row_height_ * autofill_values().size());
+      kRowHeight * autofill_values().size());
 }
 
 gboolean AutofillPopupViewGtk::HandleButtonRelease(GtkWidget* widget,
@@ -152,13 +164,13 @@ gboolean AutofillPopupViewGtk::HandleExpose(GtkWidget* widget,
   actual_content_height /= PANGO_SCALE;
 
   for (size_t i = 0; i < autofill_values().size(); ++i) {
-    gfx::Rect line_rect = GetRectForRow(i, window_rect.width(), row_height_);
+    gfx::Rect line_rect = GetRectForRow(i, window_rect.width());
     // Only repaint and layout damaged lines.
     if (!line_rect.Intersects(damage_rect))
       continue;
 
     if (IsSeparatorIndex(i)) {
-      int line_y = i * row_height_;
+      int line_y = i * kRowHeight;
 
       cairo_save(cr);
       cairo_move_to(cr, 0, line_y);
@@ -177,7 +189,7 @@ gboolean AutofillPopupViewGtk::HandleExpose(GtkWidget* widget,
     // Center the text within the line.
     int content_y = std::max(
         line_rect.y(),
-        line_rect.y() + ((row_height_ - actual_content_height) / 2));
+        line_rect.y() + ((kRowHeight - actual_content_height) / 2));
 
     // Draw the value.
     gtk_util::SetLayoutText(layout_, autofill_values()[i]);
@@ -190,12 +202,34 @@ gboolean AutofillPopupViewGtk::HandleExpose(GtkWidget* widget,
     // Draw the label.
     int x_align_left = window_rect.width() -
         font_.GetStringWidth(autofill_labels()[i]);
+
+    if (has_icons())

[Ilya Sherman, 2012/05/11 22:12:17]

Hmm, interesting.  Did you try the other option, of having rows that lack icons
have their labels extend further to the right?  I'm not sure offhand which would
be preferable -- I can see advantages and disadvantages to both.  If you tried
both and prefer how this looks, then that works for me :)

[csharp, 2012/05/14 14:19:03]

In my quick testing I don't think there is output where lines with just labels
and lines with labels and icons mixed. Only credit cards have the icons and I
don't think credit card profiles and other profiles can get mixed (although I
could be wrong). Do you know of some way to get that mix?

So I don't think it matters which one we pick (atm), but after thinking about it
a bit it might be better to right align the labels with no icons if we do end up
having them intermixed.

[Ilya Sherman, 2012/05/14 20:38:22]

The only way that I can think of to have mixed icons and labels is if a credit
card form field has a <datalist> element associated with it.  Since <datalist>
isn't supported by the new UI yet, I agree that it doesn't matter which we pick
for now.  Looking forward for when we do add support for <datalist>, I'm happy
with the current choice to right-align the labels without icons.

+      x_align_left -= 2 * kIconPadding + kIconWidth;
+
     gtk_util::SetLayoutText(layout_, autofill_labels()[i]);
 
     cairo_save(cr);
-    cairo_move_to(cr, x_align_left, line_rect.y());
+    cairo_move_to(cr, x_align_left, content_y);
     pango_cairo_show_layout(cr, layout_);
     cairo_restore(cr);
+
+    // Draw the icon, if one exists
+    if (autofill_icons()[i] != string16()) {

[Ilya Sherman, 2012/05/11 22:12:17]

nit: if (!autofill_icons()[i].empty())

[csharp, 2012/05/14 14:19:03]

Done.

+      int icon = GetIconResourceID(autofill_icons()[i].c_str());

[Ilya Sherman, 2012/05/11 22:12:17]

nit: Why are you passing the c_str()?

[csharp, 2012/05/14 14:19:03]

Good catch, that was from when I was trying different things earlier. Fixed.

+      if (icon != -1) {

[Ilya Sherman, 2012/05/11 22:12:17]

nit: Can this be a DCHECK rather than an if-stmt?

[csharp, 2012/05/14 14:19:03]

Yup, I had thought GetImageNamed might crash with bad input, but it looks like
it will just return an empty image.

+        int icon_y = line_rect.y() + ((kRowHeight - kIconHeight) / 2);
+
+        cairo_save(cr);
+        gtk_util::DrawFullImage(cr,
+                                window_,
+                                theme_service_->GetImageNamed(icon),
+                                window_rect.width() -
+                                  (kIconPadding + kIconWidth),
+                                icon_y);
+        cairo_restore(cr);
+      }
+
+    }
   }
 
   cairo_destroy(cr);
@@ -247,7 +281,7 @@ void AutofillPopupViewGtk::SetupLayout(const gfx::Rect& window_rect,
                                        const GdkColor& text_color) {
   int allocated_content_width = window_rect.width();
   pango_layout_set_width(layout_, allocated_content_width * PANGO_SCALE);
-  pango_layout_set_height(layout_, row_height_ * PANGO_SCALE);
+  pango_layout_set_height(layout_, kRowHeight * PANGO_SCALE);
 
   PangoAttrList* attrs = pango_attr_list_new();
 
@@ -270,7 +304,7 @@ void AutofillPopupViewGtk::SetBounds() {
 
   int bottom_of_field = origin_y + element_bounds().y() +
       element_bounds().height();
-  int popup_size =  row_height_ * autofill_values().size();
+  int popup_size =  kRowHeight * autofill_values().size();
 
   // Find the correct top position of the popup so that is doesn't go off
   // the screen.
@@ -291,20 +325,23 @@ void AutofillPopupViewGtk::SetBounds() {
 }
 
 int AutofillPopupViewGtk::GetPopupRequiredWidth() {
-  // TODO(csharp): Once the icon is also displayed it will affect the required
-  // size so it will need to be included in the calculation.
   int popup_width = element_bounds().width();
   DCHECK_EQ(autofill_values().size(), autofill_labels().size());
   for (size_t i = 0; i < autofill_values().size(); ++i) {
-    popup_width = std::max(popup_width,
-                           font_.GetStringWidth(autofill_values()[i]) +
-                           kMiddlePadding +
-                           font_.GetStringWidth(autofill_labels()[i]));
+    int row_size = font_.GetStringWidth(autofill_values()[i]) +
+        kMiddlePadding +
+        font_.GetStringWidth(autofill_labels()[i]);
+
+    // Add the icon size if required.
+    if (autofill_icons()[i] != string16())

[Ilya Sherman, 2012/05/11 22:12:17]

nit: if (!autofill_icons()[i].empty())

[csharp, 2012/05/14 14:19:03]

Done.

+      row_size += kIconWidth + 2 * kIconPadding;
+
+    popup_width = std::max(popup_width, row_size);
   }
 
   return popup_width;
 }
 
 int AutofillPopupViewGtk::LineFromY(int y) {
-  return y / row_height_;
+  return y / kRowHeight;
 }