Adjust positioning of cros tray bubbles.

Adjust positioning of cros tray bubbles.

This consolidates the anchoring logic of all tray bubbles. In the common
case (bottom aligned, LTR) this is right bubble edge to right button
edge, and vertically, flushed to the tray (i.e. the area that turns
black when you maximize a window).

There are never arrows on tray bubbles these days, so a ton of arrow-
related logic is removed from TrayBubbleView.

Since all bubbles position themselves the same way (and because
set_anchor_view_insets is flexible enough to cover any differences),
there's no need to delegate GetAnchorRect any more. Many other settings
such as the shadow are also not needed since they're always the same.

Trying to maintain pre-MD behavior and have both coexist would be a
mess, so the change is not gated on the MD flag. However, it works fine
in both worlds.

BUG=663016
Committed: https://crrev.com/3acb088db0632abae388129760f33bdf5fbe09a2
Cr-Commit-Position: refs/heads/master@{#432567}

[Evan Stade, 2016-11-13 03:41:39]

Description was changed from

==========
WIP - adjust positioning of cros tray bubbles.

BUG=
==========

to

==========
Adjust positioning of cros tray bubbles.

This consolidates the anchoring logic of all tray bubbles. In the common case
(bottom aligned, LTR) this is right bubble edge to right button edge, and
vertically, flushed to the tray (i.e. the area that turns black when you
maximize a window).

There are never arrows on tray bubbles these days, so a ton of arrow-related
logic is removed from TrayBubbleView.

Since all bubbles position themselves the same way (and because
set_anchor_view_insets is flexible enough to cover any differences), there's no
need to delegate GetAnchorRect any more. Many other settings such as the shadow
are also not needed since they're always the same.

Trying to maintain pre-MD behavior and have both coexist would be a mess, so the
change is not gated on the MD flag. However, it works fine in both worlds.

BUG=663016
==========

[Evan Stade, 2016-11-13 04:28:39]

Description was changed from

==========
Adjust positioning of cros tray bubbles.

This consolidates the anchoring logic of all tray bubbles. In the common case
(bottom aligned, LTR) this is right bubble edge to right button edge, and
vertically, flushed to the tray (i.e. the area that turns black when you
maximize a window).

There are never arrows on tray bubbles these days, so a ton of arrow-related
logic is removed from TrayBubbleView.

Since all bubbles position themselves the same way (and because
set_anchor_view_insets is flexible enough to cover any differences), there's no
need to delegate GetAnchorRect any more. Many other settings such as the shadow
are also not needed since they're always the same.

Trying to maintain pre-MD behavior and have both coexist would be a mess, so the
change is not gated on the MD flag. However, it works fine in both worlds.

BUG=663016
==========

to

==========
Adjust positioning of cros tray bubbles.

This consolidates the anchoring logic of all tray bubbles. In the common
case (bottom aligned, LTR) this is right bubble edge to right button 
edge, and vertically, flushed to the tray (i.e. the area that turns 
black when you maximize a window).

There are never arrows on tray bubbles these days, so a ton of arrow-
related logic is removed from TrayBubbleView.

Since all bubbles position themselves the same way (and because 
set_anchor_view_insets is flexible enough to cover any differences), 
there's no need to delegate GetAnchorRect any more. Many other settings 
such as the shadow are also not needed since they're always the same.

Trying to maintain pre-MD behavior and have both coexist would be a 
mess, so the change is not gated on the MD flag. However, it works fine 
in both worlds.

BUG=663016
==========

[Evan Stade, 2016-11-13 04:28:39]

estade@chromium.org changed reviewers:
+ sky@chromium.org, tdanderson@chromium.org

[Evan Stade, 2016-11-13 04:28:44]

The CQ bit was checked by estade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-13 04:28:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Evan Stade, 2016-11-13 04:28:59]

(I'm sure there will be test failures)

[commit-bot: I haz the power, 2016-11-13 06:34:18]

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

[commit-bot: I haz the power, 2016-11-13 06:34:19]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[tdanderson, 2016-11-14 04:03:22]

Thanks a lot for this code cleanup! ash/common/system LG but it would definitely
not hurt for sky@ to lend a second pair of eyes on those files too when
reviewing ui/views and ui/message_center.

https://codereview.chromium.org/2491033006/diff/80001/ash/common/system/tray/...
File ash/common/system/tray/system_tray.cc (right):

https://codereview.chromium.org/2491033006/diff/80001/ash/common/system/tray/...
ash/common/system/tray/system_tray.cc:628: // used the message center
notifications instead. See crbug.com/664770
See also crbug.com/630641

https://codereview.chromium.org/2491033006/diff/80001/ash/common/system/tray/...
File ash/common/system/tray/tray_constants.cc (right):

https://codereview.chromium.org/2491033006/diff/80001/ash/common/system/tray/...
ash/common/system/tray/tray_constants.cc:107: const int
kTrayPaddingFromEdgeOfShelf[] = {3, 3, 3};
With this change, can you check that the system menu is still clickable when the
mouse cursor is in the very bottom right corner of the screen (for a horizontal
or right-aligned shelf) and in the very bottom left corner of the screen (for a
left-aligned shelf)? IIRC that is the purpose of |extended_padding| in
StatusAreaWidgetDelegate::SetBorderOnChild().

[sky, 2016-11-14 16:18:48]

sky@chromium.org changed reviewers:
+ msw@chromium.org

[sky, 2016-11-14 16:18:49]

+msw as he is likely a better reviewer for this than I am.

https://codereview.chromium.org/2491033006/diff/80001/ui/views/bubble/tray_bu...
File ui/views/bubble/tray_bubble_view.h (right):

https://codereview.chromium.org/2491033006/diff/80001/ui/views/bubble/tray_bu...
ui/views/bubble/tray_bubble_view.h:31: // Specialized bubble view for bubbles
associated with a tray icon (e.g. the
Any idea why we have this code in ui/views? If it's ash specific it should live
else where.

[Evan Stade, 2016-11-14 17:00:00]

https://codereview.chromium.org/2491033006/diff/80001/ui/views/bubble/tray_bu...
File ui/views/bubble/tray_bubble_view.h (right):

https://codereview.chromium.org/2491033006/diff/80001/ui/views/bubble/tray_bu...
ui/views/bubble/tray_bubble_view.h:31: // Specialized bubble view for bubbles
associated with a tray icon (e.g. the
On 2016/11/14 16:18:49, sky wrote:
> Any idea why we have this code in ui/views? If it's ash specific it should
live
> else where.

Probably because it's referenced by MessageCenter. MessageCenter should probably
move to ash now but at the moment it can't depend on ash (as ash depends on
message center).

[sky, 2016-11-14 17:16:35]

https://codereview.chromium.org/2491033006/diff/80001/ui/views/bubble/tray_bu...
File ui/views/bubble/tray_bubble_view.h (right):

https://codereview.chromium.org/2491033006/diff/80001/ui/views/bubble/tray_bu...
ui/views/bubble/tray_bubble_view.h:31: // Specialized bubble view for bubbles
associated with a tray icon (e.g. the
On 2016/11/14 17:00:00, Evan Stade wrote:
> On 2016/11/14 16:18:49, sky wrote:
> > Any idea why we have this code in ui/views? If it's ash specific it should
> live
> > else where.
> 
> Probably because it's referenced by MessageCenter. MessageCenter should
probably
> move to ash now but at the moment it can't depend on ash (as ash depends on
> message center).

ash/shared exists for code that can be consumed by apps, including ash. See the
README for details.

[Evan Stade, 2016-11-15 01:11:06]

The CQ bit was checked by estade@chromium.org to run a CQ dry run

[Evan Stade, 2016-11-15 01:11:55]

+msw, hate to nag, but we'd like to land this by Thursday, so if major changes
are needed it'd be nice to know sooner rather than later.

https://codereview.chromium.org/2491033006/diff/80001/ash/common/system/tray/...
File ash/common/system/tray/system_tray.cc (right):

https://codereview.chromium.org/2491033006/diff/80001/ash/common/system/tray/...
ash/common/system/tray/system_tray.cc:628: // used the message center
notifications instead. See crbug.com/664770
On 2016/11/14 04:03:21, tdanderson wrote:
> See also crbug.com/630641

indeed, I failed to search for previously filed bugs.

https://codereview.chromium.org/2491033006/diff/80001/ash/common/system/tray/...
File ash/common/system/tray/tray_constants.cc (right):

https://codereview.chromium.org/2491033006/diff/80001/ash/common/system/tray/...
ash/common/system/tray/tray_constants.cc:107: const int
kTrayPaddingFromEdgeOfShelf[] = {3, 3, 3};
On 2016/11/14 04:03:21, tdanderson wrote:
> With this change, can you check that the system menu is still clickable when
the
> mouse cursor is in the very bottom right corner of the screen (for a
horizontal
> or right-aligned shelf) and in the very bottom left corner of the screen (for
a
> left-aligned shelf)? IIRC that is the purpose of |extended_padding| in
> StatusAreaWidgetDelegate::SetBorderOnChild().

it's hard to check that when chromeos is running in a window (it seems to still
work but I can't be sure I'm clicking on the cornermost pixel). That said I
don't see why this would cause a problem --- it should just move the last tray
item one dp closer to the edge.

https://codereview.chromium.org/2491033006/diff/80001/ui/views/bubble/tray_bu...
File ui/views/bubble/tray_bubble_view.h (right):

https://codereview.chromium.org/2491033006/diff/80001/ui/views/bubble/tray_bu...
ui/views/bubble/tray_bubble_view.h:31: // Specialized bubble view for bubbles
associated with a tray icon (e.g. the
On 2016/11/14 17:16:35, sky wrote:
> On 2016/11/14 17:00:00, Evan Stade wrote:
> > On 2016/11/14 16:18:49, sky wrote:
> > > Any idea why we have this code in ui/views? If it's ash specific it should
> > live
> > > else where.
> > 
> > Probably because it's referenced by MessageCenter. MessageCenter should
> probably
> > move to ash now but at the moment it can't depend on ash (as ash depends on
> > message center).
> 
> ash/shared exists for code that can be consumed by apps, including ash. See
the
> README for details.

OK. Do you think message center should stay where it is? Do you want me to try
to move message center or TrayBubbleView in this CL or leave it as a future
cleanup?

[commit-bot: I haz the power, 2016-11-15 01:11:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-15 01:18:36]

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

[commit-bot: I haz the power, 2016-11-15 01:18:37]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[msw, 2016-11-15 02:59:37]

msw@chromium.org changed reviewers:
+ skuhne@chromium.org

[msw, 2016-11-15 02:59:38]

skuhne@ knows tray bubble better than me, but I have some comments.

https://codereview.chromium.org/2491033006/diff/100001/ash/common/system/tray...
File ash/common/system/tray/system_tray.cc (right):

https://codereview.chromium.org/2491033006/diff/100001/ash/common/system/tray...
ash/common/system/tray/system_tray.cc:132: TrayBubbleView::InitParams*
init_params,
should the other params to InitView be part of InitParams?

https://codereview.chromium.org/2491033006/diff/100001/ash/common/system/tray...
ash/common/system/tray/system_tray.cc:640: views::View* anchor;
nit: init to GetBubbleAnchor() (or nullptr)

https://codereview.chromium.org/2491033006/diff/100001/ash/common/system/tray...
ash/common/system/tray/system_tray.cc:647: gfx::Insets anchor_insets;
optional nit: init to GetBubbleAnchorInsets() and remove the else case below.

https://codereview.chromium.org/2491033006/diff/100001/ash/common/system/tray...
ash/common/system/tray/system_tray.cc:653: init_params.anchor_alignment =
TrayBubbleView::ANCHOR_ALIGNMENT_BOTTOM;
does this need to check the shelf alignment?

https://codereview.chromium.org/2491033006/diff/100001/ash/common/system/tray...
File ash/common/system/tray/tray_background_view.cc (right):

https://codereview.chromium.org/2491033006/diff/100001/ash/common/system/tray...
ash/common/system/tray/tray_background_view.cc:479: // TODO(estade): for reasons
I don't understand, BubbleBorder distances the
What's the action on this TODO? Is it worth keeping?

https://codereview.chromium.org/2491033006/diff/100001/ash/common/system/tray...
ash/common/system/tray/tray_background_view.cc:483: return gfx::Insets(
This may be more legible with if (bottom) return ...; return ...; instead of
ternaries for each arg.

https://codereview.chromium.org/2491033006/diff/100001/ui/views/bubble/tray_b...
File ui/views/bubble/tray_bubble_view.h (right):

https://codereview.chromium.org/2491033006/diff/100001/ui/views/bubble/tray_b...
ui/views/bubble/tray_bubble_view.h:23: class TrayBubbleBorder;
remove

https://codereview.chromium.org/2491033006/diff/100001/ui/views/bubble/tray_b...
ui/views/bubble/tray_bubble_view.h:128: // gfx::Rect GetAnchorRect() const
override;
Remove

[sky, 2016-11-15 03:00:03]

Leave moving for a future patch. I should have made that clear, sorry.

  -Scott

On Mon, Nov 14, 2016 at 5:11 PM,  <estade@chromium.org> wrote:
> +msw, hate to nag, but we'd like to land this by Thursday, so if major
> changes
> are needed it'd be nice to know sooner rather than later.
>
>
>
https://codereview.chromium.org/2491033006/diff/80001/ash/common/system/tray/...
> File ash/common/system/tray/system_tray.cc (right):
>
>
https://codereview.chromium.org/2491033006/diff/80001/ash/common/system/tray/...
> ash/common/system/tray/system_tray.cc:628: // used the message center
> notifications instead. See crbug.com/664770
> On 2016/11/14 04:03:21, tdanderson wrote:
>> See also crbug.com/630641
>
> indeed, I failed to search for previously filed bugs.
>
>
https://codereview.chromium.org/2491033006/diff/80001/ash/common/system/tray/...
> File ash/common/system/tray/tray_constants.cc (right):
>
>
https://codereview.chromium.org/2491033006/diff/80001/ash/common/system/tray/...
> ash/common/system/tray/tray_constants.cc:107: const int
> kTrayPaddingFromEdgeOfShelf[] = {3, 3, 3};
> On 2016/11/14 04:03:21, tdanderson wrote:
>> With this change, can you check that the system menu is still
> clickable when the
>> mouse cursor is in the very bottom right corner of the screen (for a
> horizontal
>> or right-aligned shelf) and in the very bottom left corner of the
> screen (for a
>> left-aligned shelf)? IIRC that is the purpose of |extended_padding| in
>> StatusAreaWidgetDelegate::SetBorderOnChild().
>
> it's hard to check that when chromeos is running in a window (it seems
> to still work but I can't be sure I'm clicking on the cornermost pixel).
> That said I don't see why this would cause a problem --- it should just
> move the last tray item one dp closer to the edge.
>
>
https://codereview.chromium.org/2491033006/diff/80001/ui/views/bubble/tray_bu...
> File ui/views/bubble/tray_bubble_view.h (right):
>
>
https://codereview.chromium.org/2491033006/diff/80001/ui/views/bubble/tray_bu...
> ui/views/bubble/tray_bubble_view.h:31: // Specialized bubble view for
> bubbles associated with a tray icon (e.g. the
> On 2016/11/14 17:16:35, sky wrote:
>> On 2016/11/14 17:00:00, Evan Stade wrote:
>> > On 2016/11/14 16:18:49, sky wrote:
>> > > Any idea why we have this code in ui/views? If it's ash specific
> it should
>> > live
>> > > else where.
>> >
>> > Probably because it's referenced by MessageCenter. MessageCenter
> should
>> probably
>> > move to ash now but at the moment it can't depend on ash (as ash
> depends on
>> > message center).
>>
>> ash/shared exists for code that can be consumed by apps, including
> ash. See the
>> README for details.
>
> OK. Do you think message center should stay where it is? Do you want me
> to try to move message center or TrayBubbleView in this CL or leave it
> as a future cleanup?
>
> https://codereview.chromium.org/2491033006/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Evan Stade, 2016-11-15 17:01:29]

https://codereview.chromium.org/2491033006/diff/100001/ash/common/system/tray...
File ash/common/system/tray/system_tray.cc (right):

https://codereview.chromium.org/2491033006/diff/100001/ash/common/system/tray...
ash/common/system/tray/system_tray.cc:132: TrayBubbleView::InitParams*
init_params,
On 2016/11/15 02:59:38, msw wrote:
> should the other params to InitView be part of InitParams?

probably not |tray|. The others could be, I suppose. I don't know what governs
what goes into InitParams. Things that have some non-default default (e.g. a
default minimum height that's not zero)?

In the case of anchor_insets, we already have this nice setter so I don't see
much purpose in making it part of init_params instead. The only reason it's
added to SystemBubbleWrapper::InitView is that it needs to be called after
bubble creation but before it's shown.

https://codereview.chromium.org/2491033006/diff/100001/ash/common/system/tray...
ash/common/system/tray/system_tray.cc:640: views::View* anchor;
On 2016/11/15 02:59:38, msw wrote:
> nit: init to GetBubbleAnchor() (or nullptr)

Done.

https://codereview.chromium.org/2491033006/diff/100001/ash/common/system/tray...
ash/common/system/tray/system_tray.cc:647: gfx::Insets anchor_insets;
On 2016/11/15 02:59:38, msw wrote:
> optional nit: init to GetBubbleAnchorInsets() and remove the else case below.

Done.

https://codereview.chromium.org/2491033006/diff/100001/ash/common/system/tray...
ash/common/system/tray/system_tray.cc:653: init_params.anchor_alignment =
TrayBubbleView::ANCHOR_ALIGNMENT_BOTTOM;
On 2016/11/15 02:59:38, msw wrote:
> does this need to check the shelf alignment?

no, the notification bubble will go on top of the system menu bubble regardless
of orientation and use left or right justification depending on locale.

https://codereview.chromium.org/2491033006/diff/100001/ash/common/system/tray...
File ash/common/system/tray/tray_background_view.cc (right):

https://codereview.chromium.org/2491033006/diff/100001/ash/common/system/tray...
ash/common/system/tray/tray_background_view.cc:479: // TODO(estade): for reasons
I don't understand, BubbleBorder distances the
On 2016/11/15 02:59:38, msw wrote:
> What's the action on this TODO? Is it worth keeping?

I think the comment has value to explain what's going on.  The TODO at the
beginning indicates that maybe it would be nice to resolve this by adjusting the
implementation of BubbleBorder::GetBounds (which someone will have to do for
Harmony anyway). Also, if someone tries to make the necessary adjustments and
audits EDGE_TO_ANCHOR_EDGE alignments, this comment should help shed some light
on the best fix.

https://codereview.chromium.org/2491033006/diff/100001/ash/common/system/tray...
ash/common/system/tray/tray_background_view.cc:483: return gfx::Insets(
On 2016/11/15 02:59:38, msw wrote:
> This may be more legible with if (bottom) return ...; return ...; instead of
> ternaries for each arg.

done (still not super legible though :)

https://codereview.chromium.org/2491033006/diff/100001/ui/views/bubble/tray_b...
File ui/views/bubble/tray_bubble_view.h (right):

https://codereview.chromium.org/2491033006/diff/100001/ui/views/bubble/tray_b...
ui/views/bubble/tray_bubble_view.h:23: class TrayBubbleBorder;
On 2016/11/15 02:59:38, msw wrote:
> remove

Done.

https://codereview.chromium.org/2491033006/diff/100001/ui/views/bubble/tray_b...
ui/views/bubble/tray_bubble_view.h:128: // gfx::Rect GetAnchorRect() const
override;
On 2016/11/15 02:59:38, msw wrote:
> Remove

Done.

[Evan Stade, 2016-11-15 17:01:37]

The CQ bit was checked by estade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-15 17:02:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-15 18:45:41]

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

[commit-bot: I haz the power, 2016-11-15 18:45:42]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[msw, 2016-11-15 19:07:44]

lgtm

[sky, 2016-11-15 20:25:33]

LGTM

[Evan Stade, 2016-11-15 22:16:25]

The CQ bit was checked by estade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-15 22:17:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-15 22:41:33]

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

[commit-bot: I haz the power, 2016-11-15 22:41:36]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[Evan Stade, 2016-11-15 22:52:42]

Patchset #8 (id:140001) has been deleted

[Evan Stade, 2016-11-15 22:53:07]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2016-11-15 22:53:07]

The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org,
sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2491033006/#ps160001
(title: "disable test on windows")

[commit-bot: I haz the power, 2016-11-15 22:53:36]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-15 23:19:26]

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

[commit-bot: I haz the power, 2016-11-15 23:19:28]

Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[Evan Stade, 2016-11-15 23:56:37]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2016-11-15 23:57:08]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-16 00:16:02]

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

[commit-bot: I haz the power, 2016-11-16 00:16:05]

Failed to apply patch for ash/common/system/tray/system_tray.cc:
While running git apply --index -p1;
  error: patch failed: ash/common/system/tray/system_tray.cc:603
  error: ash/common/system/tray/system_tray.cc: patch does not apply

Patch:       ash/common/system/tray/system_tray.cc
Index: ash/common/system/tray/system_tray.cc
diff --git a/ash/common/system/tray/system_tray.cc
b/ash/common/system/tray/system_tray.cc
index
0dcd1ea32aed3fb47db5c8d10058149a4c71165f..a0b10465c5e0346a044a337bc0169dee0a9ce2cc
100644
--- a/ash/common/system/tray/system_tray.cc
+++ b/ash/common/system/tray/system_tray.cc
@@ -43,6 +43,7 @@
 #include "ui/events/event_constants.h"
 #include "ui/gfx/canvas.h"
 #include "ui/gfx/skia_util.h"
+#include "ui/message_center/message_center_style.h"
 #include "ui/views/border.h"
 #include "ui/views/controls/label.h"
 #include "ui/views/view.h"
@@ -127,15 +128,15 @@ class SystemBubbleWrapper {
   // Initializes the bubble view and creates |bubble_wrapper_|.
   void InitView(TrayBackgroundView* tray,
                 views::View* anchor,
+                const gfx::Insets& anchor_insets,
                 TrayBubbleView::InitParams* init_params,
                 bool is_persistent) {
     DCHECK(anchor);
     LoginStatus login_status =
         WmShell::Get()->system_tray_delegate()->GetUserLoginStatus();
     bubble_->InitView(anchor, login_status, init_params);
+    bubble_->bubble_view()->set_anchor_view_insets(anchor_insets);
     bubble_wrapper_.reset(new TrayBubbleWrapper(tray, bubble_->bubble_view()));
-    // The system bubble should not have an arrow.
-    bubble_->bubble_view()->SetArrowPaintType(views::BubbleBorder::PAINT_NONE);
     is_persistent_ = is_persistent;
 
     // If ChromeVox is enabled, focus the default item if no item is focused
and
@@ -347,13 +348,13 @@ const std::vector<SystemTrayItem*>&
SystemTray::GetTrayItems() const {
 }
 
 void SystemTray::ShowDefaultView(BubbleCreationType creation_type) {
-  ShowDefaultViewWithOffset(
-      creation_type, TrayBubbleView::InitParams::kArrowDefaultOffset, false);
+  if (creation_type != BUBBLE_USE_EXISTING)
+    WmShell::Get()->RecordUserMetricsAction(UMA_STATUS_AREA_MENU_OPENED);
+  ShowItems(items_.get(), false, true, creation_type, false);
 }
 
 void SystemTray::ShowPersistentDefaultView() {
-  ShowItems(items_.get(), false, false, BUBBLE_CREATE_NEW,
-            TrayBubbleView::InitParams::kArrowDefaultOffset, true);
+  ShowItems(items_.get(), false, false, BUBBLE_CREATE_NEW, true);
 }
 
 void SystemTray::ShowDetailedView(SystemTrayItem* item,
@@ -367,8 +368,7 @@ void SystemTray::ShowDetailedView(SystemTrayItem* item,
   bool persistent =
       (!activate && close_delay > 0 && creation_type == BUBBLE_CREATE_NEW);
   items.push_back(item);
-  ShowItems(items, true, activate, creation_type, GetTrayXOffset(item),
-            persistent);
+  ShowItems(items, true, activate, creation_type, persistent);
   if (system_bubble_)
     system_bubble_->bubble()->StartAutoCloseTimer(close_delay);
 }
@@ -529,41 +529,10 @@ base::string16 SystemTray::GetAccessibleNameForTray() {
                                     time, battery);
 }
 
-int SystemTray::GetTrayXOffset(SystemTrayItem* item) const {
-  // Don't attempt to align the arrow if the shelf is on the left or right.
-  if (!IsHorizontalAlignment(shelf_alignment()))
-    return TrayBubbleView::InitParams::kArrowDefaultOffset;
-
-  std::map<SystemTrayItem*, views::View*>::const_iterator it =
-      tray_item_map_.find(item);
-  if (it == tray_item_map_.end())
-    return TrayBubbleView::InitParams::kArrowDefaultOffset;
-
-  const views::View* item_view = it->second;
-  if (item_view->bounds().IsEmpty()) {
-    // The bounds of item could be still empty if it does not have a visible
-    // tray view. In that case, use the default (minimum) offset.
-    return TrayBubbleView::InitParams::kArrowDefaultOffset;
-  }
-
-  gfx::Point point(item_view->width() / 2, 0);
-  ConvertPointToWidget(item_view, &point);
-  return point.x();
-}
-
-void SystemTray::ShowDefaultViewWithOffset(BubbleCreationType creation_type,
-                                           int arrow_offset,
-                                           bool persistent) {
-  if (creation_type != BUBBLE_USE_EXISTING)
-    WmShell::Get()->RecordUserMetricsAction(UMA_STATUS_AREA_MENU_OPENED);
-  ShowItems(items_.get(), false, true, creation_type, arrow_offset,
persistent);
-}
-
 void SystemTray::ShowItems(const std::vector<SystemTrayItem*>& items,
                            bool detailed,
                            bool can_activate,
                            BubbleCreationType creation_type,
-                           int arrow_offset,
                            bool persistent) {
   // No system tray bubbles in kiosk mode.
   if (WmShell::Get()->system_tray_delegate()->GetUserLoginStatus() ==
@@ -603,32 +572,25 @@ void SystemTray::ShowItems(const
std::vector<SystemTrayItem*>& items,
             : kMinimumSystemTrayMenuWidth,
         WmShell::Get()->system_tray_delegate()->GetSystemTrayMenuWidth());
 
-    TrayBubbleView::InitParams init_params(TrayBubbleView::ANCHOR_TYPE_TRAY,
-                                           GetAnchorAlignment(), menu_width,
+    TrayBubbleView::InitParams init_params(GetAnchorAlignment(), menu_width,
                                            kTrayPopupMaxWidth);
     // TODO(oshima): Change TrayBubbleView itself.
     init_params.can_activate = false;
-    init_params.first_item_has_no_margin = true;
     if (detailed) {
       // This is the case where a volume control or brightness control bubble
       // is created.
       init_params.max_height = default_bubble_height_;
-      init_params.arrow_color = kBackgroundColor;
+      init_params.bg_color = kBackgroundColor;
     } else {
-      init_params.arrow_color = kHeaderBackgroundColor;
+      init_params.bg_color = kHeaderBackgroundColor;
     }
-    init_params.arrow_offset = arrow_offset;
     if (bubble_type == SystemTrayBubble::BUBBLE_TYPE_DEFAULT)
       init_params.close_on_deactivate = !persistent;
-    // For Volume and Brightness we don't want to show an arrow when
-    // they are shown in a bubble by themselves.
-    init_params.arrow_paint_type = views::BubbleBorder::PAINT_NORMAL;
-    if (items.size() == 1 && items[0]->ShouldHideArrow())
-      init_params.arrow_paint_type = views::BubbleBorder::PAINT_NONE;
     SystemTrayBubble* bubble = new SystemTrayBubble(this, items, bubble_type);
 
     system_bubble_.reset(new SystemBubbleWrapper(bubble));
-    system_bubble_->InitView(this, tray_container(), &init_params, persistent);
+    system_bubble_->InitView(this, GetBubbleAnchor(), GetBubbleAnchorInsets(),
+                             &init_params, persistent);
 
     activation_observer_.reset(persistent ? nullptr
                                           : new ActivationObserver(this));
@@ -661,6 +623,9 @@ void SystemTray::ShowItems(const
std::vector<SystemTrayItem*>& items,
     SetIsActive(true);
 }
 
+// TODO(estade): there's only one thing that triggers a notification bubble,
+// and that's TraySms. We could delete a lot of code in SystemTray if that
+// used the message center notifications instead. See crbug.com/630641
 void SystemTray::UpdateNotificationBubble() {
   // Only show the notification bubble if we have notifications.
   if (notification_items_.empty()) {
@@ -672,28 +637,25 @@ void SystemTray::UpdateNotificationBubble() {
   SystemTrayBubble* notification_bubble;
   notification_bubble = new SystemTrayBubble(
       this, notification_items_, SystemTrayBubble::BUBBLE_TYPE_NOTIFICATION);
-  views::View* anchor;
-  TrayBubbleView::AnchorType anchor_type;
   // Tray items might want to show notifications while we are creating and
   // initializing the |system_bubble_| - but it might not be fully initialized
   // when coming here - this would produce a crashed like crbug.com/247416.
   // As such we check the existence of the widget here.
+  TrayBubbleView::InitParams init_params(
+      GetAnchorAlignment(), kTrayPopupMinWidth, kTrayPopupMaxWidth);
+  views::View* anchor = GetBubbleAnchor();
+  gfx::Insets anchor_insets = GetBubbleAnchorInsets();
+  // If there's already a system menu bubble, stack this one on top.
   if (system_bubble_.get() && system_bubble_->bubble_view() &&
       system_bubble_->bubble_view()->GetWidget()) {
     anchor = system_bubble_->bubble_view();
-    anchor_type = TrayBubbleView::ANCHOR_TYPE_BUBBLE;
-  } else {
-    anchor = tray_container();
-    anchor_type = TrayBubbleView::ANCHOR_TYPE_TRAY;
+    anchor_insets.Set(-message_center::kMarginBetweenItems, 0, 0, 0);
+    init_params.anchor_alignment = TrayBubbleView::ANCHOR_ALIGNMENT_BOTTOM;
   }
-  TrayBubbleView::InitParams init_params(anchor_type, GetAnchorAlignment(),
-                                         kTrayPopupMinWidth,
-                                         kTrayPopupMaxWidth);
-  init_params.first_item_has_no_margin = true;
-  init_params.arrow_color = kBackgroundColor;
-  init_params.arrow_offset = GetTrayXOffset(notification_items_[0]);
+  init_params.bg_color = kBackgroundColor;
   notification_bubble_.reset(new SystemBubbleWrapper(notification_bubble));
-  notification_bubble_->InitView(this, anchor, &init_params, false);
+  notification_bubble_->InitView(this, anchor, anchor_insets, &init_params,
+                                 false);
 
   if (notification_bubble->bubble_view()->child_count() == 0) {
     // It is possible that none of the items generated actual notifications.
@@ -806,13 +768,6 @@ base::string16 SystemTray::GetAccessibleNameForBubble() {
   return GetAccessibleNameForTray();
 }
 
-gfx::Rect SystemTray::GetAnchorRect(
-    views::Widget* anchor_widget,
-    TrayBubbleView::AnchorType anchor_type,
-    TrayBubbleView::AnchorAlignment anchor_alignment) const {
-  return GetBubbleAnchorRect(anchor_widget, anchor_type, anchor_alignment);
-}
-
 void SystemTray::OnBefo…
(message too large)

[Mr4D (OOO till 08-26), 2016-11-16 00:25:53]

lgtm

[Evan Stade, 2016-11-16 01:25:10]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2016-11-16 01:25:17]

The CQ bit was unchecked by estade@chromium.org

[Evan Stade, 2016-11-16 03:47:17]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2016-11-16 03:47:18]

The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org,
skuhne@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2491033006/#ps180001
(title: "rebase")

[commit-bot: I haz the power, 2016-11-16 03:48:07]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-16 05:49:20]

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

[commit-bot: I haz the power, 2016-11-16 05:49:22]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Evan Stade, 2016-11-16 18:00:24]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2016-11-16 18:01:19]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-16 18:34:01]

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

[commit-bot: I haz the power, 2016-11-16 18:34:04]

Failed to apply patch for ash/common/system/chromeos/audio/tray_audio.cc:
While running git apply --index -p1;
  error: patch failed: ash/common/system/chromeos/audio/tray_audio.cc:92
  error: ash/common/system/chromeos/audio/tray_audio.cc: patch does not apply

Patch:       ash/common/system/chromeos/audio/tray_audio.cc
Index: ash/common/system/chromeos/audio/tray_audio.cc
diff --git a/ash/common/system/chromeos/audio/tray_audio.cc
b/ash/common/system/chromeos/audio/tray_audio.cc
index
db597c0a9b6f7c89e8985facdab10b24a0b876d4..a203026ed54fa43a0fe1ab24fba1c2ad99525753
100644
--- a/ash/common/system/chromeos/audio/tray_audio.cc
+++ b/ash/common/system/chromeos/audio/tray_audio.cc
@@ -92,10 +92,6 @@ void TrayAudio::DestroyDetailedView() {
   }
 }
 
-bool TrayAudio::ShouldHideArrow() const {
-  return true;
-}
-
 bool TrayAudio::ShouldShowShelf() const {
   return !pop_up_volume_view_;
 }

[commit-bot: I haz the power, 2016-11-16 18:52:46]

Description was changed from

==========
Adjust positioning of cros tray bubbles.

This consolidates the anchoring logic of all tray bubbles. In the common
case (bottom aligned, LTR) this is right bubble edge to right button 
edge, and vertically, flushed to the tray (i.e. the area that turns 
black when you maximize a window).

There are never arrows on tray bubbles these days, so a ton of arrow-
related logic is removed from TrayBubbleView.

Since all bubbles position themselves the same way (and because 
set_anchor_view_insets is flexible enough to cover any differences), 
there's no need to delegate GetAnchorRect any more. Many other settings 
such as the shadow are also not needed since they're always the same.

Trying to maintain pre-MD behavior and have both coexist would be a 
mess, so the change is not gated on the MD flag. However, it works fine 
in both worlds.

BUG=663016
==========

to

==========
Adjust positioning of cros tray bubbles.

This consolidates the anchoring logic of all tray bubbles. In the common
case (bottom aligned, LTR) this is right bubble edge to right button
edge, and vertically, flushed to the tray (i.e. the area that turns
black when you maximize a window).

There are never arrows on tray bubbles these days, so a ton of arrow-
related logic is removed from TrayBubbleView.

Since all bubbles position themselves the same way (and because
set_anchor_view_insets is flexible enough to cover any differences),
there's no need to delegate GetAnchorRect any more. Many other settings
such as the shadow are also not needed since they're always the same.

Trying to maintain pre-MD behavior and have both coexist would be a
mess, so the change is not gated on the MD flag. However, it works fine
in both worlds.

BUG=663016

Committed: https://crrev.com/3acb088db0632abae388129760f33bdf5fbe09a2
Cr-Commit-Position: refs/heads/master@{#432567}
==========

[commit-bot: I haz the power, 2016-11-16 18:52:47]

Patchset 9 (id:??) landed as
https://crrev.com/3acb088db0632abae388129760f33bdf5fbe09a2
Cr-Commit-Position: refs/heads/master@{#432567}