Update look of profile menu (Views)

Update look of profile menu (Views)

This change updates the look of the profile menu to match the latest mocks. See bug for more info.

BUG=93837
TEST=

[sail, 2011-10-01 00:56:52]

Ping!

[Peter Kasting, 2011-10-01 04:51:45]

On 2011/10/01 00:56:52, sail wrote:
> Ping!

I'm sorry, I was out Thursday giving a talk at Berkeley and barely managed to
dig out of my inbox today.  I'll look at this Monday.

[Peter Kasting, 2011-10-04 20:48:52]

http://codereview.chromium.org/8070021/diff/6001/chrome/browser/ui/views/avat...
File chrome/browser/ui/views/avatar_menu_bubble_view.cc (right):

http://codereview.chromium.org/8070021/diff/6001/chrome/browser/ui/views/avat...
chrome/browser/ui/views/avatar_menu_bubble_view.cc:27: const int
kBubbleViewMinWidth = 175;
Nit: Personally I'd find it more readable if the constants were declared in the
functions that use them, just above their actual use, if possible.  Then the
function of each constant would be clearer.

http://codereview.chromium.org/8070021/diff/6001/chrome/browser/ui/views/avat...
chrome/browser/ui/views/avatar_menu_bubble_view.cc:151:
edit_link_->SetNormalColor(SkColorSetRGB(0, 0x79, 0xda));
This is dangerous.  The background color could very well make this unreadable on
some systems.

You should use color_utils::GetReadableColor() here.

http://codereview.chromium.org/8070021/diff/6001/chrome/browser/ui/views/avat...
chrome/browser/ui/views/avatar_menu_bubble_view.cc:159:
canvas->FillRectInt(SkColorSetRGB(227, 237, 246), 0, 0,
Nit: You used hex above and decimal here.

This is another case where you need to be careful about hardcoding colors.  You
might want to either alpha-blend something with the existing background color or
use GetReadableColor() with this as the foreground color and the system text
color as the background to pick a good color.

http://codereview.chromium.org/8070021/diff/6001/chrome/browser/ui/views/avat...
chrome/browser/ui/views/avatar_menu_bubble_view.cc:182:
name_size.set_width(std::max(name_size.width(), max_label_width));
Do you mean std::min() in these three cases?

http://codereview.chromium.org/8070021/diff/6001/chrome/browser/ui/views/avat...
chrome/browser/ui/views/avatar_menu_bubble_view.cc:194:
std::max(state_size.height(), edit_size.height());
If you're using max(state, edit) for the height here, shouldn't you also do so
when calculating |labels_height|?

[sail, 2011-10-04 22:09:20]

http://codereview.chromium.org/8070021/diff/6001/chrome/browser/ui/views/avat...
File chrome/browser/ui/views/avatar_menu_bubble_view.cc (right):

http://codereview.chromium.org/8070021/diff/6001/chrome/browser/ui/views/avat...
chrome/browser/ui/views/avatar_menu_bubble_view.cc:27: const int
kBubbleViewMinWidth = 175;
On 2011/10/04 20:48:52, Peter Kasting wrote:
> Nit: Personally I'd find it more readable if the constants were declared in
the
> functions that use them, just above their actual use, if possible.  Then the
> function of each constant would be clearer.

Fixed. Moved all the constants that were only being used in one function to
their place of use.

http://codereview.chromium.org/8070021/diff/6001/chrome/browser/ui/views/avat...
chrome/browser/ui/views/avatar_menu_bubble_view.cc:151:
edit_link_->SetNormalColor(SkColorSetRGB(0, 0x79, 0xda));
On 2011/10/04 20:48:52, Peter Kasting wrote:
> This is dangerous.  The background color could very well make this unreadable
on
> some systems.
> 
> You should use color_utils::GetReadableColor() here.

I'd like to fix this separately as a part of bug 92250. (Note, overall, with
this CL this UI now has fewer hardcoded colors than before).

http://codereview.chromium.org/8070021/diff/6001/chrome/browser/ui/views/avat...
chrome/browser/ui/views/avatar_menu_bubble_view.cc:159:
canvas->FillRectInt(SkColorSetRGB(227, 237, 246), 0, 0,
On 2011/10/04 20:48:52, Peter Kasting wrote:
> Nit: You used hex above and decimal here.
> 
> This is another case where you need to be careful about hardcoding colors. 
You
> might want to either alpha-blend something with the existing background color
or
> use GetReadableColor() with this as the foreground color and the system text
> color as the background to pick a good color.

Changed to hex. As above, I'm working on fixing the color selection as a part of
bug 92250.

http://codereview.chromium.org/8070021/diff/6001/chrome/browser/ui/views/avat...
chrome/browser/ui/views/avatar_menu_bubble_view.cc:182:
name_size.set_width(std::max(name_size.width(), max_label_width));
On 2011/10/04 20:48:52, Peter Kasting wrote:
> Do you mean std::min() in these three cases?

Ouch, good catch. Fixed.

http://codereview.chromium.org/8070021/diff/6001/chrome/browser/ui/views/avat...
chrome/browser/ui/views/avatar_menu_bubble_view.cc:194:
std::max(state_size.height(), edit_size.height());
On 2011/10/04 20:48:52, Peter Kasting wrote:
> If you're using max(state, edit) for the height here, shouldn't you also do so
> when calculating |labels_height|?

Done. Good idea.

[Peter Kasting, 2011-10-04 22:12:48]

LGTM

http://codereview.chromium.org/8070021/diff/6001/chrome/browser/ui/views/avat...
File chrome/browser/ui/views/avatar_menu_bubble_view.cc (right):

http://codereview.chromium.org/8070021/diff/6001/chrome/browser/ui/views/avat...
chrome/browser/ui/views/avatar_menu_bubble_view.cc:151:
edit_link_->SetNormalColor(SkColorSetRGB(0, 0x79, 0xda));
On 2011/10/04 22:09:20, sail wrote:
> On 2011/10/04 20:48:52, Peter Kasting wrote:
> > This is dangerous.  The background color could very well make this
unreadable
> on
> > some systems.
> > 
> > You should use color_utils::GetReadableColor() here.
> 
> I'd like to fix this separately as a part of bug 92250. (Note, overall, with
> this CL this UI now has fewer hardcoded colors than before).

That's fine.

[commit-bot: I haz the power, 2011-10-04 22:13:44]

CQ is trying the patch. Follow status at
https://chromium-status.appspot.com/cq/sail@chromium.org/8070021/13001

[commit-bot: I haz the power, 2011-10-04 23:53:21]

Change committed as 104031