Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9972011:
Speech refactoring: Reimplemented SpeechRecognitionManagerImpl as a FSM. (CL1.7) (Closed)
|
Created:
8 years, 8 months ago by Primiano Tucci (use gerrit) Modified:
8 years, 8 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam, stuartmorgan Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionSpeech refactoring: Reimplemented SpeechRecognitionManagerImpl as a FSM. (CL1.7)
BUG=116954
TEST=none.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=133967
Patch Set 1 : #Patch Set 2 : Rebased from master. #
Total comments: 78
Patch Set 3 : Fixed according to Satish review. #
Total comments: 28
Patch Set 4 : (Partially) fixed according to Satish review. #Patch Set 5 : Missing part of previous patch. #
Total comments: 30
Patch Set 6 : Fixed according to Hans review. #
Total comments: 10
Patch Set 7 : Fixed according to Satish review. #
Total comments: 2
Patch Set 8 : Renamed detach -> background. #
Total comments: 4
Patch Set 9 : Further Satish comments. #
Total comments: 28
Patch Set 10 : Fixed according to jam@ review (1/2) #Patch Set 11 : Fixed according to jam@ review (2/2) #
Total comments: 10
Patch Set 12 : Fixed according to hans and jam review. #Messages
Total messages: 21 (0 generated)
http://codereview.chromium.org/9972011/diff/5001/content/browser/speech/speec... File content/browser/speech/speech_recognition_manager_impl.cc (right): http://codereview.chromium.org/9972011/diff/5001/content/browser/speech/speec... content/browser/speech/speech_recognition_manager_impl.cc:33: using media::AudioManager; same as below http://codereview.chromium.org/9972011/diff/5001/content/browser/speech/speec... content/browser/speech/speech_recognition_manager_impl.cc:34: using std::string; could remove this as string is used only in 1 place http://codereview.chromium.org/9972011/diff/5001/content/browser/speech/speec... content/browser/speech/speech_recognition_manager_impl.cc:38: // A dummy implementation of the SpeechRecognitionManagerDelegate interface no indentation required within namespaces http://codereview.chromium.org/9972011/diff/5001/content/browser/speech/speec... content/browser/speech/speech_recognition_manager_impl.cc:65: } //namespace add 2 spaces before // and 1 space after // http://codereview.chromium.org/9972011/diff/5001/content/browser/speech/speec... content/browser/speech/speech_recognition_manager_impl.cc:89: if (delegate_ == NULL) use 'if (!delegate_)' http://codereview.chromium.org/9972011/diff/5001/content/browser/speech/speec... content/browser/speech/speech_recognition_manager_impl.cc:94: // Recognition sessions will be aborted by the corresponding destructors. I see the destructor of Session not explicitly aborting ongoing recording etc. Probably that happens automatically when the SpeechRecognizer object gets destroyed implicitly? Even then we should be informing the listener of that session about the event (e.g. what SessionAbort does). So looks like we need to explicitly abort all ongoing sessions here? http://codereview.chromium.org/9972011/diff/5001/content/browser/speech/speec... content/browser/speech/speech_recognition_manager_impl.cc:99: SpeechRecognitionSessionConfig& config, can this be changed to a const reference? http://codereview.chromium.org/9972011/diff/5001/content/browser/speech/speec... content/browser/speech/speech_recognition_manager_impl.cc:148: if (interactive_session_id_ > 0 && interactive_session_id_ != session_id) { could remove braces http://codereview.chromium.org/9972011/diff/5001/content/browser/speech/speec... content/browser/speech/speech_recognition_manager_impl.cc:148: if (interactive_session_id_ > 0 && interactive_session_id_ != session_id) { > 0 should be changed to != kSessionIDInvalid here and other places http://codereview.chromium.org/9972011/diff/5001/content/browser/speech/speec... content/browser/speech/speech_recognition_manager_impl.cc:235: DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO)); could move this check to the very beginning (same for all methods below). Also this is probably something to add in all the listener methods http://codereview.chromium.org/9972011/diff/5001/content/browser/speech/speec... content/browser/speech/speech_recognition_manager_impl.cc:248: event_args.speech_result = &result; can this be passed by value to avoid any use-after-free type situations in future? http://codereview.chromium.org/9972011/diff/5001/content/browser/speech/speec... content/browser/speech/speech_recognition_manager_impl.cc:260: event_args.speech_error = &error; ditto http://codereview.chromium.org/9972011/diff/5001/content/browser/speech/speec... content/browser/speech/speech_recognition_manager_impl.cc:281: int SpeechRecognitionManagerImpl::LookupSessionByContext( Since this class is the broker for SessionContext data between the dispatcher host and delegates, I think it is fine for it to know and access the members instead of treating it as an opaque entity. So this method can be changed to a simpler one where caller passes in a SessionContext and it just matches the fields before returning the right session id http://codereview.chromium.org/9972011/diff/5001/content/browser/speech/speec... content/browser/speech/speech_recognition_manager_impl.cc:282: Callback<bool(const SpeechRecognitionSessionContext&)> matcher) const { indent by 4 spaces only http://codereview.chromium.org/9972011/diff/5001/content/browser/speech/speec... content/browser/speech/speech_recognition_manager_impl.cc:295: // TODO(primiano) are we really sure that iterator->second will always give us I've seen more code rely on that behavior so could remove this todo http://codereview.chromium.org/9972011/diff/5001/content/browser/speech/speec... content/browser/speech/speech_recognition_manager_impl.cc:298: SpeechRecognitionSessionContext& could return this by value or if you prefer pass in a pointer where the contents get copied to http://codereview.chromium.org/9972011/diff/5001/content/browser/speech/speec... content/browser/speech/speech_recognition_manager_impl.cc:308: // AbortSession is asyncrhonous and the session will not be removed from the asyncrhonous -> asynchronous http://codereview.chromium.org/9972011/diff/5001/content/browser/speech/speec... content/browser/speech/speech_recognition_manager_impl.cc:336: session.state == STATE_ENDED_WITH_ERROR) suggest braces for multiline if statements like these http://codereview.chromium.org/9972011/diff/5001/content/browser/speech/speec... content/browser/speech/speech_recognition_manager_impl.cc:379: return DoNothing(session, event_args); should be calling NotFeasible instead of DoNothing http://codereview.chromium.org/9972011/diff/5001/content/browser/speech/speec... content/browser/speech/speech_recognition_manager_impl.cc:394: case STATE_DETACHABLE: it would be clearer to understand if 'detachable' can be renamed to 'waiting_for_result' or something similar. http://codereview.chromium.org/9972011/diff/5001/content/browser/speech/speec... content/browser/speech/speech_recognition_manager_impl.cc:416: case STATE_DETACHED: since this seems to be used only in one shot recognition, you could move to STATE_IDLE and handle EVENT_RECOGNITION_ENDED there to call SessionDelete. That way we don't need a separate STATE_DETACHED. Same suggestion for the ABORTING & ENDED_WITH_ERROR states, perhaps they all can be the same as IDLE so after the session ends it goes back to IDLE http://codereview.chromium.org/9972011/diff/5001/content/browser/speech/speec... content/browser/speech/speech_recognition_manager_impl.cc:481: if (interactive_session_id_ != 0) replace 0 with kSessionIDInvalid here and other places http://codereview.chromium.org/9972011/diff/5001/content/browser/speech/speec... File content/browser/speech/speech_recognition_manager_impl.h (right): http://codereview.chromium.org/9972011/diff/5001/content/browser/speech/speec... content/browser/speech/speech_recognition_manager_impl.h:26: struct SpeechRecognitionError; order alphabetically http://codereview.chromium.org/9972011/diff/5001/content/browser/speech/speec... content/browser/speech/speech_recognition_manager_impl.h:37: // user attention (showing a bubble). For privacy reasons, only the interactive since the bubble concept is not applicable for all users of this class (e.g. extension api and in future continuous recognition) suggest removing that from the comment http://codereview.chromium.org/9972011/diff/5001/content/browser/speech/speec... content/browser/speech/speech_recognition_manager_impl.h:44: // one of them accesses the audio device coherently. coherently -> at any given time http://codereview.chromium.org/9972011/diff/5001/content/browser/speech/speec... content/browser/speech/speech_recognition_manager_impl.h:170: int interactive_session_id_; interactive -> active ? http://codereview.chromium.org/9972011/diff/5001/content/content_browser.gypi File content/content_browser.gypi (right): http://codereview.chromium.org/9972011/diff/5001/content/content_browser.gypi... content/content_browser.gypi:139: 'public/browser/speech_recognition_session_context.cc', please also include the header files in here http://codereview.chromium.org/9972011/diff/5001/content/public/browser/speec... File content/public/browser/speech_recognition_manager.h (right): http://codereview.chromium.org/9972011/diff/5001/content/public/browser/speec... content/public/browser/speech_recognition_manager.h:19: // The SpeechRecognitionManager (SRM) is a singleton class that handles SR I like this comment block :) but feels verbose. Can it be written more succinctly? http://codereview.chromium.org/9972011/diff/5001/content/public/browser/speec... File content/public/browser/speech_recognition_manager_delegate.h (right): http://codereview.chromium.org/9972011/diff/5001/content/public/browser/speec... content/public/browser/speech_recognition_manager_delegate.h:30: // on SpeechInutManager in response. TODO(primiano) ?!?. is this TODO required? If so, should be at the start of the todo comment http://codereview.chromium.org/9972011/diff/5001/content/public/browser/speec... content/public/browser/speech_recognition_manager_delegate.h:33: // Check if current UI set-up allow speech recognition. allow -> allows http://codereview.chromium.org/9972011/diff/5001/content/public/browser/speec... content/public/browser/speech_recognition_manager_delegate.h:34: virtual bool CheckRecognitionIsAllowed(int session_id) = 0; could rename as 'IsRecognitionAllowed' http://codereview.chromium.org/9972011/diff/5001/content/public/browser/speec... File content/public/browser/speech_recognition_session_config.cc (right): http://codereview.chromium.org/9972011/diff/5001/content/public/browser/speec... content/public/browser/speech_recognition_session_config.cc:10: : filter_profanities(NULL), remove extra space after colon http://codereview.chromium.org/9972011/diff/5001/content/public/browser/speec... content/public/browser/speech_recognition_session_config.cc:10: : filter_profanities(NULL), NULL -> false http://codereview.chromium.org/9972011/diff/5001/content/public/browser/speec... content/public/browser/speech_recognition_session_config.cc:14: SpeechRecognitionSessionConfig::~SpeechRecognitionSessionConfig() { it this empty destructor required and if not could remove? http://codereview.chromium.org/9972011/diff/5001/content/public/browser/speec... File content/public/browser/speech_recognition_session_config.h (right): http://codereview.chromium.org/9972011/diff/5001/content/public/browser/speec... content/public/browser/speech_recognition_session_config.h:11: #include "ui/gfx/rect.h" this doesn't seem required http://codereview.chromium.org/9972011/diff/5001/content/public/browser/speec... content/public/browser/speech_recognition_session_config.h:26: net::URLRequestContextGetter* url_context_getter; url_context_getter -> url_request_context_getter http://codereview.chromium.org/9972011/diff/5001/content/public/browser/speec... File content/public/browser/speech_recognition_session_context.cc (right): http://codereview.chromium.org/9972011/diff/5001/content/public/browser/speec... content/public/browser/speech_recognition_session_context.cc:13: }; remove semicolon http://codereview.chromium.org/9972011/diff/5001/content/public/browser/speec... content/public/browser/speech_recognition_session_context.cc:15: SpeechRecognitionSessionContext::~SpeechRecognitionSessionContext() { could remove empty destructor
http://codereview.chromium.org/9972011/diff/5001/content/browser/speech/speec... File content/browser/speech/speech_recognition_manager_impl.cc (right): http://codereview.chromium.org/9972011/diff/5001/content/browser/speech/speec... content/browser/speech/speech_recognition_manager_impl.cc:33: using media::AudioManager; On 2012/04/19 13:03:19, Satish wrote: > same as below Done. http://codereview.chromium.org/9972011/diff/5001/content/browser/speech/speec... content/browser/speech/speech_recognition_manager_impl.cc:34: using std::string; On 2012/04/19 13:03:19, Satish wrote: > could remove this as string is used only in 1 place Done. http://codereview.chromium.org/9972011/diff/5001/content/browser/speech/speec... content/browser/speech/speech_recognition_manager_impl.cc:38: // A dummy implementation of the SpeechRecognitionManagerDelegate interface On 2012/04/19 13:03:19, Satish wrote: > no indentation required within namespaces Done. http://codereview.chromium.org/9972011/diff/5001/content/browser/speech/speec... content/browser/speech/speech_recognition_manager_impl.cc:65: } //namespace On 2012/04/19 13:03:19, Satish wrote: > add 2 spaces before // and 1 space after // Done. http://codereview.chromium.org/9972011/diff/5001/content/browser/speech/speec... content/browser/speech/speech_recognition_manager_impl.cc:89: if (delegate_ == NULL) On 2012/04/19 13:03:19, Satish wrote: > use 'if (!delegate_)' Hmm I'd prefer the == / != NULL form since: 1. It is IMHO easier to read. 2. It works seamlessly with both naked and smart pointers. 3. I like only boolean conditions to be evaluated inside an if statement (even if, from the C viewpoint, it would work anyway) http://codereview.chromium.org/9972011/diff/5001/content/browser/speech/speec... content/browser/speech/speech_recognition_manager_impl.cc:94: // Recognition sessions will be aborted by the corresponding destructors. On 2012/04/19 13:03:19, Satish wrote: > I see the destructor of Session not explicitly aborting ongoing recording etc. > Probably that happens automatically when the SpeechRecognizer object gets > destroyed implicitly? Yes, I meant, by destructor calls of the sessions_ container dtor. >Even then we should be informing the listener of that > session about the event (e.g. what SessionAbort does). So looks like we need to > explicitly abort all ongoing sessions here? Well, the point is that since SpeechRecognitionManagerImpl is a singleton, the only case in which the destructor is called is the browser shutdown. Do we really need to do all these graceful aborts? Even though, calling Abort on sessions upon destructor will not really gracefully abort them, rather it will just post a task, which will never be executed. http://codereview.chromium.org/9972011/diff/5001/content/browser/speech/speec... content/browser/speech/speech_recognition_manager_impl.cc:99: SpeechRecognitionSessionConfig& config, On 2012/04/19 13:03:19, Satish wrote: > can this be changed to a const reference? Done. http://codereview.chromium.org/9972011/diff/5001/content/browser/speech/speec... content/browser/speech/speech_recognition_manager_impl.cc:148: if (interactive_session_id_ > 0 && interactive_session_id_ != session_id) { On 2012/04/19 13:03:19, Satish wrote: > could remove braces Done. http://codereview.chromium.org/9972011/diff/5001/content/browser/speech/speec... content/browser/speech/speech_recognition_manager_impl.cc:148: if (interactive_session_id_ > 0 && interactive_session_id_ != session_id) { On 2012/04/19 13:03:19, Satish wrote: > > 0 should be changed to != kSessionIDInvalid here and other places Oops, right! http://codereview.chromium.org/9972011/diff/5001/content/browser/speech/speec... content/browser/speech/speech_recognition_manager_impl.cc:235: DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO)); On 2012/04/19 13:03:19, Satish wrote: > could move this check to the very beginning (same for all methods below). > > Also this is probably something to add in all the listener methods Right. Added in all public methods. http://codereview.chromium.org/9972011/diff/5001/content/browser/speech/speec... content/browser/speech/speech_recognition_manager_impl.cc:248: event_args.speech_result = &result; On 2012/04/19 13:03:19, Satish wrote: > can this be passed by value to avoid any use-after-free type situations in > future? Removed. Was not used at all. http://codereview.chromium.org/9972011/diff/5001/content/browser/speech/speec... content/browser/speech/speech_recognition_manager_impl.cc:260: event_args.speech_error = &error; On 2012/04/19 13:03:19, Satish wrote: > ditto Done. http://codereview.chromium.org/9972011/diff/5001/content/browser/speech/speec... content/browser/speech/speech_recognition_manager_impl.cc:281: int SpeechRecognitionManagerImpl::LookupSessionByContext( On 2012/04/19 13:03:19, Satish wrote: > Since this class is the broker for SessionContext data between the dispatcher > host and delegates, I think it is fine for it to know and access the members > instead of treating it as an opaque entity. So this method can be changed to a > simpler one where caller passes in a SessionContext and it just matches the > fields before returning the right session id Can we keep this as a TODO and do it after CL2 and WebKit patch is stabilized? I am not 100% sure that the (future) speech_recognition_dispatcher_host (equivalent of input_tag_dispatcher_host for JS APIs) will perform the lookup on the basis of the same information. http://codereview.chromium.org/9972011/diff/5001/content/browser/speech/speec... content/browser/speech/speech_recognition_manager_impl.cc:282: Callback<bool(const SpeechRecognitionSessionContext&)> matcher) const { On 2012/04/19 13:03:19, Satish wrote: > indent by 4 spaces only Done. http://codereview.chromium.org/9972011/diff/5001/content/browser/speech/speec... content/browser/speech/speech_recognition_manager_impl.cc:295: // TODO(primiano) are we really sure that iterator->second will always give us On 2012/04/19 13:03:19, Satish wrote: > I've seen more code rely on that behavior so could remove this todo Done. http://codereview.chromium.org/9972011/diff/5001/content/browser/speech/speec... content/browser/speech/speech_recognition_manager_impl.cc:298: SpeechRecognitionSessionContext& On 2012/04/19 13:03:19, Satish wrote: > could return this by value or if you prefer pass in a pointer where the contents > get copied to Done. http://codereview.chromium.org/9972011/diff/5001/content/browser/speech/speec... content/browser/speech/speech_recognition_manager_impl.cc:308: // AbortSession is asyncrhonous and the session will not be removed from the On 2012/04/19 13:03:19, Satish wrote: > asyncrhonous -> asynchronous Done. http://codereview.chromium.org/9972011/diff/5001/content/browser/speech/speec... content/browser/speech/speech_recognition_manager_impl.cc:336: session.state == STATE_ENDED_WITH_ERROR) On 2012/04/19 13:03:19, Satish wrote: > suggest braces for multiline if statements like these Done. http://codereview.chromium.org/9972011/diff/5001/content/browser/speech/speec... content/browser/speech/speech_recognition_manager_impl.cc:379: return DoNothing(session, event_args); On 2012/04/19 13:03:19, Satish wrote: > should be calling NotFeasible instead of DoNothing This code is changed. Btw the idea is that in this class I consider the caller as untrusted (since this class will be almost directly visible to the future JS APIs). Therefore I use DoNothing for actions that are possible but don't make sense. NotFeasible, instead, is reserved for events that structurally cannot happen (highlighting the presence of a bug in the code) http://codereview.chromium.org/9972011/diff/5001/content/browser/speech/speec... content/browser/speech/speech_recognition_manager_impl.cc:394: case STATE_DETACHABLE: On 2012/04/19 13:03:19, Satish wrote: > it would be clearer to understand if 'detachable' can be renamed to > 'waiting_for_result' or something similar. Simlpified the FSM. Now should be clearer. http://codereview.chromium.org/9972011/diff/5001/content/browser/speech/speec... content/browser/speech/speech_recognition_manager_impl.cc:416: case STATE_DETACHED: On 2012/04/19 13:03:19, Satish wrote: > since this seems to be used only in one shot recognition, you could move to > STATE_IDLE and handle EVENT_RECOGNITION_ENDED there to call SessionDelete. That > way we don't need a separate STATE_DETACHED. > Same suggestion for the ABORTING & ENDED_WITH_ERROR states, perhaps they all can > be the same as IDLE so after the session ends it goes back to IDLE Ditto. http://codereview.chromium.org/9972011/diff/5001/content/browser/speech/speec... content/browser/speech/speech_recognition_manager_impl.cc:481: if (interactive_session_id_ != 0) On 2012/04/19 13:03:19, Satish wrote: > replace 0 with kSessionIDInvalid here and other places Done. http://codereview.chromium.org/9972011/diff/5001/content/browser/speech/speec... File content/browser/speech/speech_recognition_manager_impl.h (right): http://codereview.chromium.org/9972011/diff/5001/content/browser/speech/speec... content/browser/speech/speech_recognition_manager_impl.h:26: struct SpeechRecognitionError; On 2012/04/19 13:03:19, Satish wrote: > order alphabetically Done. http://codereview.chromium.org/9972011/diff/5001/content/browser/speech/speec... content/browser/speech/speech_recognition_manager_impl.h:37: // user attention (showing a bubble). For privacy reasons, only the interactive On 2012/04/19 13:03:19, Satish wrote: > since the bubble concept is not applicable for all users of this class (e.g. > extension api and in future continuous recognition) suggest removing that from > the comment Done. http://codereview.chromium.org/9972011/diff/5001/content/browser/speech/speec... content/browser/speech/speech_recognition_manager_impl.h:44: // one of them accesses the audio device coherently. On 2012/04/19 13:03:19, Satish wrote: > coherently -> at any given time Done. http://codereview.chromium.org/9972011/diff/5001/content/browser/speech/speec... content/browser/speech/speech_recognition_manager_impl.h:170: int interactive_session_id_; On 2012/04/19 13:03:19, Satish wrote: > interactive -> active ? A session can be active but not interactive (while waiting for a result) http://codereview.chromium.org/9972011/diff/5001/content/content_browser.gypi File content/content_browser.gypi (right): http://codereview.chromium.org/9972011/diff/5001/content/content_browser.gypi... content/content_browser.gypi:139: 'public/browser/speech_recognition_session_context.cc', On 2012/04/19 13:03:19, Satish wrote: > please also include the header files in here Done. http://codereview.chromium.org/9972011/diff/5001/content/public/browser/speec... File content/public/browser/speech_recognition_manager.h (right): http://codereview.chromium.org/9972011/diff/5001/content/public/browser/speec... content/public/browser/speech_recognition_manager.h:19: // The SpeechRecognitionManager (SRM) is a singleton class that handles SR On 2012/04/19 13:03:19, Satish wrote: > I like this comment block :) but feels verbose. Can it be written more > succinctly? Done. http://codereview.chromium.org/9972011/diff/5001/content/public/browser/speec... File content/public/browser/speech_recognition_manager_delegate.h (right): http://codereview.chromium.org/9972011/diff/5001/content/public/browser/speec... content/public/browser/speech_recognition_manager_delegate.h:30: // on SpeechInutManager in response. TODO(primiano) ?!?. On 2012/04/19 13:03:19, Satish wrote: > is this TODO required? If so, should be at the start of the todo comment Done. http://codereview.chromium.org/9972011/diff/5001/content/public/browser/speec... content/public/browser/speech_recognition_manager_delegate.h:33: // Check if current UI set-up allow speech recognition. On 2012/04/19 13:03:19, Satish wrote: > allow -> allows Done. http://codereview.chromium.org/9972011/diff/5001/content/public/browser/speec... content/public/browser/speech_recognition_manager_delegate.h:34: virtual bool CheckRecognitionIsAllowed(int session_id) = 0; On 2012/04/19 13:03:19, Satish wrote: > could rename as 'IsRecognitionAllowed' Done. http://codereview.chromium.org/9972011/diff/5001/content/public/browser/speec... File content/public/browser/speech_recognition_session_config.cc (right): http://codereview.chromium.org/9972011/diff/5001/content/public/browser/speec... content/public/browser/speech_recognition_session_config.cc:10: : filter_profanities(NULL), On 2012/04/19 13:03:19, Satish wrote: > remove extra space after colon Done. http://codereview.chromium.org/9972011/diff/5001/content/public/browser/speec... content/public/browser/speech_recognition_session_config.cc:10: : filter_profanities(NULL), On 2012/04/19 13:03:19, Satish wrote: > NULL -> false Done. http://codereview.chromium.org/9972011/diff/5001/content/public/browser/speec... content/public/browser/speech_recognition_session_config.cc:14: SpeechRecognitionSessionConfig::~SpeechRecognitionSessionConfig() { On 2012/04/19 13:03:19, Satish wrote: > it this empty destructor required and if not could remove? Yes, otherwise [chromium-style] Complex class/struct needs an explicit out-of-line destructor http://codereview.chromium.org/9972011/diff/5001/content/public/browser/speec... File content/public/browser/speech_recognition_session_config.h (right): http://codereview.chromium.org/9972011/diff/5001/content/public/browser/speec... content/public/browser/speech_recognition_session_config.h:11: #include "ui/gfx/rect.h" On 2012/04/19 13:03:19, Satish wrote: > this doesn't seem required Done. http://codereview.chromium.org/9972011/diff/5001/content/public/browser/speec... content/public/browser/speech_recognition_session_config.h:26: net::URLRequestContextGetter* url_context_getter; On 2012/04/19 13:03:19, Satish wrote: > url_context_getter -> url_request_context_getter Done. http://codereview.chromium.org/9972011/diff/5001/content/public/browser/speec... File content/public/browser/speech_recognition_session_context.cc (right): http://codereview.chromium.org/9972011/diff/5001/content/public/browser/speec... content/public/browser/speech_recognition_session_context.cc:13: }; On 2012/04/19 13:03:19, Satish wrote: > remove semicolon Done. http://codereview.chromium.org/9972011/diff/5001/content/public/browser/speec... content/public/browser/speech_recognition_session_context.cc:15: SpeechRecognitionSessionContext::~SpeechRecognitionSessionContext() { On 2012/04/19 13:03:19, Satish wrote: > could remove empty destructor No, as above. Clang will complain about non-POD due to gfx::Rect.
Thanks, some more comments.. http://codereview.chromium.org/9972011/diff/5001/content/browser/speech/speec... File content/browser/speech/speech_recognition_manager_impl.cc (right): http://codereview.chromium.org/9972011/diff/5001/content/browser/speech/speec... content/browser/speech/speech_recognition_manager_impl.cc:94: // Recognition sessions will be aborted by the corresponding destructors. > Well, the point is that since SpeechRecognitionManagerImpl is a singleton, the > only case in which the destructor is called is the browser shutdown. Do we > really need to do all these graceful aborts? Even though, calling Abort on > sessions upon destructor will not really gracefully abort them, rather it will > just post a task, which will never be executed. That is a bit worrying because the session could be capturing audio and the connection may need to be gracefully closed. Since this isn't a self contained class not interfacing with anyone else, I think a graceful shutdown is better. We could bypass the FSM, not post a task and get this closed immediately since the object is going away anyway http://codereview.chromium.org/9972011/diff/5001/content/browser/speech/speec... content/browser/speech/speech_recognition_manager_impl.cc:281: int SpeechRecognitionManagerImpl::LookupSessionByContext( On 2012/04/20 16:06:43, Primiano Tucci wrote: > On 2012/04/19 13:03:19, Satish wrote: > > Since this class is the broker for SessionContext data between the dispatcher > > host and delegates, I think it is fine for it to know and access the members > > instead of treating it as an opaque entity. So this method can be changed to a > > simpler one where caller passes in a SessionContext and it just matches the > > fields before returning the right session id > Can we keep this as a TODO and do it after CL2 and WebKit patch is stabilized? I > am not 100% sure that the (future) speech_recognition_dispatcher_host > (equivalent of input_tag_dispatcher_host for JS APIs) will perform the lookup on > the basis of the same information. I'd prefer we make this simpler now and change it in a future CL if required. Its cheap to change when required but leaving it as is now and we may forget/accumulate cruft over time. For the current usage using base::Bind and a matcher is complicated compared to passing in the 3 fields to check http://codereview.chromium.org/9972011/diff/13002/chrome/browser/speech/chrom... File chrome/browser/speech/chrome_speech_recognition_manager_delegate.cc (right): http://codereview.chromium.org/9972011/diff/13002/chrome/browser/speech/chrom... chrome/browser/speech/chrome_speech_recognition_manager_delegate.cc:13: #include "base/synchronization/waitable_event.h" move up by 2 lines http://codereview.chromium.org/9972011/diff/13002/chrome/browser/speech/chrom... chrome/browser/speech/chrome_speech_recognition_manager_delegate.cc:152: BrowserThread::UI, FROM_HERE, indent by 4 spaces http://codereview.chromium.org/9972011/diff/13002/chrome/browser/speech/chrom... chrome/browser/speech/chrome_speech_recognition_manager_delegate.cc:248: if (!render_view_host || !render_view_host->GetDelegate()) { could merge this and the first 'else if' clause http://codereview.chromium.org/9972011/diff/13002/chrome/browser/speech/chrom... File chrome/browser/speech/chrome_speech_recognition_manager_delegate.h (right): http://codereview.chromium.org/9972011/diff/13002/chrome/browser/speech/chrom... chrome/browser/speech/chrome_speech_recognition_manager_delegate.h:52: static void CheckRenderViewType(int render_process_id, int render_view_id, Use a single parameter per line for such long declarations http://codereview.chromium.org/9972011/diff/13002/content/browser/speech/goog... File content/browser/speech/google_one_shot_remote_engine.h (right): http://codereview.chromium.org/9972011/diff/13002/content/browser/speech/goog... content/browser/speech/google_one_shot_remote_engine.h:78: bool url_fetcher_destroyed_; Should these changes be removed? http://codereview.chromium.org/9972011/diff/13002/content/browser/speech/inpu... File content/browser/speech/input_tag_speech_dispatcher_host.cc (right): http://codereview.chromium.org/9972011/diff/13002/content/browser/speech/inpu... content/browser/speech/input_tag_speech_dispatcher_host.cc:20: using content::SpeechRecognitionSessionContext; perhaps cleaner to move this to line 18? http://codereview.chromium.org/9972011/diff/13002/content/browser/speech/inpu... content/browser/speech/input_tag_speech_dispatcher_host.cc:21: bool ContextMatcher(int render_process_id, suggest renaming this to something like 'IsSameContext' http://codereview.chromium.org/9972011/diff/13002/content/browser/speech/inpu... content/browser/speech/input_tag_speech_dispatcher_host.cc:29: } add a " // namespace" comment http://codereview.chromium.org/9972011/diff/13002/content/browser/speech/inpu... content/browser/speech/input_tag_speech_dispatcher_host.cc:110: if (session_id == content::SpeechRecognitionManager::kSessionIDInvalid) should this be a DCHECK or is it an expected failure condition? http://codereview.chromium.org/9972011/diff/13002/content/browser/speech/inpu... content/browser/speech/input_tag_speech_dispatcher_host.cc:118: DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO)); indent by 1 more space http://codereview.chromium.org/9972011/diff/13002/content/browser/speech/inpu... File content/browser/speech/input_tag_speech_dispatcher_host.h (right): http://codereview.chromium.org/9972011/diff/13002/content/browser/speech/inpu... content/browser/speech/input_tag_speech_dispatcher_host.h:27: // from the SpeechRecognitionManager to IPC messages (and viceversa). viceversa -> vice versa http://codereview.chromium.org/9972011/diff/13002/content/browser/speech/spee... File content/browser/speech/speech_recognition_manager_impl.cc (right): http://codereview.chromium.org/9972011/diff/13002/content/browser/speech/spee... content/browser/speech/speech_recognition_manager_impl.cc:395: return DoNothing(session, event_args); why are these 2 events valid in this state? shouldn't they not be feasible? http://codereview.chromium.org/9972011/diff/13002/content/browser/speech/spee... content/browser/speech/speech_recognition_manager_impl.cc:423: case STATE_DETACHED: As I mentioned earlier the concept of a 'detached' session isn't clear still while the others are quite clear. STATE_DETACHED and STATE_WAITING_FOR_DELETION seem very close in purpose, both can't get the session back into active state and are just different stages of cleaning up the session. Can we combine them into a single state?
http://codereview.chromium.org/9972011/diff/13002/chrome/browser/speech/chrom... File chrome/browser/speech/chrome_speech_recognition_manager_delegate.cc (right): http://codereview.chromium.org/9972011/diff/13002/chrome/browser/speech/chrom... chrome/browser/speech/chrome_speech_recognition_manager_delegate.cc:13: #include "base/synchronization/waitable_event.h" On 2012/04/23 10:25:17, Satish wrote: > move up by 2 lines Done. http://codereview.chromium.org/9972011/diff/13002/chrome/browser/speech/chrom... chrome/browser/speech/chrome_speech_recognition_manager_delegate.cc:152: BrowserThread::UI, FROM_HERE, On 2012/04/23 10:25:17, Satish wrote: > indent by 4 spaces Done. http://codereview.chromium.org/9972011/diff/13002/chrome/browser/speech/chrom... chrome/browser/speech/chrome_speech_recognition_manager_delegate.cc:248: if (!render_view_host || !render_view_host->GetDelegate()) { On 2012/04/23 10:25:17, Satish wrote: > could merge this and the first 'else if' clause Done. http://codereview.chromium.org/9972011/diff/13002/chrome/browser/speech/chrom... File chrome/browser/speech/chrome_speech_recognition_manager_delegate.h (right): http://codereview.chromium.org/9972011/diff/13002/chrome/browser/speech/chrom... chrome/browser/speech/chrome_speech_recognition_manager_delegate.h:52: static void CheckRenderViewType(int render_process_id, int render_view_id, On 2012/04/23 10:25:17, Satish wrote: > Use a single parameter per line for such long declarations Done. http://codereview.chromium.org/9972011/diff/13002/content/browser/speech/goog... File content/browser/speech/google_one_shot_remote_engine.h (right): http://codereview.chromium.org/9972011/diff/13002/content/browser/speech/goog... content/browser/speech/google_one_shot_remote_engine.h:78: bool url_fetcher_destroyed_; On 2012/04/23 10:25:17, Satish wrote: > Should these changes be removed? I did remove them in the last patch-set. Probably the codereview did not show the last patch-set? http://codereview.chromium.org/9972011/diff/13002/content/browser/speech/inpu... File content/browser/speech/input_tag_speech_dispatcher_host.cc (right): http://codereview.chromium.org/9972011/diff/13002/content/browser/speech/inpu... content/browser/speech/input_tag_speech_dispatcher_host.cc:20: using content::SpeechRecognitionSessionContext; On 2012/04/23 10:25:17, Satish wrote: > perhaps cleaner to move this to line 18? Oops, sure. Don't know how it ended there. http://codereview.chromium.org/9972011/diff/13002/content/browser/speech/inpu... content/browser/speech/input_tag_speech_dispatcher_host.cc:21: bool ContextMatcher(int render_process_id, On 2012/04/23 10:25:17, Satish wrote: > suggest renaming this to something like 'IsSameContext' Done. http://codereview.chromium.org/9972011/diff/13002/content/browser/speech/inpu... content/browser/speech/input_tag_speech_dispatcher_host.cc:29: } On 2012/04/23 10:25:17, Satish wrote: > add a " // namespace" comment Done. http://codereview.chromium.org/9972011/diff/13002/content/browser/speech/inpu... content/browser/speech/input_tag_speech_dispatcher_host.cc:110: if (session_id == content::SpeechRecognitionManager::kSessionIDInvalid) On 2012/04/23 10:25:17, Satish wrote: > should this be a DCHECK or is it an expected failure condition? It can actually fail if IsRecognitionAllowed() is not satsfied (Recognition requested when host delegate is not VIEW_TYPE_WEB_CONTENTS) http://codereview.chromium.org/9972011/diff/13002/content/browser/speech/inpu... content/browser/speech/input_tag_speech_dispatcher_host.cc:118: DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO)); On 2012/04/23 10:25:17, Satish wrote: > indent by 1 more space Done. http://codereview.chromium.org/9972011/diff/13002/content/browser/speech/inpu... File content/browser/speech/input_tag_speech_dispatcher_host.h (right): http://codereview.chromium.org/9972011/diff/13002/content/browser/speech/inpu... content/browser/speech/input_tag_speech_dispatcher_host.h:27: // from the SpeechRecognitionManager to IPC messages (and viceversa). On 2012/04/23 10:25:17, Satish wrote: > viceversa -> vice versa Done. http://codereview.chromium.org/9972011/diff/13002/content/browser/speech/spee... File content/browser/speech/speech_recognition_manager_impl.cc (right): http://codereview.chromium.org/9972011/diff/13002/content/browser/speech/spee... content/browser/speech/speech_recognition_manager_impl.cc:395: return DoNothing(session, event_args); On 2012/04/23 10:25:17, Satish wrote: > why are these 2 events valid in this state? shouldn't they not be feasible? STOP_CAPTURE is feasible (though it makes no sense) since it is an event requested through a public method. According to the contract of SRM (see content/public/browser/speech_recognition_manager.h) "the SRM has the further responsibility of handling separately and reliably (taking into account also call sequences that might not make sense, e.g., two subsequent AbortSession calls)". From a more practical viewpoint, it is not expected to happen in the current CL. But it might happen in the near future when we'll have JS support. In that case we can end up in this case if the StopAudioCapture is called on the JS object without prior starting recognition. EVENT_RECOGNITION_ENDED is feasible (and happens) in the case of an error. Perhaps the name "IDLE" is not 99.9% correct, since there is the current corner case in which we are not exactly IDLE, yet for a very short time interval: SRM is in the state INTERACTIVE and receives a RECOGNITION_ERROR event -> the bubble error is shown (through the delegate) calling SessionReportError and SRM transits to IDLE state. However, since the OnError event is always guaranteed to precede the OnRecognitionEnd event, the RECOGNITION_ENDED event will be received after the transition to the IDLE state. This is a consequence of merging together the previous states IDLE and ENDED_WITH_ERROR (which were almost identical, expect for this detail). Any comment is welcome, and if you prefer, we can switch back adding a separate ENDED_WITH_ERROR state. http://codereview.chromium.org/9972011/diff/13002/content/browser/speech/spee... content/browser/speech/speech_recognition_manager_impl.cc:423: case STATE_DETACHED: On 2012/04/23 10:25:17, Satish wrote: > As I mentioned earlier the concept of a 'detached' session isn't clear still > while the others are quite clear. STATE_DETACHED and STATE_WAITING_FOR_DELETION > seem very close in purpose, both can't get the session back into active state > and are just different stages of cleaning up the session. Can we combine them > into a single state? Ah sorry, I thought the earlier comment was just referred to DETACHABLE <> DETACHED states. My idea that DETACHED (we can change the name if we find a more suitable one, perhaps BACKGROUND?) shall be used most in the near future for continuous recognition sessions that don't use the bubble (or any other UI interaction for start/stop), that are, upcoming JS APIs and speech input extensions (that currently are "managing" speech on their own). Merging WAITING_FOR_DELETION and DETACHED should work in the current implementation, but perhaps (I am not 100% sure about this now) DETACHED will be changed in the near future for continuous recognition (so the merge will probably need to be reverted). Should I add a TODO to see if we can merge them in future, after the continuous speech APIs??
i'm pretty happy with this, just some nits http://codereview.chromium.org/9972011/diff/38005/chrome/browser/speech/chrom... File chrome/browser/speech/chrome_speech_recognition_manager_delegate.h (right): http://codereview.chromium.org/9972011/diff/38005/chrome/browser/speech/chrom... chrome/browser/speech/chrome_speech_recognition_manager_delegate.h:19: // This is Chrome's implementation of the SpeechRecognitionManager interface. I guess this should really say that it's an implementation of the SpeechRecognitionManagerDelegate interface? http://codereview.chromium.org/9972011/diff/38005/chrome/browser/speech/chrom... chrome/browser/speech/chrome_speech_recognition_manager_delegate.h:52: static void CheckRenderViewType(int render_process_id, a comment describing what this does would be nice http://codereview.chromium.org/9972011/diff/38005/content/browser/speech/inpu... File content/browser/speech/input_tag_speech_dispatcher_host.cc (right): http://codereview.chromium.org/9972011/diff/38005/content/browser/speech/inpu... content/browser/speech/input_tag_speech_dispatcher_host.cc:108: int session_id = manager()->CreateSession(config, this); i'd put a blank line above this to separate it from the block of lines that initialize the config http://codereview.chromium.org/9972011/diff/38005/content/browser/speech/inpu... content/browser/speech/input_tag_speech_dispatcher_host.cc:124: if (session_id != 0) should 0 be replaced by kSessionIDInvalid? http://codereview.chromium.org/9972011/diff/38005/content/browser/speech/inpu... content/browser/speech/input_tag_speech_dispatcher_host.cc:136: if (session_id != 0) ditto http://codereview.chromium.org/9972011/diff/38005/content/browser/speech/spee... File content/browser/speech/speech_recognition_browsertest.cc (right): http://codereview.chromium.org/9972011/diff/38005/content/browser/speech/spee... content/browser/speech/speech_recognition_browsertest.cc:127: virtual bool HasAudioInputDevices() OVERRIDE {return true;} there should be spaces inside the {} when they're not empty applies here and below http://codereview.chromium.org/9972011/diff/38005/content/browser/speech/spee... File content/browser/speech/speech_recognition_manager_impl.cc (right): http://codereview.chromium.org/9972011/diff/38005/content/browser/speech/spee... content/browser/speech/speech_recognition_manager_impl.cc:110: // it on SessionStart in order to repeat the check every time?. both ? and . http://codereview.chromium.org/9972011/diff/38005/content/browser/speech/spee... content/browser/speech/speech_recognition_manager_impl.cc:305: for(iter = sessions_.begin(); iter != sessions_.end(); iter++) { please use the prefix increment operator: ++iter http://codereview.chromium.org/9972011/diff/38005/content/browser/speech/spee... content/browser/speech/speech_recognition_manager_impl.cc:320: return const_cast<SpeechRecognitionSessionContext&>(iter->second.context); i'm curious why the const_cast is needed here http://codereview.chromium.org/9972011/diff/38005/content/browser/speech/spee... content/browser/speech/speech_recognition_manager_impl.cc:329: while (it != sessions_.end()) { i'd prefer a for loop http://codereview.chromium.org/9972011/diff/38005/content/browser/speech/spee... content/browser/speech/speech_recognition_manager_impl.cc:463: if (interactive_session_id_ != 0) indent is off by one http://codereview.chromium.org/9972011/diff/38005/content/browser/speech/spee... content/browser/speech/speech_recognition_manager_impl.cc:463: if (interactive_session_id_ != 0) s/0/kInvalidSessionID/ ? http://codereview.chromium.org/9972011/diff/38005/content/browser/speech/spee... content/browser/speech/speech_recognition_manager_impl.cc:551: // Next state is ininfluent, the session will be deleted afterwards. ininfluent? http://codereview.chromium.org/9972011/diff/38005/content/browser/speech/spee... File content/browser/speech/speech_recognition_manager_impl.h (right): http://codereview.chromium.org/9972011/diff/38005/content/browser/speech/spee... content/browser/speech/speech_recognition_manager_impl.h:33: // The manager is unique for each renderer, and can serve several requests. Each I find the two first lines here a little confusing: "this is the manager" suggests that there is only one, but "unique for each renderer" suggests that there's one per renderer. Which is it? http://codereview.chromium.org/9972011/diff/38005/content/public/browser/spee... File content/public/browser/speech_recognition_session_context.h (right): http://codereview.chromium.org/9972011/diff/38005/content/public/browser/spee... content/public/browser/speech_recognition_session_context.h:18: // not aware of the content if this struct and does NOT use it for its purposes. s/content if this struct/content of this struct/
http://codereview.chromium.org/9972011/diff/38005/chrome/browser/speech/chrom... File chrome/browser/speech/chrome_speech_recognition_manager_delegate.h (right): http://codereview.chromium.org/9972011/diff/38005/chrome/browser/speech/chrom... chrome/browser/speech/chrome_speech_recognition_manager_delegate.h:19: // This is Chrome's implementation of the SpeechRecognitionManager interface. On 2012/04/23 16:04:14, hans wrote: > I guess this should really say that it's an implementation of the > SpeechRecognitionManagerDelegate interface? Indeed. And it is not a singleton anymore! http://codereview.chromium.org/9972011/diff/38005/chrome/browser/speech/chrom... chrome/browser/speech/chrome_speech_recognition_manager_delegate.h:52: static void CheckRenderViewType(int render_process_id, On 2012/04/23 16:04:14, hans wrote: > a comment describing what this does would be nice Done. http://codereview.chromium.org/9972011/diff/38005/content/browser/speech/inpu... File content/browser/speech/input_tag_speech_dispatcher_host.cc (right): http://codereview.chromium.org/9972011/diff/38005/content/browser/speech/inpu... content/browser/speech/input_tag_speech_dispatcher_host.cc:108: int session_id = manager()->CreateSession(config, this); On 2012/04/23 16:04:14, hans wrote: > i'd put a blank line above this to separate it from the block of lines that > initialize the config Done. http://codereview.chromium.org/9972011/diff/38005/content/browser/speech/inpu... content/browser/speech/input_tag_speech_dispatcher_host.cc:124: if (session_id != 0) On 2012/04/23 16:04:14, hans wrote: > should 0 be replaced by kSessionIDInvalid? Uh! I was pretty sure I did this replacement, but apparently I didn't. :/ http://codereview.chromium.org/9972011/diff/38005/content/browser/speech/inpu... content/browser/speech/input_tag_speech_dispatcher_host.cc:136: if (session_id != 0) On 2012/04/23 16:04:14, hans wrote: > ditto Done. http://codereview.chromium.org/9972011/diff/38005/content/browser/speech/spee... File content/browser/speech/speech_recognition_browsertest.cc (right): http://codereview.chromium.org/9972011/diff/38005/content/browser/speech/spee... content/browser/speech/speech_recognition_browsertest.cc:127: virtual bool HasAudioInputDevices() OVERRIDE {return true;} On 2012/04/23 16:04:14, hans wrote: > there should be spaces inside the {} when they're not empty > applies here and below Done. http://codereview.chromium.org/9972011/diff/38005/content/browser/speech/spee... File content/browser/speech/speech_recognition_manager_impl.cc (right): http://codereview.chromium.org/9972011/diff/38005/content/browser/speech/spee... content/browser/speech/speech_recognition_manager_impl.cc:110: // it on SessionStart in order to repeat the check every time?. On 2012/04/23 16:04:14, hans wrote: > both ? and . Done. http://codereview.chromium.org/9972011/diff/38005/content/browser/speech/spee... content/browser/speech/speech_recognition_manager_impl.cc:305: for(iter = sessions_.begin(); iter != sessions_.end(); iter++) { On 2012/04/23 16:04:14, hans wrote: > please use the prefix increment operator: ++iter Done. http://codereview.chromium.org/9972011/diff/38005/content/browser/speech/spee... content/browser/speech/speech_recognition_manager_impl.cc:320: return const_cast<SpeechRecognitionSessionContext&>(iter->second.context); On 2012/04/23 16:04:14, hans wrote: > i'm curious why the const_cast is needed here Uh right. Some patches ago it was returning a reference, to allow callers to modify the context. Than, according to Satish review, I turned it into a copy but forgot to remove the const_cast. http://codereview.chromium.org/9972011/diff/38005/content/browser/speech/spee... content/browser/speech/speech_recognition_manager_impl.cc:329: while (it != sessions_.end()) { On 2012/04/23 16:04:14, hans wrote: > i'd prefer a for loop Definitely agree. http://codereview.chromium.org/9972011/diff/38005/content/browser/speech/spee... content/browser/speech/speech_recognition_manager_impl.cc:463: if (interactive_session_id_ != 0) On 2012/04/23 16:04:14, hans wrote: > indent is off by one Done. http://codereview.chromium.org/9972011/diff/38005/content/browser/speech/spee... content/browser/speech/speech_recognition_manager_impl.cc:463: if (interactive_session_id_ != 0) On 2012/04/23 16:04:14, hans wrote: > s/0/kInvalidSessionID/ ? Done. http://codereview.chromium.org/9972011/diff/38005/content/browser/speech/spee... content/browser/speech/speech_recognition_manager_impl.cc:551: // Next state is ininfluent, the session will be deleted afterwards. irrelevant! :) http://codereview.chromium.org/9972011/diff/38005/content/browser/speech/spee... File content/browser/speech/speech_recognition_manager_impl.h (right): http://codereview.chromium.org/9972011/diff/38005/content/browser/speech/spee... content/browser/speech/speech_recognition_manager_impl.h:33: // The manager is unique for each renderer, and can serve several requests. Each On 2012/04/23 16:04:14, hans wrote: > I find the two first lines here a little confusing: "this is the manager" > suggests that there is only one, but "unique for each renderer" suggests that > there's one per renderer. Which is it? It was very misleading. Comment fixed. http://codereview.chromium.org/9972011/diff/38005/content/public/browser/spee... File content/public/browser/speech_recognition_session_context.h (right): http://codereview.chromium.org/9972011/diff/38005/content/public/browser/spee... content/public/browser/speech_recognition_session_context.h:18: // not aware of the content if this struct and does NOT use it for its purposes. On 2012/04/23 16:04:14, hans wrote: > s/content if this struct/content of this struct/ Done.
On 2012/04/23 18:32:17, Primiano Tucci wrote: > http://codereview.chromium.org/9972011/diff/38005/chrome/browser/speech/chrom... > File chrome/browser/speech/chrome_speech_recognition_manager_delegate.h (right): > > http://codereview.chromium.org/9972011/diff/38005/chrome/browser/speech/chrom... > chrome/browser/speech/chrome_speech_recognition_manager_delegate.h:19: // This > is Chrome's implementation of the SpeechRecognitionManager interface. > On 2012/04/23 16:04:14, hans wrote: > > I guess this should really say that it's an implementation of the > > SpeechRecognitionManagerDelegate interface? > Indeed. And it is not a singleton anymore! > > http://codereview.chromium.org/9972011/diff/38005/chrome/browser/speech/chrom... > chrome/browser/speech/chrome_speech_recognition_manager_delegate.h:52: static > void CheckRenderViewType(int render_process_id, > On 2012/04/23 16:04:14, hans wrote: > > a comment describing what this does would be nice > > Done. > > http://codereview.chromium.org/9972011/diff/38005/content/browser/speech/inpu... > File content/browser/speech/input_tag_speech_dispatcher_host.cc (right): > > http://codereview.chromium.org/9972011/diff/38005/content/browser/speech/inpu... > content/browser/speech/input_tag_speech_dispatcher_host.cc:108: int session_id = > manager()->CreateSession(config, this); > On 2012/04/23 16:04:14, hans wrote: > > i'd put a blank line above this to separate it from the block of lines that > > initialize the config > > Done. > > http://codereview.chromium.org/9972011/diff/38005/content/browser/speech/inpu... > content/browser/speech/input_tag_speech_dispatcher_host.cc:124: if (session_id > != 0) > On 2012/04/23 16:04:14, hans wrote: > > should 0 be replaced by kSessionIDInvalid? > Uh! I was pretty sure I did this replacement, but apparently I didn't. :/ > > http://codereview.chromium.org/9972011/diff/38005/content/browser/speech/inpu... > content/browser/speech/input_tag_speech_dispatcher_host.cc:136: if (session_id > != 0) > On 2012/04/23 16:04:14, hans wrote: > > ditto > > Done. > > http://codereview.chromium.org/9972011/diff/38005/content/browser/speech/spee... > File content/browser/speech/speech_recognition_browsertest.cc (right): > > http://codereview.chromium.org/9972011/diff/38005/content/browser/speech/spee... > content/browser/speech/speech_recognition_browsertest.cc:127: virtual bool > HasAudioInputDevices() OVERRIDE {return true;} > On 2012/04/23 16:04:14, hans wrote: > > there should be spaces inside the {} when they're not empty > > applies here and below > > Done. > > http://codereview.chromium.org/9972011/diff/38005/content/browser/speech/spee... > File content/browser/speech/speech_recognition_manager_impl.cc (right): > > http://codereview.chromium.org/9972011/diff/38005/content/browser/speech/spee... > content/browser/speech/speech_recognition_manager_impl.cc:110: // it on > SessionStart in order to repeat the check every time?. > On 2012/04/23 16:04:14, hans wrote: > > both ? and . > > Done. > > http://codereview.chromium.org/9972011/diff/38005/content/browser/speech/spee... > content/browser/speech/speech_recognition_manager_impl.cc:305: for(iter = > sessions_.begin(); iter != sessions_.end(); iter++) { > On 2012/04/23 16:04:14, hans wrote: > > please use the prefix increment operator: ++iter > > Done. > > http://codereview.chromium.org/9972011/diff/38005/content/browser/speech/spee... > content/browser/speech/speech_recognition_manager_impl.cc:320: return > const_cast<SpeechRecognitionSessionContext&>(iter->second.context); > On 2012/04/23 16:04:14, hans wrote: > > i'm curious why the const_cast is needed here > > Uh right. Some patches ago it was returning a reference, to allow callers to > modify the context. Than, according to Satish review, I turned it into a copy > but forgot to remove the const_cast. > > http://codereview.chromium.org/9972011/diff/38005/content/browser/speech/spee... > content/browser/speech/speech_recognition_manager_impl.cc:329: while (it != > sessions_.end()) { > On 2012/04/23 16:04:14, hans wrote: > > i'd prefer a for loop > > Definitely agree. > > http://codereview.chromium.org/9972011/diff/38005/content/browser/speech/spee... > content/browser/speech/speech_recognition_manager_impl.cc:463: if > (interactive_session_id_ != 0) > On 2012/04/23 16:04:14, hans wrote: > > indent is off by one > > Done. > > http://codereview.chromium.org/9972011/diff/38005/content/browser/speech/spee... > content/browser/speech/speech_recognition_manager_impl.cc:463: if > (interactive_session_id_ != 0) > On 2012/04/23 16:04:14, hans wrote: > > s/0/kInvalidSessionID/ ? > > Done. > > http://codereview.chromium.org/9972011/diff/38005/content/browser/speech/spee... > content/browser/speech/speech_recognition_manager_impl.cc:551: // Next state is > ininfluent, the session will be deleted afterwards. > irrelevant! :) > > http://codereview.chromium.org/9972011/diff/38005/content/browser/speech/spee... > File content/browser/speech/speech_recognition_manager_impl.h (right): > > http://codereview.chromium.org/9972011/diff/38005/content/browser/speech/spee... > content/browser/speech/speech_recognition_manager_impl.h:33: // The manager is > unique for each renderer, and can serve several requests. Each > On 2012/04/23 16:04:14, hans wrote: > > I find the two first lines here a little confusing: "this is the manager" > > suggests that there is only one, but "unique for each renderer" suggests that > > there's one per renderer. Which is it? > It was very misleading. Comment fixed. > > http://codereview.chromium.org/9972011/diff/38005/content/public/browser/spee... > File content/public/browser/speech_recognition_session_context.h (right): > > http://codereview.chromium.org/9972011/diff/38005/content/public/browser/spee... > content/public/browser/speech_recognition_session_context.h:18: // not aware of > the content if this struct and does NOT use it for its purposes. > On 2012/04/23 16:04:14, hans wrote: > > s/content if this struct/content of this struct/ > > Done. Thanks! lgtm
LGTM with the nits below http://codereview.chromium.org/9972011/diff/13002/content/browser/speech/spee... File content/browser/speech/speech_recognition_manager_impl.cc (right): http://codereview.chromium.org/9972011/diff/13002/content/browser/speech/spee... content/browser/speech/speech_recognition_manager_impl.cc:395: return DoNothing(session, event_args); On 2012/04/23 12:52:25, Primiano Tucci wrote: > On 2012/04/23 10:25:17, Satish wrote: > > why are these 2 events valid in this state? shouldn't they not be feasible? > > STOP_CAPTURE is feasible (though it makes no sense) since it is an event > requested through a public method. According to the contract of SRM (see > content/public/browser/speech_recognition_manager.h) "the SRM has the further > responsibility of handling separately and reliably (taking into account also > call sequences that might not make sense, e.g., two subsequent AbortSession > calls)". > From a more practical viewpoint, it is not expected to happen in the current CL. > But it might happen in the near future when we'll have JS support. In that case > we can end up in this case if the StopAudioCapture is called on the JS object > without prior starting recognition. > > EVENT_RECOGNITION_ENDED is feasible (and happens) in the case of an error. > Perhaps the name "IDLE" is not 99.9% correct, since there is the current corner > case in which we are not exactly IDLE, yet for a very short time interval: SRM > is in the state INTERACTIVE and receives a RECOGNITION_ERROR event -> the bubble > error is shown (through the delegate) calling SessionReportError and SRM > transits to IDLE state. However, since the OnError event is always guaranteed > to precede the OnRecognitionEnd event, the RECOGNITION_ENDED event will be > received after the transition to the IDLE state. > This is a consequence of merging together the previous states IDLE and > ENDED_WITH_ERROR (which were almost identical, expect for this detail). Any > comment is welcome, and if you prefer, we can switch back adding a separate > ENDED_WITH_ERROR state. Makes sense. Can you mention a shorter version of this as a comment here? http://codereview.chromium.org/9972011/diff/13002/content/browser/speech/spee... content/browser/speech/speech_recognition_manager_impl.cc:423: case STATE_DETACHED: On 2012/04/23 12:52:25, Primiano Tucci wrote: > On 2012/04/23 10:25:17, Satish wrote: > > As I mentioned earlier the concept of a 'detached' session isn't clear still > > while the others are quite clear. STATE_DETACHED and > STATE_WAITING_FOR_DELETION > > seem very close in purpose, both can't get the session back into active state > > and are just different stages of cleaning up the session. Can we combine them > > into a single state? > Ah sorry, I thought the earlier comment was just referred to DETACHABLE <> > DETACHED states. > My idea that DETACHED (we can change the name if we find a more suitable one, > perhaps BACKGROUND?) shall be used most in the near future for continuous > recognition sessions that don't use the bubble (or any other UI interaction for > start/stop), that are, upcoming JS APIs and speech input extensions (that > currently are "managing" speech on their own). Merging WAITING_FOR_DELETION and > DETACHED should work in the current implementation, but perhaps (I am not 100% > sure about this now) DETACHED will be changed in the near future for continuous > recognition (so the merge will probably need to be reverted). > Should I add a TODO to see if we can merge them in future, after the continuous > speech APIs?? Yes renaming DETACH to BACKGROUND would be clearer and goes well with INTERACTIVE as well. No need to merge otherwise. http://codereview.chromium.org/9972011/diff/42004/content/browser/speech/inpu... File content/browser/speech/input_tag_speech_dispatcher_host.cc (right): http://codereview.chromium.org/9972011/diff/42004/content/browser/speech/inpu... content/browser/speech/input_tag_speech_dispatcher_host.cc:147: LookupContext(session_id, &render_view_id, &render_request_id); This method seems unnecessary and doing the same directly is using lesser lines of code. For e.g. const SpeechRecognitionSessionContext& context = manager()->GetSessionContext(session_id); Send(new InputTagSpeechMsg_SetRecognitionResult( context.render_view_id, context.render_request_id, result)); http://codereview.chromium.org/9972011/diff/42004/content/browser/speech/inpu... content/browser/speech/input_tag_speech_dispatcher_host.cc:161: LookupContext(session_id, &render_view_id, &render_request_id); ditto http://codereview.chromium.org/9972011/diff/42004/content/browser/speech/inpu... content/browser/speech/input_tag_speech_dispatcher_host.cc:172: LookupContext(session_id, &render_view_id, &render_request_id); ditto http://codereview.chromium.org/9972011/diff/42004/content/public/browser/spee... File content/public/browser/speech_recognition_session_config.h (right): http://codereview.chromium.org/9972011/diff/42004/content/public/browser/spee... content/public/browser/speech_recognition_session_config.h:27: SpeechRecognitionSessionConfig(); Move these to the first line of the struct declaration http://codereview.chromium.org/9972011/diff/42004/content/public/browser/spee... File content/public/browser/speech_recognition_session_context.h (right): http://codereview.chromium.org/9972011/diff/42004/content/public/browser/spee... content/public/browser/speech_recognition_session_context.h:29: SpeechRecognitionSessionContext(); Move these to the first line of the struct declaration
http://codereview.chromium.org/9972011/diff/42004/content/browser/speech/inpu... File content/browser/speech/input_tag_speech_dispatcher_host.cc (right): http://codereview.chromium.org/9972011/diff/42004/content/browser/speech/inpu... content/browser/speech/input_tag_speech_dispatcher_host.cc:147: LookupContext(session_id, &render_view_id, &render_request_id); On 2012/04/24 08:51:32, Satish wrote: > This method seems unnecessary and doing the same directly is using lesser lines > of code. For e.g. > const SpeechRecognitionSessionContext& context = > manager()->GetSessionContext(session_id); > Send(new InputTagSpeechMsg_SetRecognitionResult( > context.render_view_id, context.render_request_id, result)); Done. http://codereview.chromium.org/9972011/diff/42004/content/browser/speech/inpu... content/browser/speech/input_tag_speech_dispatcher_host.cc:161: LookupContext(session_id, &render_view_id, &render_request_id); On 2012/04/24 08:51:32, Satish wrote: > ditto Done. http://codereview.chromium.org/9972011/diff/42004/content/browser/speech/inpu... content/browser/speech/input_tag_speech_dispatcher_host.cc:172: LookupContext(session_id, &render_view_id, &render_request_id); On 2012/04/24 08:51:32, Satish wrote: > ditto Done. http://codereview.chromium.org/9972011/diff/42004/content/public/browser/spee... File content/public/browser/speech_recognition_session_config.h (right): http://codereview.chromium.org/9972011/diff/42004/content/public/browser/spee... content/public/browser/speech_recognition_session_config.h:27: SpeechRecognitionSessionConfig(); On 2012/04/24 08:51:32, Satish wrote: > Move these to the first line of the struct declaration Done. http://codereview.chromium.org/9972011/diff/42004/content/public/browser/spee... File content/public/browser/speech_recognition_session_context.h (right): http://codereview.chromium.org/9972011/diff/42004/content/public/browser/spee... content/public/browser/speech_recognition_session_context.h:29: SpeechRecognitionSessionContext(); On 2012/04/24 08:51:32, Satish wrote: > Move these to the first line of the struct declaration Done.
http://codereview.chromium.org/9972011/diff/49001/content/public/browser/spee... File content/public/browser/speech_recognition_manager.h (right): http://codereview.chromium.org/9972011/diff/49001/content/public/browser/spee... content/public/browser/speech_recognition_manager.h:61: virtual void DetachSession(int session_id) = 0; should also change this 'Detach' to something about 'Background' similar to the state change?
http://codereview.chromium.org/9972011/diff/49001/content/public/browser/spee... File content/public/browser/speech_recognition_manager.h (right): http://codereview.chromium.org/9972011/diff/49001/content/public/browser/spee... content/public/browser/speech_recognition_manager.h:61: virtual void DetachSession(int session_id) = 0; On 2012/04/24 09:52:01, Satish wrote: > should also change this 'Detach' to something about 'Background' similar to the > state change? Done.
http://codereview.chromium.org/9972011/diff/52004/content/public/browser/spee... File content/public/browser/speech_recognition_manager.h (right): http://codereview.chromium.org/9972011/diff/52004/content/public/browser/spee... content/public/browser/speech_recognition_manager.h:58: // case it already finished capturing audio and was just waiting for the 'in case it already finished capturing audio' - this is true for the current one shot recognition api but no longer true once we have a continuous reco JS API. Just pointing out so we remember to update this at that time. http://codereview.chromium.org/9972011/diff/52004/content/public/browser/spee... content/public/browser/speech_recognition_manager.h:61: virtual void SendToBackgroundSession(int session_id) = 0; change to SendSessionToBackground
http://codereview.chromium.org/9972011/diff/52004/content/public/browser/spee... File content/public/browser/speech_recognition_manager.h (right): http://codereview.chromium.org/9972011/diff/52004/content/public/browser/spee... content/public/browser/speech_recognition_manager.h:58: // case it already finished capturing audio and was just waiting for the On 2012/04/24 10:48:08, Satish wrote: > 'in case it already finished capturing audio' - this is true for the current one > shot recognition api but no longer true once we have a continuous reco JS API. > Just pointing out so we remember to update this at that time. Done. http://codereview.chromium.org/9972011/diff/52004/content/public/browser/spee... content/public/browser/speech_recognition_manager.h:61: virtual void SendToBackgroundSession(int session_id) = 0; On 2012/04/24 10:48:08, Satish wrote: > change to SendSessionToBackground Done.
http://codereview.chromium.org/9972011/diff/45003/chrome/browser/speech/chrom... File chrome/browser/speech/chrome_speech_recognition_manager_delegate.cc (right): http://codereview.chromium.org/9972011/diff/45003/chrome/browser/speech/chrom... chrome/browser/speech/chrome_speech_recognition_manager_delegate.cc:26: #include "content/public/browser/speech_recognition_session_context.h" nit: order http://codereview.chromium.org/9972011/diff/45003/chrome/browser/speech/chrom... chrome/browser/speech/chrome_speech_recognition_manager_delegate.cc:148: WaitableEvent event(false /*manual_reset*/, false /*initially_signaled*/); we most definitely do not block one thread on another, this is a recipe for deadlock and jank. you'll need to find another way of doing this asynchronously. ideally all the SpeechManagerDelegate methods are called on the same thread (UI) as this keeps the public api simple. see the first parts of http://dev.chromium.org/developers/design-documents/threading http://codereview.chromium.org/9972011/diff/45003/content/browser/speech/spee... File content/browser/speech/speech_recognition_manager_impl.cc (right): http://codereview.chromium.org/9972011/diff/45003/content/browser/speech/spee... content/browser/speech/speech_recognition_manager_impl.cc:38: class VoidRecognitionManagerDelegate : the convention we have for other delegates is to null check. i think given this boilerplate that you have to add otherwise, it's not more work to null check http://codereview.chromium.org/9972011/diff/45003/content/browser/speech/spee... File content/browser/speech/speech_recognition_manager_impl.h (right): http://codereview.chromium.org/9972011/diff/45003/content/browser/speech/spee... content/browser/speech/speech_recognition_manager_impl.h:15: #include "content/browser/speech/speech_recognizer_impl.h" nit: just forward declare instead of including the header http://codereview.chromium.org/9972011/diff/45003/content/browser/speech/spee... content/browser/speech/speech_recognition_manager_impl.h:16: #include "content/common/content_export.h" nit: not needed http://codereview.chromium.org/9972011/diff/45003/content/browser/speech/spee... content/browser/speech/speech_recognition_manager_impl.h:23: struct SpeechRecognitionError; are you sure you need all these forward declared given all the above includes? http://codereview.chromium.org/9972011/diff/45003/content/browser/speech/spee... content/browser/speech/speech_recognition_manager_impl.h:98: enum FSMState { nit: you have FSM all over but what does it stand for? it's not entirely obvious http://codereview.chromium.org/9972011/diff/45003/content/public/browser/spee... File content/public/browser/speech_recognition_manager.h (right): http://codereview.chromium.org/9972011/diff/45003/content/public/browser/spee... content/public/browser/speech_recognition_manager.h:30: class CONTENT_EXPORT SpeechRecognitionManager { nit: why move the CONTENT_EXPORT? the convention for the content api has been to put it only on static functions and not on the whole interface http://codereview.chromium.org/9972011/diff/45003/content/public/browser/spee... File content/public/browser/speech_recognition_session_config.cc (right): http://codereview.chromium.org/9972011/diff/45003/content/public/browser/spee... content/public/browser/speech_recognition_session_config.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. just to double check, are you sure you need a cc file in this case to avoid clang errors? http://codereview.chromium.org/9972011/diff/45003/content/public/browser/spee... File content/public/browser/speech_recognition_session_config.h (right): http://codereview.chromium.org/9972011/diff/45003/content/public/browser/spee... content/public/browser/speech_recognition_session_config.h:28: net::URLRequestContextGetter* url_request_context_getter; nit: why not hold this in scoped_refcounted? http://codereview.chromium.org/9972011/diff/45003/content/public/browser/spee... File content/public/browser/speech_recognition_session_context.cc (right): http://codereview.chromium.org/9972011/diff/45003/content/public/browser/spee... content/public/browser/speech_recognition_session_context.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. are you sure you need this cc file? http://codereview.chromium.org/9972011/diff/45003/content/public/browser/spee... File content/public/browser/speech_recognition_session_context.h (right): http://codereview.chromium.org/9972011/diff/45003/content/public/browser/spee... content/public/browser/speech_recognition_session_context.h:15: // (ChromeSpeechRecognitionManagerDelegate) for mapping the recognition session nit: don't mention chrome classes in the public api for content http://codereview.chromium.org/9972011/diff/45003/content/public/browser/spee... content/public/browser/speech_recognition_session_context.h:23: struct CONTENT_EXPORT SpeechRecognitionSessionContext { nit: are you sure you need a CONTENT_EXPORT for this? and also are you usre you need a cc file?
http://codereview.chromium.org/9972011/diff/45003/chrome/browser/speech/chrom... File chrome/browser/speech/chrome_speech_recognition_manager_delegate.cc (right): http://codereview.chromium.org/9972011/diff/45003/chrome/browser/speech/chrom... chrome/browser/speech/chrome_speech_recognition_manager_delegate.cc:26: #include "content/public/browser/speech_recognition_session_context.h" On 2012/04/24 15:56:32, John Abd-El-Malek wrote: > nit: order Ops, sorry. http://codereview.chromium.org/9972011/diff/45003/chrome/browser/speech/chrom... chrome/browser/speech/chrome_speech_recognition_manager_delegate.cc:148: WaitableEvent event(false /*manual_reset*/, false /*initially_signaled*/); On 2012/04/24 15:56:32, John Abd-El-Malek wrote: > we most definitely do not block one thread on another, this is a recipe for > deadlock and jank. you'll need to find another way of doing this asynchronously. > ideally all the SpeechManagerDelegate methods are called on the same thread (UI) > as this keeps the public api simple. > > see the first parts of > http://dev.chromium.org/developers/design-documents/threading Hmm, right. The problem is that I am doing a very trivial check (render_view_host->GetDelegate()->GetRenderViewType() == content::VIEW_TYPE_WEB_CONTENTS) that, unfortunately, needs to be performed in the UI thread. I modified the implementation making that check asynchronous. However any different (and hopefully simpler) approach is more than welcome. http://codereview.chromium.org/9972011/diff/45003/content/browser/speech/spee... File content/browser/speech/speech_recognition_manager_impl.cc (right): http://codereview.chromium.org/9972011/diff/45003/content/browser/speech/spee... content/browser/speech/speech_recognition_manager_impl.cc:38: class VoidRecognitionManagerDelegate : On 2012/04/24 15:56:32, John Abd-El-Malek wrote: > the convention we have for other delegates is to null check. i think given this > boilerplate that you have to add otherwise, it's not more work to null check I thought it was more efficient and "robust" (no need to remember the check) for future changes. Switched back to "if" checks. http://codereview.chromium.org/9972011/diff/45003/content/browser/speech/spee... File content/browser/speech/speech_recognition_manager_impl.h (right): http://codereview.chromium.org/9972011/diff/45003/content/browser/speech/spee... content/browser/speech/speech_recognition_manager_impl.h:15: #include "content/browser/speech/speech_recognizer_impl.h" On 2012/04/24 15:56:32, John Abd-El-Malek wrote: > nit: just forward declare instead of including the header Done. http://codereview.chromium.org/9972011/diff/45003/content/browser/speech/spee... content/browser/speech/speech_recognition_manager_impl.h:16: #include "content/common/content_export.h" On 2012/04/24 15:56:32, John Abd-El-Malek wrote: > nit: not needed Done. http://codereview.chromium.org/9972011/diff/45003/content/browser/speech/spee... content/browser/speech/speech_recognition_manager_impl.h:23: struct SpeechRecognitionError; On 2012/04/24 15:56:32, John Abd-El-Malek wrote: > are you sure you need all these forward declared given all the above includes? Done. http://codereview.chromium.org/9972011/diff/45003/content/browser/speech/spee... content/browser/speech/speech_recognition_manager_impl.h:98: enum FSMState { On 2012/04/24 15:56:32, John Abd-El-Malek wrote: > nit: you have FSM all over but what does it stand for? it's not entirely obvious Right. Added comment. http://codereview.chromium.org/9972011/diff/45003/content/public/browser/spee... File content/public/browser/speech_recognition_manager.h (right): http://codereview.chromium.org/9972011/diff/45003/content/public/browser/spee... content/public/browser/speech_recognition_manager.h:30: class CONTENT_EXPORT SpeechRecognitionManager { On 2012/04/24 15:56:32, John Abd-El-Malek wrote: > nit: why move the CONTENT_EXPORT? the convention for the content api has been to > put it only on static functions and not on the whole interface Done. http://codereview.chromium.org/9972011/diff/45003/content/public/browser/spee... File content/public/browser/speech_recognition_session_config.cc (right): http://codereview.chromium.org/9972011/diff/45003/content/public/browser/spee... content/public/browser/speech_recognition_session_config.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2012/04/24 15:56:32, John Abd-El-Malek wrote: > just to double check, are you sure you need a cc file in this case to avoid > clang errors? Yes, it exceeds the 10 points score: [chromium-style] Complex class/struct needs an explicit out-of-line destructor. http://codereview.chromium.org/9972011/diff/45003/content/public/browser/spee... File content/public/browser/speech_recognition_session_config.h (right): http://codereview.chromium.org/9972011/diff/45003/content/public/browser/spee... content/public/browser/speech_recognition_session_config.h:28: net::URLRequestContextGetter* url_request_context_getter; On 2012/04/24 15:56:32, John Abd-El-Malek wrote: > nit: why not hold this in scoped_refcounted? Ah, didn't noted that it is refcounted. But this will require to include "net/url_request/url_request_context_getter.h" in the .cc file. Is it ok? After all this struct is only used synchronously for initialization and is never held. http://codereview.chromium.org/9972011/diff/45003/content/public/browser/spee... File content/public/browser/speech_recognition_session_context.cc (right): http://codereview.chromium.org/9972011/diff/45003/content/public/browser/spee... content/public/browser/speech_recognition_session_context.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2012/04/24 15:56:32, John Abd-El-Malek wrote: > are you sure you need this cc file? Removed. http://codereview.chromium.org/9972011/diff/45003/content/public/browser/spee... File content/public/browser/speech_recognition_session_context.h (right): http://codereview.chromium.org/9972011/diff/45003/content/public/browser/spee... content/public/browser/speech_recognition_session_context.h:15: // (ChromeSpeechRecognitionManagerDelegate) for mapping the recognition session On 2012/04/24 15:56:32, John Abd-El-Malek wrote: > nit: don't mention chrome classes in the public api for content Done. http://codereview.chromium.org/9972011/diff/45003/content/public/browser/spee... content/public/browser/speech_recognition_session_context.h:23: struct CONTENT_EXPORT SpeechRecognitionSessionContext { On 2012/04/24 15:56:32, John Abd-El-Malek wrote: > nit: are you sure you need a CONTENT_EXPORT for this? It is used by tests (speech_recognition_browsertest.cc) and, IIRC, without it fails compilation on win bots. > and also are you usre you > need a cc file? Uhm, seems not. I just realized that the "complex class/struct" error is based on a score system, not just the presence of non POD fields.
http://codereview.chromium.org/9972011/diff/61001/chrome/browser/speech/chrom... File chrome/browser/speech/chrome_speech_recognition_manager_delegate.cc (right): http://codereview.chromium.org/9972011/diff/61001/chrome/browser/speech/chrom... chrome/browser/speech/chrome_speech_recognition_manager_delegate.cc:148: // postingt CheckRenderViewType, which will issue the callback on our behalf. nit: s/postingt/posting to/ http://codereview.chromium.org/9972011/diff/61001/chrome/browser/speech/chrom... chrome/browser/speech/chrome_speech_recognition_manager_delegate.cc:154: context.render_view_id)); i think this way of solving the problem of checking it on the right thread is ok http://codereview.chromium.org/9972011/diff/61001/chrome/browser/speech/chrom... chrome/browser/speech/chrome_speech_recognition_manager_delegate.cc:242: // speech input extension API should be used instead. i would add a blank line before and after this comment block to make it easier to read
http://codereview.chromium.org/9972011/diff/45003/content/public/browser/spee... File content/public/browser/speech_recognition_session_config.h (right): http://codereview.chromium.org/9972011/diff/45003/content/public/browser/spee... content/public/browser/speech_recognition_session_config.h:28: net::URLRequestContextGetter* url_request_context_getter; On 2012/04/25 11:30:03, Primiano Tucci wrote: > On 2012/04/24 15:56:32, John Abd-El-Malek wrote: > > nit: why not hold this in scoped_refcounted? > Ah, didn't noted that it is refcounted. But this will require to include > "net/url_request/url_request_context_getter.h" in the .cc file. Is it ok? After > all this struct is only used synchronously for initialization and is never held. yeah there are no issues with including other .h files from this cc file. it seems safer to wrap this in a scoped_refptr http://codereview.chromium.org/9972011/diff/61001/content/browser/speech/spee... File content/browser/speech/speech_recognition_manager_impl.cc (right): http://codereview.chromium.org/9972011/diff/61001/content/browser/speech/spee... content/browser/speech/speech_recognition_manager_impl.cc:76: if (delegate_ != NULL) nit: all the if (delegate_ != NULL) should just be if (delegate_) per convention http://codereview.chromium.org/9972011/diff/61001/content/public/browser/spee... File content/public/browser/speech_recognition_manager_delegate.h (right): http://codereview.chromium.org/9972011/diff/61001/content/public/browser/spee... content/public/browser/speech_recognition_manager_delegate.h:33: virtual void CheckRecognitionIsAllowedAsync( nit: no need to have "async" in the function name, it's clear from the arguments
http://codereview.chromium.org/9972011/diff/45003/content/public/browser/spee... File content/public/browser/speech_recognition_session_config.h (right): http://codereview.chromium.org/9972011/diff/45003/content/public/browser/spee... content/public/browser/speech_recognition_session_config.h:28: net::URLRequestContextGetter* url_request_context_getter; On 2012/04/25 15:14:08, John Abd-El-Malek wrote: > On 2012/04/25 11:30:03, Primiano Tucci wrote: > > On 2012/04/24 15:56:32, John Abd-El-Malek wrote: > > > nit: why not hold this in scoped_refcounted? > > Ah, didn't noted that it is refcounted. But this will require to include > > "net/url_request/url_request_context_getter.h" in the .cc file. Is it ok? > After > > all this struct is only used synchronously for initialization and is never > held. > > yeah there are no issues with including other .h files from this cc file. it > seems safer to wrap this in a scoped_refptr Done. http://codereview.chromium.org/9972011/diff/61001/chrome/browser/speech/chrom... File chrome/browser/speech/chrome_speech_recognition_manager_delegate.cc (right): http://codereview.chromium.org/9972011/diff/61001/chrome/browser/speech/chrom... chrome/browser/speech/chrome_speech_recognition_manager_delegate.cc:148: // postingt CheckRenderViewType, which will issue the callback on our behalf. On 2012/04/25 13:06:46, hans wrote: > nit: s/postingt/posting to/ Done. http://codereview.chromium.org/9972011/diff/61001/chrome/browser/speech/chrom... chrome/browser/speech/chrome_speech_recognition_manager_delegate.cc:154: context.render_view_id)); On 2012/04/25 13:06:46, hans wrote: > i think this way of solving the problem of checking it on the right thread is ok Done. http://codereview.chromium.org/9972011/diff/61001/chrome/browser/speech/chrom... chrome/browser/speech/chrome_speech_recognition_manager_delegate.cc:242: // speech input extension API should be used instead. On 2012/04/25 13:06:46, hans wrote: > i would add a blank line before and after this comment block to make it easier > to read Done. http://codereview.chromium.org/9972011/diff/61001/content/browser/speech/spee... File content/browser/speech/speech_recognition_manager_impl.cc (right): http://codereview.chromium.org/9972011/diff/61001/content/browser/speech/spee... content/browser/speech/speech_recognition_manager_impl.cc:76: if (delegate_ != NULL) On 2012/04/25 15:14:08, John Abd-El-Malek wrote: > nit: all the > if (delegate_ != NULL) > should just be > if (delegate_) > per convention Done. http://codereview.chromium.org/9972011/diff/61001/content/public/browser/spee... File content/public/browser/speech_recognition_manager_delegate.h (right): http://codereview.chromium.org/9972011/diff/61001/content/public/browser/spee... content/public/browser/speech_recognition_manager_delegate.h:33: virtual void CheckRecognitionIsAllowedAsync( On 2012/04/25 15:14:08, John Abd-El-Malek wrote: > nit: no need to have "async" in the function name, it's clear from the arguments Done.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/primiano@chromium.org/9972011/67003
Change committed as 133967 |
