Support displaying the avatar label on the right side.

Support displaying the avatar label on the right side.

The avatar label should always be displayed on the same side
as the avatar button. Currently, the avatar label is always
displayed on the left, but the avatar icon can also be
displayed on the right. This CL moves the avatar label adds
support for displaying the avatar label on the right and moves
the avatar label to the right if the avatar icon is also on
the right.
A screenshot of the placement on the right can be found in the bug description.

BUG=332392
TEST=unit_tests

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

[Adrian Kuegel, 2014-01-08 16:54:57]

Peter, can you please review this CL?

[Adrian Kuegel, 2014-01-10 17:02:49]

On 2014/01/08 16:54:57, Adrian Kuegel wrote:
> Peter, can you please review this CL?

Ping?

[Adrian Kuegel, 2014-01-14 17:21:04]

On 2014/01/10 17:02:49, Adrian Kuegel wrote:
> On 2014/01/08 16:54:57, Adrian Kuegel wrote:
> > Peter, can you please review this CL?
> 
> Ping?

Peter, can you please take a look at this CL, or in case you have no time, tell
me that I can find another reviewer?
Without this patch, the label currently is displayed behind the tabs if the
avatar icon is displayed on the right.

[Peter Kasting, 2014-01-14 20:06:40]

On 2014/01/14 17:21:04, Adrian Kuegel wrote:
> Peter, can you please take a look at this CL, or in case you have no time,
tell
> me that I can find another reviewer?

Sorry -- I was OOO all last week and just got through most of my email backlog
last night.  I'll try and get to this today.

[Peter Kasting, 2014-01-15 02:06:25]

https://codereview.chromium.org/127253004/diff/70001/chrome/browser/ui/views/...
File chrome/browser/ui/views/avatar_label.cc (right):

https://codereview.chromium.org/127253004/diff/70001/chrome/browser/ui/views/...
chrome/browser/ui/views/avatar_label.cc:100: browser_view_(browser_view),
label_on_right_(false) {
Nit: One member per line

https://codereview.chromium.org/127253004/diff/70001/chrome/browser/ui/views/...
chrome/browser/ui/views/avatar_label.cc:135: if (label_on_right_ !=
label_on_right) {
Nit: Do we actually need this conditional?  If we just did things
unconditionally, we could also replace the initializer and set_border() call in
the constructor with a single call to this function.  Or is
OpaqueBrowserFrameViewLayout::LayoutAvatar() called too often?

https://codereview.chromium.org/127253004/diff/70001/chrome/browser/ui/views/...
File chrome/browser/ui/views/avatar_label.h (right):

https://codereview.chromium.org/127253004/diff/70001/chrome/browser/ui/views/...
chrome/browser/ui/views/avatar_label.h:25: static const char kViewClassName[];
This, and GetClassName(), are only used for a DCHECK.  That's not worth the
cost.  Remove these.  It would be nice to also file a bug to remove any other
such class names that are only used for similar DCHECKs.

https://codereview.chromium.org/127253004/diff/70001/chrome/browser/ui/views/...
File chrome/browser/ui/views/frame/opaque_browser_frame_view_layout.cc (right):

https://codereview.chromium.org/127253004/diff/70001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/opaque_browser_frame_view_layout.cc:444: // avatar
label should be slightly to the right of the avatar menu button.
Nit: This whole comment can just be:

The outside edge of the avatar label should be just outside that of the avatar
menu button.

https://codereview.chromium.org/127253004/diff/70001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/opaque_browser_frame_view_layout.cc:600:
view->GetClassName());
As alluded to in the other file: I don't really see why we have any of these
DCHECKs.  I don't think they add much and I bet we could remove some members and
overrides by removing all of them.

https://codereview.chromium.org/127253004/diff/70001/chrome/browser/ui/views/...
File chrome/browser/ui/views/frame/opaque_browser_frame_view_layout_unittest.cc
(right):

https://codereview.chromium.org/127253004/diff/70001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/opaque_browser_frame_view_layout_unittest.cc:461:
const int kAvatarIconToLabelSpacing = 2;
There are a lot of magic numbers in here.  It would be nice to only use numbers
defined in the implementation, instead of having to define more numbers in the
test.

Furthermore, we can avoid the need for these numbers, and be more resilient
against future changes, by loosening the checks.  For example, instead of
checking that the label bounds are at some exact set of coordinates, we can just
use e.g. EXPECT_GT() and similar inequality checks to ensure things appear in
the right order.

[Adrian Kuegel, 2014-01-15 16:24:51]

Thanks for the review :)
Sorry for the ping, I didn't know you were OOO.

https://codereview.chromium.org/127253004/diff/70001/chrome/browser/ui/views/...
File chrome/browser/ui/views/avatar_label.cc (right):

https://codereview.chromium.org/127253004/diff/70001/chrome/browser/ui/views/...
chrome/browser/ui/views/avatar_label.cc:100: browser_view_(browser_view),
label_on_right_(false) {
On 2014/01/15 02:06:26, Peter Kasting wrote:
> Nit: One member per line

Done.

https://codereview.chromium.org/127253004/diff/70001/chrome/browser/ui/views/...
chrome/browser/ui/views/avatar_label.cc:135: if (label_on_right_ !=
label_on_right) {
On 2014/01/15 02:06:26, Peter Kasting wrote:
> Nit: Do we actually need this conditional?  If we just did things
> unconditionally, we could also replace the initializer and set_border() call
in
> the constructor with a single call to this function.  Or is
> OpaqueBrowserFrameViewLayout::LayoutAvatar() called too often?

I checked it now, and it looks it is called only on user interaction (apart from
initialization), like when clicking on some item. So it should be fine to always
call set_border(), and I changed it like this.

https://codereview.chromium.org/127253004/diff/70001/chrome/browser/ui/views/...
File chrome/browser/ui/views/avatar_label.h (right):

https://codereview.chromium.org/127253004/diff/70001/chrome/browser/ui/views/...
chrome/browser/ui/views/avatar_label.h:25: static const char kViewClassName[];
On 2014/01/15 02:06:26, Peter Kasting wrote:
> This, and GetClassName(), are only used for a DCHECK.  That's not worth the
> cost.  Remove these.  It would be nice to also file a bug to remove any other
> such class names that are only used for similar DCHECKs.

Done. And filed crbug/334666.

https://codereview.chromium.org/127253004/diff/70001/chrome/browser/ui/views/...
File chrome/browser/ui/views/frame/opaque_browser_frame_view_layout.cc (right):

https://codereview.chromium.org/127253004/diff/70001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/opaque_browser_frame_view_layout.cc:444: // avatar
label should be slightly to the right of the avatar menu button.
On 2014/01/15 02:06:26, Peter Kasting wrote:
> Nit: This whole comment can just be:
> 
> The outside edge of the avatar label should be just outside that of the avatar
> menu button.

Done.

https://codereview.chromium.org/127253004/diff/70001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/opaque_browser_frame_view_layout.cc:600:
view->GetClassName());
On 2014/01/15 02:06:26, Peter Kasting wrote:
> As alluded to in the other file: I don't really see why we have any of these
> DCHECKs.  I don't think they add much and I bet we could remove some members
and
> overrides by removing all of them.

I filed a bug for that and removed the DCHECK for AvatarLabel.

https://codereview.chromium.org/127253004/diff/70001/chrome/browser/ui/views/...
File chrome/browser/ui/views/frame/opaque_browser_frame_view_layout_unittest.cc
(right):

https://codereview.chromium.org/127253004/diff/70001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/opaque_browser_frame_view_layout_unittest.cc:461:
const int kAvatarIconToLabelSpacing = 2;
On 2014/01/15 02:06:26, Peter Kasting wrote:
> There are a lot of magic numbers in here.  It would be nice to only use
numbers
> defined in the implementation, instead of having to define more numbers in the
> test.
> 
> Furthermore, we can avoid the need for these numbers, and be more resilient
> against future changes, by loosening the checks.  For example, instead of
> checking that the label bounds are at some exact set of coordinates, we can
just
> use e.g. EXPECT_GT() and similar inequality checks to ensure things appear in
> the right order.

I tried to rewrite my changes to do only relative checks (expect for the exact
checks that are also used elsewhere). This should now ensure that the avatar
label will not be in the region of the tab strip, and that the avatar icon/
avatar label have the correct relative order of x(), right(), y() and bottom()
positions.

[Peter Kasting, 2014-01-16 21:46:17]

LGTM

https://codereview.chromium.org/127253004/diff/190001/chrome/browser/ui/views...
File chrome/browser/ui/views/avatar_label.h (left):

https://codereview.chromium.org/127253004/diff/190001/chrome/browser/ui/views...
chrome/browser/ui/views/avatar_label.h:27: // views::TextButton:
Nit: Don't remove this.

https://codereview.chromium.org/127253004/diff/190001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/opaque_browser_frame_view_layout.cc (right):

https://codereview.chromium.org/127253004/diff/190001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/opaque_browser_frame_view_layout.cc:443:
host->width() - trailing_button_start_ - label_size.width() :
Nit: Consider parens around this subexpression

https://codereview.chromium.org/127253004/diff/190001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/opaque_browser_frame_view_layout_unittest.cc
(right):

https://codereview.chromium.org/127253004/diff/190001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/opaque_browser_frame_view_layout_unittest.cc:471:
EXPECT_GT(
Nit: I slightly prefer breaking after the first param and indenting the next
line even

[Adrian Kuegel, 2014-01-17 09:42:16]

https://codereview.chromium.org/127253004/diff/190001/chrome/browser/ui/views...
File chrome/browser/ui/views/avatar_label.h (left):

https://codereview.chromium.org/127253004/diff/190001/chrome/browser/ui/views...
chrome/browser/ui/views/avatar_label.h:27: // views::TextButton:
On 2014/01/16 21:46:17, Peter Kasting wrote:
> Nit: Don't remove this.

Thanks for catching this. I removed this accidentally when removing the other
method.

https://codereview.chromium.org/127253004/diff/190001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/opaque_browser_frame_view_layout.cc (right):

https://codereview.chromium.org/127253004/diff/190001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/opaque_browser_frame_view_layout.cc:443:
host->width() - trailing_button_start_ - label_size.width() :
On 2014/01/16 21:46:17, Peter Kasting wrote:
> Nit: Consider parens around this subexpression

Done.

https://codereview.chromium.org/127253004/diff/190001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/opaque_browser_frame_view_layout_unittest.cc
(right):

https://codereview.chromium.org/127253004/diff/190001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/opaque_browser_frame_view_layout_unittest.cc:471:
EXPECT_GT(
On 2014/01/16 21:46:17, Peter Kasting wrote:
> Nit: I slightly prefer breaking after the first param and indenting the next
> line even

Done.

[commit-bot: I haz the power, 2014-01-17 09:42:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/akuegel@chromium.org/127253004/310001

[commit-bot: I haz the power, 2014-01-17 13:54:50]

Change committed as 245503