Fix position of web notification bubble and arrow

Fix position of web notification bubble and arrow 
* Move GetAnchorRect into TrayBubbleView 
* Remove double RTL inversion and modify arrow offset logic 
* Add rtl_mirrored to BubbleBorder to support unmirrored rtl bubbles
* Correctly align notifications and rebuild on shelf alignment change.


BUG=137154
TEST=tests pass; alignment for web notifications matches the system tray bubble and works correctly on left and right launcher and wirth RTL.

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

[stevenjb, 2012-07-23 21:19:23]

Ready for review.
This fixes a lot of issues with the web notification tray, and a few edge cases
with the system tray with side launcher + RTL.

[msw, 2012-07-23 22:20:48]

Can you explain what this CL is trying to accomplish with the mirroring of
bubble margins/arrows? It may be my lack of understanding, but this seems to
make the code itself unnecessarily more complex.

http://codereview.chromium.org/10808066/diff/8001/ash/system/tray/tray_bubble...
File ash/system/tray/tray_bubble_view.cc (right):

http://codereview.chromium.org/10808066/diff/8001/ash/system/tray/tray_bubble...
ash/system/tray/tray_bubble_view.cc:375: views::BubbleFrameView*
bubble_frame_view =
Instead of changing the BubbleFrameView construction pattern, why not just call
GetBubbleFrameView()->SetBubbleBorder() with the custom bubble border?

http://codereview.chromium.org/10808066/diff/8001/ui/views/bubble/bubble_bord...
File ui/views/bubble/bubble_border.cc (right):

http://codereview.chromium.org/10808066/diff/8001/ui/views/bubble/bubble_bord...
ui/views/bubble/bubble_border.cc:178: // Insets will get mirrored in the View
code for normal layout. If
Don't you need to update arrow_location() to conditionally suppress flipping the
actual arrow location for |rtl_mirrored_|? Otherwise, it seems like this will
keep the insets but still flip the arrow.

http://codereview.chromium.org/10808066/diff/8001/ui/views/bubble/bubble_bord...
ui/views/bubble/bubble_border.cc:187: void
BubbleBorder::GetInsetsForArrowLocation(gfx::Insets* insets,
Can you merge this function back into GetInsets() and just build the conditional
arrow mirroring logic into a new GetArrowLocation() that replaces
arrow_location()?

http://codereview.chromium.org/10808066/diff/8001/ui/views/bubble/bubble_bord...
File ui/views/bubble/bubble_border.h (right):

http://codereview.chromium.org/10808066/diff/8001/ui/views/bubble/bubble_bord...
ui/views/bubble/bubble_border.h:126: // Specifies the arrow location for
calculating insets (for rtl mirroring).
I don't understand this comment, nor do I see why this function is worthwhile.

http://codereview.chromium.org/10808066/diff/8001/ui/views/bubble/bubble_bord...
ui/views/bubble/bubble_border.h:176: bool rtl_mirrored_;
nit: rename for clarity |mirror_in_rtl_| (or similar); specify default value.

http://codereview.chromium.org/10808066/diff/8001/ui/views/bubble/bubble_dele...
File ui/views/bubble/bubble_delegate.cc (right):

http://codereview.chromium.org/10808066/diff/8001/ui/views/bubble/bubble_dele...
ui/views/bubble/bubble_delegate.cc:184: BubbleFrameView* frame_view = new
BubbleFrameView(margins());
What's wrong with the current construction pattern?

http://codereview.chromium.org/10808066/diff/8001/ui/views/bubble/bubble_dele...
ui/views/bubble/bubble_delegate.cc:187: arrow_loc =
BubbleBorder::horizontal_mirror(arrow_loc);
Why add this new arrow mirroring here?

http://codereview.chromium.org/10808066/diff/8001/ui/views/bubble/bubble_fram...
File ui/views/bubble/bubble_frame_view_unittest.cc (right):

http://codereview.chromium.org/10808066/diff/8001/ui/views/bubble/bubble_fram...
ui/views/bubble/bubble_frame_view_unittest.cc:80: TestBubbleFrameView
frame(gfx::Rect(0, 0, 1000, 1000));
Why change the margins from kDefaultMargin?

[stevenjb, 2012-07-23 23:32:06]

I agree that the code is very (and possibly unnecessarily) complex. 

I'll try to take another pass at simplifying the changes, particularly to the
ui/views/bubble code, but much of the existing code is pretty challenging to
parse and debug, especially trying to follow what's going on with RTL mirroring.

http://codereview.chromium.org/10808066/diff/8001/ash/system/tray/tray_bubble...
File ash/system/tray/tray_bubble_view.cc (right):

http://codereview.chromium.org/10808066/diff/8001/ash/system/tray/tray_bubble...
ash/system/tray/tray_bubble_view.cc:375: views::BubbleFrameView*
bubble_frame_view =
On 2012/07/23 22:20:49, msw wrote:
> Instead of changing the BubbleFrameView construction pattern, why not just
call
> GetBubbleFrameView()->SetBubbleBorder() with the custom bubble border?

The problem was that first a BubbleBorder() was getting constructed and then a
BubbleBackground depending on that BubbleBorder. In addition to being extra
work, this could cause a subtle crash if GetBubbleFrameView()->set_background()
was not also called, resulting in an obscure extra call that cause me a great
deal of confusion while debugging this.

http://codereview.chromium.org/10808066/diff/8001/ui/views/bubble/bubble_bord...
File ui/views/bubble/bubble_border.cc (right):

http://codereview.chromium.org/10808066/diff/8001/ui/views/bubble/bubble_bord...
ui/views/bubble/bubble_border.cc:178: // Insets will get mirrored in the View
code for normal layout. If
On 2012/07/23 22:20:49, msw wrote:
> Don't you need to update arrow_location() to conditionally suppress flipping
the
> actual arrow location for |rtl_mirrored_|? Otherwise, it seems like this will
> keep the insets but still flip the arrow.
See comment below.

http://codereview.chromium.org/10808066/diff/8001/ui/views/bubble/bubble_bord...
ui/views/bubble/bubble_border.cc:187: void
BubbleBorder::GetInsetsForArrowLocation(gfx::Insets* insets,
On 2012/07/23 22:20:49, msw wrote:
> Can you merge this function back into GetInsets() and just build the
conditional
> arrow mirroring logic into a new GetArrowLocation() that replaces
> arrow_location()?

The problem is that most of the code in BubbleBorder needs arrow_location to be
mirrored (as set in BubbleDelegateView::CreateNonClientFrameView, previously set
in BubbleFrameView::BubbleFrameView). TrayBubbleBorder::Paint however needs it
not to be mirrored.

http://codereview.chromium.org/10808066/diff/8001/ui/views/bubble/bubble_bord...
File ui/views/bubble/bubble_border.h (right):

http://codereview.chromium.org/10808066/diff/8001/ui/views/bubble/bubble_bord...
ui/views/bubble/bubble_border.h:126: // Specifies the arrow location for
calculating insets (for rtl mirroring).
On 2012/07/23 22:20:49, msw wrote:
> I don't understand this comment, nor do I see why this function is worthwhile.

It gets used in ash::TrayBubbleBorder::Paint. I can try to clarify the comment,
but basically it allows that code to get the correct insets without the extra
mirroring.

http://codereview.chromium.org/10808066/diff/8001/ui/views/bubble/bubble_dele...
File ui/views/bubble/bubble_delegate.cc (right):

http://codereview.chromium.org/10808066/diff/8001/ui/views/bubble/bubble_dele...
ui/views/bubble/bubble_delegate.cc:187: arrow_loc =
BubbleBorder::horizontal_mirror(arrow_loc);
On 2012/07/23 22:20:49, msw wrote:
> Why add this new arrow mirroring here?
This was previously in the constructor.

http://codereview.chromium.org/10808066/diff/8001/ui/views/bubble/bubble_fram...
File ui/views/bubble/bubble_frame_view_unittest.cc (right):

http://codereview.chromium.org/10808066/diff/8001/ui/views/bubble/bubble_fram...
ui/views/bubble/bubble_frame_view_unittest.cc:80: TestBubbleFrameView
frame(gfx::Rect(0, 0, 1000, 1000));
On 2012/07/23 22:20:49, msw wrote:
> Why change the margins from kDefaultMargin?
TestBubbleFrameView sets the insets using kDefaultMargin. This rect is the
monitor bounds, unimportant in this test, copied from other TestBubbleFrameView
constructors. I'll replace the rect with something like kDefaultMonitorBounds.

[msw, 2012-07-24 00:32:43]

I guess I'm still unclear why the arrow must be flipped in all but that one
instance; it'd be nice to deprecate that duplicate arrow painting code
altogether. Any cleanup/extension of the Bubble[Border] code to support that use
case is greatly appreciated!

http://codereview.chromium.org/10808066/diff/8001/ash/system/tray/tray_bubble...
File ash/system/tray/tray_bubble_view.cc (right):

http://codereview.chromium.org/10808066/diff/8001/ash/system/tray/tray_bubble...
ash/system/tray/tray_bubble_view.cc:139: // Draw the arrow after drawing child
borders, so that the arrow can cover
This should be calling the default implementation of BubbleBorder::Paint() (or
an extracted subset) after drawing the custom pieces (a blurred shadow and
'bottom line', afaict). If you need a custom arrow offset, try
BubbleBorder::SetArrowOffset. I'd prefer to deprecate this duplicated arrow
drawing code (the views Bubble code is well tested and maintained, we just had
some nice pixel-perfect painting adjustments).

http://codereview.chromium.org/10808066/diff/8001/ash/system/tray/tray_bubble...
ash/system/tray/tray_bubble_view.cc:140: // the its overlap section with child
border.
grammar nit: "the its"

http://codereview.chromium.org/10808066/diff/8001/ash/system/tray/tray_bubble...
ash/system/tray/tray_bubble_view.cc:375: views::BubbleFrameView*
bubble_frame_view =
On 2012/07/23 23:32:06, stevenjb (chromium) wrote:
> On 2012/07/23 22:20:49, msw wrote:
> > Instead of changing the BubbleFrameView construction pattern, why not just
> call
> > GetBubbleFrameView()->SetBubbleBorder() with the custom bubble border?
> 
> The problem was that first a BubbleBorder() was getting constructed and then a
> BubbleBackground depending on that BubbleBorder. In addition to being extra
> work, this could cause a subtle crash if
GetBubbleFrameView()->set_background()
> was not also called, resulting in an obscure extra call that cause me a great
> deal of confusion while debugging this.

Ah, that's fair. Consider passing a BubbleBorder instance in the BubbleFrameView
ctor for it to own.

http://codereview.chromium.org/10808066/diff/8001/ui/views/bubble/bubble_bord...
File ui/views/bubble/bubble_border.cc (right):

http://codereview.chromium.org/10808066/diff/8001/ui/views/bubble/bubble_bord...
ui/views/bubble/bubble_border.cc:187: void
BubbleBorder::GetInsetsForArrowLocation(gfx::Insets* insets,
On 2012/07/23 23:32:06, stevenjb (chromium) wrote:
> On 2012/07/23 22:20:49, msw wrote:
> > Can you merge this function back into GetInsets() and just build the
> conditional
> > arrow mirroring logic into a new GetArrowLocation() that replaces
> > arrow_location()?
> 
> The problem is that most of the code in BubbleBorder needs arrow_location to
be
> mirrored (as set in BubbleDelegateView::CreateNonClientFrameView, previously
set
> in BubbleFrameView::BubbleFrameView). TrayBubbleBorder::Paint however needs it
> not to be mirrored.
>  

I guess I'm unfamiliar with the use case, but this sounds odd to me. Why should
the arrow be mirrored everywhere except in that one instance? See my suggestion
there to de-duplicate the arrow painting code.

http://codereview.chromium.org/10808066/diff/8001/ui/views/bubble/bubble_bord...
File ui/views/bubble/bubble_border.h (right):

http://codereview.chromium.org/10808066/diff/8001/ui/views/bubble/bubble_bord...
ui/views/bubble/bubble_border.h:126: // Specifies the arrow location for
calculating insets (for rtl mirroring).
On 2012/07/23 23:32:06, stevenjb (chromium) wrote:
> On 2012/07/23 22:20:49, msw wrote:
> > I don't understand this comment, nor do I see why this function is
worthwhile.
> 
> It gets used in ash::TrayBubbleBorder::Paint. I can try to clarify the
comment,
> but basically it allows that code to get the correct insets without the extra
> mirroring.

Hmm, hopefully we'll end up with a single point of truth for the RTL arrow
mirroring and resulting Insets and this won't be needed.

http://codereview.chromium.org/10808066/diff/8001/ui/views/bubble/bubble_fram...
File ui/views/bubble/bubble_frame_view_unittest.cc (right):

http://codereview.chromium.org/10808066/diff/8001/ui/views/bubble/bubble_fram...
ui/views/bubble/bubble_frame_view_unittest.cc:80: TestBubbleFrameView
frame(gfx::Rect(0, 0, 1000, 1000));
On 2012/07/23 23:32:06, stevenjb (chromium) wrote:
> On 2012/07/23 22:20:49, msw wrote:
> > Why change the margins from kDefaultMargin?
> TestBubbleFrameView sets the insets using kDefaultMargin. This rect is the
> monitor bounds, unimportant in this test, copied from other
TestBubbleFrameView
> constructors. I'll replace the rect with something like kDefaultMonitorBounds.

Ah, you're right, thanks.

[stevenjb, 2012-07-26 23:48:30]

NOTE: I have a separate CL that extracts some of the many changes in ash/system,
so I would really only like signoff on the ui/vies/bubble changes for this CL.
I've left the other changes in for context, and will re-upload just the Bubble
changes in a new CL if we're OK with this approach (for now)...

I spent the last couple of days trying to unify TrayBubbleBorder and
BubbleBorder. I got frustratingly close, but could not get past one problem that
appears to be in the Bubble code when IsRTL is true:

When BubbleDelegateView::GetWidget()->non_client_view_->client_view_ gets laid
out initially, BubbleBorder.arrow_location_ is mirrored (in BubbleFrameView() in
the IsRTL check, now moved to BubbleDelegateView::CreateNonClientFrameView()).
During the layout, BubbleBorder::GetBounds() returns the mirrored bounds.

At some point *after* the ClientView bounds get set,
BubbleDelegateView::GetBubbleBounds() gets called from SizeToContents(). This
calls BubbleFrameView::GetUpdatedWindowBounds() with try_mirroring_arrow=true.
This will *unmirror* the bubble so that it is now on screen, and change
BubbleBorder.arrow_location_.

That will *not* trigger a bounds change for the ClientView. However, if a layout
for the bubble *does* trigger a bounds change for ClientView (as is the case for
WebNotificationTray when a notification gets added), the fact that
BubbleBorder.arrow_location_ is now unmirrored causes the layout to be subtly
incorrect.

I would absolutely love to fix this and write the tests to support that. However
this could take a while to do properly and test thoroughly. I've got a bunch of
other things on my plate and need to check in with other folks to see if I can
prioritize that.

In the meanwhile, I tried to minimize the changes to ui/views/bubble, and
improve the comments in TrayBubbleView. I am hoping that this will be tolerable
for now.

http://codereview.chromium.org/10808066/diff/8001/ash/system/tray/tray_bubble...
File ash/system/tray/tray_bubble_view.cc (right):

http://codereview.chromium.org/10808066/diff/8001/ash/system/tray/tray_bubble...
ash/system/tray/tray_bubble_view.cc:140: // the its overlap section with child
border.
On 2012/07/24 00:32:44, msw wrote:
> grammar nit: "the its"

Done.

http://codereview.chromium.org/10808066/diff/8001/ash/system/tray/tray_bubble...
ash/system/tray/tray_bubble_view.cc:375: views::BubbleFrameView*
bubble_frame_view =
On 2012/07/24 00:32:44, msw wrote:
> On 2012/07/23 23:32:06, stevenjb (chromium) wrote:
> > On 2012/07/23 22:20:49, msw wrote:
> > > Instead of changing the BubbleFrameView construction pattern, why not just
> > call
> > > GetBubbleFrameView()->SetBubbleBorder() with the custom bubble border?
> > 
> > The problem was that first a BubbleBorder() was getting constructed and then
a
> > BubbleBackground depending on that BubbleBorder. In addition to being extra
> > work, this could cause a subtle crash if
> GetBubbleFrameView()->set_background()
> > was not also called, resulting in an obscure extra call that cause me a
great
> > deal of confusion while debugging this.
> 
> Ah, that's fair. Consider passing a BubbleBorder instance in the
BubbleFrameView
> ctor for it to own.

Done.

http://codereview.chromium.org/10808066/diff/8001/ui/views/bubble/bubble_bord...
File ui/views/bubble/bubble_border.h (right):

http://codereview.chromium.org/10808066/diff/8001/ui/views/bubble/bubble_bord...
ui/views/bubble/bubble_border.h:176: bool rtl_mirrored_;
On 2012/07/23 22:20:49, msw wrote:
> nit: rename for clarity |mirror_in_rtl_| (or similar); specify default value.
Removed this and orverrode SetBounds() instead, moving logic to TrayBubbleView.

[msw, 2012-07-27 20:32:04]

Thanks for cleaning up the Bubble changes and attempting to consolidate the
arrow/border code etc. (hopefully that will eventually happen). I'm sympathetic
to your frustration the complexity and nuances here; if you can believe it, the
bubble code used to be much worse.

Does it make sense to split up this CL? There's a lot going on and some changes
seem tangential to one another. Please update the CL description.

ui/views/bubble/* LGTM with nits (and updated CL description), but I'm not
familiar with the Ash code, so hopefully another reviewer can offer a more
substantial review there. Thanks!

http://codereview.chromium.org/10808066/diff/21001/ash/system/tray/system_tra...
File ash/system/tray/system_tray.cc (right):

http://codereview.chromium.org/10808066/diff/21001/ash/system/tray/system_tra...
ash/system/tray/system_tray.cc:380: gfx::Rect rect(item_view->width() / 2, 0, 0,
0);
nit: use a Point and ConvertPointToWidget; assign the converted value to the
same local point.

http://codereview.chromium.org/10808066/diff/21001/ash/system/tray/system_tra...
ash/system/tray/system_tray.cc:382: if (!item_view->bounds().IsEmpty())
Invert the check and make this an early return of kArrowDefaultOffset
(potentially combine with line 376/377 above). If the bounds are empty,
converting the point is unnecessary.

http://codereview.chromium.org/10808066/diff/21001/ash/system/tray/system_tra...
ash/system/tray/system_tray.cc:520: // Destroy an existing bubble so that it is
rebuilt correctly.
nit: not sure if you meant to write "any".

http://codereview.chromium.org/10808066/diff/21001/ash/system/tray/system_tra...
ash/system/tray/system_tray.cc:541: gfx::Rect bounds(located_event.x(), 0, 0,
0);
nit: use a Point and ConvertPointToWidget.

http://codereview.chromium.org/10808066/diff/21001/ash/system/tray/tray_bubbl...
File ash/system/tray/tray_bubble_view.cc (right):

http://codereview.chromium.org/10808066/diff/21001/ash/system/tray/tray_bubbl...
ash/system/tray/tray_bubble_view.cc:155: int tip_x;
nit: init to 0? Same for tip_y below.

http://codereview.chromium.org/10808066/diff/21001/ash/system/tray/tray_bubbl...
ash/system/tray/tray_bubble_view.cc:255: views::BubbleBorder::ArrowLocation
arrow_location;
nit: init arrow_location to some value.

http://codereview.chromium.org/10808066/diff/21001/ash/system/tray/tray_bubbl...
File ash/system/tray/tray_bubble_view.h (right):

http://codereview.chromium.org/10808066/diff/21001/ash/system/tray/tray_bubbl...
ash/system/tray/tray_bubble_view.h:38: void PostCreateBubble(views::Widget*
widget,
Add a comment; consider renaming InitBubble (naturally follows CreateBubble) or
ShowBubble (a significant step within this function) or similar?

http://codereview.chromium.org/10808066/diff/21001/ash/system/tray/tray_bubbl...
ash/system/tray/tray_bubble_view.h:88: // Called whenever the bubble size or
location may have moved.
nit: s/moved/changed/

http://codereview.chromium.org/10808066/diff/21001/ash/system/web_notificatio...
File ash/system/web_notification/web_notification_tray.cc (right):

http://codereview.chromium.org/10808066/diff/21001/ash/system/web_notificatio...
ash/system/web_notification/web_notification_tray.cc:345: int padding_width =
kTrayPopupPaddingHorizontal/2;
nit: const

http://codereview.chromium.org/10808066/diff/21001/ash/system/web_notificatio...
ash/system/web_notification/web_notification_tray.cc:357: int message_width =
kWebNotificationWidth - kWebNotificationIconSize -
nit: const

http://codereview.chromium.org/10808066/diff/21001/ash/system/web_notificatio...
ash/system/web_notification/web_notification_tray.cc:566: bounds =
anchor->ConvertRectToWidget(bounds);
nit: Use Point.

http://codereview.chromium.org/10808066/diff/21001/ui/views/bubble/bubble_bor...
File ui/views/bubble/bubble_border.h (right):

http://codereview.chromium.org/10808066/diff/21001/ui/views/bubble/bubble_bor...
ui/views/bubble/bubble_border.h:126: // Specifies the arrow location for
calculating insets. Normally called from
nit: wording, consider "Calculate the insets for a specific arrow location."

http://codereview.chromium.org/10808066/diff/21001/ui/views/bubble/bubble_del...
File ui/views/bubble/bubble_delegate.cc (right):

http://codereview.chromium.org/10808066/diff/21001/ui/views/bubble/bubble_del...
ui/views/bubble/bubble_delegate.cc:189: BubbleBorder* bubble_border =
nit: rename border to fit on one line.

[stevenjb, 2012-07-30 20:09:03]

I split some of the unrelated changes into
http://codereview.chromium.org/10833041/.

I was going to split out the ui/views/bubble changes, but with the other CL
removed I think this is now reasonable as-is.

msw: Thanks a ton for the detailed review, hopefully I addressed everything.

jennyz: This is ready for review now.

sadrul: I'll need an OWNER lg also.

http://codereview.chromium.org/10808066/diff/21001/ash/system/tray/system_tra...
File ash/system/tray/system_tray.cc (right):

http://codereview.chromium.org/10808066/diff/21001/ash/system/tray/system_tra...
ash/system/tray/system_tray.cc:380: gfx::Rect rect(item_view->width() / 2, 0, 0,
0);
On 2012/07/27 20:32:04, msw wrote:
> nit: use a Point and ConvertPointToWidget; assign the converted value to the
> same local point.

Done.

http://codereview.chromium.org/10808066/diff/21001/ash/system/tray/system_tra...
ash/system/tray/system_tray.cc:382: if (!item_view->bounds().IsEmpty())
Good call, done.

http://codereview.chromium.org/10808066/diff/21001/ash/system/tray/system_tra...
ash/system/tray/system_tray.cc:520: // Destroy an existing bubble so that it is
rebuilt correctly.
On 2012/07/27 20:32:04, msw wrote:
> nit: not sure if you meant to write "any".

Done.

http://codereview.chromium.org/10808066/diff/21001/ash/system/tray/system_tra...
ash/system/tray/system_tray.cc:541: gfx::Rect bounds(located_event.x(), 0, 0,
0);
On 2012/07/27 20:32:04, msw wrote:
> nit: use a Point and ConvertPointToWidget.

Done.

http://codereview.chromium.org/10808066/diff/21001/ash/system/tray/tray_bubbl...
File ash/system/tray/tray_bubble_view.cc (right):

http://codereview.chromium.org/10808066/diff/21001/ash/system/tray/tray_bubbl...
ash/system/tray/tray_bubble_view.cc:155: int tip_x;
On 2012/07/27 20:32:04, msw wrote:
> nit: init to 0? Same for tip_y below.
I actually prefer not to initialize this to allow the compiler to catch any
cases where it might accidentally be used uninitialized.

http://codereview.chromium.org/10808066/diff/21001/ash/system/tray/tray_bubbl...
ash/system/tray/tray_bubble_view.cc:255: views::BubbleBorder::ArrowLocation
arrow_location;
On 2012/07/27 20:32:04, msw wrote:
> nit: init arrow_location to some value.
Same as above - we don't want to accidentally skip a case and unintentionally
use an arbitrary default value.

http://codereview.chromium.org/10808066/diff/21001/ash/system/tray/tray_bubbl...
File ash/system/tray/tray_bubble_view.h (right):

http://codereview.chromium.org/10808066/diff/21001/ash/system/tray/tray_bubbl...
ash/system/tray/tray_bubble_view.h:38: void PostCreateBubble(views::Widget*
widget,
Good suggestion, added comment and renamed InitializeAndShowBubble to be
explicit.

http://codereview.chromium.org/10808066/diff/21001/ash/system/tray/tray_bubbl...
ash/system/tray/tray_bubble_view.h:88: // Called whenever the bubble size or
location may have moved.
On 2012/07/27 20:32:04, msw wrote:
> nit: s/moved/changed/

Done.

http://codereview.chromium.org/10808066/diff/21001/ash/system/web_notificatio...
File ash/system/web_notification/web_notification_tray.cc (right):

http://codereview.chromium.org/10808066/diff/21001/ash/system/web_notificatio...
ash/system/web_notification/web_notification_tray.cc:345: int padding_width =
kTrayPopupPaddingHorizontal/2;
On 2012/07/27 20:32:04, msw wrote:
> nit: const

Done.

http://codereview.chromium.org/10808066/diff/21001/ash/system/web_notificatio...
ash/system/web_notification/web_notification_tray.cc:357: int message_width =
kWebNotificationWidth - kWebNotificationIconSize -
On 2012/07/27 20:32:04, msw wrote:
> nit: const

Done.

http://codereview.chromium.org/10808066/diff/21001/ash/system/web_notificatio...
ash/system/web_notification/web_notification_tray.cc:566: bounds =
anchor->ConvertRectToWidget(bounds);
On 2012/07/27 20:32:04, msw wrote:
> nit: Use Point.

Done.

http://codereview.chromium.org/10808066/diff/21001/ui/views/bubble/bubble_bor...
File ui/views/bubble/bubble_border.h (right):

http://codereview.chromium.org/10808066/diff/21001/ui/views/bubble/bubble_bor...
ui/views/bubble/bubble_border.h:126: // Specifies the arrow location for
calculating insets. Normally called from
On 2012/07/27 20:32:04, msw wrote:
> nit: wording, consider "Calculate the insets for a specific arrow location."

Done.

http://codereview.chromium.org/10808066/diff/21001/ui/views/bubble/bubble_del...
File ui/views/bubble/bubble_delegate.cc (right):

http://codereview.chromium.org/10808066/diff/21001/ui/views/bubble/bubble_del...
ui/views/bubble/bubble_delegate.cc:189: BubbleBorder* bubble_border =
On 2012/07/27 20:32:04, msw wrote:
> nit: rename border to fit on one line.

Done.

[jennyz, 2012-07-31 02:29:47]

http://codereview.chromium.org/10808066/diff/25001/ash/system/tray/system_tra...
File ash/system/tray/system_tray.cc (right):

http://codereview.chromium.org/10808066/diff/25001/ash/system/tray/system_tra...
ash/system/tray/system_tray.cc:479: init_params.arrow_color = kBackgroundColor;
kBackgroundColor is a fixed const, uber tray bubble arrow needs to be the same
as the bottom tray item, not the tray background color, this is sadrul fixed in
crbug.com/129428. Have you tried if this work for both uber tray bubble and
notification bubble?

http://codereview.chromium.org/10808066/diff/25001/ash/system/tray/system_tra...
File ash/system/tray/system_tray_bubble.cc (right):

http://codereview.chromium.org/10808066/diff/25001/ash/system/tray/system_tra...
ash/system/tray/system_tray_bubble.cc:280: init_params.max_height =
kDetailedBubbleMaxHeight;
I don't see bubble_view_ called SetMaxHeight(init_params.max_height) in the new
code, is this not necessary anymore with the new logic?

http://codereview.chromium.org/10808066/diff/25001/ash/system/tray/tray_bubbl...
File ash/system/tray/tray_bubble_view.cc (left):

http://codereview.chromium.org/10808066/diff/25001/ash/system/tray/tray_bubbl...
ash/system/tray/tray_bubble_view.cc:94: }
I wonder if the arrow color will be right after removing this code from sadrul's
cl: https://chromiumcodereview.appspot.com/10566003

http://codereview.chromium.org/10808066/diff/25001/ash/system/tray/tray_bubbl...
File ash/system/tray/tray_bubble_view.cc (right):

http://codereview.chromium.org/10808066/diff/25001/ash/system/tray/tray_bubbl...
ash/system/tray/tray_bubble_view.cc:125: return
GetInsetsForArrowLocation(insets, arrow_loc);
It seems make more sense for bubble infrastructure code to support this, instead
of client code to deal with it. I guess it probably not easy to do, so this is a
work around?

http://codereview.chromium.org/10808066/diff/25001/ash/system/tray/tray_bubbl...
ash/system/tray/tray_bubble_view.cc:132: // Get the unmirrored insets for the
arrow location.
Can you add some comments to explain why we need to use unmirrored insets here?

http://codereview.chromium.org/10808066/diff/25001/ash/system/tray/tray_bubbl...
ash/system/tray/tray_bubble_view.cc:300: void TrayBubbleView::SetMaxHeight(int
height) {
This function seems not called in the new code path? Is this intentional? If
yes, we should remove it.

[msw, 2012-07-31 14:41:55]

http://codereview.chromium.org/10808066/diff/25001/ash/system/tray/tray_bubbl...
File ash/system/tray/tray_bubble_view.cc (right):

http://codereview.chromium.org/10808066/diff/25001/ash/system/tray/tray_bubbl...
ash/system/tray/tray_bubble_view.cc:115: // TrayBubbleView supports dynamically
updated bubbles. This does not
FYI: Check out chrome/browser/ui/views/page_info_bubble_view.cc
PageInfoBubbleView grows via animation, and it may offer some inspiration.

[stevenjb, 2012-07-31 16:35:28]

http://codereview.chromium.org/10808066/diff/25001/ash/system/tray/system_tra...
File ash/system/tray/system_tray.cc (right):

http://codereview.chromium.org/10808066/diff/25001/ash/system/tray/system_tra...
ash/system/tray/system_tray.cc:479: init_params.arrow_color = kBackgroundColor;
Making the previous code work reliably for both web and system bubbles +
notifications would have been difficult and I think ultimately challenging to
maintain. I tested the code with all current uber tray views and notifications
(plus all web notification bubbles). I think it will be easier to explicitly
change the color for any exceptions when and if they come up.

http://codereview.chromium.org/10808066/diff/25001/ash/system/tray/system_tra...
File ash/system/tray/system_tray_bubble.cc (right):

http://codereview.chromium.org/10808066/diff/25001/ash/system/tray/system_tra...
ash/system/tray/system_tray_bubble.cc:280: init_params.max_height =
kDetailedBubbleMaxHeight;
On 2012/07/31 02:29:47, jennyz wrote:
> I don't see bubble_view_ called SetMaxHeight(init_params.max_height) in the
new
> code, is this not necessary anymore with the new logic?
It still gets called from SystemTrayBubble::UpdateView().

http://codereview.chromium.org/10808066/diff/25001/ash/system/tray/tray_bubbl...
File ash/system/tray/tray_bubble_view.cc (left):

http://codereview.chromium.org/10808066/diff/25001/ash/system/tray/tray_bubbl...
ash/system/tray/tray_bubble_view.cc:94: }
On 2012/07/31 02:29:47, jennyz wrote:
> I wonder if the arrow color will be right after removing this code from
sadrul's
> cl: https://chromiumcodereview.appspot.com/10566003

I think sadrul's CL predates the current design which always has a blue section
at the bottom of the uber tray; I tested these changes pretty thoroughly.
Unfortunately the code above did not work for web notifications, and would be
difficult to maintain when the bubble layout changes; setting the color
explicitly seemed easier to maintain.

http://codereview.chromium.org/10808066/diff/25001/ash/system/tray/tray_bubbl...
File ash/system/tray/tray_bubble_view.cc (right):

http://codereview.chromium.org/10808066/diff/25001/ash/system/tray/tray_bubbl...
ash/system/tray/tray_bubble_view.cc:115: // TrayBubbleView supports dynamically
updated bubbles. This does not
On 2012/07/31 14:41:56, msw wrote:
> FYI: Check out chrome/browser/ui/views/page_info_bubble_view.cc
> PageInfoBubbleView grows via animation, and it may offer some inspiration.

I'll take a look. As discussed, I plan to write unit tests with the existing
bubble code to see if I can break it, then fix that code and/or this code so
that we don't need the customization. Added issue 139813 and added it to the
comment.

http://codereview.chromium.org/10808066/diff/25001/ash/system/tray/tray_bubbl...
ash/system/tray/tray_bubble_view.cc:125: return
GetInsetsForArrowLocation(insets, arrow_loc);
Yes, this is a work around until we have more/better tests defining the Bubble
behavior (I didn't want to risk breaking for other bubbles without the
additional tests). Created an issue and added it to the comment above.

http://codereview.chromium.org/10808066/diff/25001/ash/system/tray/tray_bubbl...
ash/system/tray/tray_bubble_view.cc:132: // Get the unmirrored insets for the
arrow location.
Added a little to the comment here, mostly see the issue referenced above.

http://codereview.chromium.org/10808066/diff/25001/ash/system/tray/tray_bubbl...
ash/system/tray/tray_bubble_view.cc:300: void TrayBubbleView::SetMaxHeight(int
height) {
On 2012/07/31 02:29:47, jennyz wrote:
> This function seems not called in the new code path? Is this intentional? If
> yes, we should remove it.
Still called in SystemTrayBubble::UpdateView()

[jennyz, 2012-07-31 16:55:58]

lgtm

[sadrul, 2012-08-01 14:55:18]

lgtm

[commit-bot: I haz the power, 2012-08-01 16:33:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stevenjb@chromium.org/10808066/13021

[commit-bot: I haz the power, 2012-08-01 16:33:22]

Failed to apply patch for ash/system/tray/tray_bubble_view.cc:
While running patch -p1 --forward --force;
patching file ash/system/tray/tray_bubble_view.cc
Hunk #2 succeeded at 38 with fuzz 1.
Hunk #7 FAILED at 212.
Hunk #8 succeeded at 239 (offset 1 line).
Hunk #9 succeeded at 298 (offset 1 line).
Hunk #10 succeeded at 319 (offset 1 line).
Hunk #11 succeeded at 368 (offset 1 line).
Hunk #12 succeeded at 400 (offset 1 line).
Hunk #13 succeeded at 431 (offset 1 line).
1 out of 13 hunks FAILED -- saving rejects to file
ash/system/tray/tray_bubble_view.cc.rej