Bug 4688: First run bubble ignores user's keyboard attempt...

Bug 4688: First run bubble ignores user's keyboard attempt
(tab) to change Search Engine. Not a good first impression.

BUG=4688
TEST=Open Chrome using --first-run, press Cancel on the
import dialog and watch the First Run bubble come up. Use Tab
and then Enter and the Change Search Engine should appear.

Then try it again, but this time click outside the bubble to
dismiss it (corner case that could lead to a crash).


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=12014

[Finnur, 2009-03-18 17:57:21]

As a break from layout tests I decided to tackle an easy 2.0 problem (which then
got deferred to 2.1). Ah, well.

[jcampan, 2009-03-18 19:17:22]

http://codereview.chromium.org/48138/diff/1004/6
File chrome/browser/views/first_run_bubble.cc (right):

http://codereview.chromium.org/48138/diff/1004/6#newcode243
Line 243: view_ = new FirstRunBubbleView(window, profile);
Instead of using a static, may be you could make it a member of FirstRunBubble
that you would set here?

[Finnur, 2009-03-18 19:50:30]

That's not a bad idea. Updated.

On 2009/03/18 19:17:22, jcampan wrote:
> http://codereview.chromium.org/48138/diff/1004/6
> File chrome/browser/views/first_run_bubble.cc (right):
> 
> http://codereview.chromium.org/48138/diff/1004/6#newcode243
> Line 243: view_ = new FirstRunBubbleView(window, profile);
> Instead of using a static, may be you could make it a member of FirstRunBubble
> that you would set here?

[jcampan, 2009-03-18 20:03:10]

http://codereview.chromium.org/48138/diff/1008/1009
File chrome/browser/views/first_run_bubble.cc (right):

http://codereview.chromium.org/48138/diff/1008/1009#newcode184
Line 184: static_cast<views::NativeButton*>(focused_now);
May be you could check the class name before casting? I am just afraid that if
we later on add a non nativebutton (for ex a link) to the bubble view, this will
break.

http://codereview.chromium.org/48138/diff/1008/1009#newcode199
Line 199: views::FocusManager* focus_manager_;
Just curious, why do you need to keep the FocusManager around? Is the HWND
already gone when you get the InfoBubbleClosing call?

[Finnur, 2009-03-18 20:22:51]

> I am just afraid that if we later on add a 
> non nativebutton (for ex a link) to the 
> bubble view, this will break.

That could never happen! Uh. I mean... done. :)

> Just curious, why do you need to keep the 
> FocusManager around? Is the HWND already gone
> when you get the InfoBubbleClosing call?

Actually, I did it this way because I hadn't added the view_ pointer to the
bubble class. Now that I have added it, I don't need to do this in the view at
all, I can just register and unregister in the bubble, which simplifies things a
bit. Thanks for asking. :) 

Updated.

[jcampan, 2009-03-18 21:11:19]

LGTM