[Password Generation] Update Aura UI

chrome/browser/ui/views/autofill/password_generation_popup_view_views.cc

Index: chrome/browser/ui/views/autofill/password_generation_popup_view_views.cc
diff --git a/chrome/browser/ui/views/autofill/password_generation_popup_view_views.cc b/chrome/browser/ui/views/autofill/password_generation_popup_view_views.cc
index d15b870e722a2f36edcdf7fa17f635f31e88fee4..d04bc34f27d6d4219d2e664c3629f3f17e8e26de 100644
--- a/chrome/browser/ui/views/autofill/password_generation_popup_view_views.cc
+++ b/chrome/browser/ui/views/autofill/password_generation_popup_view_views.cc
@@ -6,9 +6,12 @@
 
 #include "base/strings/string16.h"
 #include "chrome/browser/ui/autofill/password_generation_popup_controller.h"
+#include "grit/theme_resources.h"
+#include "ui/base/resource/resource_bundle.h"
 #include "ui/gfx/canvas.h"
 #include "ui/views/background.h"
 #include "ui/views/border.h"
+#include "ui/views/controls/image_view.h"
 #include "ui/views/controls/label.h"
 #include "ui/views/controls/styled_label.h"
 #include "ui/views/layout/box_layout.h"
@@ -20,35 +23,65 @@ namespace {
 
 // The amount of whitespace that is present when there is no padding. Used
 // to get the proper spacing in the help section.
-const int kHelpVerticalOffset = 3;
+const int kHelpVerticalOffset = 5;
+
+class PasswordTextBox : public views::View {

[Evan Stade, 2014/06/19 00:16:35]

class level comment

[Garrett Casto, 2014/06/20 23:47:01]

Done.

+ public:
+  PasswordTextBox(const base::string16& suggestion_text,
+                  const base::string16& password,

[Evan Stade, 2014/06/19 00:16:35]

is this actually a password? that's what the var name makes it look like. Or is
it simply the text "Password:"? Maybe some docs would help.

[Garrett Casto, 2014/06/20 23:47:01]

Added docs.

+                  const gfx::FontList& font_list) {

[Evan Stade, 2014/06/19 00:16:36]

nit: technically, we're supposed to avoid doing complicated stuff in a ctor;
instead do it in Init(). Most Views code doesn't follow this pattern but there's
no reason not to move in that general direction when writing new code.

(And despite what Label does, the c'tor shouldn't call Init().)

[Garrett Casto, 2014/06/20 23:47:01]

My understanding of this has been "Don't do things that can fail in the
constructor". The wording of the top level style guide rule does seem more
strict than that, though the actual decision seems to be more in line with my
understanding. Regardless, I'll use Init().

[Evan Stade, 2014/06/23 18:20:40]

it's also "don't call virtual methods" (or functions that call functions that
call virtual methods)

[Garrett Casto, 2014/06/24 01:15:12]

Yeah, I suppose keeping track of the viral nature of this is difficult, even
though SetLayoutManager and AddChildView are not virtual.

+    views::BoxLayout* box_layout = new views::BoxLayout(
+        views::BoxLayout::kVertical, 0, 13, 5);
+    box_layout->set_main_axis_alignment(
+        views::BoxLayout::MAIN_AXIS_ALIGNMENT_START);
+    SetLayoutManager(box_layout);
+
+    views::Label* suggestion_label = new views::Label(
+        suggestion_text, font_list.DeriveWithStyle(gfx::Font::BOLD));
+    suggestion_label->SetHorizontalAlignment(gfx::ALIGN_LEFT);
+    AddChildView(suggestion_label);
+
+    views::Label* password_label = new views::Label(password, font_list);
+    password_label->SetHorizontalAlignment(gfx::ALIGN_LEFT);
+    AddChildView(password_label);
+  }
+
+  virtual ~PasswordTextBox() {}
+
+  // views::View:
+  virtual bool CanProcessEventsWithinSubtree() const OVERRIDE {
+    // Send events to the parent view for handling.
+    return false;
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(PasswordTextBox);
+};
 
 // Class that shows the password and the suggestion side-by-side.
-class PasswordRow : public views::View {
+class PasswordBox : public views::View {
  public:
-  PasswordRow(const base::string16& password,
+  PasswordBox(const base::string16& password,
               const base::string16& suggestion,
               const gfx::FontList& font_list,
               int horizontal_border) {
-    set_clip_insets(gfx::Insets(
-        PasswordGenerationPopupView::kPasswordVerticalInset, 0,
-        PasswordGenerationPopupView::kPasswordVerticalInset, 0));
     views::BoxLayout* box_layout = new views::BoxLayout(
-        views::BoxLayout::kHorizontal, horizontal_border, 0, 0);
+        views::BoxLayout::kHorizontal, horizontal_border, 0, 15);
     box_layout->set_main_axis_alignment(
-        views::BoxLayout::MAIN_AXIS_ALIGNMENT_FILL);
+        views::BoxLayout::MAIN_AXIS_ALIGNMENT_START);
     SetLayoutManager(box_layout);
 
-    password_label_ = new views::Label(password, font_list);
-    password_label_->SetHorizontalAlignment(gfx::ALIGN_LEFT);
-    AddChildView(password_label_);
+    views::ImageView* key_image = new views::ImageView();
+    key_image->SetImage(
+        ui::ResourceBundle::GetSharedInstance().GetImageNamed(
+            IDR_GENERATE_PASSWORD_KEY).ToImageSkia());
+    AddChildView(key_image);
 
-    suggestion_label_ = new views::Label(suggestion, font_list);
-    suggestion_label_->SetHorizontalAlignment(gfx::ALIGN_RIGHT);
-    suggestion_label_->SetEnabledColor(
-        PasswordGenerationPopupView::kExplanatoryTextColor);
-    AddChildView(suggestion_label_);
+    PasswordTextBox* password_text_box =
+        new PasswordTextBox(suggestion, password, font_list);
+    AddChildView(password_text_box);
   }
-  virtual ~PasswordRow() {}
+  virtual ~PasswordBox() {}
 
   // views::View:
   virtual bool CanProcessEventsWithinSubtree() const OVERRIDE {
@@ -57,11 +90,7 @@ class PasswordRow : public views::View {
   }
 
  private:
-  // Child views. Not owned.
-  views::Label* suggestion_label_;
-  views::Label* password_label_;
-
-  DISALLOW_COPY_AND_ASSIGN(PasswordRow);
+  DISALLOW_COPY_AND_ASSIGN(PasswordBox);
 };
 
 }  // namespace
@@ -77,6 +106,7 @@ PasswordGenerationPopupViewViews::PasswordGenerationPopupViewViews(
 
   help_label_ = new views::StyledLabel(controller_->HelpText(), this);
   help_label_->SetBaseFontList(controller_->font_list());
+  help_label_->SetInterlineSpacing(3);
   views::StyledLabel::RangeStyleInfo default_style;
   default_style.color = kExplanatoryTextColor;
   help_label_->SetDefaultStyle(default_style);
@@ -97,7 +127,8 @@ PasswordGenerationPopupViewViews::PasswordGenerationPopupViewViews(
       controller_->kHorizontalPadding));
   AddChildView(help_label_);
 
-  set_background(views::Background::CreateSolidBackground(kPopupBackground));
+  const SkColor kPasswordBackground = SkColorSetARGB(0xFF, 0xFB, 0xFB, 0xFB);

[Evan Stade, 2014/06/19 00:16:35]

nit: SkColorSetRGB(0xFB, 0xFB, 0xFB);

Not sure if you should be hardcoding this color though. Is the password
generation bubble legible on gtk theme mode using high contrast inverse?

[Garrett Casto, 2014/06/20 23:47:01]

Done.
How would I go about testing this?

[Evan Stade, 2014/06/23 18:20:40]

with a desktop linux (not cros) build,

1. chrome://settings/ - "use gtk theme"
2. gnome-tweak-tool - theme - gtk+ theme - HighContrastInverse

[Garrett Casto, 2014/06/24 01:15:11]

Yeah, so that doesn't work for the generated password because we don't set the
text color explicitly (so it basically white on white). The help text is fine.
Note that normal autofill works fine because it's explicit about everything,
including text color, so it just completely ignores the theme. 

It seems like making this theme aware either involves adding a new color to the
theme code or ignoring the mocks and using plain white instead of off white.
Rebecca can hopefully comment on this tomorrow when she gets back from vacation
since it's the same issue as the link coloring.

[Evan Stade, 2014/06/24 21:16:58]

Yea, that's better than illegible, but not as good as respecting the theme.
Should be fairly straightforward to add a new color to the theme. Hopefully it
would be used in more than just this place, and adding it to the theme also
makes it accessible from other parts of the code base.

You might also want to try this code with an inverted theme on Windows. I don't
know how to set the theme to inverted, but msw@ or dmazzoni@ might. (Support for
these inverted color schemes is an a11y issue.)

+  set_background(views::Background::CreateSolidBackground(kPasswordBackground));
 }
 
 PasswordGenerationPopupViewViews::~PasswordGenerationPopupViewViews() {}
@@ -106,7 +137,7 @@ void PasswordGenerationPopupViewViews::CreatePasswordView() {
   if (password_view_)
     return;
 
-  password_view_ = new PasswordRow(controller_->password(),
+  password_view_ = new PasswordBox(controller_->password(),
                                    controller_->SuggestedText(),
                                    controller_->font_list(),
                                    controller_->kHorizontalPadding);