Close bubble widget when its anchor widget bounds has changed and move_with_anchor is false.

Close bubble widget when its anchor widget bounds has changed and move_with_anchor is false.

The bookmark bubble will close when the browser window size changes with this CL.

We already close it when the browser window moves, so this is more consistent with that behavior.

BUG=321034
TEST=BubbleDelegateTest.CloseWhenAnchorWidgetBoundsChanged.
manual. see bug for repro step.

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

[msw, 2013-11-27 00:09:40]

Thanks for working on this; I think it should apply to all such bubbles.

https://codereview.chromium.org/89503003/diff/1/chrome/browser/ui/views/bookm...
File chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc (right):

https://codereview.chromium.org/89503003/diff/1/chrome/browser/ui/views/bookm...
chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:107: if
(anchor_widget() == widget)
I think all bubbles should do this if they haven't set move_with_anchor.
Can you move this to the BubbleDelegateView impl of OnWidgetBoundsChanged?

[oshima, 2013-11-27 00:32:39]

done. PTAL.

[msw, 2013-11-27 01:05:00]

LGTM with test nits; thanks!

https://codereview.chromium.org/89503003/diff/20001/ui/views/bubble/bubble_de...
File ui/views/bubble/bubble_delegate_unittest.cc (right):

https://codereview.chromium.org/89503003/diff/20001/ui/views/bubble/bubble_de...
ui/views/bubble/bubble_delegate_unittest.cc:252: test::TestWidgetObserver
bubble_observer(bubble_widget);
nit: you can remove bubble_observer since you test bubble_widget visibility
directly instead.

https://codereview.chromium.org/89503003/diff/20001/ui/views/bubble/bubble_de...
ui/views/bubble/bubble_delegate_unittest.cc:253:
EXPECT_FALSE(bubble_observer.widget_closed());
nit: you can remove this.

https://codereview.chromium.org/89503003/diff/20001/ui/views/bubble/bubble_de...
ui/views/bubble/bubble_delegate_unittest.cc:257:
anchor_widget->SetBounds(gfx::Rect(10, 10, 100, 100));
nit: consider making the bubble move_with_anchor for a first move (and no
close), then making it not move_with_anchor and close like this.

[oshima, 2013-11-27 18:19:11]

On 2013/11/27 01:05:00, msw wrote:
> LGTM with test nits; thanks!
> 
>
https://codereview.chromium.org/89503003/diff/20001/ui/views/bubble/bubble_de...
> File ui/views/bubble/bubble_delegate_unittest.cc (right):
> 
>
https://codereview.chromium.org/89503003/diff/20001/ui/views/bubble/bubble_de...
> ui/views/bubble/bubble_delegate_unittest.cc:252: test::TestWidgetObserver
> bubble_observer(bubble_widget);
> nit: you can remove bubble_observer since you test bubble_widget visibility
> directly instead.
> 
>
https://codereview.chromium.org/89503003/diff/20001/ui/views/bubble/bubble_de...
> ui/views/bubble/bubble_delegate_unittest.cc:253:
> EXPECT_FALSE(bubble_observer.widget_closed());
> nit: you can remove this.
> 
>
https://codereview.chromium.org/89503003/diff/20001/ui/views/bubble/bubble_de...
> ui/views/bubble/bubble_delegate_unittest.cc:257:
> anchor_widget->SetBounds(gfx::Rect(10, 10, 100, 100));
> nit: consider making the bubble move_with_anchor for a first move (and no
> close), then making it not move_with_anchor and close like this.

all done.

[commit-bot: I haz the power, 2013-11-27 18:19:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/oshima@chromium.org/89503003/40001

[commit-bot: I haz the power, 2013-11-28 01:42:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/oshima@chromium.org/89503003/40001

[commit-bot: I haz the power, 2013-11-28 02:10:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/oshima@chromium.org/89503003/40001

[commit-bot: I haz the power, 2013-11-28 02:41:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/oshima@chromium.org/89503003/40001

[commit-bot: I haz the power, 2013-11-28 03:09:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/oshima@chromium.org/89503003/40001

[commit-bot: I haz the power, 2013-11-28 03:36:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/oshima@chromium.org/89503003/40001

[commit-bot: I haz the power, 2013-11-28 04:20:38]

Retried try job too often on linux_chromeos for step(s) ash_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[oshima, 2013-12-02 19:28:24]

+mukai who may know why TrayBubbleView does not set move_with_anchor=true.

do you know why it does have this flag set? Or is it ok to
set move-with_anchor = true in TrayBubbleView?

[msw, 2013-12-02 19:52:59]

On 2013/12/02 19:28:24, oshima wrote:
> +mukai who may know why TrayBubbleView does not set move_with_anchor=true.
> 
> do you know why it does have this flag set? Or is it ok to
> set move-with_anchor = true in TrayBubbleView?

Maybe it avoids moving bubbles as they close and the launcher bar hides?
Perhaps skuhne, stevenjb, or jennyz may know.

[Jun Mukai, 2013-12-02 20:38:15]

On 2013/12/02 19:52:59, msw wrote:
> On 2013/12/02 19:28:24, oshima wrote:
> > +mukai who may know why TrayBubbleView does not set move_with_anchor=true.
> > 
> > do you know why it does have this flag set? Or is it ok to
> > set move-with_anchor = true in TrayBubbleView?
> 
> Maybe it avoids moving bubbles as they close and the launcher bar hides?
> Perhaps skuhne, stevenjb, or jennyz may know.

I didn't know that flag itself, sorry.
What I know is that SystemTrayBubble and WebNotificationTray are the only
instances of TrayBubbleView.
Which means, you can set the flag to TrayBubbleView, and actually
you'd better do so to keep the consistent behavior among those bubbles.

[oshima, 2013-12-02 20:50:00]

On 2013/12/02 19:52:59, msw wrote:
> On 2013/12/02 19:28:24, oshima wrote:
> > +mukai who may know why TrayBubbleView does not set move_with_anchor=true.
> > 
> > do you know why it does have this flag set? Or is it ok to
> > set move-with_anchor = true in TrayBubbleView?
> 
> Maybe it avoids moving bubbles as they close and the launcher bar hides?
> Perhaps skuhne, stevenjb, or jennyz may know.

I already asked them which led to him as final destination :)

[oshima, 2013-12-02 21:18:05]

On 2013/12/02 20:38:15, Jun Mukai wrote:
> On 2013/12/02 19:52:59, msw wrote:
> > On 2013/12/02 19:28:24, oshima wrote:
> > > +mukai who may know why TrayBubbleView does not set move_with_anchor=true.
> > > 
> > > do you know why it does have this flag set? Or is it ok to
> > > set move-with_anchor = true in TrayBubbleView?
> > 
> > Maybe it avoids moving bubbles as they close and the launcher bar hides?
> > Perhaps skuhne, stevenjb, or jennyz may know.
> 
> I didn't know that flag itself, sorry.
> What I know is that SystemTrayBubble and WebNotificationTray are the only
> instances of TrayBubbleView.
> Which means, you can set the flag to TrayBubbleView, and actually
> you'd better do so to keep the consistent behavior among those bubbles.

Done, as we discussed. msw@, mukai@, PTAL.

[Jun Mukai, 2013-12-03 02:19:05]

lgtm

[msw, 2013-12-03 03:10:48]

LGTM

[commit-bot: I haz the power, 2013-12-03 03:14:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/oshima@chromium.org/89503003/80001

[commit-bot: I haz the power, 2013-12-03 04:51:33]

Retried try job too often on linux_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-12-03 08:06:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/oshima@chromium.org/89503003/80001

[commit-bot: I haz the power, 2013-12-03 10:38:42]

Change committed as 238339