Introducing the basic functionality to the speech input experimental API.

Introducing the basic functionality to the speech input experimental API.

BUG=97388
TEST=none

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

[Satish, 2011-10-08 23:15:07]

First pass comments below. Will review more after these are addressed

http://codereview.chromium.org/8201030/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_speech_input_api.cc (right):

http://codereview.chromium.org/8201030/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_speech_input_api.cc:29: registrar_.Add(this,
TransitionNotification(),
when do you remove as a notification listener? should it be done in Observe
after you got the relevant notification or failure?

http://codereview.chromium.org/8201030/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_speech_input_api.cc:45: if (type ==
chrome::NOTIFICATION_EXTENSION_SPEECH_INPUT_FAILED)
suggest using braces for if-else blocks

http://codereview.chromium.org/8201030/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_speech_input_api.cc:50: Run();
is there a possibility that in this call to Run() that the extension manager's
state is still the same as TransitionState() so we'll go in a loop here?

http://codereview.chromium.org/8201030/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_speech_input_api.cc:78: return false;
DCHECK(options) after this

http://codereview.chromium.org/8201030/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_speech_input_api.cc:81:
options->GetString(constants::kLanguageKey, &language);
should you check the return value of these calls?

http://codereview.chromium.org/8201030/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_speech_input_api.cc:118: switch
(ExtensionSpeechInputManager::GetForProfile(profile())->state()) {
This only allows you to tell if another extension was using speech input. What
if a web page was using instead? Ideally this call should tell the state of
speech reco in Chrome, not just about extensions, so the caller can make a
decision on what to do with UI buttons and what not

http://codereview.chromium.org/8201030/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_speech_input_api.cc:119: case
ExtensionSpeechInputManager::kIdle:
case statements indented to right by 2 spaces
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Loops_and_Swit...

http://codereview.chromium.org/8201030/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_speech_input_api.h (right):

http://codereview.chromium.org/8201030/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_speech_input_api.h:24: virtual int
StartState() = 0;
it may be simpler to define 4 const integers as member variables in this class
and initialize them in the derived class constructor.

http://codereview.chromium.org/8201030/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_speech_input_api_constants.cc (right):

http://codereview.chromium.org/8201030/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_speech_input_api_constants.cc:13: const char
kProfanitiesBlock[] = "block";
what are these 3 used for? filterProfanities is a boolean now so these shouldn't
be required.

http://codereview.chromium.org/8201030/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_speech_input_api_constants.cc:36: const char
kDefaultGrammar[] = "builtin:search";
should this default to 'builtin:dictation' as extensions aren't always going to
be built for searching stuff? just something to think about and talk with more
people

http://codereview.chromium.org/8201030/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_speech_input_api_constants.h (right):

http://codereview.chromium.org/8201030/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_speech_input_api_constants.h:39: extern
const bool kDefaultProcessPendingResults;
Should this be removed? I don't see it used anywhere or defined

http://codereview.chromium.org/8201030/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_speech_input_manager.cc (right):

http://codereview.chromium.org/8201030/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_speech_input_manager.cc:70: static int
per_profile_id_;
add a comment here explaining that this is used for generating the caller ids
for speech sessions and why it is needed

http://codereview.chromium.org/8201030/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_speech_input_manager.cc:461: recognizer_ =
new SpeechRecognizer(this, speech_caller_id_, language, grammar,
since you are directly using a SpeechRecognizer object, is there a way to avoid
clashes with webpage requested speech input sessions? Right now it looks like
SpeechInputManager and this extension manager don't know about each other so
they'll clash with each other if the page and an extension use speech input at
the same time.

http://codereview.chromium.org/8201030/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_speech_input_manager.h (right):

http://codereview.chromium.org/8201030/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_speech_input_manager.h:42:
ExtensionSpeechInputManager(Profile* profile, size_t speech_caller_id);
why is the second parameter a size_t and not int?

http://codereview.chromium.org/8201030/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_speech_input_manager.h:68: const
NotificationSource& source,
suggest indenting these params to right and match the first one, or move the
first one to the next line so all params are aligned with each other

http://codereview.chromium.org/8201030/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_speech_input_manager.h:81: virtual void
OnRecognizerError(int caller_id,
ditto

http://codereview.chromium.org/8201030/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_speech_input_manager.h:86: private:
add a newline above

http://codereview.chromium.org/8201030/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_speech_input_manager.h:102: const
std::string& json_args);
align with param in previous line

http://codereview.chromium.org/8201030/diff/1/chrome/common/chrome_notificati...
File chrome/common/chrome_notification_types.h (right):

http://codereview.chromium.org/8201030/diff/1/chrome/common/chrome_notificati...
chrome/common/chrome_notification_types.h:509:
NOTIFICATION_EXTENSION_SPEECH_INPUT_STARTED,
should these be named as SPEECH_INPUT_RECORDING_STARTED (and similarly below) as
they are specifically about recording and not about the full session (i.e.
results available as well) ?

http://codereview.chromium.org/8201030/diff/1/chrome/common/extensions/docs/e...
File chrome/common/extensions/docs/experimental.speechInput.html (right):

http://codereview.chromium.org/8201030/diff/1/chrome/common/extensions/docs/e...
chrome/common/extensions/docs/experimental.speechInput.html:1014: <p>Called when
the system detects enough silence to consider the ongoing input has ended.</p>
input -> speech

http://codereview.chromium.org/8201030/diff/1/chrome/common/extensions/docs/s...
File chrome/common/extensions/docs/samples.json (right):

http://codereview.chromium.org/8201030/diff/1/chrome/common/extensions/docs/s...
chrome/common/extensions/docs/samples.json:85:
"chrome.experimental.speechInput.onSpeechResult":
"experimental.speechInput.html#event-onSpeechResult",
any reason for this name change? Seems like speechInput.onResult wouldn't have
clashed with other event handlers

[Leandro Graciá Gil, 2011-10-09 10:29:21]

http://codereview.chromium.org/8201030/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_speech_input_api.cc (right):

http://codereview.chromium.org/8201030/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_speech_input_api.cc:29: registrar_.Add(this,
TransitionNotification(),
Removed on destruction with a flag to ensure we're listening (once).

On 2011/10/08 23:15:07, Satish wrote:
> when do you remove as a notification listener? should it be done in Observe
> after you got the relevant notification or failure?

http://codereview.chromium.org/8201030/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_speech_input_api.cc:45: if (type ==
chrome::NOTIFICATION_EXTENSION_SPEECH_INPUT_FAILED)
On 2011/10/08 23:15:07, Satish wrote:
> suggest using braces for if-else blocks

Done.

http://codereview.chromium.org/8201030/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_speech_input_api.cc:50: Run();
It should never be the case, as the transition notifications always come
together with state changes in the manager. In any case, I'm adding a flag to
ensure that we never try to register twice for listening.

On 2011/10/08 23:15:07, Satish wrote:
> is there a possibility that in this call to Run() that the extension manager's
> state is still the same as TransitionState() so we'll go in a loop here?

http://codereview.chromium.org/8201030/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_speech_input_api.cc:78: return false;
On 2011/10/08 23:15:07, Satish wrote:
> DCHECK(options) after this

Done.

http://codereview.chromium.org/8201030/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_speech_input_api.cc:81:
options->GetString(constants::kLanguageKey, &language);
The intention is that if they fail the variables will keep their default values.

On 2011/10/08 23:15:07, Satish wrote:
> should you check the return value of these calls?

http://codereview.chromium.org/8201030/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_speech_input_api.cc:118: switch
(ExtensionSpeechInputManager::GetForProfile(profile())->state()) {
Will take a look into this after the next patch to avoid delaying the iterative
process.

On 2011/10/08 23:15:07, Satish wrote:
> This only allows you to tell if another extension was using speech input. What
> if a web page was using instead? Ideally this call should tell the state of
> speech reco in Chrome, not just about extensions, so the caller can make a
> decision on what to do with UI buttons and what not

http://codereview.chromium.org/8201030/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_speech_input_api.cc:119: case
ExtensionSpeechInputManager::kIdle:
Good to know. I completely missed that style point. Thanks.

On 2011/10/08 23:15:07, Satish wrote:
> case statements indented to right by 2 spaces
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Loops_and_Swit...

http://codereview.chromium.org/8201030/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_speech_input_api.h (right):

http://codereview.chromium.org/8201030/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_speech_input_api.h:24: virtual int
StartState() = 0;
On 2011/10/08 23:15:07, Satish wrote:
> it may be simpler to define 4 const integers as member variables in this class
> and initialize them in the derived class constructor.

Done.

http://codereview.chromium.org/8201030/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_speech_input_api_constants.cc (right):

http://codereview.chromium.org/8201030/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_speech_input_api_constants.cc:13: const char
kProfanitiesBlock[] = "block";
On 2011/10/08 23:15:07, Satish wrote:
> what are these 3 used for? filterProfanities is a boolean now so these
shouldn't
> be required.

Done.

http://codereview.chromium.org/8201030/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_speech_input_api_constants.cc:36: const char
kDefaultGrammar[] = "builtin:search";
Yes, you got a point. That's something we should probably discuss.

On 2011/10/08 23:15:07, Satish wrote:
> should this default to 'builtin:dictation' as extensions aren't always going
to
> be built for searching stuff? just something to think about and talk with more
> people

http://codereview.chromium.org/8201030/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_speech_input_api_constants.h (right):

http://codereview.chromium.org/8201030/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_speech_input_api_constants.h:39: extern
const bool kDefaultProcessPendingResults;
On 2011/10/08 23:15:07, Satish wrote:
> Should this be removed? I don't see it used anywhere or defined

Done.

http://codereview.chromium.org/8201030/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_speech_input_manager.cc (right):

http://codereview.chromium.org/8201030/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_speech_input_manager.cc:70: static int
per_profile_id_;
On 2011/10/08 23:15:07, Satish wrote:
> add a comment here explaining that this is used for generating the caller ids
> for speech sessions and why it is needed

Done.

http://codereview.chromium.org/8201030/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_speech_input_manager.cc:461: recognizer_ =
new SpeechRecognizer(this, speech_caller_id_, language, grammar,
If that happens they will clash but also produce an error coming from the
expected error-handling methods. It should be notified to the user as usual, and
in any case either handling the problem here or later would be completely
asynchronous from the user's point of view.

I'll do some testing to ensure no assertions fail in these situations.

On 2011/10/08 23:15:07, Satish wrote:
> since you are directly using a SpeechRecognizer object, is there a way to
avoid
> clashes with webpage requested speech input sessions? Right now it looks like
> SpeechInputManager and this extension manager don't know about each other so
> they'll clash with each other if the page and an extension use speech input at
> the same time.

http://codereview.chromium.org/8201030/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_speech_input_manager.h (right):

http://codereview.chromium.org/8201030/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_speech_input_manager.h:42:
ExtensionSpeechInputManager(Profile* profile, size_t speech_caller_id);
On 2011/10/08 23:15:07, Satish wrote:
> why is the second parameter a size_t and not int?

Done.

http://codereview.chromium.org/8201030/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_speech_input_manager.h:68: const
NotificationSource& source,
On 2011/10/08 23:15:07, Satish wrote:
> suggest indenting these params to right and match the first one, or move the
> first one to the next line so all params are aligned with each other

Done.

http://codereview.chromium.org/8201030/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_speech_input_manager.h:81: virtual void
OnRecognizerError(int caller_id,
On 2011/10/08 23:15:07, Satish wrote:
> ditto

Done.

http://codereview.chromium.org/8201030/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_speech_input_manager.h:86: private:
On 2011/10/08 23:15:07, Satish wrote:
> add a newline above

Done.

http://codereview.chromium.org/8201030/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_speech_input_manager.h:102: const
std::string& json_args);
On 2011/10/08 23:15:07, Satish wrote:
> align with param in previous line

Done.

http://codereview.chromium.org/8201030/diff/1/chrome/common/chrome_notificati...
File chrome/common/chrome_notification_types.h (right):

http://codereview.chromium.org/8201030/diff/1/chrome/common/chrome_notificati...
chrome/common/chrome_notification_types.h:509:
NOTIFICATION_EXTENSION_SPEECH_INPUT_STARTED,
Fixed. Result availability is not required as these notifications are actually
dispatched to allow SpeechInputAsyncFunction to detect manager state machine
changes.

On 2011/10/08 23:15:07, Satish wrote:
> should these be named as SPEECH_INPUT_RECORDING_STARTED (and similarly below)
as
> they are specifically about recording and not about the full session (i.e.
> results available as well) ?

http://codereview.chromium.org/8201030/diff/1/chrome/common/extensions/docs/e...
File chrome/common/extensions/docs/experimental.speechInput.html (right):

http://codereview.chromium.org/8201030/diff/1/chrome/common/extensions/docs/e...
chrome/common/extensions/docs/experimental.speechInput.html:1014: <p>Called when
the system detects enough silence to consider the ongoing input has ended.</p>
On 2011/10/08 23:15:07, Satish wrote:
> input -> speech

Done.

http://codereview.chromium.org/8201030/diff/1/chrome/common/extensions/docs/s...
File chrome/common/extensions/docs/samples.json (right):

http://codereview.chromium.org/8201030/diff/1/chrome/common/extensions/docs/s...
chrome/common/extensions/docs/samples.json:85:
"chrome.experimental.speechInput.onSpeechResult":
"experimental.speechInput.html#event-onSpeechResult",
No real reason, just thought it might be clearer. Leaving it as it was and
updating the doc.

On 2011/10/08 23:15:07, Satish wrote:
> any reason for this name change? Seems like speechInput.onResult wouldn't have
> clashed with other event handlers

[Satish, 2011-10-09 20:01:47]

Sorry, this one is a bit long.

http://codereview.chromium.org/8201030/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_speech_input_manager.cc (right):

http://codereview.chromium.org/8201030/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_speech_input_manager.cc:461: recognizer_ =
new SpeechRecognizer(this, speech_caller_id_, language, grammar,
There is one issue - if an extension was already doing speech input and another
asked for it, we are returning permission-denied error now. We should return the
same error if a web page was currently doing speech recognition. And similarly
in the html speech input, the error returned should be the same irrespective of
whether another webpage was recording or an extension was. Can you ensure that?

On 2011/10/09 10:29:21, Leandro Graciá Gil wrote:
> If that happens they will clash but also produce an error coming from the
> expected error-handling methods. It should be notified to the user as usual,
and
> in any case either handling the problem here or later would be completely
> asynchronous from the user's point of view.
> 
> I'll do some testing to ensure no assertions fail in these situations.
> 
> On 2011/10/08 23:15:07, Satish wrote:
> > since you are directly using a SpeechRecognizer object, is there a way to
> avoid
> > clashes with webpage requested speech input sessions? Right now it looks
like
> > SpeechInputManager and this extension manager don't know about each other so
> > they'll clash with each other if the page and an extension use speech input
at
> > the same time.
>

http://codereview.chromium.org/8201030/diff/10001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_speech_input_api.cc (right):

http://codereview.chromium.org/8201030/diff/10001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_speech_input_api.cc:26: if (listening_)
I looked at NotificationRegistrar's destructor and it seems to call RemoveAll()
already, so this call is not required. My earlier comment regd. RemoveAll was to
check whether you still need to be subscribed for notifications after you
receive the notification in Observe - and whether your Observe implementation
handles it properly if there were multiple notifications received. Could you
check on that?

http://codereview.chromium.org/8201030/diff/10001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_speech_input_api.cc:63: if (type ==
chrome::NOTIFICATION_EXTENSION_SPEECH_INPUT_RECORDING_FAILED) {
You aren't registering for this notification type anywhere. Are you sure you'll
be receiving it here?

http://codereview.chromium.org/8201030/diff/10001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_speech_input_api.h (right):

http://codereview.chromium.org/8201030/diff/10001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_speech_input_api.h:20: int end_state, int
transition_notification);
parameters should either be on the same line or aligned together.
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Decla...

http://codereview.chromium.org/8201030/diff/10001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_speech_input_api.h:27: const int
kStartState_;
these can be private as they are not used by the derived classes

http://codereview.chromium.org/8201030/diff/10001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_speech_input_api.h:27: const int
kStartState_;
looking at other code I see the trend is to usually have variables like these
(set in constructor) as non-const variables. Could you drop the const for these?

http://codereview.chromium.org/8201030/diff/10001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_speech_input_manager.cc (right):

http://codereview.chromium.org/8201030/diff/10001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_speech_input_manager.cc:73: static int
per_profile_id_;
The caller id give to SpeechRecognizer is useful only when there is a
possibility of more than 1 speech input session active at the same time. For
example in the html speech input case one element may call stopRecognition and
while the results are still being fetched from server another element may call
startRecognition resulting in 2 active sessions (but only 1 recording audio). I
see in ExtensionSpeechInputManager::ForceStopOnIOThread() you always call
SpeechRecognizer::CancelRecognition(), so you will never have 2 active sessions
for extensions ever. In this case the caller ID doesn't have to be unique so you
could just pass a constant (even 1) for this id.

http://codereview.chromium.org/8201030/diff/10001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_speech_input_manager.cc:149:
state_lock_.Acquire();
use base::AutoLock instead of acquiring and releasing manually everywhere

http://codereview.chromium.org/8201030/diff/10001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_speech_input_manager.cc:173: VLOG(1) <<
"Extension unloaded. Requesting to enforce stop...";
should this be after the below if block?

http://codereview.chromium.org/8201030/diff/10001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_speech_input_manager.cc:180: if
(!extension_id_.empty() && extension_id_ == id) {
is the .empty() check required since it is a string and == would fail anyway?

http://codereview.chromium.org/8201030/diff/10001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_speech_input_manager.cc:181:
extension_id_.clear();
You are clearing this id here but in ForceStopOnIOThread() extension_id is being
sent as part of the notification (since state != kShutdown). Is that the
expected behaviour?

http://codereview.chromium.org/8201030/diff/10001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_speech_input_manager.cc:200:
&ExtensionSpeechInputManager::SetRecognitionResultOnUIThread, result));
You may need to convert all NewRunnableMethod calls to the new base::Bind. See
the mail in chromium-dev with subject "Removing NewRunnableMethod() and
NewCallback()"

On a related note, we'll need to do this for the rest of speech input code as
well. Perhaps a task for a future CL?

http://codereview.chromium.org/8201030/diff/10001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_speech_input_manager.cc:204: const
SpeechInputResult& result) {
fix indentation

http://codereview.chromium.org/8201030/diff/10001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_speech_input_manager.cc:251: VLOG(1) <<
"DidCompleteRecording";
if you don't intend to add any code here and the next method i'd advise to
reduce log spam and keep these methods empty. same for other similar methods in
this file

http://codereview.chromium.org/8201030/diff/10001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_speech_input_manager.cc:295: case
kErrorNone:
fix indentation of case statements

http://codereview.chromium.org/8201030/diff/10001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_speech_input_manager.cc:299: if (state_ ==
kStarting)
use braces

http://codereview.chromium.org/8201030/diff/10001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_speech_input_manager.cc:333: if
(!event_error_code.empty())
ditto

http://codereview.chromium.org/8201030/diff/10001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_speech_input_manager.cc:352:
base::JSONWriter::Write(&args, false, &json_args);
is it really required to send an empty list as arguments to events without
parameters such as this? Or can you send an empty/null string which would be
simpler..

If it is required, may be simpler to do this once within
DispatchEventToExtension

http://codereview.chromium.org/8201030/diff/10001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_speech_input_manager.cc:382: if (profile_ &&
profile_->GetExtensionEventRouter())
use braces

http://codereview.chromium.org/8201030/diff/10001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_speech_input_manager.cc:416: if
(!extension_id_.empty() && extension_id != extension_id_) {
is the .empty check required as asked above?

http://codereview.chromium.org/8201030/diff/10001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_speech_input_manager.cc:422: if (state_ !=
kIdle) {
could be simpler to merge this check with the 'if (ShuttingDown())' check above

http://codereview.chromium.org/8201030/diff/10001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_speech_input_manager.cc:483: if (state_ !=
kRecording) {
merge this with the first if statement here?

http://codereview.chromium.org/8201030/diff/10001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_speech_input_manager.cc:508: recognizer_ =
NULL;
recognizer_.release() ?

http://codereview.chromium.org/8201030/diff/10001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_speech_input_manager.h (right):

http://codereview.chromium.org/8201030/diff/10001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_speech_input_manager.h:119: base::Lock
state_lock_;
since you have a lock only for accessing/modifying the state_ variable, can you
comment here why this is the case? It would also be useful to separate out the
list of member vars based on which threads they are used in and comment on that
so it is clear to readers

http://codereview.chromium.org/8201030/diff/10001/chrome/common/chrome_notifi...
File chrome/common/chrome_notification_types.h (right):

http://codereview.chromium.org/8201030/diff/10001/chrome/common/chrome_notifi...
chrome/common/chrome_notification_types.h:515:
NOTIFICATION_EXTENSION_SPEECH_INPUT_RECORDING_FAILED,
this shouldn't have RECORDING_ in the name and comment because it is a
notification for all failures, not just recording failures

http://codereview.chromium.org/8201030/diff/10001/chrome/common/extensions/ap...
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/8201030/diff/10001/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api.json:583: "id": "ResultEvent",
while events and methods are always accessed via the speechInput namespace, I'm
not sure if types are. Can you check? Would this cause a name clash with any
other api if that api also uses the name 'ResultEvent' as a type name? In that
case a prefix of 'SpeechInput' may be appropriate for all types of this api

[Leandro Graciá Gil, 2011-10-10 18:59:38]

IsRecording validity still pending to be discussed and fixed. Patch tested and
working properly.

http://codereview.chromium.org/8201030/diff/10001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_speech_input_api.cc (right):

http://codereview.chromium.org/8201030/diff/10001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_speech_input_api.cc:26: if (listening_)
The idea was that the object would be destroyed after sending the response so I
didn't matter much. In any case I'm moving the unregistration to just before
sending the response. This should avoid any problems with multiple notifications
before the object is destroyed.

On 2011/10/09 20:01:47, Satish wrote:
> I looked at NotificationRegistrar's destructor and it seems to call
RemoveAll()
> already, so this call is not required. My earlier comment regd. RemoveAll was
to
> check whether you still need to be subscribed for notifications after you
> receive the notification in Observe - and whether your Observe implementation
> handles it properly if there were multiple notifications received. Could you
> check on that?

http://codereview.chromium.org/8201030/diff/10001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_speech_input_api.cc:63: if (type ==
chrome::NOTIFICATION_EXTENSION_SPEECH_INPUT_RECORDING_FAILED) {
You've got a point. I'll fix it.

On 2011/10/09 20:01:47, Satish wrote:
> You aren't registering for this notification type anywhere. Are you sure
you'll
> be receiving it here?

http://codereview.chromium.org/8201030/diff/10001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_speech_input_api.h (right):

http://codereview.chromium.org/8201030/diff/10001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_speech_input_api.h:20: int end_state, int
transition_notification);
On 2011/10/09 20:01:47, Satish wrote:
> parameters should either be on the same line or aligned together.
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Decla...

Done.

http://codereview.chromium.org/8201030/diff/10001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_speech_input_api.h:27: const int
kStartState_;
On 2011/10/09 20:01:47, Satish wrote:
> these can be private as they are not used by the derived classes

Done.

http://codereview.chromium.org/8201030/diff/10001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_speech_input_api.h:27: const int
kStartState_;
On 2011/10/09 20:01:47, Satish wrote:
> looking at other code I see the trend is to usually have variables like these
> (set in constructor) as non-const variables. Could you drop the const for
these?

Done.

http://codereview.chromium.org/8201030/diff/10001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_speech_input_manager.cc (right):

http://codereview.chromium.org/8201030/diff/10001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_speech_input_manager.cc:73: static int
per_profile_id_;
On 2011/10/09 20:01:47, Satish wrote:
> The caller id give to SpeechRecognizer is useful only when there is a
> possibility of more than 1 speech input session active at the same time. For
> example in the html speech input case one element may call stopRecognition and
> while the results are still being fetched from server another element may call
> startRecognition resulting in 2 active sessions (but only 1 recording audio).
I
> see in ExtensionSpeechInputManager::ForceStopOnIOThread() you always call
> SpeechRecognizer::CancelRecognition(), so you will never have 2 active
sessions
> for extensions ever. In this case the caller ID doesn't have to be unique so
you
> could just pass a constant (even 1) for this id.

Done.

http://codereview.chromium.org/8201030/diff/10001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_speech_input_manager.cc:149:
state_lock_.Acquire();
On 2011/10/09 20:01:47, Satish wrote:
> use base::AutoLock instead of acquiring and releasing manually everywhere

Done.

http://codereview.chromium.org/8201030/diff/10001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_speech_input_manager.cc:173: VLOG(1) <<
"Extension unloaded. Requesting to enforce stop...";
On 2011/10/09 20:01:47, Satish wrote:
> should this be after the below if block?

Done.

http://codereview.chromium.org/8201030/diff/10001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_speech_input_manager.cc:180: if
(!extension_id_.empty() && extension_id_ == id) {
On 2011/10/09 20:01:47, Satish wrote:
> is the .empty() check required since it is a string and == would fail anyway?

Done.

http://codereview.chromium.org/8201030/diff/10001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_speech_input_manager.cc:181:
extension_id_.clear();
Now clearing only on ForceStopOnIOThread. Fixed.

On 2011/10/09 20:01:47, Satish wrote:
> You are clearing this id here but in ForceStopOnIOThread() extension_id is
being
> sent as part of the notification (since state != kShutdown). Is that the
> expected behaviour?

http://codereview.chromium.org/8201030/diff/10001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_speech_input_manager.cc:200:
&ExtensionSpeechInputManager::SetRecognitionResultOnUIThread, result));
Fixed here. I'll prepare a CL for the speech input code when I have some time.
It should be fast to review and land.

On 2011/10/09 20:01:47, Satish wrote:
> You may need to convert all NewRunnableMethod calls to the new base::Bind. See
> the mail in chromium-dev with subject "Removing NewRunnableMethod() and
> NewCallback()"
> 
> On a related note, we'll need to do this for the rest of speech input code as
> well. Perhaps a task for a future CL?

http://codereview.chromium.org/8201030/diff/10001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_speech_input_manager.cc:204: const
SpeechInputResult& result) {
On 2011/10/09 20:01:47, Satish wrote:
> fix indentation

Done.

http://codereview.chromium.org/8201030/diff/10001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_speech_input_manager.cc:251: VLOG(1) <<
"DidCompleteRecording";
On 2011/10/09 20:01:47, Satish wrote:
> if you don't intend to add any code here and the next method i'd advise to
> reduce log spam and keep these methods empty. same for other similar methods
in
> this file

Done.

http://codereview.chromium.org/8201030/diff/10001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_speech_input_manager.cc:295: case
kErrorNone:
On 2011/10/09 20:01:47, Satish wrote:
> fix indentation of case statements

Done.

http://codereview.chromium.org/8201030/diff/10001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_speech_input_manager.cc:299: if (state_ ==
kStarting)
On 2011/10/09 20:01:47, Satish wrote:
> use braces

Done.

http://codereview.chromium.org/8201030/diff/10001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_speech_input_manager.cc:333: if
(!event_error_code.empty())
On 2011/10/09 20:01:47, Satish wrote:
> ditto

Done.

http://codereview.chromium.org/8201030/diff/10001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_speech_input_manager.cc:352:
base::JSONWriter::Write(&args, false, &json_args);
It is required to send the empty list, otherwise you will get wrong type errors
on JS. I'm adding a check in DispatchEventToExtension that provides this empty
list for null argument lists.

On 2011/10/09 20:01:47, Satish wrote:
> is it really required to send an empty list as arguments to events without
> parameters such as this? Or can you send an empty/null string which would be
> simpler..
> 
> If it is required, may be simpler to do this once within
> DispatchEventToExtension

http://codereview.chromium.org/8201030/diff/10001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_speech_input_manager.cc:382: if (profile_ &&
profile_->GetExtensionEventRouter())
On 2011/10/09 20:01:47, Satish wrote:
> use braces

Done.

http://codereview.chromium.org/8201030/diff/10001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_speech_input_manager.cc:416: if
(!extension_id_.empty() && extension_id != extension_id_) {
Here it is, otherwise it will generate an error even when no extension is making
use of the API.

On 2011/10/09 20:01:47, Satish wrote:
> is the .empty check required as asked above?

http://codereview.chromium.org/8201030/diff/10001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_speech_input_manager.cc:422: if (state_ !=
kIdle) {
On 2011/10/09 20:01:47, Satish wrote:
> could be simpler to merge this check with the 'if (ShuttingDown())' check
above

Done.

http://codereview.chromium.org/8201030/diff/10001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_speech_input_manager.cc:483: if (state_ !=
kRecording) {
On 2011/10/09 20:01:47, Satish wrote:
> merge this with the first if statement here?

Done.

http://codereview.chromium.org/8201030/diff/10001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_speech_input_manager.cc:508: recognizer_ =
NULL;
On 2011/10/09 20:01:47, Satish wrote:
> recognizer_.release() ?

Done.

http://codereview.chromium.org/8201030/diff/10001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_speech_input_manager.h (right):

http://codereview.chromium.org/8201030/diff/10001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_speech_input_manager.h:119: base::Lock
state_lock_;
On 2011/10/09 20:01:47, Satish wrote:
> since you have a lock only for accessing/modifying the state_ variable, can
you
> comment here why this is the case? It would also be useful to separate out the
> list of member vars based on which threads they are used in and comment on
that
> so it is clear to readers

Done.

http://codereview.chromium.org/8201030/diff/10001/chrome/common/chrome_notifi...
File chrome/common/chrome_notification_types.h (right):

http://codereview.chromium.org/8201030/diff/10001/chrome/common/chrome_notifi...
chrome/common/chrome_notification_types.h:515:
NOTIFICATION_EXTENSION_SPEECH_INPUT_RECORDING_FAILED,
On 2011/10/09 20:01:47, Satish wrote:
> this shouldn't have RECORDING_ in the name and comment because it is a
> notification for all failures, not just recording failures

Done.

http://codereview.chromium.org/8201030/diff/10001/chrome/common/extensions/ap...
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/8201030/diff/10001/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api.json:583: "id": "ResultEvent",
Looks like you're right. I've tried to use a different name for the same type
contents inside the accessibility namespace and it keeps working. I'm prefixing
SpeechInput to all the local types.

On 2011/10/09 20:01:47, Satish wrote:
> while events and methods are always accessed via the speechInput namespace,
I'm
> not sure if types are. Can you check? Would this cause a name clash with any
> other api if that api also uses the name 'ResultEvent' as a type name? In that
> case a prefix of 'SpeechInput' may be appropriate for all types of this api

[Satish, 2011-10-11 13:37:16]

Thank, looking quite close.

http://codereview.chromium.org/8201030/diff/11001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_speech_input_api.cc (right):

http://codereview.chromium.org/8201030/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_speech_input_api.cc:81:
ExtensionSpeechInputManager::ExtensionError *error_details =
move '*' to be before the space

http://codereview.chromium.org/8201030/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_speech_input_api.cc:82:
Details<ExtensionSpeechInputManager::ExtensionError>(details).ptr();
indent by 2 more spaces

http://codereview.chromium.org/8201030/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_speech_input_api.cc:91:
!expecting_transition_) {
should you do a DCHECK(expecting_transition_) if the extension id was the same
as this class?

http://codereview.chromium.org/8201030/diff/11001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_speech_input_manager.cc (right):

http://codereview.chromium.org/8201030/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_speech_input_manager.cc:400: *error =
constants::kErrorRequestOngoing;
'RequestOngoing' - This error message is cryptic. Can we rename it to
'RequestInProgress' or 'AlreadyInUse' ? Same for the string sent to javascript

http://codereview.chromium.org/8201030/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_speech_input_manager.cc:454: *error =
constants::kErrorRequestOngoing;
Is this the correct error to return when Stop is called and no session is
active?

http://codereview.chromium.org/8201030/diff/11001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_speech_input_manager.h (right):

http://codereview.chromium.org/8201030/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_speech_input_manager.h:43: const
std::string& extension_id_;
these shouldn't be a reference as they'll point to invalid objects once the
calling code goes out of scope

http://codereview.chromium.org/8201030/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_speech_input_manager.h:123: static const int
kSpeechCallerId = 1;
suggest moving this to the anonymous namespace in the .cc file, its an
implementation detail that users of this class shouldn't care about

[Leandro Graciá Gil, 2011-10-11 18:08:00]

Errors separated into call (start/stop) errors and recognition (onError event)
ones. Added some refactoring to allow API tests to be implemented by a later
patch.

Currently working on the IsRecording issue and the exclusivity with the
web-based speech input feature. To be addressed in the next review patch.

http://codereview.chromium.org/8201030/diff/11001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_speech_input_api.cc (right):

http://codereview.chromium.org/8201030/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_speech_input_api.cc:81:
ExtensionSpeechInputManager::ExtensionError *error_details =
On 2011/10/11 13:37:16, Satish wrote:
> move '*' to be before the space

Done.

http://codereview.chromium.org/8201030/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_speech_input_api.cc:82:
Details<ExtensionSpeechInputManager::ExtensionError>(details).ptr();
On 2011/10/11 13:37:16, Satish wrote:
> indent by 2 more spaces

Done.

http://codereview.chromium.org/8201030/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_speech_input_api.cc:91:
!expecting_transition_) {
On 2011/10/11 13:37:16, Satish wrote:
> should you do a DCHECK(expecting_transition_) if the extension id was the same
> as this class?

Done.

http://codereview.chromium.org/8201030/diff/11001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_speech_input_manager.cc (right):

http://codereview.chromium.org/8201030/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_speech_input_manager.cc:400: *error =
constants::kErrorRequestOngoing;
On 2011/10/11 13:37:16, Satish wrote:
> 'RequestOngoing' - This error message is cryptic. Can we rename it to
> 'RequestInProgress' or 'AlreadyInUse' ? Same for the string sent to javascript

Done.

http://codereview.chromium.org/8201030/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_speech_input_manager.cc:454: *error =
constants::kErrorRequestOngoing;
Fixed in later modifications of the code.

On 2011/10/11 13:37:16, Satish wrote:
> Is this the correct error to return when Stop is called and no session is
> active?

http://codereview.chromium.org/8201030/diff/11001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_speech_input_manager.h (right):

http://codereview.chromium.org/8201030/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_speech_input_manager.h:43: const
std::string& extension_id_;
Used references because the notification is synchronous. In any case, it's safer
to do as you suggest. Fixed.

On 2011/10/11 13:37:16, Satish wrote:
> these shouldn't be a reference as they'll point to invalid objects once the
> calling code goes out of scope

http://codereview.chromium.org/8201030/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_speech_input_manager.h:123: static const int
kSpeechCallerId = 1;
On 2011/10/11 13:37:16, Satish wrote:
> suggest moving this to the anonymous namespace in the .cc file, its an
> implementation detail that users of this class shouldn't care about

Done.

[Leandro Graciá Gil, 2011-10-11 18:10:51]

http://codereview.chromium.org/8201030/diff/17004/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_speech_input_manager.cc (right):

http://codereview.chromium.org/8201030/diff/17004/chrome/browser/extensions/e...
chrome/browser/extensions/extension_speech_input_manager.cc:72: virtual bool
ServiceRedirectedInIncognito() { return false; }
Adding OVERRIDE to these.

[Aaron Boodman, 2011-10-12 08:04:08]

chrome/browser/extensions has too many files. Can we move
extension_speech_input_* elsewhere?

It seems like a subdirectory of chrome/browser might be appropriate since
extension_speech_input_manager isn't super extension-specific.

Otherwise a subdirectory of chrome/browser/extensions would be OK.

[Leandro Graciá Gil, 2011-10-12 14:29:32]

Adding exclusive recording between the web speech input feature and the
extension API. Moving the extension API files to a subfolder of
chrome/browser/extensions as suggested.

[Leandro Graciá Gil, 2011-10-12 14:32:39]

Alpha, could you please review the changes on the media files of this patch?
Thanks.

[Leandro Graciá Gil, 2011-10-12 16:20:01]

Moving all media-related changes to issue 8208013.

Alpha, no need to look further on this patch. Thanks for your help.

[Leandro Graciá Gil, 2011-10-12 18:24:46]

http://codereview.chromium.org/8201030/diff/24002/chrome/browser/extensions/s...
File chrome/browser/extensions/speech_input/extension_speech_input_manager.cc
(right):

http://codereview.chromium.org/8201030/diff/24002/chrome/browser/extensions/s...
chrome/browser/extensions/speech_input/extension_speech_input_manager.cc:218:
/*{
Oops, this should be removed as it's not required. I'll fix it.

[Satish, 2011-10-12 20:11:59]

LGTM, thanks for the changes.

http://codereview.chromium.org/8201030/diff/22034/chrome/browser/extensions/s...
File chrome/browser/extensions/speech_input/extension_speech_input_api.cc
(right):

http://codereview.chromium.org/8201030/diff/22034/chrome/browser/extensions/s...
chrome/browser/extensions/speech_input/extension_speech_input_api.cc:17: int
transition_state, int end_state, int transition_notification)
indent parameters at the same column and one per line since they all don't fit
in a single line

http://codereview.chromium.org/8201030/diff/22034/chrome/browser/extensions/s...
File chrome/browser/extensions/speech_input/extension_speech_input_manager.h
(right):

http://codereview.chromium.org/8201030/diff/22034/chrome/browser/extensions/s...
chrome/browser/extensions/speech_input/extension_speech_input_manager.h:50:
class ExtensionSpeechInputManager : public NotificationObserver,
move NotificationObserver to next line and indent the rest to right by 2 spaces

[Leandro Graciá Gil, 2011-10-13 10:40:22]

Nits fixed. Landing shortly.

http://codereview.chromium.org/8201030/diff/22034/chrome/browser/extensions/s...
File chrome/browser/extensions/speech_input/extension_speech_input_api.cc
(right):

http://codereview.chromium.org/8201030/diff/22034/chrome/browser/extensions/s...
chrome/browser/extensions/speech_input/extension_speech_input_api.cc:17: int
transition_state, int end_state, int transition_notification)
On 2011/10/12 20:11:59, Satish wrote:
> indent parameters at the same column and one per line since they all don't fit
> in a single line

Done.

http://codereview.chromium.org/8201030/diff/22034/chrome/browser/extensions/s...
File chrome/browser/extensions/speech_input/extension_speech_input_manager.h
(right):

http://codereview.chromium.org/8201030/diff/22034/chrome/browser/extensions/s...
chrome/browser/extensions/speech_input/extension_speech_input_manager.h:50:
class ExtensionSpeechInputManager : public NotificationObserver,
On 2011/10/12 20:11:59, Satish wrote:
> move NotificationObserver to next line and indent the rest to right by 2
spaces

Done.