Rebase SpeechInputBubble on the new views bubble.

Rebase SpeechInputBubble on the new views bubble.

I chose to use the local ContentView class to implement BubbleDelegateView.
SpeechInputBubbleImpl didn't fit well as a BubbleDelegateView, because:
-Instances are expected to outlive Widget closure or hide (not implemented).
-SpeechInputBubbleBase::Show would cause a naming conflict.
-SpeechInputBubbleController interactions made that approach difficult.

BUG=98323
TEST=SpeechInputBubbles function as before.

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

[msw, 2011-11-17 07:05:24]

Alice, can you take a look? Thanks!

http://codereview.chromium.org/8565003/diff/6002/chrome/browser/speech/speech...
File chrome/browser/speech/speech_input_bubble_views.cc (right):

http://codereview.chromium.org/8565003/diff/6002/chrome/browser/speech/speech...
chrome/browser/speech/speech_input_bubble_views.cc:100: set_close_on_esc(false);
indentation fixed.

[alicet1, 2011-11-17 17:59:13]

oy, I can see what a nasty one this is, thanks for tackling it. some questions
below.

thanx,
alice

http://codereview.chromium.org/8565003/diff/6002/chrome/browser/speech/speech...
File chrome/browser/speech/speech_input_bubble_views.cc (left):

http://codereview.chromium.org/8565003/diff/6002/chrome/browser/speech/speech...
chrome/browser/speech/speech_input_bubble_views.cc:321:
delegate_->InfoBubbleFocusChanged();
I think you moved this to be called when activation changed. is that more
frequent than when the bubble close? I had it in my global error bubble change
to move what was originally done in BubbleClosing ->
WidgetDelegate::WindowClosing instead.

http://codereview.chromium.org/8565003/diff/6002/chrome/browser/speech/speech...
chrome/browser/speech/speech_input_bubble_views.cc:324: bool
SpeechInputBubbleImpl::CloseOnEscape() {
this doesn't want close on esc? we should honor?

http://codereview.chromium.org/8565003/diff/6002/chrome/browser/speech/speech...
chrome/browser/speech/speech_input_bubble_views.cc:342: if (toplevel_widget) {
how come you don't have to check for this now? I think the bubble only shows
under this if clause?

http://codereview.chromium.org/8565003/diff/6002/chrome/browser/speech/speech...
File chrome/browser/speech/speech_input_bubble_views.cc (right):

http://codereview.chromium.org/8565003/diff/6002/chrome/browser/speech/speech...
chrome/browser/speech/speech_input_bubble_views.cc:174: SetImage(image);
is this method needed anywhere else? otherwise just icon_->SetImage(image) will
be ok.

[msw, 2011-11-17 20:42:52]

Comments addressed, made fixes; please take another look.

http://codereview.chromium.org/8565003/diff/6002/chrome/browser/speech/speech...
File chrome/browser/speech/speech_input_bubble_views.cc (left):

http://codereview.chromium.org/8565003/diff/6002/chrome/browser/speech/speech...
chrome/browser/speech/speech_input_bubble_views.cc:321:
delegate_->InfoBubbleFocusChanged();
On 2011/11/17 17:59:13, alicet1 wrote:
> I think you moved this to be called when activation changed. is that more
> frequent than when the bubble close? I had it in my global error bubble change
> to move what was originally done in BubbleClosing ->
> WidgetDelegate::WindowClosing instead.
>  

In general you would be right to move such code to WindowClosing. However, in
this case, the intent is that InfoBubbleFocusChanged be called when the bubble
is being closed due to deactivation; so I'm calling it as we observe the
deactivation. Previously this was hackily marking did_invoke_close_ when the
SpeechInputBubble deletes this SpeechInputBubbleImpl, and then checking the flag
on window close.

I'm also removing the observer on explicit close, to avoid triggering a DCHECK
in SpeechInputController::InfoBubbleFocusChanged, which expects only to be
called when the focus changes by the user action, and not by the bubble closing
via deletion from the controller (on the Cancel button or when the bubble is
done recording).

http://codereview.chromium.org/8565003/diff/6002/chrome/browser/speech/speech...
chrome/browser/speech/speech_input_bubble_views.cc:324: bool
SpeechInputBubbleImpl::CloseOnEscape() {
On 2011/11/17 17:59:13, alicet1 wrote:
> this doesn't want close on esc? we should honor?

Yup, I call set_close_on_esc(false); in the SpeechInputBubbleView ctor.

I also needed to set_close_on_deactivate(false); because this is implemented
through SpeechInputBubbleController callbacks and message passing.

SpeechInputBubbleController likes to close the bubble by deleting the impl... :p

http://codereview.chromium.org/8565003/diff/6002/chrome/browser/speech/speech...
chrome/browser/speech/speech_input_bubble_views.cc:342: if (toplevel_widget) {
On 2011/11/17 17:59:13, alicet1 wrote:
> how come you don't have to check for this now? I think the bubble only shows
> under this if clause?

This ensured that the toplevel widget was available as a parent for the old
bubble. With my change, we're instead supplying the location icon view. tracing
through my new code:
1) It looks like the old code assumed tab_contents() was not NULL.
2) browser_context() shouldn't be NULL (the NavigationController context DCHECKs
that).
3) Profile::FromBrowserContext just static casts the browser context to a
Profile.
4) GetOrCreateTabbedBrowser creates a Browser* and calls InitBrowserWindow as
needed.
5) GetBrowserViewForBrowser returns the Browser's |window_|, initialized by
InitBrowserWindow.
6) GetLocationBarView might return NULL if toolbar_ is NULL!!! (I've added a
check here).
7) location_icon_view() could be NULL too, but the bubble is robust enough to
handle a NULL anchor_view.

I just tested the code on an --app window and it works just as it did before
(the icon is non-NULL, but the bubble points to a seemingly random screen point
if the microphone was partially outside the browsers contents view I filed
crbug.com/104648). 

Can we think of other suspicious supported scenarios worth checking? Otherwise,
we can fix any as they come up.

http://codereview.chromium.org/8565003/diff/6002/chrome/browser/speech/speech...
File chrome/browser/speech/speech_input_bubble_views.cc (right):

http://codereview.chromium.org/8565003/diff/6002/chrome/browser/speech/speech...
chrome/browser/speech/speech_input_bubble_views.cc:174: SetImage(image);
On 2011/11/17 17:59:13, alicet1 wrote:
> is this method needed anywhere else? otherwise just icon_->SetImage(image)
will
> be ok.

SetImage is also called by SpeechInputBubbleImpl::UpdateImage.

[alicet1, 2011-11-17 23:22:50]

lgtm

yay, looking good.

[msw, 2011-11-17 23:24:58]

Ben PTAL/stamp; thanks!

[Ben Goodger (Google), 2011-11-17 23:28:38]

lgtm

[commit-bot: I haz the power, 2011-11-17 23:31:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/msw@chromium.org/8565003/5005

[commit-bot: I haz the power, 2011-11-18 01:30:53]

Change committed as 110622