Elide text in the new Autofill UI

chrome/browser/ui/autofill/autofill_popup_controller_impl.cc

Index: chrome/browser/ui/autofill/autofill_popup_controller_impl.cc
diff --git a/chrome/browser/ui/autofill/autofill_popup_controller_impl.cc b/chrome/browser/ui/autofill/autofill_popup_controller_impl.cc
index 6473184eadf65474d343151d4f7d8ea8de7e3194..96add74ae5e1826a18d8d6ae8b748913c74f4e75 100644
--- a/chrome/browser/ui/autofill/autofill_popup_controller_impl.cc
+++ b/chrome/browser/ui/autofill/autofill_popup_controller_impl.cc
@@ -12,6 +12,10 @@
 #include "grit/webkit_resources.h"
 #include "third_party/WebKit/Source/WebKit/chromium/public/WebAutofillClient.h"
 #include "ui/base/events/event.h"
+#include "ui/base/text/text_elider.h"
+#include "ui/gfx/display.h"
+#include "ui/gfx/screen.h"
+#include "ui/gfx/vector2d.h"
 
 using WebKit::WebAutofillClient;
 
@@ -118,12 +122,36 @@ void AutofillPopupControllerImpl::Show(
 #if !defined(OS_ANDROID)
   // Android displays the long text with ellipsis using the view attributes.
 
-  // TODO(csharp): Fix crbug.com/156163 and use better logic when clipping.
+  // Don't let the popup have a smaller width then then element it is a popup

[Ilya Sherman, 2013/01/10 23:34:36]

nit: "then then" -> "than the"

[csharp, 2013/01/11 17:51:12]

Done.

+  // for. If the text field is already going of the screen, then the popup
+  // should follow.

[Ilya Sherman, 2013/01/10 23:34:36]

As I mention below, this is almost certainly wrong, for security reasons.

[csharp, 2013/01/11 17:51:12]

Done.

+  int max_popup_width = std::max(MaxVisiblePopupWidth(),
+                                 element_bounds().width());
+
   for (size_t i = 0; i < names_.size(); ++i) {

[Ilya Sherman, 2013/01/10 23:34:36]

nit: Please add a comment indicating that this loop is for eliding strings to
fit in the available width.

[csharp, 2013/01/11 17:51:12]

Done.

-    if (names_[i].length() > 15)
-      names_[i].erase(15);
-    if (subtexts[i].length() > 15)
-      subtexts_[i].erase(15);
+    int total_text_length = name_font().GetStringWidth(names[i])+

[Ilya Sherman, 2013/01/10 23:34:36]

nit: Add a space before the plus sign.

[csharp, 2013/01/11 17:51:12]

Done.

+        subtext_font().GetStringWidth(subtexts_[i]);
+    // The line can have no strings if it represents a UI element, such as
+    // a separator line.
+    if (total_text_length == 0)
+      continue;
+
+    int row_available_width = max_popup_width - RowWidthWithoutText(i);

[Ilya Sherman, 2013/01/10 23:34:36]

Optional nit: Drop the "row_" prefix IMO

[csharp, 2013/01/11 17:51:12]

Done.

+
+    // Each field recieves space in proportion to it length.

[Ilya Sherman, 2013/01/10 23:34:36]

nit: "it length" -> "its length"

[csharp, 2013/01/11 17:51:12]

Done.

+    int name_size = row_available_width *
+        name_font().GetStringWidth(names_[i]) / total_text_length;
+    names_[i] = ui::ElideText(names_[i],
+                              name_font(),
+                              name_size,
+                              ui::ELIDE_AT_END);
+
+    int subtext_size = row_available_width *
+        subtext_font().GetStringWidth(subtexts[i]) / total_text_length;
+    subtexts_[i] = ui::ElideText(subtexts_[i],
+                                 subtext_font(),
+                                 subtext_size,
+                                 ui::ELIDE_AT_END);
   }
 #endif
 
@@ -209,21 +237,9 @@ int AutofillPopupControllerImpl::GetPopupRequiredWidth() {
   int popup_width = element_bounds().width();
   DCHECK_EQ(names().size(), subtexts().size());
   for (size_t i = 0; i < names().size(); ++i) {
-    int row_size = kEndPadding +
-        name_font_.GetStringWidth(names()[i]) +
-        kNamePadding +
-        subtext_font_.GetStringWidth(subtexts()[i]);
-
-    // Add the Autofill icon size, if required.
-    if (!icons()[i].empty())
-      row_size += kAutofillIconWidth + kIconPadding;
-
-    // Add delete icon, if required.
-    if (CanDelete(i))
-      row_size += kDeleteIconWidth + kIconPadding;
-
-    // Add the padding at the end
-    row_size += kEndPadding;
+    int row_size = name_font_.GetStringWidth(names()[i]) +
+        subtext_font_.GetStringWidth(subtexts()[i]) +
+        RowWidthWithoutText(i);
 
     popup_width = std::max(popup_width, row_size);
   }
@@ -504,3 +520,34 @@ void AutofillPopupControllerImpl::ShowView() {
 void AutofillPopupControllerImpl::InvalidateRow(size_t row) {
   view_->InvalidateRow(row);
 }
+
+int AutofillPopupControllerImpl::RowWidthWithoutText(int row) {
+  int row_size = kEndPadding + kNamePadding;
+
+  // Add the Autofill icon size, if required.
+  if (!icons_[row].empty())
+    row_size += kAutofillIconWidth + kIconPadding;
+
+  // Add delete icon, if required.

[Ilya Sherman, 2013/01/10 23:34:36]

nit: "Add the delete icon size", or perhaps "Add space for the delete icon"?

[csharp, 2013/01/11 17:51:12]

Done.

+  if (CanDelete(row))
+    row_size += kDeleteIconWidth + kIconPadding;
+
+  // Add the padding at the end
+  row_size += kEndPadding;
+
+  return row_size;
+}
+
+

[Ilya Sherman, 2013/01/10 23:34:36]

nit: Extra newline; please remove.

[csharp, 2013/01/11 17:51:12]

Done.

+int AutofillPopupControllerImpl::MaxVisiblePopupWidth() {
+  gfx::Point right_corner_of_popup = element_bounds().origin() +

[Ilya Sherman, 2013/01/10 23:34:36]

When anchoring to the left edge of the field, we should never anchor beyond the
left-most on-screen pixel of the text field.  That is, we shouldn't assume that
element_bounds.origin() is visible on the screen.  This is important to get
right for security reasons: a malicious website designer shouldn't be able to
force us to show any part of the popup off screen.

[csharp, 2013/01/11 17:51:12]

Ah, I hadn't been aware of that issue. Fixed.

+      gfx::Vector2d(GetPopupRequiredWidth(), 0);
+  gfx::Screen* screen =
+      gfx::Screen::GetScreenFor(container_view());
+  gfx::Display display = screen->GetDisplayNearestPoint(right_corner_of_popup);

[Ilya Sherman, 2013/01/10 23:34:36]

In addition to anchoring to the left edge of the field, we should be able to
anchor to the right edge.  If the window happens to be positioned so that the
field is near the right edge of the screen, the popup should grow to the left
rather than to the right.  Again, this is important for security reasons, so
that an attacker can't force us to display a popup with all of the text elided
by simply placing a field super near the edge of the screen.

[csharp, 2013/01/11 17:51:12]

Done.

+
+  int rightmost_display_x =  display.GetSizeInPixel().width() +

[Ilya Sherman, 2013/01/10 23:34:36]

nit: Extra space after the equals sign.

[csharp, 2013/01/11 17:51:12]

Done.

+      display.bounds().x() * display.device_scale_factor();
+
+  return rightmost_display_x - element_bounds().origin().x();
+}