[Password Generation] Update UI to match final mocks.

[Password Generation] Update UI to match final mocks.

Specifically
  - Change the text of the generation popup.
  - Add a suggestion label.
  - Show editing popup when password is focused after generation.
  - Display password when it is focused after generation.
  - Various other small UI cleanups.

BUG=318977
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=251494

[Garrett Casto, 2014-01-31 22:07:16]

sky: StyledLabel changes
estade: The rest.

Note that this change depends on https://codereview.chromium.org/151503006/

[Garrett Casto, 2014-01-31 23:59:40]

On 2014/01/31 22:07:16, Garrett Casto wrote:
> sky: StyledLabel changes
> estade: The rest.
> 
> Note that this change depends on https://codereview.chromium.org/151503006/

Prerequisite CL submitted.

[Evan Stade, 2014-02-03 18:49:14]

I think this still contains changes from the other patchset

https://codereview.chromium.org/147533005/diff/100001/chrome/browser/password...
File chrome/browser/password_manager/password_generation_interactive_uitest.cc
(right):

https://codereview.chromium.org/147533005/diff/100001/chrome/browser/password...
chrome/browser/password_manager/password_generation_interactive_uitest.cc:63: //
test tests to hang on Mac.
test tests?

https://codereview.chromium.org/147533005/diff/100001/chrome/browser/password...
chrome/browser/password_manager/password_generation_interactive_uitest.cc:77: //
Cleanup UI.
nit: Clean up

https://codereview.chromium.org/147533005/diff/100001/chrome/browser/password...
chrome/browser/password_manager/password_generation_interactive_uitest.cc:137:
// Enabled on these platforms
nit: final punctuation.

https://codereview.chromium.org/147533005/diff/100001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/password_generation_popup_observer.h (right):

https://codereview.chromium.org/147533005/diff/100001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/password_generation_popup_observer.h:11: class
PasswordGenerationPopupObserver {
I don't think this class is the best way to add tests because it adds too much
test-only code to production. You could either:

a) use a mock popup controller
or
b) use a mock view
or
c) use a Testing class the extends controller or view
or
d) add a class that wraps the controller or view and has friend privileges

https://codereview.chromium.org/147533005/diff/100001/chrome/browser/ui/views...
File chrome/browser/ui/views/autofill/password_generation_popup_view_views.cc
(right):

https://codereview.chromium.org/147533005/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/autofill/password_generation_popup_view_views.cc:51:
suggestion_label->SetBoundsRect(bounds);
set child bounds in the layout() function, either via a layout manager or in
|this|::Layout()

https://codereview.chromium.org/147533005/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/autofill/password_generation_popup_view_views.cc:57:
SetBoundsRect(bounds);
same applies here, only the parent view is responsible for setting the bounds of
|this|

https://codereview.chromium.org/147533005/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/autofill/password_generation_popup_view_views.cc:63:
return parent();
I think the normal way of doing this is

virtual bool MyView::HitTestRect(const gfx::Rect& rect) OVERRIDE const {
  return false;
}

https://codereview.chromium.org/147533005/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/autofill/password_generation_popup_view_views.cc:96:
help_label_->SetBoundsRect(controller_->help_bounds());
ditto

https://codereview.chromium.org/147533005/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/autofill/password_generation_popup_view_views.cc:151:
password_view_->set_background(
i feel like this should happen when the password selection changes, not on every
paint

https://codereview.chromium.org/147533005/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/autofill/password_generation_popup_view_views.cc:160:
views::View::OnPaint(canvas);
you can probably always call this first, then if (!password_view_) return;

https://codereview.chromium.org/147533005/diff/100001/ui/views/controls/style...
File ui/views/controls/styled_label.h (right):

https://codereview.chromium.org/147533005/diff/100001/ui/views/controls/style...
ui/views/controls/styled_label.h:65: void SetFontList(const gfx::FontList&
font_list);
why isn't this part of RangeStyleInfo?

[Garrett Casto, 2014-02-04 17:54:57]

https://codereview.chromium.org/147533005/diff/100001/chrome/browser/password...
File chrome/browser/password_manager/password_generation_interactive_uitest.cc
(right):

https://codereview.chromium.org/147533005/diff/100001/chrome/browser/password...
chrome/browser/password_manager/password_generation_interactive_uitest.cc:63: //
test tests to hang on Mac.
On 2014/02/03 18:49:15, Evan Stade wrote:
> test tests?

Done.

https://codereview.chromium.org/147533005/diff/100001/chrome/browser/password...
chrome/browser/password_manager/password_generation_interactive_uitest.cc:77: //
Cleanup UI.
On 2014/02/03 18:49:15, Evan Stade wrote:
> nit: Clean up

Done.

https://codereview.chromium.org/147533005/diff/100001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/password_generation_popup_observer.h (right):

https://codereview.chromium.org/147533005/diff/100001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/password_generation_popup_observer.h:11: class
PasswordGenerationPopupObserver {
On 2014/02/03 18:49:15, Evan Stade wrote:
> I don't think this class is the best way to add tests because it adds too much
> test-only code to production. You could either:
> 
> a) use a mock popup controller
> or
> b) use a mock view
> or
> c) use a Testing class the extends controller or view
> or
> d) add a class that wraps the controller or view and has friend privileges

a) and b) seem like they don't actually test what I want since I want the view
and controller to actually run. c) or d) seem better, though injection isn't
possible with the current setup. I could make a factory vends
PasswordGenerationPopupControllers and use that in the PasswordGenerationManager
instead. Does this seem reasonable?

https://codereview.chromium.org/147533005/diff/100001/chrome/browser/ui/views...
File chrome/browser/ui/views/autofill/password_generation_popup_view_views.cc
(right):

https://codereview.chromium.org/147533005/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/autofill/password_generation_popup_view_views.cc:51:
suggestion_label->SetBoundsRect(bounds);
On 2014/02/03 18:49:15, Evan Stade wrote:
> set child bounds in the layout() function, either via a layout manager or in
> |this|::Layout()

Done.

https://codereview.chromium.org/147533005/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/autofill/password_generation_popup_view_views.cc:57:
SetBoundsRect(bounds);
On 2014/02/03 18:49:15, Evan Stade wrote:
> same applies here, only the parent view is responsible for setting the bounds
of
> |this|

Done.

https://codereview.chromium.org/147533005/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/autofill/password_generation_popup_view_views.cc:63:
return parent();
On 2014/02/03 18:49:15, Evan Stade wrote:
> I think the normal way of doing this is
> 
> virtual bool MyView::HitTestRect(const gfx::Rect& rect) OVERRIDE const {
>   return false;
> }

Done.

https://codereview.chromium.org/147533005/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/autofill/password_generation_popup_view_views.cc:96:
help_label_->SetBoundsRect(controller_->help_bounds());
On 2014/02/03 18:49:15, Evan Stade wrote:
> ditto

Done.

https://codereview.chromium.org/147533005/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/autofill/password_generation_popup_view_views.cc:151:
password_view_->set_background(
On 2014/02/03 18:49:15, Evan Stade wrote:
> i feel like this should happen when the password selection changes, not on
every
> paint

Done.

https://codereview.chromium.org/147533005/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/autofill/password_generation_popup_view_views.cc:160:
views::View::OnPaint(canvas);
On 2014/02/03 18:49:15, Evan Stade wrote:
> you can probably always call this first, then if (!password_view_) return;

N/A I think

https://codereview.chromium.org/147533005/diff/100001/ui/views/controls/style...
File ui/views/controls/styled_label.h (right):

https://codereview.chromium.org/147533005/diff/100001/ui/views/controls/style...
ui/views/controls/styled_label.h:65: void SetFontList(const gfx::FontList&
font_list);
On 2014/02/03 18:49:15, Evan Stade wrote:
> why isn't this part of RangeStyleInfo?

1) It's hard to calculate the height of a line when the height may change
depending on future text.
2) It doesn't seem necessary as no one was changing the font at all before this
point.

I'm also not sure if in principle we would want Chrome UI that changes font in
the same paragraph.

[Garrett Casto, 2014-02-06 09:42:44]

On 2014/02/04 17:54:57, Garrett Casto wrote:
>
https://codereview.chromium.org/147533005/diff/100001/chrome/browser/password...
> File chrome/browser/password_manager/password_generation_interactive_uitest.cc
> (right):
> 
>
https://codereview.chromium.org/147533005/diff/100001/chrome/browser/password...
> chrome/browser/password_manager/password_generation_interactive_uitest.cc:63:
//
> test tests to hang on Mac.
> On 2014/02/03 18:49:15, Evan Stade wrote:
> > test tests?
> 
> Done.
> 
>
https://codereview.chromium.org/147533005/diff/100001/chrome/browser/password...
> chrome/browser/password_manager/password_generation_interactive_uitest.cc:77:
//
> Cleanup UI.
> On 2014/02/03 18:49:15, Evan Stade wrote:
> > nit: Clean up
> 
> Done.
> 
>
https://codereview.chromium.org/147533005/diff/100001/chrome/browser/ui/autof...
> File chrome/browser/ui/autofill/password_generation_popup_observer.h (right):
> 
>
https://codereview.chromium.org/147533005/diff/100001/chrome/browser/ui/autof...
> chrome/browser/ui/autofill/password_generation_popup_observer.h:11: class
> PasswordGenerationPopupObserver {
> On 2014/02/03 18:49:15, Evan Stade wrote:
> > I don't think this class is the best way to add tests because it adds too
much
> > test-only code to production. You could either:
> > 
> > a) use a mock popup controller
> > or
> > b) use a mock view
> > or
> > c) use a Testing class the extends controller or view
> > or
> > d) add a class that wraps the controller or view and has friend privileges
> 
> a) and b) seem like they don't actually test what I want since I want the view
> and controller to actually run. c) or d) seem better, though injection isn't
> possible with the current setup. I could make a factory vends
> PasswordGenerationPopupControllers and use that in the
PasswordGenerationManager
> instead. Does this seem reasonable?
> 
>
https://codereview.chromium.org/147533005/diff/100001/chrome/browser/ui/views...
> File chrome/browser/ui/views/autofill/password_generation_popup_view_views.cc
> (right):
> 
>
https://codereview.chromium.org/147533005/diff/100001/chrome/browser/ui/views...
> chrome/browser/ui/views/autofill/password_generation_popup_view_views.cc:51:
> suggestion_label->SetBoundsRect(bounds);
> On 2014/02/03 18:49:15, Evan Stade wrote:
> > set child bounds in the layout() function, either via a layout manager or in
> > |this|::Layout()
> 
> Done.
> 
>
https://codereview.chromium.org/147533005/diff/100001/chrome/browser/ui/views...
> chrome/browser/ui/views/autofill/password_generation_popup_view_views.cc:57:
> SetBoundsRect(bounds);
> On 2014/02/03 18:49:15, Evan Stade wrote:
> > same applies here, only the parent view is responsible for setting the
bounds
> of
> > |this|
> 
> Done.
> 
>
https://codereview.chromium.org/147533005/diff/100001/chrome/browser/ui/views...
> chrome/browser/ui/views/autofill/password_generation_popup_view_views.cc:63:
> return parent();
> On 2014/02/03 18:49:15, Evan Stade wrote:
> > I think the normal way of doing this is
> > 
> > virtual bool MyView::HitTestRect(const gfx::Rect& rect) OVERRIDE const {
> >   return false;
> > }
> 
> Done.
> 
>
https://codereview.chromium.org/147533005/diff/100001/chrome/browser/ui/views...
> chrome/browser/ui/views/autofill/password_generation_popup_view_views.cc:96:
> help_label_->SetBoundsRect(controller_->help_bounds());
> On 2014/02/03 18:49:15, Evan Stade wrote:
> > ditto
> 
> Done.
> 
>
https://codereview.chromium.org/147533005/diff/100001/chrome/browser/ui/views...
> chrome/browser/ui/views/autofill/password_generation_popup_view_views.cc:151:
> password_view_->set_background(
> On 2014/02/03 18:49:15, Evan Stade wrote:
> > i feel like this should happen when the password selection changes, not on
> every
> > paint
> 
> Done.
> 
>
https://codereview.chromium.org/147533005/diff/100001/chrome/browser/ui/views...
> chrome/browser/ui/views/autofill/password_generation_popup_view_views.cc:160:
> views::View::OnPaint(canvas);
> On 2014/02/03 18:49:15, Evan Stade wrote:
> > you can probably always call this first, then if (!password_view_) return;
> 
> N/A I think
> 
>
https://codereview.chromium.org/147533005/diff/100001/ui/views/controls/style...
> File ui/views/controls/styled_label.h (right):
> 
>
https://codereview.chromium.org/147533005/diff/100001/ui/views/controls/style...
> ui/views/controls/styled_label.h:65: void SetFontList(const gfx::FontList&
> font_list);
> On 2014/02/03 18:49:15, Evan Stade wrote:
> > why isn't this part of RangeStyleInfo?
> 
> 1) It's hard to calculate the height of a line when the height may change
> depending on future text.
> 2) It doesn't seem necessary as no one was changing the font at all before
this
> point.
> 
> I'm also not sure if in principle we would want Chrome UI that changes font in
> the same paragraph.

Ping?

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

https://codereview.chromium.org/147533005/diff/280001/chrome/browser/ui/views...
File chrome/browser/ui/views/autofill/password_generation_popup_view_views.cc
(right):

https://codereview.chromium.org/147533005/diff/280001/chrome/browser/ui/views...
chrome/browser/ui/views/autofill/password_generation_popup_view_views.cc:33:
class PasswordLabel : public views::View {
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?

https://codereview.chromium.org/147533005/diff/280001/chrome/browser/ui/views...
chrome/browser/ui/views/autofill/password_generation_popup_view_views.cc:42:
views::Border::CreateEmptyBorder(0, horizontal_border, 0, 0));
one wonders why you place the inset on password_label_ and suggestion_label_
instead of PasswordLabel.

https://codereview.chromium.org/147533005/diff/280001/chrome/browser/ui/views...
chrome/browser/ui/views/autofill/password_generation_popup_view_views.cc:55:
password_label_->SetBoundsRect(bounds());
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).

https://codereview.chromium.org/147533005/diff/280001/chrome/browser/ui/views...
chrome/browser/ui/views/autofill/password_generation_popup_view_views.cc:56:
suggestion_label_->SetBoundsRect(bounds());
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.

https://codereview.chromium.org/147533005/diff/280001/chrome/browser/ui/views...
chrome/browser/ui/views/autofill/password_generation_popup_view_views.cc:122:
password_view_->set_clip_insets(gfx::Insets(
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?

https://codereview.chromium.org/147533005/diff/280001/chrome/browser/ui/views...
chrome/browser/ui/views/autofill/password_generation_popup_view_views.cc:142: if
(controller_->display_password()) {
nit: no curlies

https://codereview.chromium.org/147533005/diff/280001/chrome/browser/ui/views...
chrome/browser/ui/views/autofill/password_generation_popup_view_views.cc:149: if
(password_view_) {
if (!password_view_)
  return;

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

https://codereview.chromium.org/147533005/diff/280001/chrome/browser/ui/views...
chrome/browser/ui/views/autofill/password_generation_popup_view_views.cc:162:
password_view_->SetBoundsRect(controller_->password_bounds());
\n

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

https://codereview.chromium.org/147533005/diff/280001/chrome/browser/ui/views...
File chrome/browser/ui/views/autofill/password_generation_popup_view_views.cc
(right):

https://codereview.chromium.org/147533005/diff/280001/chrome/browser/ui/views...
chrome/browser/ui/views/autofill/password_generation_popup_view_views.cc:33:
class PasswordLabel : public views::View {
On 2014/02/06 16:13:52, Evan Stade wrote:
> 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?

Done.

https://codereview.chromium.org/147533005/diff/280001/chrome/browser/ui/views...
chrome/browser/ui/views/autofill/password_generation_popup_view_views.cc:42:
views::Border::CreateEmptyBorder(0, horizontal_border, 0, 0));
On 2014/02/06 16:13:52, Evan Stade wrote:
> one wonders why you place the inset on password_label_ and suggestion_label_
> instead of PasswordLabel.

Leftover from an earlier time. Changed.

https://codereview.chromium.org/147533005/diff/280001/chrome/browser/ui/views...
chrome/browser/ui/views/autofill/password_generation_popup_view_views.cc:55:
password_label_->SetBoundsRect(bounds());
On 2014/02/06 16:13:52, Evan Stade wrote:
> 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).

Done.

https://codereview.chromium.org/147533005/diff/280001/chrome/browser/ui/views...
chrome/browser/ui/views/autofill/password_generation_popup_view_views.cc:56:
suggestion_label_->SetBoundsRect(bounds());
On 2014/02/06 16:13:52, Evan Stade wrote:
> 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.

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.

https://codereview.chromium.org/147533005/diff/280001/chrome/browser/ui/views...
chrome/browser/ui/views/autofill/password_generation_popup_view_views.cc:122:
password_view_->set_clip_insets(gfx::Insets(
On 2014/02/06 16:13:52, Evan Stade wrote:
> i feel like you should be able to do this by setting a border on something
> 

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.

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

Moved.

https://codereview.chromium.org/147533005/diff/280001/chrome/browser/ui/views...
chrome/browser/ui/views/autofill/password_generation_popup_view_views.cc:142: if
(controller_->display_password()) {
On 2014/02/06 16:13:52, Evan Stade wrote:
> nit: no curlies

Done.

https://codereview.chromium.org/147533005/diff/280001/chrome/browser/ui/views...
chrome/browser/ui/views/autofill/password_generation_popup_view_views.cc:149: if
(password_view_) {
On 2014/02/06 16:13:52, Evan Stade wrote:
> if (!password_view_)
>   return;
> 
>
password_view_->set_background(views::Background::CreateSolidBackground(controller_->password_selected()
> ? kHoveredBackgroundColor : kPopupBackground));

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

https://codereview.chromium.org/147533005/diff/280001/chrome/browser/ui/views...
chrome/browser/ui/views/autofill/password_generation_popup_view_views.cc:162:
password_view_->SetBoundsRect(controller_->password_bounds());
On 2014/02/06 16:13:52, Evan Stade wrote:
> \n

Done.

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

https://codereview.chromium.org/147533005/diff/280001/chrome/browser/ui/views...
File chrome/browser/ui/views/autofill/password_generation_popup_view_views.cc
(right):

https://codereview.chromium.org/147533005/diff/280001/chrome/browser/ui/views...
chrome/browser/ui/views/autofill/password_generation_popup_view_views.cc:56:
suggestion_label_->SetBoundsRect(bounds());
On 2014/02/06 18:39:57, Garrett Casto wrote:
> On 2014/02/06 16:13:52, Evan Stade wrote:
> > 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.
> 
> 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.

no, box layouts flip appropriately in rtl

> 
> 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.

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.

https://codereview.chromium.org/147533005/diff/120031/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/password_generation_popup_controller_impl.h
(right):

https://codereview.chromium.org/147533005/diff/120031/chrome/browser/ui/autof...
chrome/browser/ui/autofill/password_generation_popup_controller_impl.h:69: void
set_form(const PasswordForm* form) { form_ = form; }
why are these public? why do you need them at all?

https://codereview.chromium.org/147533005/diff/120031/chrome/browser/ui/autof...
chrome/browser/ui/autofill/password_generation_popup_controller_impl.h:137:
const PasswordForm* form_;
why can this be null?

https://codereview.chromium.org/147533005/diff/120031/chrome/browser/ui/views...
File chrome/browser/ui/views/autofill/password_generation_popup_view_views.cc
(right):

https://codereview.chromium.org/147533005/diff/120031/chrome/browser/ui/views...
chrome/browser/ui/views/autofill/password_generation_popup_view_views.cc:26:
const int kPasswordVerticalInset = 7;
why is this here but the horizontal one is in the controller?

https://codereview.chromium.org/147533005/diff/120031/chrome/browser/ui/views...
chrome/browser/ui/views/autofill/password_generation_popup_view_views.cc:154:
views::Background::CreateSolidBackground(kHoveredBackgroundColor) :
CreateSolidBackground(foo ? bar : baz);

[Garrett Casto, 2014-02-10 11:02:43]

https://codereview.chromium.org/147533005/diff/280001/chrome/browser/ui/views...
File chrome/browser/ui/views/autofill/password_generation_popup_view_views.cc
(right):

https://codereview.chromium.org/147533005/diff/280001/chrome/browser/ui/views...
chrome/browser/ui/views/autofill/password_generation_popup_view_views.cc:56:
suggestion_label_->SetBoundsRect(bounds());
On 2014/02/07 21:45:48, Evan Stade wrote:
> On 2014/02/06 18:39:57, Garrett Casto wrote:
> > On 2014/02/06 16:13:52, Evan Stade wrote:
> > > 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.
> > 
> > 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.
> 
> no, box layouts flip appropriately in rtl
> 

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.

> > 
> > 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.
> 
> 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.

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.

https://codereview.chromium.org/147533005/diff/120031/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/password_generation_popup_controller_impl.h
(right):

https://codereview.chromium.org/147533005/diff/120031/chrome/browser/ui/autof...
chrome/browser/ui/autofill/password_generation_popup_controller_impl.h:69: void
set_form(const PasswordForm* form) { form_ = form; }
On 2014/02/07 21:45:48, Evan Stade wrote:
> why are these public? why do you need them at all?

They were there because in the editing form of the UI you don't actually need
any of these elements. For cleanliness of this interface I've removed them and
given non-NULL references even Show() is called with display = false. We can
know the the values these should take even if we don't intend to use them.

https://codereview.chromium.org/147533005/diff/120031/chrome/browser/ui/autof...
chrome/browser/ui/autofill/password_generation_popup_controller_impl.h:137:
const PasswordForm* form_;
On 2014/02/07 21:45:48, Evan Stade wrote:
> why can this be null?

N/A now, but if a password isn't displayed this value isn't used.

https://codereview.chromium.org/147533005/diff/120031/chrome/browser/ui/views...
File chrome/browser/ui/views/autofill/password_generation_popup_view_views.cc
(right):

https://codereview.chromium.org/147533005/diff/120031/chrome/browser/ui/views...
chrome/browser/ui/views/autofill/password_generation_popup_view_views.cc:26:
const int kPasswordVerticalInset = 7;
On 2014/02/07 21:45:48, Evan Stade wrote:
> why is this here but the horizontal one is in the controller?

Not entirely sure what you mean by horizontal one (kMinimumWidth?). It's here
mostly because it isn't needed for layout, though it will be used by other
implementations eventually so I moved it to PasswordGenerationPopupView.

https://codereview.chromium.org/147533005/diff/120031/chrome/browser/ui/views...
chrome/browser/ui/views/autofill/password_generation_popup_view_views.cc:154:
views::Background::CreateSolidBackground(kHoveredBackgroundColor) :
On 2014/02/07 21:45:48, Evan Stade wrote:
> CreateSolidBackground(foo ? bar : baz);

Done.

[Garrett Casto, 2014-02-10 11:10:17]

And unfortunately 'git cl' doesn't seem to work right now. I'll update the patch
in the morning.

[Garrett Casto, 2014-02-10 21:33:25]

On 2014/02/10 11:10:17, Garrett Casto wrote:
> And unfortunately 'git cl' doesn't seem to work right now. I'll update the
patch
> in the morning.

Uploaded. PTAL.

[Evan Stade, 2014-02-11 19:42:13]

https://codereview.chromium.org/147533005/diff/530001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/147533005/diff/530001/chrome/app/generated_re...
chrome/app/generated_resources.grd:14678: +      saved passwords.
this will make translations somewhere between hard and impossible.

You should do something like "Chrome will add this to your |saved passwords|."
Then parse out the thing in the bars as the linkified text.

https://codereview.chromium.org/147533005/diff/530001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/password_generation_popup_controller_impl.cc
(right):

https://codereview.chromium.org/147533005/diff/530001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/password_generation_popup_controller_impl.cc:32: //
All measurements in pixels
final punctuation. Also, don't see the point of defining this here; there is no
context, there's no useful documentation, and it forces me to search through the
file to figure out what it's used for.

https://codereview.chromium.org/147533005/diff/530001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/password_generation_popup_controller_impl.cc:154: //
Make sure that the width will always be large enought to display the
spelling

https://codereview.chromium.org/147533005/diff/530001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/password_generation_popup_controller_impl.h
(right):

https://codereview.chromium.org/147533005/diff/530001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/password_generation_popup_controller_impl.h:140:
gfx::FontList font_list_;
const ref

https://codereview.chromium.org/147533005/diff/530001/ui/views/controls/style...
File ui/views/controls/styled_label.h (right):

https://codereview.chromium.org/147533005/diff/530001/ui/views/controls/style...
ui/views/controls/styled_label.h:65: void SetFontList(const gfx::FontList&
font_list);
I think you should call this SetBaseFontList and note that modifications are
still possible for ranges (e.g. via font_style)

[Garrett Casto, 2014-02-11 23:24:05]

https://codereview.chromium.org/147533005/diff/530001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/147533005/diff/530001/chrome/app/generated_re...
chrome/app/generated_resources.grd:14678: +      saved passwords.
On 2014/02/11 19:42:14, Evan Stade wrote:
> this will make translations somewhere between hard and impossible.
> 
> You should do something like "Chrome will add this to your |saved passwords|."
> Then parse out the thing in the bars as the linkified text.

Done.

https://codereview.chromium.org/147533005/diff/530001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/password_generation_popup_controller_impl.cc
(right):

https://codereview.chromium.org/147533005/diff/530001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/password_generation_popup_controller_impl.cc:32: //
All measurements in pixels
On 2014/02/11 19:42:14, Evan Stade wrote:
> final punctuation. Also, don't see the point of defining this here; there is
no
> context, there's no useful documentation, and it forces me to search through
the
> file to figure out what it's used for.

I've always seen constants defined at the top of the cc file. I thought about
moving them down to where they are actually used (since they are only used once
ATM), but they felt redundant at that point. Added additional comments.

https://codereview.chromium.org/147533005/diff/530001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/password_generation_popup_controller_impl.cc:154: //
Make sure that the width will always be large enought to display the
On 2014/02/11 19:42:14, Evan Stade wrote:
> spelling

Done.

https://codereview.chromium.org/147533005/diff/530001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/password_generation_popup_controller_impl.h
(right):

https://codereview.chromium.org/147533005/diff/530001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/password_generation_popup_controller_impl.h:140:
gfx::FontList font_list_;
On 2014/02/11 19:42:14, Evan Stade wrote:
> const ref

Done.

https://codereview.chromium.org/147533005/diff/530001/ui/views/controls/style...
File ui/views/controls/styled_label.h (right):

https://codereview.chromium.org/147533005/diff/530001/ui/views/controls/style...
ui/views/controls/styled_label.h:65: void SetFontList(const gfx::FontList&
font_list);
On 2014/02/11 19:42:14, Evan Stade wrote:
> I think you should call this SetBaseFontList and note that modifications are
> still possible for ranges (e.g. via font_style)

Done.

[Garrett Casto, 2014-02-12 22:39:00]

On 2014/02/11 23:24:05, Garrett Casto wrote:
>
https://codereview.chromium.org/147533005/diff/530001/chrome/app/generated_re...
> File chrome/app/generated_resources.grd (right):
> 
>
https://codereview.chromium.org/147533005/diff/530001/chrome/app/generated_re...
> chrome/app/generated_resources.grd:14678: +      saved passwords.
> On 2014/02/11 19:42:14, Evan Stade wrote:
> > this will make translations somewhere between hard and impossible.
> > 
> > You should do something like "Chrome will add this to your |saved
passwords|."
> > Then parse out the thing in the bars as the linkified text.
> 
> Done.
> 
>
https://codereview.chromium.org/147533005/diff/530001/chrome/browser/ui/autof...
> File chrome/browser/ui/autofill/password_generation_popup_controller_impl.cc
> (right):
> 
>
https://codereview.chromium.org/147533005/diff/530001/chrome/browser/ui/autof...
> chrome/browser/ui/autofill/password_generation_popup_controller_impl.cc:32: //
> All measurements in pixels
> On 2014/02/11 19:42:14, Evan Stade wrote:
> > final punctuation. Also, don't see the point of defining this here; there is
> no
> > context, there's no useful documentation, and it forces me to search through
> the
> > file to figure out what it's used for.
> 
> I've always seen constants defined at the top of the cc file. I thought about
> moving them down to where they are actually used (since they are only used
once
> ATM), but they felt redundant at that point. Added additional comments.
> 
>
https://codereview.chromium.org/147533005/diff/530001/chrome/browser/ui/autof...
> chrome/browser/ui/autofill/password_generation_popup_controller_impl.cc:154:
//
> Make sure that the width will always be large enought to display the
> On 2014/02/11 19:42:14, Evan Stade wrote:
> > spelling
> 
> Done.
> 
>
https://codereview.chromium.org/147533005/diff/530001/chrome/browser/ui/autof...
> File chrome/browser/ui/autofill/password_generation_popup_controller_impl.h
> (right):
> 
>
https://codereview.chromium.org/147533005/diff/530001/chrome/browser/ui/autof...
> chrome/browser/ui/autofill/password_generation_popup_controller_impl.h:140:
> gfx::FontList font_list_;
> On 2014/02/11 19:42:14, Evan Stade wrote:
> > const ref
> 
> Done.
> 
>
https://codereview.chromium.org/147533005/diff/530001/ui/views/controls/style...
> File ui/views/controls/styled_label.h (right):
> 
>
https://codereview.chromium.org/147533005/diff/530001/ui/views/controls/style...
> ui/views/controls/styled_label.h:65: void SetFontList(const gfx::FontList&
> font_list);
> On 2014/02/11 19:42:14, Evan Stade wrote:
> > I think you should call this SetBaseFontList and note that modifications are
> > still possible for ranges (e.g. via font_style)
> 
> Done.

Ping?

[Evan Stade, 2014-02-12 23:10:56]

https://codereview.chromium.org/147533005/diff/530001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/password_generation_popup_controller_impl.cc
(right):

https://codereview.chromium.org/147533005/diff/530001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/password_generation_popup_controller_impl.cc:32: //
All measurements in pixels
On 2014/02/11 23:24:06, Garrett Casto wrote:
> On 2014/02/11 19:42:14, Evan Stade wrote:
> > final punctuation. Also, don't see the point of defining this here; there is
> no
> > context, there's no useful documentation, and it forces me to search through
> the
> > file to figure out what it's used for.
> 
> I've always seen constants defined at the top of the cc file. I thought about
> moving them down to where they are actually used (since they are only used
once
> ATM), but they felt redundant at that point. Added additional comments. 

yes indeed, they are redundant. Consider removing redundant code.

https://codereview.chromium.org/147533005/diff/620001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/password_generation_popup_controller_impl.cc
(right):

https://codereview.chromium.org/147533005/diff/620001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/password_generation_popup_controller_impl.cc:40:
const base::char16 kRangeSeperator = '|';
spelling

also, don't define this here

https://codereview.chromium.org/147533005/diff/620001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/password_generation_popup_controller_impl.cc:103:
DCHECK(pieces.size() == 2);
confused why this is 2 and not 3?

also, nit: DCHECK_EQ

https://codereview.chromium.org/147533005/diff/620001/chrome/browser/ui/views...
File chrome/browser/ui/views/autofill/password_generation_popup_view_views.cc
(right):

https://codereview.chromium.org/147533005/diff/620001/chrome/browser/ui/views...
chrome/browser/ui/views/autofill/password_generation_popup_view_views.cc:80: new
views::StyledLabel(controller_->HelpText(), this);
doesn't need to wrap

https://codereview.chromium.org/147533005/diff/620001/chrome/browser/ui/views...
chrome/browser/ui/views/autofill/password_generation_popup_view_views.cc:109:
new PasswordRow(controller_->password(),
don't need to wrap

[Garrett Casto, 2014-02-12 23:38:50]

https://codereview.chromium.org/147533005/diff/530001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/password_generation_popup_controller_impl.cc
(right):

https://codereview.chromium.org/147533005/diff/530001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/password_generation_popup_controller_impl.cc:32: //
All measurements in pixels
On 2014/02/12 23:10:57, Evan Stade wrote:
> On 2014/02/11 23:24:06, Garrett Casto wrote:
> > On 2014/02/11 19:42:14, Evan Stade wrote:
> > > final punctuation. Also, don't see the point of defining this here; there
is
> > no
> > > context, there's no useful documentation, and it forces me to search
through
> > the
> > > file to figure out what it's used for.
> > 
> > I've always seen constants defined at the top of the cc file. I thought
about
> > moving them down to where they are actually used (since they are only used
> once
> > ATM), but they felt redundant at that point. Added additional comments. 
> 
> yes indeed, they are redundant. Consider removing redundant code.

I've been told that bare constants are frowned upon even when commented, so I've
avoided them. Guess not. Removed.

https://codereview.chromium.org/147533005/diff/620001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/password_generation_popup_controller_impl.cc
(right):

https://codereview.chromium.org/147533005/diff/620001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/password_generation_popup_controller_impl.cc:40:
const base::char16 kRangeSeperator = '|';
On 2014/02/12 23:10:57, Evan Stade wrote:
> spelling
> 
> also, don't define this here

Removed.

https://codereview.chromium.org/147533005/diff/620001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/password_generation_popup_controller_impl.cc:103:
DCHECK(pieces.size() == 2);
On 2014/02/12 23:10:57, Evan Stade wrote:
> confused why this is 2 and not 3?
> 
> also, nit: DCHECK_EQ

Done, and changed to 3. Forgot to re-upload after fixing.

https://codereview.chromium.org/147533005/diff/620001/chrome/browser/ui/views...
File chrome/browser/ui/views/autofill/password_generation_popup_view_views.cc
(right):

https://codereview.chromium.org/147533005/diff/620001/chrome/browser/ui/views...
chrome/browser/ui/views/autofill/password_generation_popup_view_views.cc:80: new
views::StyledLabel(controller_->HelpText(), this);
On 2014/02/12 23:10:57, Evan Stade wrote:
> doesn't need to wrap

Done.

https://codereview.chromium.org/147533005/diff/620001/chrome/browser/ui/views...
chrome/browser/ui/views/autofill/password_generation_popup_view_views.cc:109:
new PasswordRow(controller_->password(),
On 2014/02/12 23:10:57, Evan Stade wrote:
> don't need to wrap

Done.

[Garrett Casto, 2014-02-12 23:39:33]

+nasko for IPC change.

[Evan Stade, 2014-02-13 02:21:09]

https://codereview.chromium.org/147533005/diff/620001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/password_generation_popup_controller_impl.cc
(right):

https://codereview.chromium.org/147533005/diff/620001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/password_generation_popup_controller_impl.cc:40:
const base::char16 kRangeSeperator = '|';
On 2014/02/12 23:38:51, Garrett Casto wrote:
> On 2014/02/12 23:10:57, Evan Stade wrote:
> > spelling
> > 
> > also, don't define this here
> 
> Removed.

forgot to upload latest changeset? This doesn't seem to have changed.

https://codereview.chromium.org/147533005/diff/680001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/password_generation_popup_controller_impl.cc
(right):

https://codereview.chromium.org/147533005/diff/680001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/password_generation_popup_controller_impl.cc:162: //
Default minimum width.
Here's how I would organize this function for maximum readability, because
constants should still be const even if they're local, and because changing the
value of said constant renders its name sort of meaningless.

// Minimum width of the dialog in pixels.
const int minimum_width = 300;
// comment
int width = std::max(minimum_width,
controller_common_.RoundedElementBounds().width();
if (display_password_) {
  // comment
  width = std::max(actual_width, /*calculations*/...);
}
return width;

[Garrett Casto, 2014-02-13 22:32:26]

https://codereview.chromium.org/147533005/diff/620001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/password_generation_popup_controller_impl.cc
(right):

https://codereview.chromium.org/147533005/diff/620001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/password_generation_popup_controller_impl.cc:40:
const base::char16 kRangeSeperator = '|';
On 2014/02/13 02:21:10, Evan Stade wrote:
> On 2014/02/12 23:38:51, Garrett Casto wrote:
> > On 2014/02/12 23:10:57, Evan Stade wrote:
> > > spelling
> > > 
> > > also, don't define this here
> > 
> > Removed.
> 
> forgot to upload latest changeset? This doesn't seem to have changed.

Just missed somehow. Fixed for real.

https://codereview.chromium.org/147533005/diff/680001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/password_generation_popup_controller_impl.cc
(right):

https://codereview.chromium.org/147533005/diff/680001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/password_generation_popup_controller_impl.cc:162: //
Default minimum width.
On 2014/02/13 02:21:10, Evan Stade wrote:
> Here's how I would organize this function for maximum readability, because
> constants should still be const even if they're local, and because changing
the
> value of said constant renders its name sort of meaningless.
> 
> // Minimum width of the dialog in pixels.
> const int minimum_width = 300;
> // comment
> int width = std::max(minimum_width,
> controller_common_.RoundedElementBounds().width();
> if (display_password_) {
>   // comment
>   width = std::max(actual_width, /*calculations*/...);
> }
> return width;

Done.

[Evan Stade, 2014-02-13 22:52:10]

lgtm

https://codereview.chromium.org/147533005/diff/820001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/password_generation_popup_controller_impl.cc
(right):

https://codereview.chromium.org/147533005/diff/820001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/password_generation_popup_controller_impl.cc:93: '|'
/* separator */,
prefer C++ style comments (//) not c-style comments

[nasko, 2014-02-13 23:35:30]

LGTM on IPC.

Please add me as the reviewer for the subsequent CL that uses the PasswordForm
parameter.

[sky, 2014-02-14 19:20:34]

LGTM

[Garrett Casto, 2014-02-14 19:37:03]

The CQ bit was checked by gcasto@chromium.org

[commit-bot: I haz the power, 2014-02-14 19:38:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gcasto@chromium.org/147533005/900001

[commit-bot: I haz the power, 2014-02-14 20:25:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-14 20:25:03]

Retried try job too often on win_rel for step(s) base_unittests, browser_tests,
interactive_ui_tests, net_unittests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[Garrett Casto, 2014-02-14 22:21:28]

The CQ bit was checked by gcasto@chromium.org

[commit-bot: I haz the power, 2014-02-14 22:26:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gcasto@chromium.org/147533005/1190001

[commit-bot: I haz the power, 2014-02-15 03:39:00]

Change committed as 251494