More flexibility in BubbleBorder arrow rendering.

More flexibility in BubbleBorder arrow rendering.

Currently causing the arrow not to be painted leaves a
margin of several pixels on the side the arrow would be
painted, causing issues when laying out bubbles next to an
anchor rectangle such as the edge of the screen.

This patch changes the API so that you can choose whether
the arrow is painted normally, there is space left for the
arrow but it is not painted, or there is no space left for
the arrow and it is not painted.

r=msw@chromium.org,sadrul@chromium.org,ben@chromium.org

BUG=NONE

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

[dewittj, 2013-02-21 01:46:37]

Please take a look.

[msw, 2013-02-21 07:29:29]

LGTM with nits. I'll look again if you add tests like you mentioned offline.
These aren't truly BubbleBorder tests, but they might be a good starting point:
ui/views/bubble/bubble_frame_view_unittest.cc

https://codereview.chromium.org/12310022/diff/2001/ash/system/tray/system_tra...
File ash/system/tray/system_tray.cc (right):

https://codereview.chromium.org/12310022/diff/2001/ash/system/tray/system_tra...
ash/system/tray/system_tray.cc:379: if (items.size() == 1 &&
items[0]->ShouldHideArrow()) {
nit: remove braces.

https://codereview.chromium.org/12310022/diff/2001/ash/system/tray/tray_backg...
File ash/system/tray/tray_background_view.cc (right):

https://codereview.chromium.org/12310022/diff/2001/ash/system/tray/tray_backg...
ash/system/tray/tray_background_view.cc:391: if (shelf->IsVisible())
nit: move the condition into the call as a ternary.

https://codereview.chromium.org/12310022/diff/2001/ui/views/bubble/bubble_bor...
File ui/views/bubble/bubble_border.h (right):

https://codereview.chromium.org/12310022/diff/2001/ui/views/bubble/bubble_bor...
ui/views/bubble/bubble_border.h:68: // The way the arrow should be painted.
nit: describe values meanings like your awesome cl description text.

[dewittj, 2013-02-21 19:13:57]

https://codereview.chromium.org/12310022/diff/2001/ash/system/tray/system_tra...
File ash/system/tray/system_tray.cc (right):

https://codereview.chromium.org/12310022/diff/2001/ash/system/tray/system_tra...
ash/system/tray/system_tray.cc:379: if (items.size() == 1 &&
items[0]->ShouldHideArrow()) {
On 2013/02/21 07:29:29, msw wrote:
> nit: remove braces.

Done.
This is my Chromium style weakness.  Sorry!

https://codereview.chromium.org/12310022/diff/2001/ash/system/tray/tray_backg...
File ash/system/tray/tray_background_view.cc (right):

https://codereview.chromium.org/12310022/diff/2001/ash/system/tray/tray_backg...
ash/system/tray/tray_background_view.cc:391: if (shelf->IsVisible())
On 2013/02/21 07:29:29, msw wrote:
> nit: move the condition into the call as a ternary.

Done.

https://codereview.chromium.org/12310022/diff/2001/ui/views/bubble/bubble_bor...
File ui/views/bubble/bubble_border.h (right):

https://codereview.chromium.org/12310022/diff/2001/ui/views/bubble/bubble_bor...
ui/views/bubble/bubble_border.h:68: // The way the arrow should be painted.
On 2013/02/21 07:29:29, msw wrote:
> nit: describe values meanings like your awesome cl description text.

Done.

[sadrul, 2013-02-21 21:16:36]

LGTM

It's weird that we need a mode that has "space left for the arrow but it is not
painted". Can we get rid of that, and either have the space and draw the arrow,
or not have the space when not drawing the arrow?

[dewittj, 2013-02-22 22:50:50]

On 2013/02/21 21:16:36, sadrul wrote:
> LGTM
> 
> It's weird that we need a mode that has "space left for the arrow but it is
not
> painted". Can we get rid of that, and either have the space and draw the
arrow,
> or not have the space when not drawing the arrow?

I left it because the way that system tray bubbles are currently implemented,
the TrayBubbleView is always anchored on an inset version of the system tray UI,
regardless of whether the arrow is rendered.  It works out that this causes nice
smooth sliding when the taskbar is hidden or re-shown.  It would be possible to
remove this feature but in my basic tests there was a little bit of jumping
around when the anchor had to change at auto-hide time.

[dewittj, 2013-02-23 01:44:11]

sadrul: would you like me to hold off on committing this change to investigate
modifying TrayBackgroundView and friends not to depend on the invisible-arrow
behavior, or can I go ahead?

[sadrul, 2013-02-23 04:36:49]

On 2013/02/23 01:44:11, dewittj wrote:
> sadrul: would you like me to hold off on committing this change to investigate
> modifying TrayBackgroundView and friends not to depend on the invisible-arrow
> behavior, or can I go ahead?

Oh no. That is already the current behaviour, and this CL is about fixing
another issue. I would like to see that fixed at some point, but this CL can
land as it is. (that's why I lgtm'ed, but I wasn't clear enough. Sorry about the
confusion)

[commit-bot: I haz the power, 2013-02-23 15:49:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dewittj@chromium.org/12310022/3009

[commit-bot: I haz the power, 2013-02-23 15:49:49]

Presubmit check for 12310022-3009 failed and returned exit status 1.

INFO:root:Found 7 file(s).

Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/ash/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/browser/ui/views/PRESUBMIT.py

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/browser/ui/views

Presubmit checks took 1.3s to calculate.

[dewittj, 2013-02-24 17:13:34]

ben: Please could you review
c/b/ui/views/message_center/notification_bubble_wrapper_win.cc

[Ben Goodger (Google), 2013-02-24 23:56:51]

lgtm

[commit-bot: I haz the power, 2013-02-25 18:31:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dewittj@chromium.org/12310022/3009

[commit-bot: I haz the power, 2013-02-25 21:48:21]

Change committed as 184487