[Password Generation] Update UI to match final mocks.

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 4fbf85b2eb4c9ee3e04048fe0f00e0bc2612d47a..36c59e1fda450627700ef5f7b3dc1e86ebe14862 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
@@ -18,8 +18,8 @@ namespace autofill {
 namespace {
 
 const SkColor kExplanatoryTextBackground = SkColorSetRGB(0xF5, 0xF5, 0xF5);
-const SkColor kExplanatoryTextColor = SkColorSetRGB(0x93, 0x93, 0x93);
-const SkColor kDividerColor = SkColorSetRGB(0xE7, 0xE7, 0xE7);
+const SkColor kExplanatoryTextColor = SkColorSetRGB(0x7F, 0x7F, 0x7F);
+const SkColor kDividerColor = SkColorSetRGB(0xE9, 0xE9, 0xE9);
 
 // This is the amount of vertical whitespace that is left above and below the
 // password when it is highlighted.
@@ -29,48 +29,101 @@ const int kPasswordVerticalInset = 7;
 // to get the proper spacing in the help section.
 const int kHelpVerticalOffset = 3;
 
+// Class that shows the password and the suggestion side-by-side.
+class PasswordLabel : public views::View {

[Evan Stade, 2014/02/06 16:13:52]

think it's a little weird to have a class called FooLabel that doesn't extend
views::Label. Also with a member var called foo_label_. Can we call this
PasswordRow or something instead?

[Garrett Casto, 2014/02/06 18:39:57]

Done.

+ public:
+  PasswordLabel(const base::string16& password,
+                const base::string16& suggestion,
+                const gfx::FontList& font_list,
+                int horizontal_border) {
+    password_label_ = new views::Label(password, font_list);
+    password_label_->SetHorizontalAlignment(gfx::ALIGN_LEFT);
+    password_label_->SetBorder(
+        views::Border::CreateEmptyBorder(0, horizontal_border, 0, 0));

[Evan Stade, 2014/02/06 16:13:52]

one wonders why you place the inset on password_label_ and suggestion_label_
instead of PasswordLabel.

[Garrett Casto, 2014/02/06 18:39:57]

Leftover from an earlier time. Changed.

+    AddChildView(password_label_);
+
+    suggestion_label_ = new views::Label(suggestion, font_list);
+    suggestion_label_->SetHorizontalAlignment(gfx::ALIGN_RIGHT);
+    suggestion_label_->SetBorder(
+        views::Border::CreateEmptyBorder(0, 0, 0, horizontal_border));
+    suggestion_label_->SetEnabledColor(kExplanatoryTextColor);
+    AddChildView(suggestion_label_);
+  }
+  virtual ~PasswordLabel() {}
+
+  virtual void Layout() OVERRIDE {
+    password_label_->SetBoundsRect(bounds());

[Evan Stade, 2014/02/06 16:13:52]

technically I think you want GetContentsBounds(), which would make the layout
respect a border if you ever set one on PasswordLabel (as the comment above
suggests).

[Garrett Casto, 2014/02/06 18:39:57]

Done.

+    suggestion_label_->SetBoundsRect(bounds());

[Evan Stade, 2014/02/06 16:13:52]

how is it these two labels never overlap? I see one is aligned left and one
right, but if one or both were long wouldn't there be issues? A BoxLayout would
prevent this.

[Garrett Casto, 2014/02/06 18:39:57]

I thought about this, but wouldn't a box layout break for RTL languages? As it
stands, the labels would just switch their orientation which is what we would
want.

We do control the minimum width and strings, so this shouldn't happen anyway. Or
at least it would be very obvious if one of the translations was so long that it
broke the UI.

[Evan Stade, 2014/02/07 21:45:48]

no, box layouts flip appropriately in rtl
We should be accounting for that and handling it at least semi-gracefully. I
think you can definitely count on there being a long translation that breaks the
UI, but you can't count on someone seeing this and filing a bug that comes to
your attention.

In general, many translations stink, so users just take it in stride when
something breaks rather than attempting to file a bug on a tracker that only
accepts English reports. We also don't have QA processes in place to test every
feature in every language.

[Garrett Casto, 2014/02/10 11:02:44]

Interesting. I had looked through the implementation and didn't see any mention
of it, but when I force switch the layout it worked fine. I guess the underlying
canvas must be switched? Anyway this does look cleaner now, thanks.
I changed the controller to make sure that the width is always wide enough admit
both string anyway. I'm not exactly sure what BoxLayout would do in this
situation instead of overlapping them, but it seems like a bad position to be in
regardless.

+  }
+
+  virtual bool HitTestRect(const gfx::Rect& rect) const OVERRIDE {
+    // Have parent do event handling.
+    return false;
+  }
+
+ private:
+  // Child views. Not owned.
+  views::Label* suggestion_label_;
+  views::Label* password_label_;
+
+  DISALLOW_COPY_AND_ASSIGN(PasswordLabel);
+};
+
 }  // namespace
 
 PasswordGenerationPopupViewViews::PasswordGenerationPopupViewViews(
     PasswordGenerationPopupController* controller,
     views::Widget* observing_widget)
     : AutofillPopupBaseView(controller, observing_widget),
+      password_view_(NULL),
       controller_(controller) {
-  password_label_ = new views::Label(controller->password());
-  password_label_->SetBoundsRect(controller->password_bounds());
-  password_label_->SetHorizontalAlignment(gfx::ALIGN_LEFT);
-  password_label_->set_clip_insets(gfx::Insets(
-      kPasswordVerticalInset, 0, kPasswordVerticalInset, 0));
-  password_label_->SetBorder(views::Border::CreateEmptyBorder(
-      0, controller_->kHorizontalPadding, 0, controller_->kHorizontalPadding));
-  AddChildView(password_label_);
-
-  base::string16 help_text = controller->HelpText();
-  base::string16 learn_more_link_text = controller->LearnMoreLink();
-  views::StyledLabel* help_label =
-      new views::StyledLabel(help_text + learn_more_link_text, this);
+  if (controller_->display_password())
+    CreatePasswordView();
+
+  base::string16 help_text = controller_->HelpText();
+  base::string16 saved_passwords_link_text = controller_->SavedPasswordsLink();
+  help_label_ =
+      new views::StyledLabel(help_text + saved_passwords_link_text, this);
+  help_label_->SetFontList(controller_->font_list());
   views::StyledLabel::RangeStyleInfo default_style;
   default_style.color = kExplanatoryTextColor;
-  help_label->SetDefaultStyle(default_style);
-  help_label->AddStyleRange(
+  help_label_->SetDefaultStyle(default_style);
+
+  views::StyledLabel::RangeStyleInfo link_style =
+      views::StyledLabel::RangeStyleInfo::CreateForLink();
+  help_label_->AddStyleRange(
       gfx::Range(help_text.size(),
-                 help_text.size() + learn_more_link_text.size()),
+                 help_text.size() + saved_passwords_link_text.size()),
       views::StyledLabel::RangeStyleInfo::CreateForLink());
-  help_label->SetBoundsRect(controller->help_bounds());
-  help_label->set_background(
+  help_label_->SetBoundsRect(controller_->help_bounds());
+  help_label_->set_background(
       views::Background::CreateSolidBackground(kExplanatoryTextBackground));
-  help_label->SetBorder(views::Border::CreateEmptyBorder(
+  help_label_->SetBorder(views::Border::CreateEmptyBorder(
       controller_->kHelpVerticalPadding - kHelpVerticalOffset,
       controller_->kHorizontalPadding,
       0,
       controller_->kHorizontalPadding));
-  AddChildView(help_label);
+  AddChildView(help_label_);
 
   set_background(views::Background::CreateSolidBackground(kPopupBackground));
 }
 
 PasswordGenerationPopupViewViews::~PasswordGenerationPopupViewViews() {}
 
+void PasswordGenerationPopupViewViews::CreatePasswordView() {
+  if (password_view_)
+    return;
+
+  password_view_ =
+      new PasswordLabel(controller_->password(),
+                        controller_->SuggestedText(),
+                        controller_->font_list(),
+                        controller_->kHorizontalPadding);
+  password_view_->set_clip_insets(gfx::Insets(

[Evan Stade, 2014/02/06 16:13:52]

i feel like you should be able to do this by setting a border on something

also, why do this here instead of in PasswordLabel's ctor?

[Garrett Casto, 2014/02/06 18:39:57]

As far as I could tell, this was the only way to set part of a view that a
background would not be drawn on.
Moved.

+      kPasswordVerticalInset, 0, kPasswordVerticalInset, 0));
+  AddChildView(password_view_);
+}
+
 void PasswordGenerationPopupViewViews::Show() {
   DoShow();
 }
@@ -83,32 +136,49 @@ void PasswordGenerationPopupViewViews::Hide() {
 }
 
 void PasswordGenerationPopupViewViews::UpdateBoundsAndRedrawPopup() {
+  // Currently the UI can change from not offering a password to offering
+  // a password (e.g. the user is editing a generated password and deletes it),
+  // but it can't change the other way around.
+  if (controller_->display_password()) {

[Evan Stade, 2014/02/06 16:13:52]

nit: no curlies

[Garrett Casto, 2014/02/06 18:39:57]

Done.

+    CreatePasswordView();
+  }
   DoUpdateBoundsAndRedrawPopup();
 }
 
+void PasswordGenerationPopupViewViews::PasswordSelectionUpdated() {
+  if (password_view_) {

[Evan Stade, 2014/02/06 16:13:52]

if (!password_view_)
  return;

password_view_->set_background(views::Background::CreateSolidBackground(controller_->password_selected()
? kHoveredBackgroundColor : kPopupBackground));

[Garrett Casto, 2014/02/06 18:39:57]

I actually already did that and forgot to upload. Whoops. Done.

+    if (controller_->password_selected()) {
+      password_view_->set_background(
+          views::Background::CreateSolidBackground(kHoveredBackgroundColor));
+    } else {
+      password_view_->set_background(
+          views::Background::CreateSolidBackground(kPopupBackground));
+    }
+  }
+}
+
+void PasswordGenerationPopupViewViews::Layout() {
+  if (password_view_)
+    password_view_->SetBoundsRect(controller_->password_bounds());

[Evan Stade, 2014/02/06 16:13:52]

\n

[Garrett Casto, 2014/02/06 18:39:57]

Done.

+  help_label_->SetBoundsRect(controller_->help_bounds());
+}
+
 void PasswordGenerationPopupViewViews::OnPaint(gfx::Canvas* canvas) {
   if (!controller_)
     return;
 
-  if (controller_->password_selected()) {
-    password_label_->set_background(
-        views::Background::CreateSolidBackground(kHoveredBackgroundColor));
-  } else {
-    password_label_->set_background(
-        views::Background::CreateSolidBackground(kPopupBackground));
-  }
-
   // Draw border and background.
   views::View::OnPaint(canvas);
 
   // Divider line needs to be drawn after OnPaint() otherwise the background
   // will overwrite the divider.
-  canvas->FillRect(controller_->divider_bounds(), kDividerColor);
+  if (password_view_)
+    canvas->FillRect(controller_->divider_bounds(), kDividerColor);
 }
 
 void PasswordGenerationPopupViewViews::StyledLabelLinkClicked(
     const gfx::Range& range, int event_flags) {
-  controller_->OnHelpLinkClicked();
+  controller_->OnSavedPasswordsLinkClicked();
 }
 
 PasswordGenerationPopupView* PasswordGenerationPopupView::Create(