Adds chromium side plumbing to pass speech input calls back and forth with WebKit.

Adds chromium side plumbing to pass speech input calls back and forth with WebKit.

Please advise if any of the code needs to be within "#if ENABLE_INPUT_SPEECH", since most of the speech input code in webkit is under "#if ENABLE(INPUT_SPEECH)"

- Created renderer/SpeechInputDispatcher, implements WebKit::WebSpeechInputController which is used by WebView for invoking speech input.
- Created browser/speech/SpeechInputDispatcherHost to receive IPC messages from the above SpeechInputDispatcher. Nothing done yet apart from receiving the messages.
- Creates new directory chrome/browser/speech, this will be used for SpeechInputDispatcherHost, network based speech recognizer, speech audio recording/buffering code as well as  speech output (Text-to-speech) code in future.

BUG=none
TEST=browser_tests --gtest_filter=SpeechInputBrowserTest.*

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

[Satish, 2010-07-29 16:06:29]

adding marcus and joth as well

[bulach, 2010-07-29 16:50:04]

it looks good overall, but a couple of points:

1. even without functionality, it'd be great to have a browser test to exercise
this plumbing. see Hans' example:
http://codereview.chromium.org/2858049/show

2. joth may have a fresher memory :) about flags, but I guess we don't need the
#if guards here since WebKit will never call us..

On 2010/07/29 16:06:29, Satish wrote:
> adding marcus and joth as well

[bulach, 2010-07-29 16:50:25]

sorry, forgot to send a couple of nits.. :)

http://codereview.chromium.org/3035035/diff/1/8
File chrome/common/render_messages_internal.h (right):

http://codereview.chromium.org/3035035/diff/1/8#newcode2553
chrome/common/render_messages_internal.h:2553: // is happening on behalf of some
other render view, this call does nothing.
since this message is per render_view_id, perhaps remove the "nor or is
happening on behalf of some other render view" part?

http://codereview.chromium.org/3035035/diff/1/8#newcode2560
chrome/common/render_messages_internal.h:2560: // happening on behalf of some
other render view, this call does nothing.
as above..

[Satish, 2010-07-29 21:26:08]

> 1. even without functionality, it'd be great to have a browser test to
exercise
> this plumbing. see Hans' example:
> http://codereview.chromium.org/2858049/show

There is no code to do round trips yet, i.e. the page can make requests but will
get no responses of any kind. Hans' CL has a 'Provider' which handles the
incoming messages and respond, and I plan to add something like that for speech
in my next CL. The browser test and mock will be added along with that.

[Satish, 2010-08-02 14:40:26]

Addressed all review comments so far:
- Added interface SpeechInputManager which gets invoked by
SpeechInputDispatcherHost to process the messages. SpeechInputDispatcherHost
holds a pointer to a factory method which is used by tests to inject a mock or
fake SpeechInputManager.
- SpeechInputDispatcher now implements a listener which will receive speech
callbacks to pass to the render view
- Added a browser test to verify the webcore -> browser process -> webcore IPC
sequence.
- wrapped all new code in namespace speech_input

[Satish, 2010-08-02 16:33:50]

This patch depends https://bugs.webkit.org/show_bug.cgi?id=43352 to be submitted
and rolled in. I'll upload the latest code and run on try servers once that gets
rolled in to verify that the test passes green.

[bulach, 2010-08-02 21:18:34]

nice, thanks for adding the tests Satish! great to see code being exercised. :)

overall looks fine, I have only a few questions about the SpeechInputManager and
whether it should be a scoped_ptr or a scoped_refptr. more details below:

http://codereview.chromium.org/3035035/diff/7001/8003
File chrome/browser/speech/speech_input_browsertest.cc (right):

http://codereview.chromium.org/3035035/diff/7001/8003#newcode13
chrome/browser/speech/speech_input_browsertest.cc:13: #include
"chrome/browser/speech/speech_input_manager.h"
move the previous two up

http://codereview.chromium.org/3035035/diff/7001/8003#newcode17
chrome/browser/speech/speech_input_browsertest.cc:17: const string16 kTestResult
= L"Pictures of the moon";
this may be considered a leak by valgrind.
perhaps just have as const char* const and convert to string where needed?

http://codereview.chromium.org/3035035/diff/7001/8003#newcode20
chrome/browser/speech/speech_input_browsertest.cc:20: public:
indent+1

http://codereview.chromium.org/3035035/diff/7001/8003#newcode21
chrome/browser/speech/speech_input_browsertest.cc:21:
FakeSpeechInputManager(Listener* listener)
explicit

http://codereview.chromium.org/3035035/diff/7001/8003#newcode22
chrome/browser/speech/speech_input_browsertest.cc:22: :
fake_result_(kTestResult),
render_view_id_(0),

..and I guess gcc might complain they're out of order..

http://codereview.chromium.org/3035035/diff/7001/8003#newcode43
chrome/browser/speech/speech_input_browsertest.cc:43: private:
indent+1

http://codereview.chromium.org/3035035/diff/7001/8003#newcode47
chrome/browser/speech/speech_input_browsertest.cc:47:
listener_->setRecognitionResult(render_view_id_, fake_result_);
fake_result_ seems to be used only here, perhaps just convert from the const
char* const here?

http://codereview.chromium.org/3035035/diff/7001/8003#newcode62
chrome/browser/speech/speech_input_browsertest.cc:62:
DISABLE_RUNNABLE_METHOD_REFCOUNT(speech_input::FakeSpeechInputManager);
hmm, afaict the factory can still return a plain pointer, and whoever uses need
to have a scoped_refptr, right?

how much code does it save versus 4 lines of comments? :)

http://codereview.chromium.org/3035035/diff/7001/8004
File chrome/browser/speech/speech_input_dispatcher_host.cc (right):

http://codereview.chromium.org/3035035/diff/7001/8004#newcode16
chrome/browser/speech/speech_input_dispatcher_host.cc:16:
SpeechInputDispatcherHost::manager_factory_ = NULL;
..or,keep as manager_factory_ but initialize with &SpeechInputManager::create,
and then remove the test in manager()

http://codereview.chromium.org/3035035/diff/7001/8005
File chrome/browser/speech/speech_input_dispatcher_host.h (right):

http://codereview.chromium.org/3035035/diff/7001/8005#newcode35
chrome/browser/speech/speech_input_dispatcher_host.h:35: static void
set_manager_factory(SpeechInputManager::FactoryMethod* factory) {
set_manager_factory_for_tests

http://codereview.chromium.org/3035035/diff/7001/8005#newcode53
chrome/browser/speech/speech_input_dispatcher_host.h:53:
scoped_ptr<SpeechInputManager> manager_;
scoped_refptr?

http://codereview.chromium.org/3035035/diff/7001/8005#newcode57
chrome/browser/speech/speech_input_dispatcher_host.h:57: static
SpeechInputManager::FactoryMethod* manager_factory_;
manager_factory_for_tests_

http://codereview.chromium.org/3035035/diff/7001/8007
File chrome/browser/speech/speech_input_manager.h (right):

http://codereview.chromium.org/3035035/diff/7001/8007#newcode17
chrome/browser/speech/speech_input_manager.h:17: // results and status events to
the render views when required.
it may be good to clarify if by "various render views" it means "for a single
tab" or "for the browser process", i.e., does it block two frames from the same
page/tab to use recognition, or two frames overall?

http://codereview.chromium.org/3035035/diff/7001/8007#newcode22
chrome/browser/speech/speech_input_manager.h:22: public:
indent+1

http://codereview.chromium.org/3035035/diff/7001/8007#newcode39
chrome/browser/speech/speech_input_manager.h:39: virtual void stopRecording(int
render_view_id) = 0;
since this is in chromium side, the first letter should be capitalized (all
methods above)

[Satish, 2010-08-02 21:31:09]

Thanks, I'll address all the formatting and style comments in the next patch
tomorrow. One reply related to refptr below.

http://codereview.chromium.org/3035035/diff/7001/8003
File chrome/browser/speech/speech_input_browsertest.cc (right):

http://codereview.chromium.org/3035035/diff/7001/8003#newcode62
chrome/browser/speech/speech_input_browsertest.cc:62:
DISABLE_RUNNABLE_METHOD_REFCOUNT(speech_input::FakeSpeechInputManager);
On 2010/08/02 21:18:34, bulach wrote:
> hmm, afaict the factory can still return a plain pointer, and whoever uses
need
> to have a scoped_refptr, right?
> 
> how much code does it save versus 4 lines of comments? :)

In the real code, there is only 1 instance of SpeechInputManager and that is
created+owned by SpeechInputDispatcherHost. So conceptually there is no need for
that to be a refptr, since the reference is not passed to anyone else and there
is no need to keep track of it. I prefer not to force the necessities of the
test code onto the real code, and this seems like a standard way to do it (as
mentioned in the comment for the macro :) ).
Of course, if it turns out later that the real implementation needs refcounting
I'll modify this to conform.

[jorlow, 2010-08-03 13:48:01]

Looking pretty good.  You don't need to wait for a LGTM from me if the others
give one.

http://codereview.chromium.org/3035035/diff/7001/8003
File chrome/browser/speech/speech_input_browsertest.cc (right):

http://codereview.chromium.org/3035035/diff/7001/8003#newcode17
chrome/browser/speech/speech_input_browsertest.cc:17: const string16 kTestResult
= L"Pictures of the moon";
On 2010/08/02 21:18:34, bulach wrote:
> this may be considered a leak by valgrind.
> perhaps just have as const char* const and convert to string where needed?

I doubt it'd be considered a leak, but I'm concerned this would have
initialization overhead.  I'm also concerned this won't work on mac/linux.

Just keep it a string literal like Marcus said.

http://codereview.chromium.org/3035035/diff/7001/8004
File chrome/browser/speech/speech_input_dispatcher_host.cc (right):

http://codereview.chromium.org/3035035/diff/7001/8004#newcode22
chrome/browser/speech/speech_input_dispatcher_host.cc:22: // initialization
here.
Maybe make it more clear that any non-trivial initialization should be done
lazily.

http://codereview.chromium.org/3035035/diff/7001/8004#newcode42
chrome/browser/speech/speech_input_dispatcher_host.cc:42: *msg_was_ok = true;
didn't we decide to not do this?

http://codereview.chromium.org/3035035/diff/7001/8004#newcode88
chrome/browser/speech/speech_input_dispatcher_host.cc:88: CallRenderViewHost(
Make a helper function to do this?  Maybe even a template one that takes in the
class of the message?

http://codereview.chromium.org/3035035/diff/7001/8014
File chrome/renderer/speech_input_dispatcher.cc (right):

http://codereview.chromium.org/3035035/diff/7001/8014#newcode51
chrome/renderer/speech_input_dispatcher.cc:51: const std::string& result) {
This should probably be converted earlier to a utf16 and passed around as a
stirng16.

[Satish, 2010-08-03 13:58:30]

Thanks, will address the comments in my next patch.. couple of questions below.

http://codereview.chromium.org/3035035/diff/7001/8004
File chrome/browser/speech/speech_input_dispatcher_host.cc (right):

http://codereview.chromium.org/3035035/diff/7001/8004#newcode42
chrome/browser/speech/speech_input_dispatcher_host.cc:42: *msg_was_ok = true;
On 2010/08/03 13:48:02, Jeremy Orlow wrote:
> didn't we decide to not do this?

I was not aware. What is the reason?

http://codereview.chromium.org/3035035/diff/7001/8004#newcode88
chrome/browser/speech/speech_input_dispatcher_host.cc:88: CallRenderViewHost(
On 2010/08/03 13:48:02, Jeremy Orlow wrote:
> Make a helper function to do this?  Maybe even a template one that takes in
the
> class of the message?

I'm not sure about the advantage, since this is a single function call with only
4 arguments, and the message arguments are not always the same.

[jorlow, 2010-08-03 14:09:17]

http://codereview.chromium.org/3035035/diff/7001/8004
File chrome/browser/speech/speech_input_dispatcher_host.cc (right):

http://codereview.chromium.org/3035035/diff/7001/8004#newcode42
chrome/browser/speech/speech_input_dispatcher_host.cc:42: *msg_was_ok = true;
On 2010/08/03 13:58:31, Satish wrote:
> On 2010/08/03 13:48:02, Jeremy Orlow wrote:
> > didn't we decide to not do this?
> 
> I was not aware. What is the reason?

Oh, it was in Hans' review.  It's just not necessary.

http://codereview.chromium.org/3035035/diff/7001/8004#newcode88
chrome/browser/speech/speech_input_dispatcher_host.cc:88: CallRenderViewHost(
On 2010/08/03 13:58:31, Satish wrote:
> On 2010/08/03 13:48:02, Jeremy Orlow wrote:
> > Make a helper function to do this?  Maybe even a template one that takes in
> the
> > class of the message?
> 
> I'm not sure about the advantage, since this is a single function call with
only
> 4 arguments, and the message arguments are not always the same.

Well, the CallRenderViewHost ones are the same at least.  And I think you should
do it just because it's a bit cleaner and not hard to do.

[hans, 2010-08-03 14:14:01]

On 2010/08/03 13:58:30, Satish wrote:
> http://codereview.chromium.org/3035035/diff/7001/8004#newcode42
> chrome/browser/speech/speech_input_dispatcher_host.cc:42: *msg_was_ok = true;
> On 2010/08/03 13:48:02, Jeremy Orlow wrote:
> > didn't we decide to not do this?
> 
> I was not aware. What is the reason?

It was discussed in http://codereview.chromium.org/2858049/show (search for
msg_was_ok).
We concluded that it should always be true before the assignment. I suppose it
has just been copied around in the code (I grabbed it from Geolocation), because
there are lots of places that set it, and lots that don't.

[jorlow, 2010-08-03 14:15:17]

On 2010/08/03 14:14:01, hans wrote:
> On 2010/08/03 13:58:30, Satish wrote:
> > http://codereview.chromium.org/3035035/diff/7001/8004#newcode42
> > chrome/browser/speech/speech_input_dispatcher_host.cc:42: *msg_was_ok =
true;
> > On 2010/08/03 13:48:02, Jeremy Orlow wrote:
> > > didn't we decide to not do this?
> > 
> > I was not aware. What is the reason?
> 
> It was discussed in http://codereview.chromium.org/2858049/show (search for
> msg_was_ok).
> We concluded that it should always be true before the assignment. I suppose it
> has just been copied around in the code (I grabbed it from Geolocation),
because
> there are lots of places that set it, and lots that don't.

Maybe fix it in the code you guys copied from?

[Satish, 2010-08-03 17:14:36]

Addressed all the comments so far. Please take another look.

[bulach, 2010-08-04 19:29:05]

LGTM

good stuff! couple of minor nits:

http://codereview.chromium.org/3035035/diff/25001/26003
File chrome/browser/speech/speech_input_browsertest.cc (right):

http://codereview.chromium.org/3035035/diff/25001/26003#newcode57
chrome/browser/speech/speech_input_browsertest.cc:57:
listener_->SetRecognitionResult(render_view_id_, ASCIIToUTF16(kTestResult));
>80cols

http://codereview.chromium.org/3035035/diff/25001/26005
File chrome/browser/speech/speech_input_dispatcher_host.h (right):

http://codereview.chromium.org/3035035/diff/25001/26005#newcode57
chrome/browser/speech/speech_input_dispatcher_host.h:57: // place of the default
factory method.
obsolete comment?

[Satish, 2010-08-04 19:36:38]

Addressed both comments, will submit the patch. Thanks!