Basic Drawn text for new GTK Autofill popup.

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 5df1bdcef9f1bea5ddab2ef80c96dbcaeed4318b..1bf2333620187118417d6a9d9799bc0ba633a5e3 100644
--- a/chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc
+++ b/chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc
@@ -5,7 +5,47 @@
 #include "autofill_popup_view_gtk.h"
 
 #include "base/logging.h"
+#include "base/utf_string_conversions.h"
+#include "ui/gfx/rect.h"
+#include "ui/gfx/native_widget_types.h"
+#include "ui/base/gtk/gtk_hig_constants.h"
 #include "ui/base/gtk/gtk_windowing.h"
+#include "ui/gfx/font.h"
+
+namespace {
+const GdkColor kBorderColor = GDK_COLOR_RGB(0xc7, 0xca, 0xce);
+const GdkColor kTextColor = GDK_COLOR_RGB(0x00, 0x00, 0x00);
+
+// The amount of minimum padding between the autofill value and label.

[Ilya Sherman, 2012/01/11 23:36:29]

nit: You should mention that this is measured in pixels.

[csharp, 2012/01/12 19:39:11]

Done.

+const int kMiddlePadding = 10;
+
+// We have a 1 pixel border around the entire results popup.
+const int kBorderThickness = 1;
+
+gfx::Rect GetWindowRect(GdkWindow* window) {

[Ilya Sherman, 2012/01/11 23:36:29]

nit: Please include a comment for every method, even if it's something very
brief for a simple/obvious method.

[csharp, 2012/01/12 19:39:11]

Done.

+  gint width, height;
+  gdk_drawable_get_size(GDK_DRAWABLE(window), &width, &height);
+  return gfx::Rect(width, height);
+}
+
+gfx::Rect GetRectForLine(size_t line, int width, int height) {

[Ilya Sherman, 2012/01/11 23:36:29]

nit: In the context of "Rect", "Line" is ambiguous.  Perhaps something like
"GetRectForItem" or "GetRectForItemByIndex"?

[csharp, 2012/01/12 19:39:11]

Done.

+  return gfx::Rect(0, (line * height), width, height);
+}
+
+void SetTextToDraw(PangoLayout* layout,
+                   const string16& text) {

[Ilya Sherman, 2012/01/11 23:36:29]

nit: This looks like it could fit on the previous line.

[csharp, 2012/01/12 19:39:11]

Done.

+  // Pango is really easy to overflow and send into a computational death
+  // spiral that can corrupt the screen. Assume that we'll never have more than
+  // 2000 characters, which should be a safe assumption until we all get robot
+  // eyes. http://crbug.com/66576
+  std::string text_utf8 = UTF16ToUTF8(text);
+  if (text_utf8.length() > 2000)
+    text_utf8 = text_utf8.substr(0, 2000);
+
+  pango_layout_set_text(layout, text_utf8.data(), text_utf8.length());
+}
+
+}  // namespace
 
 AutofillPopupViewGtk::AutofillPopupViewGtk(content::WebContents* web_contents,
                                            GtkWidget* parent)
@@ -21,9 +61,15 @@ AutofillPopupViewGtk::AutofillPopupViewGtk(content::WebContents* web_contents,
   gtk_widget_add_events(window_, GDK_EXPOSURE_MASK);
   g_signal_connect(window_, "expose-event",
                    G_CALLBACK(HandleExposeThunk), this);
+
+  // 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();

[Ilya Sherman, 2012/01/11 23:36:29]

Hmm, where is the font_ initialized?

[csharp, 2012/01/12 19:39:11]

I just use the default constructor. Should I still have it listed in the
initialization list as font_()? 

[Ilya Sherman, 2012/01/13 02:45:36]

No, this is fine as is.  It just seemed a little weird to me that you were
caching a property of the default font -- wanted to make sure that's what you
intended :)

 }
 
 AutofillPopupViewGtk::~AutofillPopupViewGtk() {
+  g_object_unref(layout_);
   gtk_widget_destroy(window_);
 }
 
@@ -31,23 +77,34 @@ void AutofillPopupViewGtk::Hide() {
   gtk_widget_hide(window_);
 }
 
-// TODO(csharp): Actually show the values.
-void AutofillPopupViewGtk::Show(const std::vector<string16>& autofill_values,
-                                const std::vector<string16>& autofill_labels,
-                                const std::vector<string16>& autofill_icons,
-                                const std::vector<int>& autofill_unique_ids,
-                                int separator_index) {
+void AutofillPopupViewGtk::ShowInternal(
+    const std::vector<string16>& autofill_values,
+    const std::vector<string16>& autofill_labels,
+    const std::vector<string16>& autofill_icons) {
   gint origin_x, origin_y;
   gdk_window_get_origin(gtk_widget_get_window(parent_), &origin_x, &origin_y);
 
+  // Move the popup to appear right below the text field it is using.
   gtk_window_move(GTK_WINDOW(window_),
                   origin_x + element_bounds().x(),
                   origin_y + element_bounds().y() + element_bounds().height());
 
+  // Find out the maximum bounds required by the popup.
+  // TODO(csharp): Once the icon is also displayed it will affect the required
+  // size so it will need to be include in the calculation.
+  int popup_required_width = element_bounds().width();

[Ilya Sherman, 2012/01/11 23:36:29]

nit: How about just "popup_width"?

[csharp, 2012/01/12 19:39:11]

Done.

+  CHECK_EQ(autofill_values.size(), autofill_labels.size());

[Ilya Sherman, 2012/01/11 23:36:29]

nit: Can this be a DCHECK, so that we don't compile it into official builds?

[csharp, 2012/01/12 19:39:11]

Done.

+  for (size_t i = 0; i < autofill_values.size(); ++i) {
+    popup_required_width = std::max(popup_required_width,
+                                    font_.GetStringWidth(autofill_values[i]) +
+                                    kMiddlePadding +
+                                    font_.GetStringWidth(autofill_labels[i]));
+  }
+
   gtk_widget_set_size_request(
       window_,
-      element_bounds().width(),
-      element_bounds().height() * autofill_values.size());
+      popup_required_width,
+      row_height_ * autofill_values.size());
 
   gtk_widget_show(window_);
 
@@ -58,5 +115,88 @@ void AutofillPopupViewGtk::Show(const std::vector<string16>& autofill_values,
 
 gboolean AutofillPopupViewGtk::HandleExpose(GtkWidget* widget,
                                             GdkEventExpose* event) {
+  gfx::Rect window_rect = GetWindowRect(event->window);
+  gfx::Rect damage_rect = gfx::Rect(event->area);
+
+  cairo_t* cr = gdk_cairo_create(GDK_DRAWABLE(gtk_widget_get_window(widget)));
+  gdk_cairo_rectangle(cr, &event->area);
+  cairo_clip(cr);
+
+  // This assert is kinda ugly, but it would be more currently unneeded work
+  // to support painting a border that isn't 1 pixel thick.  There is no point
+  // in writing that code now, and explode if that day ever comes.
+  COMPILE_ASSERT(kBorderThickness == 1, border_1px_implied);

[Ilya Sherman, 2012/01/11 23:36:29]

Huh, I've never seen this macro used before.  Nifty!

[Elliot Glaysher, 2012/01/12 20:34:11]

OT: I'm using it in profile_impl to make sure that object's size doesn't
regress.

+  // Draw the 1px border around the entire window.
+  gdk_cairo_set_source_color(cr, &kBorderColor);
+  cairo_rectangle(cr, 0, 0, window_rect.width(), window_rect.height());
+  cairo_stroke(cr);
+
+  SetupLayout(window_rect, &kTextColor);
+
+  int actual_content_width, actual_content_height;
+  pango_layout_get_size(layout_, &actual_content_width, &actual_content_height);
+  actual_content_width /= PANGO_SCALE;
+  actual_content_height /= PANGO_SCALE;
+
+  for (size_t i = 0; i < autofill_values().size(); ++i) {
+    gfx::Rect line_rect = GetRectForLine(i, window_rect.width(), row_height_);
+    // Only repaint and layout damaged lines.
+    if (!line_rect.Intersects(damage_rect))
+      continue;
+
+    if (separator_index() == static_cast<int>(i)) {
+      int line_y = i * row_height_;
+
+      cairo_save(cr);
+      cairo_move_to(cr, 0, line_y);
+      cairo_line_to(cr, window_rect.width(), line_y);
+      cairo_stroke(cr);
+      cairo_restore(cr);
+    }
+
+    // Center the text within the line.
+    int content_y = std::max(
+        line_rect.y(),
+        line_rect.y() + ((row_height_ - actual_content_height) / 2));

[Ilya Sherman, 2012/01/11 23:36:29]

I don't understand what this computation does -- wouldn't (row_height_ -
actual_content_height) pretty much always be negative?

[csharp, 2012/01/12 19:39:11]

It will be positive because row_height_ >= actual_content_height. row_height_ is
the size of the row and the actual_content_height is how much space the text
will take.

[Ilya Sherman, 2012/01/13 02:45:36]

Hmm, where is actual_content_height updated to be the size of the text?  It
looks to me like actual_content_width and actual_content_height are set to the
dimensions of the entire frame... is that not so?

[csharp, 2012/01/13 18:09:03]

When I checked with the debugger the value for height was not the entire frame,
just a single line. I tried looking into the docs more to be sure but they don't
seem too explicit on this. If it ever does become the whole frame and make the
result negative, it shouldn't have an effect, since we'd then just take
line_rect.y()

Maybe Elliot knows why?

[Elliot Glaysher, 2012/01/13 20:39:30]

I unfortunately don't. And the original author of this code has left Google in
the interim.

+
+    // Draw the autofill value.
+    SetTextToDraw(layout_, autofill_values()[i]);
+
+    cairo_save(cr);
+    cairo_move_to(cr, 0, content_y);
+    pango_cairo_show_layout(cr, layout_);
+    cairo_restore(cr);
+
+    // Draw the autofill label.
+    int x_align_left = window_rect.width()
+        - font_.GetStringWidth(autofill_labels()[i]);

[Ilya Sherman, 2012/01/11 23:36:29]

nit: The minus sign should be on the preceding line (same as boolean operators
in if-stmts that wrap).

[csharp, 2012/01/12 19:39:11]

Done.

+    SetTextToDraw(layout_, autofill_labels()[i]);
+
+    cairo_save(cr);
+    cairo_move_to(cr, x_align_left, line_rect.y());
+    pango_cairo_show_layout(cr, layout_);
+    cairo_restore(cr);
+  }
+
+  cairo_destroy(cr);
+
   return TRUE;
 }
+
+void AutofillPopupViewGtk::SetupLayout(const gfx::Rect& window_rect,
+                                       const GdkColor* text_color) {

[Ilya Sherman, 2012/01/11 23:36:29]

nit: Can this be passed by const-reference instead?  (We generally only pass a
pointer if it is non-const, or if it can be NULL)

[csharp, 2012/01/12 19:39:11]

Done.

+  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);
+
+  PangoAttrList* attrs = pango_attr_list_new();
+
+  PangoAttribute* fg_attr = pango_attr_foreground_new(text_color->red,
+                                                      text_color->green,
+                                                      text_color->blue);
+  pango_attr_list_insert(attrs, fg_attr);  // Ownership taken.
+
+
+  pango_layout_set_attributes(layout_, attrs);  // Ref taken.
+  pango_attr_list_unref(attrs);
+}