Ensure that the first run bubble displays as an inactive bubble window which ensures that it does n…

The first run bubble now displays as an inactive bubble window which ensures that it does not grab keyboard focus.

We now dismiss the bubble by observing the anchor widget and its parent for keyboard events.
This functionality is provided by the FirstRunBubbleCloser class.

BUG=553526
TEST=Covered by unittest FirstRunBubbleTest.CloseBubbleOnKeyEvent

Committed: https://crrev.com/8ae15e81337f00236c58bb49b3707872f61d599f
Cr-Commit-Position: refs/heads/master@{#359409}

[ananta, 2015-11-10 22:04:30]

Description was changed from

==========
Ensure that the first run bubble displays as an inactive bubble window which
ensures that it does not grab keyboard focus.

We now dismiss the bubble by observing the anchor widget and its parent for
keyboard events.
This functionality is provided by the FirstRunBubbleCloser class.

BUG=553526
TEST=Covered by unittest FirstRunBubbleTest.CloseBubbleOnKeyEvent
==========

to

==========
The first run bubble now displays as an inactive bubble window which ensures
that it does not grab keyboard focus.

We now dismiss the bubble by observing the anchor widget and its parent for
keyboard events.
This functionality is provided by the FirstRunBubbleCloser class.

BUG=553526
TEST=Covered by unittest FirstRunBubbleTest.CloseBubbleOnKeyEvent
==========

[ananta, 2015-11-10 22:05:26]

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

[sky, 2015-11-10 22:09:20]

https://codereview.chromium.org/1411423014/diff/1/chrome/browser/ui/views/fir...
File chrome/browser/ui/views/first_run_bubble.cc (right):

https://codereview.chromium.org/1411423014/diff/1/chrome/browser/ui/views/fir...
chrome/browser/ui/views/first_run_bubble.cc:120:
target_view_->AddPreTargetHandler(this);
Why do you need to add to both target_view_ and the parent? I would expect you
to add the handler to the widget. Does this route to the same place?

I'm not clear on the lifetime. How do you know target_view outlives this?

[ananta, 2015-11-10 22:35:36]

https://codereview.chromium.org/1411423014/diff/1/chrome/browser/ui/views/fir...
File chrome/browser/ui/views/first_run_bubble.cc (right):

https://codereview.chromium.org/1411423014/diff/1/chrome/browser/ui/views/fir...
chrome/browser/ui/views/first_run_bubble.cc:120:
target_view_->AddPreTargetHandler(this);
On 2015/11/10 22:09:20, sky wrote:
> Why do you need to add to both target_view_ and the parent? I would expect you
> to add the handler to the widget. Does this route to the same place?
> 
> I'm not clear on the lifetime. How do you know target_view outlives this?

We now add the observer only on the root view of the widget. The bubble does not
outlive the location bar which is the anchor in chrome

[sky, 2015-11-10 22:45:27]

https://codereview.chromium.org/1411423014/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/first_run_bubble.cc (right):

https://codereview.chromium.org/1411423014/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/first_run_bubble.cc:107: }
What about if FirstBubbleCloser is destroyed because the bubble closes? Don't
you need to remove keyboard event handelr then?

[ananta, 2015-11-10 23:00:12]

https://codereview.chromium.org/1411423014/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/first_run_bubble.cc (right):

https://codereview.chromium.org/1411423014/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/first_run_bubble.cc:107: }
On 2015/11/10 22:45:26, sky wrote:
> What about if FirstBubbleCloser is destroyed because the bubble closes? Don't
> you need to remove keyboard event handelr then?

Done.

[sky, 2015-11-11 18:45:52]

https://codereview.chromium.org/1411423014/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/first_run_bubble.cc (right):

https://codereview.chromium.org/1411423014/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/first_run_bubble.cc:103: views::View* target_view)
It's ok to take target_view, but you shouldn't cache it. In particular there is
nothing stopping the anchor view (which is target-view) from being destroyed
while the bubble is open. Instead cache the widget, which is all you really need
anyway.

[ananta, 2015-11-12 20:19:17]

https://codereview.chromium.org/1411423014/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/first_run_bubble.cc (right):

https://codereview.chromium.org/1411423014/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/first_run_bubble.cc:103: views::View* target_view)
On 2015/11/11 18:45:52, sky wrote:
> It's ok to take target_view, but you shouldn't cache it. In particular there
is
> nothing stopping the anchor view (which is target-view) from being destroyed
> while the bubble is open. Instead cache the widget, which is all you really
need
> anyway.

Done.

[sky, 2015-11-12 22:11:21]

LGTM

[ananta, 2015-11-12 22:17:28]

The CQ bit was checked by ananta@chromium.org

[commit-bot: I haz the power, 2015-11-12 22:18:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1411423014/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1411423014/100001

[commit-bot: I haz the power, 2015-11-12 22:25:42]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-11-12 22:26:51]

Patchset 6 (id:??) landed as
https://crrev.com/8ae15e81337f00236c58bb49b3707872f61d599f
Cr-Commit-Position: refs/heads/master@{#359409}