[Password Generation] Update Aura UI

[Password Generation] Update Aura UI

See bug for new mocks and screenshots.

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

[Garrett Casto, 2014-06-18 22:12:44]

oshima - chrome/app/theme/* changes
estade - chrome/browser/ui/* changes
sky - styled_label changes

[oshima, 2014-06-18 22:24:20]

c/a/theme lgtm

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

https://codereview.chromium.org/342833002/diff/20001/chrome/browser/ui/autofi...
File chrome/browser/ui/autofill/password_generation_popup_controller_impl.cc
(right):

https://codereview.chromium.org/342833002/diff/20001/chrome/browser/ui/autofi...
chrome/browser/ui/autofill/password_generation_popup_controller_impl.cc:186: int
PasswordGenerationPopupControllerImpl::GetDesiredHeight(int width) {
size calculations belong in view implementations. The size calculations should
be easy; given a hard-coded width, you can lean on GetHeightForWidth() (for
views anyway, I of course have no idea how cocoa works).

https://codereview.chromium.org/342833002/diff/20001/chrome/browser/ui/autofi...
chrome/browser/ui/autofill/password_generation_popup_controller_impl.cc:222:
((num_lines-1) *
spaces around binary operators

https://codereview.chromium.org/342833002/diff/20001/chrome/browser/ui/autofi...
File chrome/browser/ui/autofill/password_generation_popup_view.cc (right):

https://codereview.chromium.org/342833002/diff/20001/chrome/browser/ui/autofi...
chrome/browser/ui/autofill/password_generation_popup_view.cc:15: const SkColor
PasswordGenerationPopupView::kLinkColor =
I have a feeling these colors should either be deleted and replaced with values
from the native theme, or moved into the native theme. (Why is the link color
different here than elsewhere in chrome?)

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

https://codereview.chromium.org/342833002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/autofill/password_generation_popup_view_views.cc:28:
class PasswordTextBox : public views::View {
class level comment

https://codereview.chromium.org/342833002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/autofill/password_generation_popup_view_views.cc:31:
const base::string16& password,
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.

https://codereview.chromium.org/342833002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/autofill/password_generation_popup_view_views.cc:32:
const gfx::FontList& font_list) {
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().)

https://codereview.chromium.org/342833002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/autofill/password_generation_popup_view_views.cc:130:
const SkColor kPasswordBackground = SkColorSetARGB(0xFF, 0xFB, 0xFB, 0xFB);
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?

https://codereview.chromium.org/342833002/diff/20001/ui/views/controls/styled...
File ui/views/controls/styled_label.cc (right):

https://codereview.chromium.org/342833002/diff/20001/ui/views/controls/styled...
ui/views/controls/styled_label.cc:309: GetInsets().top() + line * line_height -
I have a feeling setting a taller line height ought to affect the vertical
position of the first line of text.

https://codereview.chromium.org/342833002/diff/20001/ui/views/controls/styled...
File ui/views/controls/styled_label.h (right):

https://codereview.chromium.org/342833002/diff/20001/ui/views/controls/styled...
ui/views/controls/styled_label.h:77: void SetInterlineSpacing(int
spacing_in_px);
It would be more CSS-like if you made this control the line height (i.e. total
spacing between lines, instead of additional spacing between lines).

Also it should probably be a multiplicative factor, e.g. 1.5 times normal line
height, so that it grows as the font size grows.

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

Adding Rebecca as some of these questions seem to touch on Chrome's design
philosophy, which I don't have good insight into.

https://codereview.chromium.org/342833002/diff/20001/chrome/browser/ui/autofi...
File chrome/browser/ui/autofill/password_generation_popup_controller_impl.cc
(right):

https://codereview.chromium.org/342833002/diff/20001/chrome/browser/ui/autofi...
chrome/browser/ui/autofill/password_generation_popup_controller_impl.cc:186: int
PasswordGenerationPopupControllerImpl::GetDesiredHeight(int width) {
On 2014/06/19 00:16:35, Evan Stade wrote:
> size calculations belong in view implementations. The size calculations should
> be easy; given a hard-coded width, you can lean on GetHeightForWidth() (for
> views anyway, I of course have no idea how cocoa works).

One problem with this is that you need to know the height of the popup in order
to determine where on the screen that it's placed, and that code is currently
cross platform. I'm sure we can change this so we can let each implementation
tell us their size before doing actual layout, but that's going to take me a
while to do especially on the Mac side. Are you okay with me adding a bug to
track this and deal with it in a separate CL? I'm hoping to get something
checked in early next week for a cafe study.

https://codereview.chromium.org/342833002/diff/20001/chrome/browser/ui/autofi...
chrome/browser/ui/autofill/password_generation_popup_controller_impl.cc:222:
((num_lines-1) *
On 2014/06/19 00:16:35, Evan Stade wrote:
> spaces around binary operators

Done.

https://codereview.chromium.org/342833002/diff/20001/chrome/browser/ui/autofi...
File chrome/browser/ui/autofill/password_generation_popup_view.cc (right):

https://codereview.chromium.org/342833002/diff/20001/chrome/browser/ui/autofi...
chrome/browser/ui/autofill/password_generation_popup_view.cc:15: const SkColor
PasswordGenerationPopupView::kLinkColor =
On 2014/06/19 00:16:35, Evan Stade wrote:
> I have a feeling these colors should either be deleted and replaced with
values
> from the native theme, or moved into the native theme. (Why is the link color
> different here than elsewhere in chrome?)

Rebecca should hopefully be able to give a better answer here. I'm assuming that
Chrome is moving towards a different UI style but isn't going to change the
entire UI all at once.

FYI, it looks like the default is in ui::Link::GetDefaultEnabledColor(), not in
the theme for whatever reason. Your point obviously still stands.

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

https://codereview.chromium.org/342833002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/autofill/password_generation_popup_view_views.cc:28:
class PasswordTextBox : public views::View {
On 2014/06/19 00:16:35, Evan Stade wrote:
> class level comment

Done.

https://codereview.chromium.org/342833002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/autofill/password_generation_popup_view_views.cc:31:
const base::string16& password,
On 2014/06/19 00:16:35, Evan Stade wrote:
> 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.

Added docs.

https://codereview.chromium.org/342833002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/autofill/password_generation_popup_view_views.cc:32:
const gfx::FontList& font_list) {
On 2014/06/19 00:16:36, Evan Stade wrote:
> 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().)

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

https://codereview.chromium.org/342833002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/autofill/password_generation_popup_view_views.cc:130:
const SkColor kPasswordBackground = SkColorSetARGB(0xFF, 0xFB, 0xFB, 0xFB);
On 2014/06/19 00:16:35, Evan Stade wrote:
> nit: SkColorSetRGB(0xFB, 0xFB, 0xFB);
> 

Done.

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

How would I go about testing this?

https://codereview.chromium.org/342833002/diff/20001/ui/views/controls/styled...
File ui/views/controls/styled_label.cc (right):

https://codereview.chromium.org/342833002/diff/20001/ui/views/controls/styled...
ui/views/controls/styled_label.cc:309: GetInsets().top() + line * line_height -
On 2014/06/19 00:16:36, Evan Stade wrote:
> I have a feeling setting a taller line height ought to affect the vertical
> position of the first line of text.

It actually doesn't affect the position of the first line, but it does add
additional space after the last line. This doesn't matter for my particular
case, but in retrospect this should be pretty easy to fix.

https://codereview.chromium.org/342833002/diff/20001/ui/views/controls/styled...
File ui/views/controls/styled_label.h (right):

https://codereview.chromium.org/342833002/diff/20001/ui/views/controls/styled...
ui/views/controls/styled_label.h:77: void SetInterlineSpacing(int
spacing_in_px);
On 2014/06/19 00:16:36, Evan Stade wrote:
> It would be more CSS-like if you made this control the line height (i.e. total
> spacing between lines, instead of additional spacing between lines).
> 

Are you saying that a value of 0 should leave no space between two lines of text
(e.g. not the current default spacing)? 


> Also it should probably be a multiplicative factor, e.g. 1.5 times normal line
> height, so that it grows as the font size grows.

I can make it multiplicative if that's preferable. Scott, what's your opinion on
this?

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

https://codereview.chromium.org/342833002/diff/20001/chrome/browser/ui/autofi...
File chrome/browser/ui/autofill/password_generation_popup_controller_impl.cc
(right):

https://codereview.chromium.org/342833002/diff/20001/chrome/browser/ui/autofi...
chrome/browser/ui/autofill/password_generation_popup_controller_impl.cc:186: int
PasswordGenerationPopupControllerImpl::GetDesiredHeight(int width) {
On 2014/06/20 23:47:00, Garrett Casto wrote:
> On 2014/06/19 00:16:35, Evan Stade wrote:
> > size calculations belong in view implementations. The size calculations
should
> > be easy; given a hard-coded width, you can lean on GetHeightForWidth() (for
> > views anyway, I of course have no idea how cocoa works).
> 
> One problem with this is that you need to know the height of the popup in
order
> to determine where on the screen that it's placed, and that code is currently
> cross platform. I'm sure we can change this so we can let each implementation
> tell us their size before doing actual layout, but that's going to take me a
> while to do especially on the Mac side. Are you okay with me adding a bug to
> track this and deal with it in a separate CL? I'm hoping to get something
> checked in early next week for a cafe study.

what platform are you using for the study? cros? Could you do it the right way
on that platform and TODO to fix Mac?

https://codereview.chromium.org/342833002/diff/20001/chrome/browser/ui/autofi...
File chrome/browser/ui/autofill/password_generation_popup_view.cc (right):

https://codereview.chromium.org/342833002/diff/20001/chrome/browser/ui/autofi...
chrome/browser/ui/autofill/password_generation_popup_view.cc:15: const SkColor
PasswordGenerationPopupView::kLinkColor =
On 2014/06/20 23:47:01, Garrett Casto wrote:
> On 2014/06/19 00:16:35, Evan Stade wrote:
> > I have a feeling these colors should either be deleted and replaced with
> values
> > from the native theme, or moved into the native theme. (Why is the link
color
> > different here than elsewhere in chrome?)
> 
> Rebecca should hopefully be able to give a better answer here. I'm assuming
that
> Chrome is moving towards a different UI style but isn't going to change the
> entire UI all at once.

I think it would be good to make sure that assumption is correct. Hardcoded
values create inconsistencies and can break themes.

> 
> FYI, it looks like the default is in ui::Link::GetDefaultEnabledColor(), not
in
> the theme for whatever reason.

from native_theme.h:

    // TODO(benrg): move other hardcoded colors here.

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

https://codereview.chromium.org/342833002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/autofill/password_generation_popup_view_views.cc:32:
const gfx::FontList& font_list) {
On 2014/06/20 23:47:01, Garrett Casto wrote:
> On 2014/06/19 00:16:36, Evan Stade wrote:
> > 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().)
> 
> 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().

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

https://codereview.chromium.org/342833002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/autofill/password_generation_popup_view_views.cc:130:
const SkColor kPasswordBackground = SkColorSetARGB(0xFF, 0xFB, 0xFB, 0xFB);
On 2014/06/20 23:47:01, Garrett Casto wrote:
> On 2014/06/19 00:16:35, Evan Stade wrote:
> > nit: SkColorSetRGB(0xFB, 0xFB, 0xFB);
> > 
> 
> Done.
> 
> > Not sure if you should be hardcoding this color though. Is the password
> > generation bubble legible on gtk theme mode using high contrast inverse?
> 
> How would I go about testing this?

with a desktop linux (not cros) build,

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

https://codereview.chromium.org/342833002/diff/20001/ui/views/controls/styled...
File ui/views/controls/styled_label.cc (right):

https://codereview.chromium.org/342833002/diff/20001/ui/views/controls/styled...
ui/views/controls/styled_label.cc:309: GetInsets().top() + line * line_height -
On 2014/06/20 23:47:01, Garrett Casto wrote:
> On 2014/06/19 00:16:36, Evan Stade wrote:
> > I have a feeling setting a taller line height ought to affect the vertical
> > position of the first line of text.
> 
> It actually doesn't affect the position of the first line, 

Right, it doesn't, but it ought to. CSS line-height does.

> but it does add
> additional space after the last line. This doesn't matter for my particular
> case, but in retrospect this should be pretty easy to fix.

https://codereview.chromium.org/342833002/diff/20001/ui/views/controls/styled...
File ui/views/controls/styled_label.h (right):

https://codereview.chromium.org/342833002/diff/20001/ui/views/controls/styled...
ui/views/controls/styled_label.h:77: void SetInterlineSpacing(int
spacing_in_px);
On 2014/06/20 23:47:01, Garrett Casto wrote:
> On 2014/06/19 00:16:36, Evan Stade wrote:
> > It would be more CSS-like if you made this control the line height (i.e.
total
> > spacing between lines, instead of additional spacing between lines).
> > 
> 
> Are you saying that a value of 0 should leave no space between two lines of
text
> (e.g. not the current default spacing)? 

yes. You could always redefine 0 to be "reset to default", but in CSS,
line-height: 0 can actually put an entire paragraph on a single line.

> 
> 
> > Also it should probably be a multiplicative factor, e.g. 1.5 times normal
line
> > height, so that it grows as the font size grows.
> 
> I can make it multiplicative if that's preferable. Scott, what's your opinion
on
> this?

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

https://codereview.chromium.org/342833002/diff/20001/chrome/browser/ui/autofi...
File chrome/browser/ui/autofill/password_generation_popup_controller_impl.cc
(right):

https://codereview.chromium.org/342833002/diff/20001/chrome/browser/ui/autofi...
chrome/browser/ui/autofill/password_generation_popup_controller_impl.cc:186: int
PasswordGenerationPopupControllerImpl::GetDesiredHeight(int width) {
On 2014/06/23 18:20:40, Evan Stade wrote:
> On 2014/06/20 23:47:00, Garrett Casto wrote:
> > On 2014/06/19 00:16:35, Evan Stade wrote:
> > > size calculations belong in view implementations. The size calculations
> should
> > > be easy; given a hard-coded width, you can lean on GetHeightForWidth()
(for
> > > views anyway, I of course have no idea how cocoa works).
> > 
> > One problem with this is that you need to know the height of the popup in
> order
> > to determine where on the screen that it's placed, and that code is
currently
> > cross platform. I'm sure we can change this so we can let each
implementation
> > tell us their size before doing actual layout, but that's going to take me a
> > while to do especially on the Mac side. Are you okay with me adding a bug to
> > track this and deal with it in a separate CL? I'm hoping to get something
> > checked in early next week for a cafe study.
> 
> what platform are you using for the study? cros? Could you do it the right way
> on that platform and TODO to fix Mac?

I actually don't know if it's CrOS or Windows. I took a stab at refactoring
this. The actual height calculation is certainly cleaner. It does mean that Mac
is going to not work at all for now, but I suppose it would have been broken in
either case.

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

https://codereview.chromium.org/342833002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/autofill/password_generation_popup_view_views.cc:32:
const gfx::FontList& font_list) {
On 2014/06/23 18:20:40, Evan Stade wrote:
> On 2014/06/20 23:47:01, Garrett Casto wrote:
> > On 2014/06/19 00:16:36, Evan Stade wrote:
> > > 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().)
> > 
> > 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().
> 
> it's also "don't call virtual methods" (or functions that call functions that
> call virtual methods)

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

https://codereview.chromium.org/342833002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/autofill/password_generation_popup_view_views.cc:130:
const SkColor kPasswordBackground = SkColorSetARGB(0xFF, 0xFB, 0xFB, 0xFB);
On 2014/06/23 18:20:40, Evan Stade wrote:
> On 2014/06/20 23:47:01, Garrett Casto wrote:
> > On 2014/06/19 00:16:35, Evan Stade wrote:
> > > nit: SkColorSetRGB(0xFB, 0xFB, 0xFB);
> > > 
> > 
> > Done.
> > 
> > > Not sure if you should be hardcoding this color though. Is the password
> > > generation bubble legible on gtk theme mode using high contrast inverse?
> > 
> > How would I go about testing this?
> 
> with a desktop linux (not cros) build,
> 
> 1. chrome://settings/ - "use gtk theme"
> 2. gnome-tweak-tool - theme - gtk+ theme - HighContrastInverse

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.

https://codereview.chromium.org/342833002/diff/20001/ui/views/controls/styled...
File ui/views/controls/styled_label.cc (right):

https://codereview.chromium.org/342833002/diff/20001/ui/views/controls/styled...
ui/views/controls/styled_label.cc:309: GetInsets().top() + line * line_height -
On 2014/06/23 18:20:40, Evan Stade wrote:
> On 2014/06/20 23:47:01, Garrett Casto wrote:
> > On 2014/06/19 00:16:36, Evan Stade wrote:
> > > I have a feeling setting a taller line height ought to affect the vertical
> > > position of the first line of text.
> > 
> > It actually doesn't affect the position of the first line, 
> 
> Right, it doesn't, but it ought to. CSS line-height does.
> 

Ah, so you want the text to be centered in the line height. I guess I'm trying
to determine how we want this to be used. Do you think that this should be like
line-height because it's what developers are used to or because it's how
designers will draw up redlines? I don't know much about the former, and I'm a
complete CSS noob, but actually matching mocks seems like it's more difficult
with this behavior. If you have a well defined border around text but you want
the text to be spread larger than normal, you need to account for the additional
spacing when determining the border.

I suppose you could argue that the borders should dynamically change as the text
size changes, but that doesn't seem to be something that normally happens in
Chrome UI (that I can tell).

> > but it does add
> > additional space after the last line. This doesn't matter for my particular
> > case, but in retrospect this should be pretty easy to fix.
>

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

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

https://codereview.chromium.org/342833002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/autofill/password_generation_popup_view_views.cc:130:
const SkColor kPasswordBackground = SkColorSetARGB(0xFF, 0xFB, 0xFB, 0xFB);
On 2014/06/24 01:15:11, Garrett Casto wrote:
> On 2014/06/23 18:20:40, Evan Stade wrote:
> > On 2014/06/20 23:47:01, Garrett Casto wrote:
> > > On 2014/06/19 00:16:35, Evan Stade wrote:
> > > > nit: SkColorSetRGB(0xFB, 0xFB, 0xFB);
> > > > 
> > > 
> > > Done.
> > > 
> > > > Not sure if you should be hardcoding this color though. Is the password
> > > > generation bubble legible on gtk theme mode using high contrast inverse?
> > > 
> > > How would I go about testing this?
> > 
> > with a desktop linux (not cros) build,
> > 
> > 1. chrome://settings/ - "use gtk theme"
> > 2. gnome-tweak-tool - theme - gtk+ theme - HighContrastInverse
> 
> 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. 

Yea, that's better than illegible, but not as good as respecting 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.

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

https://codereview.chromium.org/342833002/diff/20001/ui/views/controls/styled...
File ui/views/controls/styled_label.cc (right):

https://codereview.chromium.org/342833002/diff/20001/ui/views/controls/styled...
ui/views/controls/styled_label.cc:309: GetInsets().top() + line * line_height -
On 2014/06/24 01:15:12, Garrett Casto wrote:
> On 2014/06/23 18:20:40, Evan Stade wrote:
> > On 2014/06/20 23:47:01, Garrett Casto wrote:
> > > On 2014/06/19 00:16:36, Evan Stade wrote:
> > > > I have a feeling setting a taller line height ought to affect the
vertical
> > > > position of the first line of text.
> > > 
> > > It actually doesn't affect the position of the first line, 
> > 
> > Right, it doesn't, but it ought to. CSS line-height does.
> > 
> 
> Ah, so you want the text to be centered in the line height. I guess I'm trying
> to determine how we want this to be used. Do you think that this should be
like
> line-height because it's what developers are used to

this^. It's what CSS does, and CSS is a mature library in widespread use, so
it's probably like that for a good reason. Even if there's no good reason, CSS
still defines convention imo.

> or because it's how
> designers will draw up redlines? I don't know much about the former, and I'm a
> complete CSS noob, but actually matching mocks seems like it's more difficult
> with this behavior. If you have a well defined border around text but you want
> the text to be spread larger than normal, you need to account for the
additional
> spacing when determining the border.
> 
> I suppose you could argue that the borders should dynamically change as the
text
> size changes, but that doesn't seem to be something that normally happens in
> Chrome UI (that I can tell).

I do think they should, but I will admit that Chrome frequently gets it wrong
(probably "always" if we're talking about native UI).

> 
> > > but it does add
> > > additional space after the last line. This doesn't matter for my
particular
> > > case, but in retrospect this should be pretty easy to fix.
> > 
>

https://codereview.chromium.org/342833002/diff/80001/chrome/browser/ui/autofi...
File chrome/browser/ui/autofill/password_generation_popup_controller_impl.cc
(right):

https://codereview.chromium.org/342833002/diff/80001/chrome/browser/ui/autofi...
chrome/browser/ui/autofill/password_generation_popup_controller_impl.cc:162:
const int minimum_required_width = 350;
it's ok to have a minimum width defined in some shared location, but the rest of
the calculations in this function belong in the view

https://codereview.chromium.org/342833002/diff/80001/chrome/browser/ui/autofi...
chrome/browser/ui/autofill/password_generation_popup_controller_impl.cc:181: int
popup_height = view_->SetBoundsForWidth(popup_width);
I think this should be GetHeight(). Just calculate bounds, don't actually set
them. Also the desired width is determined in the view so no need for
"ForWidth".

[Garrett Casto, 2014-06-24 23:51:23]

For the moment I've hard coded all the text in the password generation bubble as
it's both easier and what Autofill currently does. I've filled crbug.com/388433
to track making these respect theme. As you mentioned in the bug, it's not clear
to me if we want this to respect theme anyway since it's essentially part of
content.

I've also changed StyledLabel to use line height that approximates CSS line
height. Unfortunately I don't know how to test this in a reasonable way. It
feels like you basically want a pixel test, but I'm not sure where such a thing
would go.

Other comments incoming. I'm hoping that we can get this in a situation by
tomorrow that it can be checked in so that we don't miss UX testing this week.

On 2014/06/24 21:16:58, Evan Stade wrote:
>
https://codereview.chromium.org/342833002/diff/20001/chrome/browser/ui/views/...
> File chrome/browser/ui/views/autofill/password_generation_popup_view_views.cc
> (right):
> 
>
https://codereview.chromium.org/342833002/diff/20001/chrome/browser/ui/views/...
> chrome/browser/ui/views/autofill/password_generation_popup_view_views.cc:130:
> const SkColor kPasswordBackground = SkColorSetARGB(0xFF, 0xFB, 0xFB, 0xFB);
> On 2014/06/24 01:15:11, Garrett Casto wrote:
> > On 2014/06/23 18:20:40, Evan Stade wrote:
> > > On 2014/06/20 23:47:01, Garrett Casto wrote:
> > > > On 2014/06/19 00:16:35, Evan Stade wrote:
> > > > > nit: SkColorSetRGB(0xFB, 0xFB, 0xFB);
> > > > > 
> > > > 
> > > > Done.
> > > > 
> > > > > Not sure if you should be hardcoding this color though. Is the
password
> > > > > generation bubble legible on gtk theme mode using high contrast
inverse?
> > > > 
> > > > How would I go about testing this?
> > > 
> > > with a desktop linux (not cros) build,
> > > 
> > > 1. chrome://settings/ - "use gtk theme"
> > > 2. gnome-tweak-tool - theme - gtk+ theme - HighContrastInverse
> > 
> > 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. 
> 
> Yea, that's better than illegible, but not as good as respecting 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.
> 
> 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.)
> 
>
https://codereview.chromium.org/342833002/diff/20001/ui/views/controls/styled...
> File ui/views/controls/styled_label.cc (right):
> 
>
https://codereview.chromium.org/342833002/diff/20001/ui/views/controls/styled...
> ui/views/controls/styled_label.cc:309: GetInsets().top() + line * line_height
-
> On 2014/06/24 01:15:12, Garrett Casto wrote:
> > On 2014/06/23 18:20:40, Evan Stade wrote:
> > > On 2014/06/20 23:47:01, Garrett Casto wrote:
> > > > On 2014/06/19 00:16:36, Evan Stade wrote:
> > > > > I have a feeling setting a taller line height ought to affect the
> vertical
> > > > > position of the first line of text.
> > > > 
> > > > It actually doesn't affect the position of the first line, 
> > > 
> > > Right, it doesn't, but it ought to. CSS line-height does.
> > > 
> > 
> > Ah, so you want the text to be centered in the line height. I guess I'm
trying
> > to determine how we want this to be used. Do you think that this should be
> like
> > line-height because it's what developers are used to
> 
> this^. It's what CSS does, and CSS is a mature library in widespread use, so
> it's probably like that for a good reason. Even if there's no good reason, CSS
> still defines convention imo.
> 
> > or because it's how
> > designers will draw up redlines? I don't know much about the former, and I'm
a
> > complete CSS noob, but actually matching mocks seems like it's more
difficult
> > with this behavior. If you have a well defined border around text but you
want
> > the text to be spread larger than normal, you need to account for the
> additional
> > spacing when determining the border.
> > 
> > I suppose you could argue that the borders should dynamically change as the
> text
> > size changes, but that doesn't seem to be something that normally happens in
> > Chrome UI (that I can tell).
> 
> I do think they should, but I will admit that Chrome frequently gets it wrong
> (probably "always" if we're talking about native UI).
> 
> > 
> > > > but it does add
> > > > additional space after the last line. This doesn't matter for my
> particular
> > > > case, but in retrospect this should be pretty easy to fix.
> > > 
> >
> 
>
https://codereview.chromium.org/342833002/diff/80001/chrome/browser/ui/autofi...
> File chrome/browser/ui/autofill/password_generation_popup_controller_impl.cc
> (right):
> 
>
https://codereview.chromium.org/342833002/diff/80001/chrome/browser/ui/autofi...
> chrome/browser/ui/autofill/password_generation_popup_controller_impl.cc:162:
> const int minimum_required_width = 350;
> it's ok to have a minimum width defined in some shared location, but the rest
of
> the calculations in this function belong in the view
> 
>
https://codereview.chromium.org/342833002/diff/80001/chrome/browser/ui/autofi...
> chrome/browser/ui/autofill/password_generation_popup_controller_impl.cc:181:
int
> popup_height = view_->SetBoundsForWidth(popup_width);
> I think this should be GetHeight(). Just calculate bounds, don't actually set
> them. Also the desired width is determined in the view so no need for
> "ForWidth".

[Garrett Casto, 2014-06-24 23:51:30]

https://codereview.chromium.org/342833002/diff/80001/chrome/browser/ui/autofi...
File chrome/browser/ui/autofill/password_generation_popup_controller_impl.cc
(right):

https://codereview.chromium.org/342833002/diff/80001/chrome/browser/ui/autofi...
chrome/browser/ui/autofill/password_generation_popup_controller_impl.cc:162:
const int minimum_required_width = 350;
On 2014/06/24 21:16:58, Evan Stade wrote:
> it's ok to have a minimum width defined in some shared location, but the rest
of
> the calculations in this function belong in the view

The first calculation requires information that is currently only known in
shared code (the size of the element that is hosting this popup). The second
actually doesn't make sense anymore.

https://codereview.chromium.org/342833002/diff/80001/chrome/browser/ui/autofi...
chrome/browser/ui/autofill/password_generation_popup_controller_impl.cc:181: int
popup_height = view_->SetBoundsForWidth(popup_width);
On 2014/06/24 21:16:58, Evan Stade wrote:
> I think this should be GetHeight(). Just calculate bounds, don't actually set
> them. Also the desired width is determined in the view so no need for
> "ForWidth".

Done.

[Evan Stade, 2014-06-25 00:02:41]

https://codereview.chromium.org/342833002/diff/80001/chrome/browser/ui/autofi...
File chrome/browser/ui/autofill/password_generation_popup_controller_impl.cc
(right):

https://codereview.chromium.org/342833002/diff/80001/chrome/browser/ui/autofi...
chrome/browser/ui/autofill/password_generation_popup_controller_impl.cc:162:
const int minimum_required_width = 350;
On 2014/06/24 23:51:30, Garrett Casto wrote:
> On 2014/06/24 21:16:58, Evan Stade wrote:
> > it's ok to have a minimum width defined in some shared location, but the
rest
> of
> > the calculations in this function belong in the view
> 
> The first calculation requires information that is currently only known in
> shared code (the size of the element that is hosting this popup).

You could expose a MinimumWidth() method to the views, which does:

return std::max(350, controller_common_.RoundedElementBounds().width());

Doing this also makes the views code cleaner because it can consult the
controller for the minimum width any time it needs to (i.e. during Layout()).

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

https://codereview.chromium.org/342833002/diff/120001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/password_generation_popup_controller_impl.cc:82:
font_list_(ResourceBundle::GetSharedInstance().GetFontList(
don't think you need this here in this class any more

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

https://codereview.chromium.org/342833002/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/autofill/password_generation_popup_view_views.cc:170:
void PasswordGenerationPopupViewViews::SetBoundsForWidth(int width) {
nit: does this need to be its own function now? Can we move it to Layout()?

[Garrett Casto, 2014-06-25 19:38:14]

+isherman for Cocoa changes. This will just disable the UI in Mac for now until
we can clean it up to match the latest mocks.

https://codereview.chromium.org/342833002/diff/80001/chrome/browser/ui/autofi...
File chrome/browser/ui/autofill/password_generation_popup_controller_impl.cc
(right):

https://codereview.chromium.org/342833002/diff/80001/chrome/browser/ui/autofi...
chrome/browser/ui/autofill/password_generation_popup_controller_impl.cc:162:
const int minimum_required_width = 350;
On 2014/06/25 00:02:41, Evan Stade wrote:
> On 2014/06/24 23:51:30, Garrett Casto wrote:
> > On 2014/06/24 21:16:58, Evan Stade wrote:
> > > it's ok to have a minimum width defined in some shared location, but the
> rest
> > of
> > > the calculations in this function belong in the view
> > 
> > The first calculation requires information that is currently only known in
> > shared code (the size of the element that is hosting this popup).
> 
> You could expose a MinimumWidth() method to the views, which does:
> 
> return std::max(350, controller_common_.RoundedElementBounds().width());
> 
> Doing this also makes the views code cleaner because it can consult the
> controller for the minimum width any time it needs to (i.e. during Layout()).

To be clear, you're suggesting that in CalculateBounds(), instead of directly
calling this function, or something like it, I should call view_->GetBound(),
which in turn just returns controller_->MinimumWidth()? I'm not sure that I see
the point, but I can do it if you want. It does change
controller_->popup_bounds().width() to controller_->MinimumWidth() in the view,
but the later doesn't seem obviously better to me.

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

https://codereview.chromium.org/342833002/diff/120001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/password_generation_popup_controller_impl.cc:82:
font_list_(ResourceBundle::GetSharedInstance().GetFontList(
On 2014/06/25 00:02:41, Evan Stade wrote:
> don't think you need this here in this class any more

Removed.

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

https://codereview.chromium.org/342833002/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/autofill/password_generation_popup_view_views.cc:170:
void PasswordGenerationPopupViewViews::SetBoundsForWidth(int width) {
On 2014/06/25 00:02:41, Evan Stade wrote:
> nit: does this need to be its own function now? Can we move it to Layout()?

Done.

[Ilya Sherman, 2014-06-25 19:46:55]

Cocoa changes LGTM

https://codereview.chromium.org/342833002/diff/160001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/autofill/password_generation_popup_view_bridge.mm
(right):

https://codereview.chromium.org/342833002/diff/160001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/autofill/password_generation_popup_view_bridge.mm:39: //
TODO: Implement this function.
nit: TODO(gcasto):

https://codereview.chromium.org/342833002/diff/160001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/autofill/password_generation_popup_view_bridge.mm:53: //
TODO: Implement this function.
Ditto

[Garrett Casto, 2014-06-25 20:29:37]

https://codereview.chromium.org/342833002/diff/160001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/autofill/password_generation_popup_view_bridge.mm
(right):

https://codereview.chromium.org/342833002/diff/160001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/autofill/password_generation_popup_view_bridge.mm:39: //
TODO: Implement this function.
On 2014/06/25 19:46:55, Ilya Sherman wrote:
> nit: TODO(gcasto):

Done.

https://codereview.chromium.org/342833002/diff/160001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/autofill/password_generation_popup_view_bridge.mm:53: //
TODO: Implement this function.
On 2014/06/25 19:46:55, Ilya Sherman wrote:
> Ditto

Done.

[Garrett Casto, 2014-06-25 21:05:47]

Adding Ben, since sky@ is unresponsive over e-mail and IM.

Ben, could you take a look at the styled_label changes? I'm trying to get this
checked in today if at all possible for UX testing tomorrow.

[Evan Stade, 2014-06-25 21:27:29]

https://codereview.chromium.org/342833002/diff/80001/chrome/browser/ui/autofi...
File chrome/browser/ui/autofill/password_generation_popup_controller_impl.cc
(right):

https://codereview.chromium.org/342833002/diff/80001/chrome/browser/ui/autofi...
chrome/browser/ui/autofill/password_generation_popup_controller_impl.cc:162:
const int minimum_required_width = 350;
On 2014/06/25 19:38:14, Garrett Casto wrote:
> On 2014/06/25 00:02:41, Evan Stade wrote:
> > On 2014/06/24 23:51:30, Garrett Casto wrote:
> > > On 2014/06/24 21:16:58, Evan Stade wrote:
> > > > it's ok to have a minimum width defined in some shared location, but the
> > rest
> > > of
> > > > the calculations in this function belong in the view
> > > 
> > > The first calculation requires information that is currently only known in
> > > shared code (the size of the element that is hosting this popup).
> > 
> > You could expose a MinimumWidth() method to the views, which does:
> > 
> > return std::max(350, controller_common_.RoundedElementBounds().width());
> > 
> > Doing this also makes the views code cleaner because it can consult the
> > controller for the minimum width any time it needs to (i.e. during
Layout()).
> 
> To be clear, you're suggesting that in CalculateBounds(), instead of directly
> calling this function, or something like it, I should call view_->GetBound(),
> which in turn just returns controller_->MinimumWidth()?

view_->GetBounds() uses controller_->MinimumWidth() to help calculate the
height, yes, but it doesn't just return the width.

> I'm not sure that I see
> the point, but I can do it if you want. It does change
> controller_->popup_bounds().width() to controller_->MinimumWidth() in the
view,
> but the later doesn't seem obviously better to me.

In Layout() you should use bounds() (as in this->bounds()).

https://codereview.chromium.org/342833002/diff/80001/chrome/browser/ui/autofi...
chrome/browser/ui/autofill/password_generation_popup_controller_impl.cc:183:
popup_bounds_ = controller_common_.GetPopupBounds(popup_height, popup_width);
width should always come before height >:(

[sky, 2014-06-25 21:53:48]

Also, how about test coverage?

https://codereview.chromium.org/342833002/diff/200001/ui/views/controls/style...
File ui/views/controls/styled_label.cc (right):

https://codereview.chromium.org/342833002/diff/200001/ui/views/controls/style...
ui/views/controls/styled_label.cc:322: int StyledLabel::GetLineHeight() {
nit: const

https://codereview.chromium.org/342833002/diff/200001/ui/views/controls/style...
ui/views/controls/styled_label.cc:328: int StyledLabel::GetOffsetForLine(int
line) {
nit: const

https://codereview.chromium.org/342833002/diff/200001/ui/views/controls/style...
ui/views/controls/styled_label.cc:339: // Center text in the space available.
Why do you need this or the if on 336. I guess more specifically, do we really
have a need to set a smaller line width?

[Garrett Casto, 2014-06-25 21:54:55]

https://codereview.chromium.org/342833002/diff/80001/chrome/browser/ui/autofi...
File chrome/browser/ui/autofill/password_generation_popup_controller_impl.cc
(right):

https://codereview.chromium.org/342833002/diff/80001/chrome/browser/ui/autofi...
chrome/browser/ui/autofill/password_generation_popup_controller_impl.cc:162:
const int minimum_required_width = 350;
On 2014/06/25 21:27:29, Evan Stade wrote:
> On 2014/06/25 19:38:14, Garrett Casto wrote:
> > On 2014/06/25 00:02:41, Evan Stade wrote:
> > > On 2014/06/24 23:51:30, Garrett Casto wrote:
> > > > On 2014/06/24 21:16:58, Evan Stade wrote:
> > > > > it's ok to have a minimum width defined in some shared location, but
the
> > > rest
> > > > of
> > > > > the calculations in this function belong in the view
> > > > 
> > > > The first calculation requires information that is currently only known
in
> > > > shared code (the size of the element that is hosting this popup).
> > > 
> > > You could expose a MinimumWidth() method to the views, which does:
> > > 
> > > return std::max(350, controller_common_.RoundedElementBounds().width());
> > > 
> > > Doing this also makes the views code cleaner because it can consult the
> > > controller for the minimum width any time it needs to (i.e. during
> Layout()).
> > 
> > To be clear, you're suggesting that in CalculateBounds(), instead of
directly
> > calling this function, or something like it, I should call
view_->GetBound(),
> > which in turn just returns controller_->MinimumWidth()?
> 
> view_->GetBounds() uses controller_->MinimumWidth() to help calculate the
> height, yes, but it doesn't just return the width.
>

Done.
 
> > I'm not sure that I see
> > the point, but I can do it if you want. It does change
> > controller_->popup_bounds().width() to controller_->MinimumWidth() in the
> view,
> > but the later doesn't seem obviously better to me.
> 
> In Layout() you should use bounds() (as in this->bounds()).

Done.

https://codereview.chromium.org/342833002/diff/80001/chrome/browser/ui/autofi...
chrome/browser/ui/autofill/password_generation_popup_controller_impl.cc:183:
popup_bounds_ = controller_common_.GetPopupBounds(popup_height, popup_width);
On 2014/06/25 21:27:29, Evan Stade wrote:
> width should always come before height >:(

Switched.

[Garrett Casto, 2014-06-25 22:06:44]

I added a really simple unittest to make sure that setting line height works,
but it doesn't check if text layout does the right thing when the line height is
larger than the text height. I wasn't sure how to do that without actually
rendering the text or adding test only hooks. Suggestions welcome.

https://codereview.chromium.org/342833002/diff/200001/ui/views/controls/style...
File ui/views/controls/styled_label.cc (right):

https://codereview.chromium.org/342833002/diff/200001/ui/views/controls/style...
ui/views/controls/styled_label.cc:322: int StyledLabel::GetLineHeight() {
On 2014/06/25 21:53:48, sky wrote:
> nit: const

Done.

https://codereview.chromium.org/342833002/diff/200001/ui/views/controls/style...
ui/views/controls/styled_label.cc:328: int StyledLabel::GetOffsetForLine(int
line) {
On 2014/06/25 21:53:48, sky wrote:
> nit: const

Done.

https://codereview.chromium.org/342833002/diff/200001/ui/views/controls/style...
ui/views/controls/styled_label.cc:339: // Center text in the space available.
On 2014/06/25 21:53:48, sky wrote:
> Why do you need this or the if on 336. I guess more specifically, do we really
> have a need to set a smaller line width?

I don't have a use case for it, but estade@ thought that this should act like
CSS line-height, which allows for both larger and small heights as well as
centering text if the line height is larger than the text height. My original
proposal was just to be able to add additional space in between lines.

[Evan Stade, 2014-06-25 22:19:27]

https://codereview.chromium.org/342833002/diff/260001/ui/views/controls/style...
File ui/views/controls/styled_label.cc (right):

https://codereview.chromium.org/342833002/diff/260001/ui/views/controls/style...
ui/views/controls/styled_label.cc:137: void StyledLabel::SetLineHeight(int
line_height) {
what happened to making this a multiplier?

you can stipulate that it must be >= 1, I doubt we'll have a use case for <1

[Garrett Casto, 2014-06-25 23:03:28]

https://codereview.chromium.org/342833002/diff/260001/ui/views/controls/style...
File ui/views/controls/styled_label.cc (right):

https://codereview.chromium.org/342833002/diff/260001/ui/views/controls/style...
ui/views/controls/styled_label.cc:137: void StyledLabel::SetLineHeight(int
line_height) {
On 2014/06/25 22:19:27, Evan Stade wrote:
> what happened to making this a multiplier?
> 
> you can stipulate that it must be >= 1, I doubt we'll have a use case for <1

Got lost in the shuffle. Changed to be a multiplier.

[sky, 2014-06-25 23:29:09]

LGTM

https://codereview.chromium.org/342833002/diff/280001/ui/views/controls/style...
File ui/views/controls/styled_label.cc (right):

https://codereview.chromium.org/342833002/diff/280001/ui/views/controls/style...
ui/views/controls/styled_label.cc:138: if (multiplier < 1.0)
DCHECK

[Garrett Casto, 2014-06-25 23:47:25]

https://codereview.chromium.org/342833002/diff/280001/ui/views/controls/style...
File ui/views/controls/styled_label.cc (right):

https://codereview.chromium.org/342833002/diff/280001/ui/views/controls/style...
ui/views/controls/styled_label.cc:138: if (multiplier < 1.0)
On 2014/06/25 23:29:09, sky wrote:
> DCHECK

Done.

[Evan Stade, 2014-06-26 00:06:04]

https://codereview.chromium.org/342833002/diff/80001/chrome/browser/ui/autofi...
File chrome/browser/ui/autofill/password_generation_popup_controller_impl.cc
(right):

https://codereview.chromium.org/342833002/diff/80001/chrome/browser/ui/autofi...
chrome/browser/ui/autofill/password_generation_popup_controller_impl.cc:183:
popup_bounds_ = controller_common_.GetPopupBounds(popup_height, popup_width);
On 2014/06/25 21:54:55, Garrett Casto wrote:
> On 2014/06/25 21:27:29, Evan Stade wrote:
> > width should always come before height >:(
> 
> Switched.

thank you

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

https://codereview.chromium.org/342833002/diff/290001/chrome/browser/ui/views...
chrome/browser/ui/views/autofill/password_generation_popup_view_views.cc:160:
static_cast<PasswordBox*>(password_view_)->Init(
nit: prefer avoiding this cast. Perhaps Init() can return |this| for
convenience. Not sure if that's unconventional.

https://codereview.chromium.org/342833002/diff/290001/chrome/browser/ui/views...
chrome/browser/ui/views/autofill/password_generation_popup_view_views.cc:164:
kItemTextColor,
Since this is a constant and PasswordBox is only used in one place, might as
well cook this into PasswordBox. Same goes for kHorizontalPadding

https://codereview.chromium.org/342833002/diff/290001/chrome/browser/ui/views...
chrome/browser/ui/views/autofill/password_generation_popup_view_views.cc:165:
controller_->kHorizontalPadding);
nit: kind of weird to refer to a static via an instance

https://codereview.chromium.org/342833002/diff/290001/chrome/browser/ui/views...
chrome/browser/ui/views/autofill/password_generation_popup_view_views.cc:172:
gfx::Size PasswordGenerationPopupViewViews::GetBounds() {
hmm, from this angle, this method name is a little confusing, because it sounds
related to View::GetBoundsInScreen() or View::bounds().

Really, this is more like GetPreferredSize(), so it should have a name like
GetPreferredSizeOfPasswordView(). (Ideally, you would actually need to implement
GetPreferredSize(), but instead the view is sized through some unorthodox dance
with the controller)

https://codereview.chromium.org/342833002/diff/290001/chrome/browser/ui/views...
chrome/browser/ui/views/autofill/password_generation_popup_view_views.cc:227: }
nit: \n

[Garrett Casto, 2014-06-26 01:48:53]

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

https://codereview.chromium.org/342833002/diff/290001/chrome/browser/ui/views...
chrome/browser/ui/views/autofill/password_generation_popup_view_views.cc:160:
static_cast<PasswordBox*>(password_view_)->Init(
On 2014/06/26 00:06:04, Evan Stade wrote:
> nit: prefer avoiding this cast. Perhaps Init() can return |this| for
> convenience. Not sure if that's unconventional.

The problem is that password_view_ is currently just a view::Views, so you need
the cast to call Init(). Changed to make PasswordBox an inner class instead of
just being in an anonymous namespace, which gets rid of the cast.

https://codereview.chromium.org/342833002/diff/290001/chrome/browser/ui/views...
chrome/browser/ui/views/autofill/password_generation_popup_view_views.cc:164:
kItemTextColor,
On 2014/06/26 00:06:04, Evan Stade wrote:
> Since this is a constant and PasswordBox is only used in one place, might as
> well cook this into PasswordBox. Same goes for kHorizontalPadding

Done, though I had to make kItemTextColor (and friends) public as it was
protected in AutofillPopupBaseView.

https://codereview.chromium.org/342833002/diff/290001/chrome/browser/ui/views...
chrome/browser/ui/views/autofill/password_generation_popup_view_views.cc:165:
controller_->kHorizontalPadding);
On 2014/06/26 00:06:04, Evan Stade wrote:
> nit: kind of weird to refer to a static via an instance

Done.

https://codereview.chromium.org/342833002/diff/290001/chrome/browser/ui/views...
chrome/browser/ui/views/autofill/password_generation_popup_view_views.cc:172:
gfx::Size PasswordGenerationPopupViewViews::GetBounds() {
On 2014/06/26 00:06:04, Evan Stade wrote:
> hmm, from this angle, this method name is a little confusing, because it
sounds
> related to View::GetBoundsInScreen() or View::bounds().
> 
> Really, this is more like GetPreferredSize(), so it should have a name like
> GetPreferredSizeOfPasswordView(). (Ideally, you would actually need to
implement
> GetPreferredSize(), but instead the view is sized through some unorthodox
dance
> with the controller)

Done.

https://codereview.chromium.org/342833002/diff/290001/chrome/browser/ui/views...
chrome/browser/ui/views/autofill/password_generation_popup_view_views.cc:227: }
On 2014/06/26 00:06:04, Evan Stade wrote:
> nit: \n

Done.

[Evan Stade, 2014-06-26 03:28:08]

I just noticed Label has a SetLineHeight (which takes an integer). We should
make StyledLabel match that, sorry for leading you down the wrong path. Also I
suspect you can make use of that to simplify the code.

https://codereview.chromium.org/342833002/diff/240001/ui/views/controls/style...
File ui/views/controls/styled_label.cc (right):

https://codereview.chromium.org/342833002/diff/240001/ui/views/controls/style...
ui/views/controls/styled_label.cc:297:
label->SetAutoColorReadabilityEnabled(auto_color_readability_enabled_);
label->SetLineHeight(line_height_);

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

https://codereview.chromium.org/342833002/diff/290001/chrome/browser/ui/views...
chrome/browser/ui/views/autofill/password_generation_popup_view_views.cc:160:
static_cast<PasswordBox*>(password_view_)->Init(
On 2014/06/26 01:48:53, Garrett Casto wrote:
> On 2014/06/26 00:06:04, Evan Stade wrote:
> > nit: prefer avoiding this cast. Perhaps Init() can return |this| for
> > convenience. Not sure if that's unconventional.
> 
> The problem is that password_view_ is currently just a view::Views, so you
need
> the cast to call Init(). Changed to make PasswordBox an inner class instead of
> just being in an anonymous namespace, which gets rid of the cast.

I understand that, but you could do:

PasswordBox* box = new PasswordBox();
box->Init();
password_view_ = box;

or, if you make the change I suggested to Init(),

password_view_ = (new PasswordBox())->Init();

Your solution is OK too.

https://codereview.chromium.org/342833002/diff/290001/chrome/browser/ui/views...
chrome/browser/ui/views/autofill/password_generation_popup_view_views.cc:164:
kItemTextColor,
On 2014/06/26 01:48:53, Garrett Casto wrote:
> On 2014/06/26 00:06:04, Evan Stade wrote:
> > Since this is a constant and PasswordBox is only used in one place, might as
> > well cook this into PasswordBox. Same goes for kHorizontalPadding
> 
> Done, though I had to make kItemTextColor (and friends) public as it was
> protected in AutofillPopupBaseView.

ah, I see

https://codereview.chromium.org/342833002/diff/320001/ui/views/controls/style...
File ui/views/controls/styled_label.cc (left):

https://codereview.chromium.org/342833002/diff/320001/ui/views/controls/style...
ui/views/controls/styled_label.cc:312: return gfx::Size(width, (line + 1) *
line_height + GetInsets().height());
revert this change?

[Garrett Casto, 2014-07-14 06:18:39]

Terribly sorry for the long delay. Was on vacation and dealing with beta
blockers.

I should have seen that views::Label had SetLineHeight(), not sure how I missed
that. It turns out that it's not reasonable to use this directly since
Label::SetLineHeight() will never have an affect on a single line of text and
StyledLabel only calculates height one line at a time. However, I actually
implemented this same thing in one of the earlier drafts, and I think it's
pretty clean.

Please take another look.

https://codereview.chromium.org/342833002/diff/240001/ui/views/controls/style...
File ui/views/controls/styled_label.cc (right):

https://codereview.chromium.org/342833002/diff/240001/ui/views/controls/style...
ui/views/controls/styled_label.cc:297:
label->SetAutoColorReadabilityEnabled(auto_color_readability_enabled_);
On 2014/06/26 03:28:08, Evan Stade wrote:
> label->SetLineHeight(line_height_);

Turns out that this doesn't do anything.

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

https://codereview.chromium.org/342833002/diff/290001/chrome/browser/ui/views...
chrome/browser/ui/views/autofill/password_generation_popup_view_views.cc:160:
static_cast<PasswordBox*>(password_view_)->Init(
On 2014/06/26 03:28:08, Evan Stade wrote:
> On 2014/06/26 01:48:53, Garrett Casto wrote:
> > On 2014/06/26 00:06:04, Evan Stade wrote:
> > > nit: prefer avoiding this cast. Perhaps Init() can return |this| for
> > > convenience. Not sure if that's unconventional.
> > 
> > The problem is that password_view_ is currently just a view::Views, so you
> need
> > the cast to call Init(). Changed to make PasswordBox an inner class instead
of
> > just being in an anonymous namespace, which gets rid of the cast.
> 
> I understand that, but you could do:
> 
> PasswordBox* box = new PasswordBox();
> box->Init();
> password_view_ = box;
> 
> or, if you make the change I suggested to Init(),
> 
> password_view_ = (new PasswordBox())->Init();
> 
> Your solution is OK too.

Ah, misunderstood your second suggestion. Thanks for clarifying.

https://codereview.chromium.org/342833002/diff/320001/ui/views/controls/style...
File ui/views/controls/styled_label.cc (left):

https://codereview.chromium.org/342833002/diff/320001/ui/views/controls/style...
ui/views/controls/styled_label.cc:312: return gfx::Size(width, (line + 1) *
line_height + GetInsets().height());
On 2014/06/26 03:28:08, Evan Stade wrote:
> revert this change?

Turns out I needed to update this again, but I agree the formatting was off
before.

[Evan Stade, 2014-07-15 22:46:29]

lgtm

https://codereview.chromium.org/342833002/diff/340001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/autofill_popup_controller_impl.cc (right):

https://codereview.chromium.org/342833002/diff/340001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:607: int
popup_required_width = GetDesiredPopupWidth();
I don't understand what "required" means here. s/required_// ?

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

https://codereview.chromium.org/342833002/diff/340001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/password_generation_popup_controller_impl.cc:160:
const int minimum_required_width = 360;
nit: s/required_//

https://codereview.chromium.org/342833002/diff/340001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/popup_controller_common.cc (right):

https://codereview.chromium.org/342833002/diff/340001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/popup_controller_common.cc:127: int
popup_required_height) const {
param name should match declaration

https://codereview.chromium.org/342833002/diff/340001/ui/views/controls/style...
File ui/views/controls/styled_label.cc (right):

https://codereview.chromium.org/342833002/diff/340001/ui/views/controls/style...
ui/views/controls/styled_label.cc:318: // The last line shouldn't extend past
the text, so use CalculateLineHeight()
"The last line shouldn't extend past the text"

It took me a while to grok this^. Consider:

"The user-specified line height only applies to interline spacing, so the final
line's height is unaffected."

[Garrett Casto, 2014-07-16 07:38:23]

https://codereview.chromium.org/342833002/diff/340001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/autofill_popup_controller_impl.cc (right):

https://codereview.chromium.org/342833002/diff/340001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:607: int
popup_required_width = GetDesiredPopupWidth();
On 2014/07/15 22:46:28, Evan Stade wrote:
> I don't understand what "required" means here. s/required_// ?

Done.

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

https://codereview.chromium.org/342833002/diff/340001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/password_generation_popup_controller_impl.cc:160:
const int minimum_required_width = 360;
On 2014/07/15 22:46:28, Evan Stade wrote:
> nit: s/required_//

Done.

https://codereview.chromium.org/342833002/diff/340001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/popup_controller_common.cc (right):

https://codereview.chromium.org/342833002/diff/340001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/popup_controller_common.cc:127: int
popup_required_height) const {
On 2014/07/15 22:46:28, Evan Stade wrote:
> param name should match declaration

Done.

https://codereview.chromium.org/342833002/diff/340001/ui/views/controls/style...
File ui/views/controls/styled_label.cc (right):

https://codereview.chromium.org/342833002/diff/340001/ui/views/controls/style...
ui/views/controls/styled_label.cc:318: // The last line shouldn't extend past
the text, so use CalculateLineHeight()
On 2014/07/15 22:46:28, Evan Stade wrote:
> "The last line shouldn't extend past the text"
> 
> It took me a while to grok this^. Consider:
> 
> "The user-specified line height only applies to interline spacing, so the
final
> line's height is unaffected."

Done.

[Garrett Casto, 2014-07-16 07:38:31]

The CQ bit was checked by gcasto@chromium.org

[commit-bot: I haz the power, 2014-07-16 07:40:35]

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

[commit-bot: I haz the power, 2014-07-16 21:37:32]

Change committed as 283517