Listen for tab close notifications and cancel active speech sessions.

Listen for tab close notifications and cancel active speech sessions.

BUG=none
TEST=Start speech input and close tab with keyboard before the UI comes up, check for crashes.

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

[hans, 2011-01-06 12:30:54]

Looks good except for the comment nits below and try bot redness.

http://codereview.chromium.org/6115001/diff/1/chrome/browser/speech/speech_in...
File chrome/browser/speech/speech_input_bubble_controller.cc (right):

http://codereview.chromium.org/6115001/diff/1/chrome/browser/speech/speech_in...
chrome/browser/speech/speech_input_bubble_controller.cc:51: // If this is the
first bubble in our list for this TabContents, register
"First in our list", but the call is to IsOnlyBubbleForTab, and the comment for
that says "only bubble in list"..

http://codereview.chromium.org/6115001/diff/1/chrome/browser/speech/speech_in...
chrome/browser/speech/speech_input_bubble_controller.cc:55: // the somewhere
while the bubble & recording are active.
nit: "crashes in the somewhere" sounds weird

[bulach, 2011-01-06 12:32:39]

LG, nice refactoring!

one suggestion below as we chatted, and the bots don't look too happy
(speech_input_bubble_controller_unittest.cc).

http://codereview.chromium.org/6115001/diff/1/chrome/browser/speech/speech_in...
File chrome/browser/speech/speech_input_bubble.h (right):

http://codereview.chromium.org/6115001/diff/1/chrome/browser/speech/speech_in...
chrome/browser/speech/speech_input_bubble.h:125:
SpeechInputBubbleBase(TabContents* tab_contents);
explicit

http://codereview.chromium.org/6115001/diff/1/chrome/browser/speech/speech_in...
File chrome/browser/speech/speech_input_bubble_controller.cc (right):

http://codereview.chromium.org/6115001/diff/1/chrome/browser/speech/speech_in...
chrome/browser/speech/speech_input_bubble_controller.cc:54: // speech button and
immediately closes the tab, or if the tab crashes in
s/in the somewhere/

http://codereview.chromium.org/6115001/diff/1/chrome/browser/speech/speech_in...
chrome/browser/speech/speech_input_bubble_controller.cc:56: if
(IsOnlyBubbleForTab(caller_id)) {
as we chatted, the "IsOnlyBubbleForTab" is the best name, but it's the comments
are split in two places and it makes it slightly less clear to understand..
suggest using something like "ManageNotificationRegistration" on both places
with an enum, and then the comments would be in one single place..

http://codereview.chromium.org/6115001/diff/1/chrome/browser/speech/speech_in...
chrome/browser/speech/speech_input_bubble_controller.cc:94: // At least one
other bubble exists for the same TabContents.
yeah, as above, this would be the best place to say something like:
// At least one other bubble exists for the same TabContents,
// no need to register / unregister for the notifications.

http://codereview.chromium.org/6115001/diff/1/chrome/browser/speech/speech_in...
File chrome/browser/speech/speech_input_bubble_controller.h (right):

http://codereview.chromium.org/6115001/diff/1/chrome/browser/speech/speech_in...
chrome/browser/speech/speech_input_bubble_controller.h:121:
scoped_ptr<NotificationRegistrar> registrar_;
I think this is normally used directly rather than via a ptr..

[Satish, 2011-01-06 12:57:02]

All comments addressed (except one, replied below) and latest uploaded. Please
have a look.

http://codereview.chromium.org/6115001/diff/1/chrome/browser/speech/speech_in...
File chrome/browser/speech/speech_input_bubble_controller.h (right):

http://codereview.chromium.org/6115001/diff/1/chrome/browser/speech/speech_in...
chrome/browser/speech/speech_input_bubble_controller.h:121:
scoped_ptr<NotificationRegistrar> registrar_;
On 2011/01/06 12:32:39, bulach wrote:
> I think this is normally used directly rather than via a ptr..

A few reviewers suggested in the past to reduce the number of includes and use
fwd decls where possible, so I tried that here.

[hans, 2011-01-06 14:28:05]

On 2011/01/06 12:57:02, Satish wrote:
> All comments addressed (except one, replied below) and latest uploaded. Please
> have a look.
> 
>
http://codereview.chromium.org/6115001/diff/1/chrome/browser/speech/speech_in...
> File chrome/browser/speech/speech_input_bubble_controller.h (right):
> 
>
http://codereview.chromium.org/6115001/diff/1/chrome/browser/speech/speech_in...
> chrome/browser/speech/speech_input_bubble_controller.h:121:
> scoped_ptr<NotificationRegistrar> registrar_;
> On 2011/01/06 12:32:39, bulach wrote:
> > I think this is normally used directly rather than via a ptr..
> 
> A few reviewers suggested in the past to reduce the number of includes and use
> fwd decls where possible, so I tried that here.

LGTM

[bulach, 2011-01-06 14:31:44]

LGTM

nit below:

http://codereview.chromium.org/6115001/diff/8001/chrome/browser/speech/speech...
File chrome/browser/speech/speech_input_bubble_controller.h (right):

http://codereview.chromium.org/6115001/diff/8001/chrome/browser/speech/speech...
chrome/browser/speech/speech_input_bubble_controller.h:118: bool
IsOnlyBubbleForTab(int caller_id);
remove?

[Satish, 2011-01-07 10:57:34]

I have now fixed the failing unittest. It required making the test derive from
the BrowserWithTestWindowTest class from which we can get a valid TabContents*.
This also required the unit test thread to be the UI thread so I am now creating
a new thread for IO and using a WaitableEvent to indicate from that to the UI
thread when the test completes.

Please take a fresh look at the unit test and let me know if it looks good to
submit.

http://codereview.chromium.org/6115001/diff/8001/chrome/browser/speech/speech...
File chrome/browser/speech/speech_input_bubble_controller.h (right):

http://codereview.chromium.org/6115001/diff/8001/chrome/browser/speech/speech...
chrome/browser/speech/speech_input_bubble_controller.h:118: bool
IsOnlyBubbleForTab(int caller_id);
On 2011/01/06 14:31:44, bulach wrote:
> remove?

Done.

[bulach, 2011-01-07 11:11:28]

one question below:

http://codereview.chromium.org/6115001/diff/16001/chrome/browser/speech/speec...
File chrome/browser/speech/speech_input_bubble_controller_unittest.cc (right):

http://codereview.chromium.org/6115001/diff/16001/chrome/browser/speech/speec...
chrome/browser/speech/speech_input_bubble_controller_unittest.cc:79:
bubble_callback_event_(false, false),
I'm not very familiar, but seems that BrowserWithTestWindowTest provides access
to its ui message_loop(), so instead of signaling the event, you may just post a
quit task to it? would that work?

[Satish, 2011-01-07 11:19:15]

http://codereview.chromium.org/6115001/diff/16001/chrome/browser/speech/speec...
File chrome/browser/speech/speech_input_bubble_controller_unittest.cc (right):

http://codereview.chromium.org/6115001/diff/16001/chrome/browser/speech/speec...
chrome/browser/speech/speech_input_bubble_controller_unittest.cc:79:
bubble_callback_event_(false, false),
On 2011/01/07 11:11:29, bulach wrote:
> I'm not very familiar, but seems that BrowserWithTestWindowTest provides
access
> to its ui message_loop(), so instead of signaling the event, you may just post
a
> quit task to it? would that work?

Done.

[bulach, 2011-01-07 11:21:32]

LGTM

great, thanks!

[hans, 2011-01-07 11:45:08]

LGTM