Close SpeechRecognitionBubble on pressing the Escape key.

Close SpeechRecognitionBubble on pressing the Escape key.

This cl adds support to cancel the SpeechRecognitionBubble on
pressing the Escape key.

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

[msw, 2014-02-20 18:30:20]

https://codereview.chromium.org/167283005/diff/1/chrome/browser/ui/views/spee...
File chrome/browser/ui/views/speech_recognition_bubble_views.cc (right):

https://codereview.chromium.org/167283005/diff/1/chrome/browser/ui/views/spee...
chrome/browser/ui/views/speech_recognition_bubble_views.cc:113:
set_close_on_esc(false);
Why not just remove this line? Many bubbles close on escape like that.

[Elliot Glaysher, 2014-02-20 18:59:17]

https://codereview.chromium.org/167283005/diff/1/chrome/browser/ui/views/spee...
File chrome/browser/ui/views/speech_recognition_bubble_views.cc (right):

https://codereview.chromium.org/167283005/diff/1/chrome/browser/ui/views/spee...
chrome/browser/ui/views/speech_recognition_bubble_views.cc:113:
set_close_on_esc(false);
On 2014/02/20 18:30:20, msw wrote:
> Why not just remove this line? Many bubbles close on escape like that.

Might have to figure out why closing on escape is unexpected by the controller.
Mike, it looks like you wrote that comment. Is it out of date?

[msw, 2014-02-20 21:57:24]

https://codereview.chromium.org/167283005/diff/1/chrome/browser/ui/views/spee...
File chrome/browser/ui/views/speech_recognition_bubble_views.cc (right):

https://codereview.chromium.org/167283005/diff/1/chrome/browser/ui/views/spee...
chrome/browser/ui/views/speech_recognition_bubble_views.cc:113:
set_close_on_esc(false);
On 2014/02/20 18:59:18, Elliot Glaysher wrote:
> On 2014/02/20 18:30:20, msw wrote:
> > Why not just remove this line? Many bubbles close on escape like that.
> 
> Might have to figure out why closing on escape is unexpected by the
controller.
> Mike, it looks like you wrote that comment. Is it out of date?

Ah, right... that comment is probably still valid, hence the need to alert the
delegate that 'cancel was clicked'.

https://codereview.chromium.org/167283005/diff/1/chrome/browser/ui/views/spee...
chrome/browser/ui/views/speech_recognition_bubble_views.cc:237: return false;
This should be: return BubbleDelegateView::AcceleratorPressed(accelerator);

[babu, 2014-02-21 16:33:40]

Thanks Mike and Elliot for reviewing. 

> > Why not just remove this line? Many bubbles close on escape like that.
> 
> Might have to figure out why closing on escape is unexpected by the
controller.
> Mike, it looks like you wrote that comment. Is it out of date?

Initially I tried enabling close_on_esc_ flag, but I noticed a crash in
SpeechRecognitionBubbleImpl() destructor accessing BubbleDelegateView object,
which was already deleted by pressing Escape. 

The reason for the crash was, pressing Escape sends OnWidgetActivationChanged()
and deletes the BubbleDelegateView object, but in OnWidgetActivationChanged() we
call InfoBubbleFocusChanged() which will try to delete the BubbleDelegateView
object that has already been deleted.

To avoid this crash, I'm handling Escape key in SpeechRecognitionBubbleView()
and preventing the default behaviour.

[babu, 2014-02-21 16:44:46]

On 2014/02/20 21:57:24, msw wrote:
> Ah, right... that comment is probably still valid, hence the need to alert the
> delegate that 'cancel was clicked'.
> 
>
https://codereview.chromium.org/167283005/diff/1/chrome/browser/ui/views/spee...
> chrome/browser/ui/views/speech_recognition_bubble_views.cc:237: return false;
> This should be: return BubbleDelegateView::AcceleratorPressed(accelerator);

Fixed.

[msw, 2014-02-21 18:06:31]

LGTM, thanks for your diligence.

[Elliot Glaysher, 2014-02-21 18:07:57]

lgtm++

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

https://codereview.chromium.org/167283005/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/speech_recognition_bubble_views.cc:112: // explicitly
closing on deactivation will cause unexpected behavior.
This comment should now be modified to take AcceleratorPressed() into account.

[babu, 2014-02-21 19:15:52]

On 2014/02/21 18:07:57, Elliot Glaysher wrote:
>
https://codereview.chromium.org/167283005/diff/80001/chrome/browser/ui/views/...
> chrome/browser/ui/views/speech_recognition_bubble_views.cc:112: // explicitly
> closing on deactivation will cause unexpected behavior.
> This comment should now be modified to take AcceleratorPressed() into account.

Done.

[babu, 2014-02-22 08:00:42]

The CQ bit was checked by sudarsana.nagineni@intel.com

[commit-bot: I haz the power, 2014-02-22 08:01:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sudarsana.nagineni@intel.com/167283005...

[commit-bot: I haz the power, 2014-02-22 19:04:25]

Change committed as 252814