Adds a speech input info bubble.

Adds a speech input info bubble.

I have not implemented the Mac/Linux part yet, will do so in a subsequent CL (does InfoBubble exist for Mac?).

This info bubble remains visible on screen until recognition ends or the user cancels it.
It also tracks the html element which requested speech recognition if it changes position
on screen (e.g. user scrolls the page or moves the browser window).

To get this working, I extended InfoBubble to query the delegate if it can be closed when
another window gets activated, as well as to reposition it when required.

TEST=none
BUG=none

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

[sky, 2010-08-23 19:51:59]

Can you e

http://codereview.chromium.org/3133034/diff/1/2
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/3133034/diff/1/2#newcode9235
chrome/app/generated_resources.grd:9235: <message
name="IDS_SPEECH_INPUT_BUBBLE_HEADING" desc="First line in the content area of
the speech input bubble. Instructs that the user can start speaking.">
Instructs the user that they can start speaking.

http://codereview.chromium.org/3133034/diff/1/3
File chrome/browser/browser_resources.grd (right):

http://codereview.chromium.org/3133034/diff/1/3#newcode76
chrome/browser/browser_resources.grd:76: <include
name="IDR_SPEECH_INPUT_RECORDING_ICON"
file="resources\speech_input\recording.png" type="BINDATA" />
How come you didn't put these in theme_resources.grd?

http://codereview.chromium.org/3133034/diff/1/6
File chrome/browser/views/info_bubble.cc (right):

http://codereview.chromium.org/3133034/diff/1/6#newcode447
chrome/browser/views/info_bubble.cc:447: void InfoBubble::SetNewPosition(const
gfx::Rect& position_relative_to) {
Position of definition should match declaration.

Also, isn't this the same as SizeToContents?

http://codereview.chromium.org/3133034/diff/1/8
File chrome/browser/views/speech_input_bubble.cc (right):

http://codereview.chromium.org/3133034/diff/1/8#newcode34
chrome/browser/views/speech_input_bubble.cc:34: class SpeechInputBubbleContent
Move this class to be inside the scope of SpeechInputBubble.

http://codereview.chromium.org/3133034/diff/1/8#newcode77
chrome/browser/views/speech_input_bubble.cc:77: gfx::Size GetPreferredSize() {
Make all your overrides virtual.

http://codereview.chromium.org/3133034/diff/1/8#newcode80
chrome/browser/views/speech_input_bubble.cc:80: width =
icon_->GetImage().width();
How come you don't just ask for the preferred size?

http://codereview.chromium.org/3133034/diff/1/8#newcode86
chrome/browser/views/speech_input_bubble.cc:86: return gfx::Size(width, height);
How come you don't inclined the cancel width in this calculation?

http://codereview.chromium.org/3133034/diff/1/9
File chrome/browser/views/speech_input_bubble.h (right):

http://codereview.chromium.org/3133034/diff/1/9#newcode27
chrome/browser/views/speech_input_bubble.h:27: public
base::RefCountedThreadSafe<SpeechInputBubble> {
Why is this object refcountedthreadsafe?

http://codereview.chromium.org/3133034/diff/1/9#newcode31
chrome/browser/views/speech_input_bubble.h:31: public:
indent by 1

http://codereview.chromium.org/3133034/diff/1/9#newcode34
chrome/browser/views/speech_input_bubble.h:34: };
Add protected virtual destructor.

http://codereview.chromium.org/3133034/diff/1/9#newcode43
chrome/browser/views/speech_input_bubble.h:43: virtual ~SpeechInputBubble() {}
Don't inline the destructor. Also, if you really need this class to be ref
counted thread then the destructor should be private.

http://codereview.chromium.org/3133034/diff/1/9#newcode69
chrome/browser/views/speech_input_bubble.h:69: void
InfoBubbleClosing(InfoBubble* info_bubble, bool closed_by_escape);
Don't inline any of these. And they should be virtual.

http://codereview.chromium.org/3133034/diff/1/9#newcode87
chrome/browser/views/speech_input_bubble.h:87: typedef
SpeechInputBubble::Delegate SpeechInputBubbleDelegate;
Why don't you just extract the delegate into a class named
SpeechInputBubbleDelegate?

[sky, 2010-08-23 19:52:48]

Sorry, hit the publish button too quickly last time.

http://codereview.chromium.org/3133034/diff/1/7
File chrome/browser/views/info_bubble.h (right):

http://codereview.chromium.org/3133034/diff/1/7#newcode172
chrome/browser/views/info_bubble.h:172: virtual bool CloseOnInactive() { return
true; }
Can you elaborate on why you need this.
The problem with this approach is that the containing window will be stuck
rendering as active when it isn't.

[Satish, 2010-08-23 23:26:02]

Thanks for the quick review, I've addressed all comments. There are now no
changes now to info_bubble.*

http://codereview.chromium.org/3133034/diff/1/2
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/3133034/diff/1/2#newcode9235
chrome/app/generated_resources.grd:9235: <message
name="IDS_SPEECH_INPUT_BUBBLE_HEADING" desc="First line in the content area of
the speech input bubble. Instructs that the user can start speaking.">
On 2010/08/23 19:51:59, sky wrote:
> Instructs the user that they can start speaking.

Done.

http://codereview.chromium.org/3133034/diff/1/3
File chrome/browser/browser_resources.grd (right):

http://codereview.chromium.org/3133034/diff/1/3#newcode76
chrome/browser/browser_resources.grd:76: <include
name="IDR_SPEECH_INPUT_RECORDING_ICON"
file="resources\speech_input\recording.png" type="BINDATA" />
On 2010/08/23 19:51:59, sky wrote:
> How come you didn't put these in theme_resources.grd?

Done.

http://codereview.chromium.org/3133034/diff/1/7
File chrome/browser/views/info_bubble.h (right):

http://codereview.chromium.org/3133034/diff/1/7#newcode172
chrome/browser/views/info_bubble.h:172: virtual bool CloseOnInactive() { return
true; }
On 2010/08/23 19:52:48, sky wrote:
> Can you elaborate on why you need this.
> The problem with this approach is that the containing window will be stuck
> rendering as active when it isn't.

removed per our discussion

http://codereview.chromium.org/3133034/diff/1/8
File chrome/browser/views/speech_input_bubble.cc (right):

http://codereview.chromium.org/3133034/diff/1/8#newcode34
chrome/browser/views/speech_input_bubble.cc:34: class SpeechInputBubbleContent
On 2010/08/23 19:51:59, sky wrote:
> Move this class to be inside the scope of SpeechInputBubble.

Done.

http://codereview.chromium.org/3133034/diff/1/8#newcode77
chrome/browser/views/speech_input_bubble.cc:77: gfx::Size GetPreferredSize() {
On 2010/08/23 19:51:59, sky wrote:
> Make all your overrides virtual.

Done.

http://codereview.chromium.org/3133034/diff/1/8#newcode80
chrome/browser/views/speech_input_bubble.cc:80: width =
icon_->GetImage().width();
On 2010/08/23 19:51:59, sky wrote:
> How come you don't just ask for the preferred size?

Done.

http://codereview.chromium.org/3133034/diff/1/8#newcode86
chrome/browser/views/speech_input_bubble.cc:86: return gfx::Size(width, height);
On 2010/08/23 19:51:59, sky wrote:
> How come you don't inclined the cancel width in this calculation?

Done.

http://codereview.chromium.org/3133034/diff/1/9
File chrome/browser/views/speech_input_bubble.h (right):

http://codereview.chromium.org/3133034/diff/1/9#newcode27
chrome/browser/views/speech_input_bubble.h:27: public
base::RefCountedThreadSafe<SpeechInputBubble> {
On 2010/08/23 19:51:59, sky wrote:
> Why is this object refcountedthreadsafe?

removed now as it is not required anymore

http://codereview.chromium.org/3133034/diff/1/9#newcode31
chrome/browser/views/speech_input_bubble.h:31: public:
On 2010/08/23 19:51:59, sky wrote:
> indent by 1

Done.

http://codereview.chromium.org/3133034/diff/1/9#newcode34
chrome/browser/views/speech_input_bubble.h:34: };
On 2010/08/23 19:51:59, sky wrote:
> Add protected virtual destructor.

Done.

http://codereview.chromium.org/3133034/diff/1/9#newcode43
chrome/browser/views/speech_input_bubble.h:43: virtual ~SpeechInputBubble() {}
On 2010/08/23 19:51:59, sky wrote:
> Don't inline the destructor. Also, if you really need this class to be ref
> counted thread then the destructor should be private.

Done.

http://codereview.chromium.org/3133034/diff/1/9#newcode69
chrome/browser/views/speech_input_bubble.h:69: void
InfoBubbleClosing(InfoBubble* info_bubble, bool closed_by_escape);
On 2010/08/23 19:51:59, sky wrote:
> Don't inline any of these. And they should be virtual.

Done.

http://codereview.chromium.org/3133034/diff/1/9#newcode87
chrome/browser/views/speech_input_bubble.h:87: typedef
SpeechInputBubble::Delegate SpeechInputBubbleDelegate;
On 2010/08/23 19:51:59, sky wrote:
> Why don't you just extract the delegate into a class named
> SpeechInputBubbleDelegate?

I preferred to keep the delegate class in scope as well as conform to other
related code which does the same. Plus this typedef is just for one compiler
artifact and may be removed in future if we move to the next version

[sky, 2010-08-23 23:50:13]

http://codereview.chromium.org/3133034/diff/13001/3006
File chrome/browser/views/speech_input_bubble.cc (right):

http://codereview.chromium.org/3133034/diff/13001/3006#newcode168
chrome/browser/views/speech_input_bubble.cc:168: }
Does this also need to delete this?

http://codereview.chromium.org/3133034/diff/13001/3006#newcode182
chrome/browser/views/speech_input_bubble.cc:182: type ==
NotificationType::TAB_CONTENTS_HIDDEN) {
You never register for TAB_CONTENTS_HIDDEN.

http://codereview.chromium.org/3133034/diff/13001/3006#newcode190
chrome/browser/views/speech_input_bubble.cc:190: void
SpeechInputBubble::SetRecognizingMode() {
The order of definitions should match that of declarations.

http://codereview.chromium.org/3133034/diff/13001/3007
File chrome/browser/views/speech_input_bubble.h (right):

http://codereview.chromium.org/3133034/diff/13001/3007#newcode23
chrome/browser/views/speech_input_bubble.h:23: class SpeechInputBubble
Please mention lifetime of this object.

http://codereview.chromium.org/3133034/diff/13001/3007#newcode54
chrome/browser/views/speech_input_bubble.h:54: void Close();
The implementation doesn't delete this. If Close is doing to delete this, then
you should make the destructor private to enforce that only SpeechInputBubble
can delete itself.

http://codereview.chromium.org/3133034/diff/13001/3007#newcode64
chrome/browser/views/speech_input_bubble.h:64: void Observe(NotificationType
type,
virtual.

http://codereview.chromium.org/3133034/diff/13001/3007#newcode79
chrome/browser/views/speech_input_bubble.h:79: bool did_close_;
Please document what this does.

[Satish, 2010-08-24 00:10:20]

http://codereview.chromium.org/3133034/diff/13001/3006
File chrome/browser/views/speech_input_bubble.cc (right):

http://codereview.chromium.org/3133034/diff/13001/3006#newcode168
chrome/browser/views/speech_input_bubble.cc:168: }
On 2010/08/23 23:50:13, sky wrote:
> Does this also need to delete this?

Not required now, I've fixed the comments and this is a plain class now, caller
manages life cycle.

http://codereview.chromium.org/3133034/diff/13001/3006#newcode182
chrome/browser/views/speech_input_bubble.cc:182: type ==
NotificationType::TAB_CONTENTS_HIDDEN) {
On 2010/08/23 23:50:13, sky wrote:
> You never register for TAB_CONTENTS_HIDDEN.

Done.

http://codereview.chromium.org/3133034/diff/13001/3006#newcode190
chrome/browser/views/speech_input_bubble.cc:190: void
SpeechInputBubble::SetRecognizingMode() {
On 2010/08/23 23:50:13, sky wrote:
> The order of definitions should match that of declarations.

Done.

http://codereview.chromium.org/3133034/diff/13001/3007
File chrome/browser/views/speech_input_bubble.h (right):

http://codereview.chromium.org/3133034/diff/13001/3007#newcode23
chrome/browser/views/speech_input_bubble.h:23: class SpeechInputBubble
On 2010/08/23 23:50:13, sky wrote:
> Please mention lifetime of this object.

I had mistaken this object's lifecycle to that of the InfoBubble object (which
gets destroyed in OnFinalMessage). This is actually a normal class with
lifecycle managed by the caller, so not documenting anything new. I've fixed
comments in the delegate and other places.

http://codereview.chromium.org/3133034/diff/13001/3007#newcode54
chrome/browser/views/speech_input_bubble.h:54: void Close();
On 2010/08/23 23:50:13, sky wrote:
> The implementation doesn't delete this. If Close is doing to delete this, then
> you should make the destructor private to enforce that only SpeechInputBubble
> can delete itself.

As mentioned above, fixed comment.

http://codereview.chromium.org/3133034/diff/13001/3007#newcode64
chrome/browser/views/speech_input_bubble.h:64: void Observe(NotificationType
type,
On 2010/08/23 23:50:13, sky wrote:
> virtual.

Done.

http://codereview.chromium.org/3133034/diff/13001/3007#newcode79
chrome/browser/views/speech_input_bubble.h:79: bool did_close_;
On 2010/08/23 23:50:13, sky wrote:
> Please document what this does.

Done.

[sky, 2010-08-24 00:19:07]

http://codereview.chromium.org/3133034/diff/21001/22005
File chrome/browser/views/speech_input_bubble.cc (right):

http://codereview.chromium.org/3133034/diff/21001/22005#newcode142
chrome/browser/views/speech_input_bubble.cc:142: DCHECK(!info_bubble_);
Can't the owner destroy the this at any time? Wouldn't it be better to invoke
Close?

[Satish, 2010-08-24 00:25:04]

http://codereview.chromium.org/3133034/diff/21001/22005
File chrome/browser/views/speech_input_bubble.cc (right):

http://codereview.chromium.org/3133034/diff/21001/22005#newcode142
chrome/browser/views/speech_input_bubble.cc:142: DCHECK(!info_bubble_);
On 2010/08/24 00:19:07, sky wrote:
> Can't the owner destroy the this at any time? Wouldn't it be better to invoke
> Close?

From what I saw, InfoBubble needs the delegate to be alive until
InfoBubbleClosing() is called. This was the reason I initially had this object
as refcounted, so until that callback is received SpeechInputBubble will remain
alive. With no refcount now, I was trying to enforce that the object stays alive
until the InfoBubble sent the callback. But now that i think about it, this
doesn't make sense since the callback can come at a later time dueto the message
loop and window destruction sequence. So looks like i should go back to
refcounted. If you agree, I'll make the change and upload a new patch.

[sky, 2010-08-24 05:05:29]

As long as you've turned off fading close should be immediate.

  -Scott

On Mon, Aug 23, 2010 at 5:25 PM,  <satish@chromium.org> wrote:
>
> http://codereview.chromium.org/3133034/diff/21001/22005
> File chrome/browser/views/speech_input_bubble.cc (right):
>
> http://codereview.chromium.org/3133034/diff/21001/22005#newcode142
> chrome/browser/views/speech_input_bubble.cc:142: DCHECK(!info_bubble_);
> On 2010/08/24 00:19:07, sky wrote:
>>
>> Can't the owner destroy the this at any time? Wouldn't it be better to
>
> invoke
>>
>> Close?
>
> From what I saw, InfoBubble needs the delegate to be alive until
> InfoBubbleClosing() is called. This was the reason I initially had this
> object as refcounted, so until that callback is received
> SpeechInputBubble will remain alive. With no refcount now, I was trying
> to enforce that the object stays alive until the InfoBubble sent the
> callback. But now that i think about it, this doesn't make sense since
> the callback can come at a later time dueto the message loop and window
> destruction sequence. So looks like i should go back to refcounted. If
> you agree, I'll make the change and upload a new patch.
>
> http://codereview.chromium.org/3133034/show
>

[Satish, 2010-08-24 11:19:59]

I have now removed the Close method since there was no use for it, the
destructor closes the info bubble as you suggested if it was not already closed
by losing focus. I have also changed the usage of the delegate's
RecognitionCancelled method, now it fires when user clicks the cancel button but
keeps the info bubble alive until caller closes it. I added one more delegate
method to indicate that the info bubble has closed because user moved focus
elsewhere, this was required to let the speech engine decide if it should stop
recognition or not based on it's internal state (i.e. recognition may continue
even after user clicked elsewhere and closed the bubble, if the audio has been
sent to the server already).

Please take another look.

[Satish, 2010-08-24 15:36:42]

I found that testing the speech bubble calling code was difficult in the current
model, so changed SpeechInputBubble to an interface and moved it to
browser/speech directory. Platform specific UI code would implement this
interface and tests can implement a mock class. The file
browser/views/speech_input_bubble.cc now contains the windows implementation of
this interface, I have cleaned it up and made it more consistent inside.

Please take a fresh look.

[sky, 2010-08-24 16:08:27]

http://codereview.chromium.org/3133034/diff/30001/16006
File chrome/browser/speech/speech_input_bubble.h (right):

http://codereview.chromium.org/3133034/diff/30001/16006#newcode16
chrome/browser/speech/speech_input_bubble.h:16: // the popup window, or by the
caller invoking the |Close| method.
Update your comments.

http://codereview.chromium.org/3133034/diff/30001/16006#newcode30
chrome/browser/speech/speech_input_bubble.h:30: virtual void InfoBubbleClosed()
= 0;
You should mention this isn't invoked if the SpeechInputBubble is deleted.

http://codereview.chromium.org/3133034/diff/30001/16006#newcode41
chrome/browser/speech/speech_input_bubble.h:41: // input (in page coordinates).
Seems like this should be in screen coordinates otherwise everyone has to
convert.

http://codereview.chromium.org/3133034/diff/30001/16006#newcode46
chrome/browser/speech/speech_input_bubble.h:46: typedef SpeechInputBubble*
(FactoryMethod)(TabContents*,
Is this needed?

http://codereview.chromium.org/3133034/diff/30001/16007
File chrome/browser/views/speech_input_bubble.cc (right):

http://codereview.chromium.org/3133034/diff/30001/16007#newcode138
chrome/browser/views/speech_input_bubble.cc:138: Delegate* delegate,
align with ( on previous line.

http://codereview.chromium.org/3133034/diff/30001/16007#newcode140
chrome/browser/views/speech_input_bubble.cc:140: ~SpeechInputBubbleImpl();
virtual

http://codereview.chromium.org/3133034/diff/30001/16007#newcode165
chrome/browser/views/speech_input_bubble.cc:165: bool did_close_;  // Set to
true if |Close()| was invoked by caller.
Update comment.

http://codereview.chromium.org/3133034/diff/30001/16007#newcode217
chrome/browser/views/speech_input_bubble.cc:217: container_rect.y() +
element_rect.y() + element_rect.height(), 1, 1);
Sounds the size be that of element_rect?

[Satish, 2010-08-24 16:22:03]

Thanks, will address the comments. A couple of questions/replies below.

http://codereview.chromium.org/3133034/diff/30001/16006
File chrome/browser/speech/speech_input_bubble.h (right):

http://codereview.chromium.org/3133034/diff/30001/16006#newcode41
chrome/browser/speech/speech_input_bubble.h:41: // input (in page coordinates).
On 2010/08/24 16:08:28, sky wrote:
> Seems like this should be in screen coordinates otherwise everyone has to
> convert.

The callers get this rect from webkit hence know about it in page coordinates.
It is easier for now to do this conversion within this class because it depends
on where the tab is positioned and to do this calculation in the UI thread. I
could change it later if there is a necessity for any of the callers to do a
conversion from some other coordinates to page coordinates.

http://codereview.chromium.org/3133034/diff/30001/16006#newcode46
chrome/browser/speech/speech_input_bubble.h:46: typedef SpeechInputBubble*
(FactoryMethod)(TabContents*,
On 2010/08/24 16:08:28, sky wrote:
> Is this needed?

Yes, for unit testing the code using this object in a subsequent CL.

http://codereview.chromium.org/3133034/diff/30001/16007
File chrome/browser/views/speech_input_bubble.cc (right):

http://codereview.chromium.org/3133034/diff/30001/16007#newcode217
chrome/browser/views/speech_input_bubble.cc:217: container_rect.y() +
element_rect.y() + element_rect.height(), 1, 1);
On 2010/08/24 16:08:28, sky wrote:
> Set the size be that of element_rect?

We want the bubble's arrow to point at x, y position calculated here. Setting
the size to be that of element_rect  makes the bubble's arrow point at the
center of the element instead, which is not the desired behavior for us.

[sky, 2010-08-24 16:49:57]

LGTM if you remove the factory method typedef. Wait until you know you need it
before adding it.

http://codereview.chromium.org/3133034/diff/30001/16006
File chrome/browser/speech/speech_input_bubble.h (right):

http://codereview.chromium.org/3133034/diff/30001/16006#newcode46
chrome/browser/speech/speech_input_bubble.h:46: typedef SpeechInputBubble*
(FactoryMethod)(TabContents*,
On 2010/08/24 16:22:04, Satish wrote:
> On 2010/08/24 16:08:28, sky wrote:
> > Is this needed?
> 
> Yes, for unit testing the code using this object in a subsequent CL.

Can you add it when you need it then.

[Satish, 2010-08-24 16:57:35]

Thanks, removed the typedef in this CL and will add in the CL where it will be
used.