Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
15907012:
Implement SpeechRecognizerImplAndroid (Closed)
|
Created:
7 years, 6 months ago by janx Modified:
7 years, 6 months ago CC:
Primiano Tucci (use gerrit), Peter Beverloo, chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionImplement SpeechRecognizerImplAndroid
Use JNI to perform speech recognition using Android's platform recognition
capabilities. This CL changes the behavior of webkitSpeechRecognizer when the
run-time flag --enable-speech-recognition is passed, but still won't recognize
speech before an AndroidSpeechRecognitionManagerDelegate is implemented to
grant according permissions.
BUG=247369
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=206398
Patch Set 1 #
Total comments: 63
Patch Set 2 : OnRecognitionEnd, state_, removed AudioLevelsChange, fixes, nits #
Total comments: 28
Patch Set 3 : nits #
Total comments: 27
Patch Set 4 : Convert error codes in Java, refactor *{,JNI} methods into single methods, nits #
Total comments: 33
Patch Set 5 : Proper event teardown on abort, nits #Patch Set 6 : Fix non-android builds by excluding speech_recognizer_impl_android #
Total comments: 39
Patch Set 7 : nits #
Total comments: 4
Patch Set 8 : Rebased, nits #Messages
Total messages: 23 (0 generated)
Not ready for review yet, uploading for discussion about how best to resolve some issues highlighted by "FIXME"s in the code.
https://codereview.chromium.org/15907012/diff/1/content/browser/speech/speech... File content/browser/speech/speech_recognizer_impl_android.cc (right): https://codereview.chromium.org/15907012/diff/1/content/browser/speech/speech... content/browser/speech/speech_recognizer_impl_android.cc:86: return false; // FIXME Recognition seems to work when IsActive() and IsCapturingAudio() always return false. Those methods are called by SpeechRecognitionManagerImpl to control a state machine. Primiano thinks that this state machine is probably not useful anymore, since its goal was to handle an edge case when a user clicked outside of a UI bubble while recognition was almost finished. Should we try to return sensible values for IsActive() and IsCapturingAudio(), or should we remove those methods? https://codereview.chromium.org/15907012/diff/1/content/browser/speech/speech... content/browser/speech/speech_recognizer_impl_android.cc:90: return false; // FIXME ditto https://codereview.chromium.org/15907012/diff/1/content/browser/speech/speech... content/browser/speech/speech_recognizer_impl_android.cc:201: // FIXME set audio_level_ and noise_level from rms The audio and noise levels are calculated from the rms on desktop with a lot of floating point arithmetic. Their only use if for an animation in the speech bubble which won't be used on Android. Should we remove OnAudioLevelsChange? https://codereview.chromium.org/15907012/diff/1/content/public/android/java/s... File content/public/android/java/src/org/chromium/content/browser/SpeechRecognition.java (right): https://codereview.chromium.org/15907012/diff/1/content/public/android/java/s... content/public/android/java/src/org/chromium/content/browser/SpeechRecognition.java:62: // FIXME call recognizer.destroy() ? I am not sure when it makes the most sense to destroy the SpeechRecognizer. After it has finished recognizing? When stopped? When cancelled? https://codereview.chromium.org/15907012/diff/1/content/public/android/java/s... content/public/android/java/src/org/chromium/content/browser/SpeechRecognition.java:101: // FIXME call recognizer.destroy() ? ditto https://codereview.chromium.org/15907012/diff/1/content/public/android/java/s... content/public/android/java/src/org/chromium/content/browser/SpeechRecognition.java:107: // FIXME call recognizer.destroy() ? ditto https://codereview.chromium.org/15907012/diff/1/content/public/android/java/s... content/public/android/java/src/org/chromium/content/browser/SpeechRecognition.java:115: private native void nativeOnAudioLevelsChangeJNI(int nativeSpeechRecognizerImplAndroid, float rms); Those lines are more than 80 characters and the presubmit checks complain about it, but the code looks really bad and confusing when wrapped, and other files in the same folder ignore the 80 char limit. Should those lines be wrapped? If so, how?
Very good job, we are very close for making it rock on Android. :) In the meanwhile, I've "some" comments :) As general indications (further explained inline): - Do not expose details (mostly names and short-circuiting events) about the Android's Speech Recognition API inside the .cc file. - Careful with callbacks, be sure that the native object exists when issuing callbacks. In order to do so, IMHO the best pattern is deciding by design that everything "dies" after a certain event (OnRecognitionEnd), so no more (JNI to Java) events should be dispatched after that event. This is the assumption that the speech_recognition_manager_impl.cc is doing and is exactly same approach that was taken in the design of the original speech_recognizer_impl.cc -After our rounds of comments will be over, please find an authoritative reviewer (at least, more authoritative than me) for the Java/JNI code. https://codereview.chromium.org/15907012/diff/1/content/browser/browser_main_... File content/browser/browser_main_loop.cc (right): https://codereview.chromium.org/15907012/diff/1/content/browser/browser_main_... content/browser/browser_main_loop.cc:36: #include "content/browser/speech/speech_recognizer_impl_android.h" Nit: swap lines 36 and 37. https://codereview.chromium.org/15907012/diff/1/content/browser/speech/speech... File content/browser/speech/speech_recognition_manager_impl.cc (right): https://codereview.chromium.org/15907012/diff/1/content/browser/speech/speech... content/browser/speech/speech_recognition_manager_impl.cc:138: config, Can you just pass session_id and use SpeechRecognitionManager.GetSessionConfig(id) to get the config inside SpeechRecognizerImplAndroid? https://codereview.chromium.org/15907012/diff/1/content/browser/speech/speech... File content/browser/speech/speech_recognizer_impl_android.cc (right): https://codereview.chromium.org/15907012/diff/1/content/browser/speech/speech... content/browser/speech/speech_recognizer_impl_android.cc:42: listener_->OnRecognitionStart(session_id_); I'd like to have feedback by some other reviewer on this comment. I think is a bit safer posting the OnRecognitionStart (and similar below), even if the source and target threads are the same (IO), rather than calling the callback method directly. What I fear about calling callbacks synchronously is that you can end up in unpleasant nested/reentrant call stacks. For instance, imagine that for some reason the listener (i.e. the S.R. manager in our case) has a code path that calls AbortRecognition in the OnRecognitionStart event handler (e.g., in a error/not authorized code path). In such a case you would end up having a call stack like this: SRManager.Something() SRImplAndroid.StartRecognition() SRManager.OnRecognitionStart(...) SRImplAndroid.AbortRecognition() SRManager.OnRecognitionEnd() I don't know if this particular example is possible with the current S.R. manager implementation, but IMHO if some code path will change in future, a nested call stack like the one depicted above could cause serious headaches. Conversely, if you post call-backs on the same (IO) thread, you basically will break any possiblity of nesting/reentrancy (at the price of a little higher latency). If you take a look to the current speech_recognizer_impl.cc implementation, all the listener_->CallbackMethods() are always called inside the FSM, which by design is always decoupled in a separate message loop task (read: all evolution functions of the FSM are "posted"). What do you think? https://codereview.chromium.org/15907012/diff/1/content/browser/speech/speech... content/browser/speech/speech_recognizer_impl_android.cc:47: void SpeechRecognizerImplAndroid::StartRecognitionJNI(bool continuous, Nit: \n https://codereview.chromium.org/15907012/diff/1/content/browser/speech/speech... content/browser/speech/speech_recognizer_impl_android.cc:48: bool partial_results) { Why not keeping the interim_results name? I understand that in the Android world our "interim" results are called "partial", but I'd prefer to not see this name translation here, while we are still in the chromium/blink world :) https://codereview.chromium.org/15907012/diff/1/content/browser/speech/speech... content/browser/speech/speech_recognizer_impl_android.cc:53: reinterpret_cast<jlong>(this))); I am definitely not a JNI expert, so I'd like someone else taking a look at this: is this the right way to pass the "this" reference for receiving callback events? Searching in cs.chromium.org for "reinterpret_cast<jlong>(this)" returns just one result, which is not really encouraging :) Maybe s/jlong/jint/? https://codereview.chromium.org/15907012/diff/1/content/browser/speech/speech... content/browser/speech/speech_recognizer_impl_android.cc:64: listener_->OnRecognitionEnd(session_id_); Keep in mind that OnRecognitionEnd is the final event that causes the manager to free the recognizer(_impl) in the following code path: OnRecognitionEnd(...) > DispatchEvent(EVENT_RECOGNITION_EVENT) > SessionDelete(...). Therefore, are you sure that, after this call, the JNI object has been cleaned and will never ever issue any further callbacks? After an offline discussion seems that this call should not be performed here, rather you should have a further JNI event (see my other comment on the .h file) and move the OnRecognitionEnd call to such method. In order to be 100% safe, I think (but I've very little JNI knowledge) that just before calling listener_->OnRecognitionEnd, you should call a JNI method on your SpeechRecognition.java class that cleans up the native pointer (mNativeSpeechRecognizerImplAndroid). Or, even better, you should have only a single call site in which you call listener_->OnRecognitionEnd (the OnRecognitionEndJNI seems the best candidate), and you should "promise" that when the OnRecognitionEndJNI method is called, the Java object has released the mNativeSpeechRecognizerImplAndroid pointer, so that no further events can come after. https://codereview.chromium.org/15907012/diff/1/content/browser/speech/speech... content/browser/speech/speech_recognizer_impl_android.cc:66: void SpeechRecognizerImplAndroid::CancelRecognitionJNI() { Similarly to my previous comment (on partial_results). The name of the Java methods (specifically, the Android SpeechRecognizer's API) going to be called should not be perceived here. https://codereview.chromium.org/15907012/diff/1/content/browser/speech/speech... content/browser/speech/speech_recognizer_impl_android.cc:66: void SpeechRecognizerImplAndroid::CancelRecognitionJNI() { Nit: \n (here and similarly below) https://codereview.chromium.org/15907012/diff/1/content/browser/speech/speech... content/browser/speech/speech_recognizer_impl_android.cc:69: j_recognition_.obj()); At this point we must be sure that the JNI object has been "killed" and won't deliver any more callback. The reason is that as soon as this method returns, the SpeechRecognizerAndroidImpl object (i.e.: this) will be freed. https://codereview.chromium.org/15907012/diff/1/content/browser/speech/speech... content/browser/speech/speech_recognizer_impl_android.cc:77: listener_->OnRecognitionEnd(session_id_); Again: OnRecognitionEnd will cause the manager to destroy the SpeechRecognizerImplAndroid object. The sense of StopAudioCapture, instead, is just stopping audio but allowing last results to come, and deliver further OnResult events. Therefore the OnRecognitionEnd call here seems a bit premature. https://codereview.chromium.org/15907012/diff/1/content/browser/speech/speech... content/browser/speech/speech_recognizer_impl_android.cc:86: return false; // FIXME On 2013/05/31 17:22:24, janx wrote: > Recognition seems to work when IsActive() and IsCapturingAudio() always return > false. Those methods are called by SpeechRecognitionManagerImpl to control a > state machine. Primiano thinks that this state machine is probably not useful > anymore, since its goal was to handle an edge case when a user clicked outside > of a UI bubble while recognition was almost finished. > > Should we try to return sensible values for IsActive() and IsCapturingAudio(), > or should we remove those methods? Hmm, actually I think you should implement this. I know that it seems to work keeping it stuck at false, but it can cause some odd behaviors. For instance, it will cause the manager to restart the recognition if two subsequent StartRecognition() calls are performed on the same session id. The expected behavior, instead, is that the second start() call is ignored. Second side effect: It seems that the primary_session_id_ is never reset in the manager, basically because without implementing properly this (and the IsCapturingAudio) method, the manager will think that the recognition never begun. P.S. In the immediate future we'll need all this cases to be covered by an extensive unittest. https://codereview.chromium.org/15907012/diff/1/content/browser/speech/speech... content/browser/speech/speech_recognizer_impl_android.cc:90: return false; // FIXME On 2013/05/31 17:22:24, janx wrote: > ditto Ditto :) Btw, you should be able to implement these two methods (this and IsActive) just using two booleans (or even better a single enum), which are simply updated in the Start/Stop/Abort methods. https://codereview.chromium.org/15907012/diff/1/content/browser/speech/speech... content/browser/speech/speech_recognizer_impl_android.cc:121: // Since Android's speech API doesn't have an AudioEnd event, we emulate it. It's ok emulating the event but not here. The same principle (see my other comments) applies here about encapsulating the Android's API. Please do it in the .java file (so introduce an OnAudioEndJNI method here). https://codereview.chromium.org/15907012/diff/1/content/browser/speech/speech... content/browser/speech/speech_recognizer_impl_android.cc:156: SpeechRecognitionErrorCode code = SPEECH_RECOGNITION_ERROR_NONE; Is it actually possible to get another error code here? Perhaps it make more sense to add a default case in the switch like this: default: NOTREACHED(); return; https://codereview.chromium.org/15907012/diff/1/content/browser/speech/speech... content/browser/speech/speech_recognizer_impl_android.cc:158: // Translate Android speech recognition errors to Web Speech API errors. Can we move this translation in the .java file ? If I understand correctly these integers here are the error codes defined by Android's Speech recognition API. However, in this file there is no trace (as it should be) about the fact that we are using Android's S.R. API. https://codereview.chromium.org/15907012/diff/1/content/browser/speech/speech... content/browser/speech/speech_recognizer_impl_android.cc:189: if (error.code != SPEECH_RECOGNITION_ERROR_NONE) See previous comment, this shouldn't be required. https://codereview.chromium.org/15907012/diff/1/content/browser/speech/speech... content/browser/speech/speech_recognizer_impl_android.cc:201: // FIXME set audio_level_ and noise_level from rms On 2013/05/31 17:22:24, janx wrote: > The audio and noise levels are calculated from the rms on desktop with a lot of > floating point arithmetic. Their only use if for an animation in the speech > bubble which won't be used on Android. Should we remove OnAudioLevelsChange? I thinks that you can safely remove completely this and the OnAudioLevelsChangeJNI method, as, IIRC (and as you mentioned), the rms levels are only required for the x-webkit-speech support (which is not supported by Chrome for Android), and not for the Web Speech API. https://codereview.chromium.org/15907012/diff/1/content/browser/speech/speech... File content/browser/speech/speech_recognizer_impl_android.h (right): https://codereview.chromium.org/15907012/diff/1/content/browser/speech/speech... content/browser/speech/speech_recognizer_impl_android.h:13: Nit: Is this newline needed here? I can't find anything in either code style. https://codereview.chromium.org/15907012/diff/1/content/browser/speech/speech... content/browser/speech/speech_recognizer_impl_android.h:27: virtual ~SpeechRecognizerImplAndroid(); Since SpeechRecognizer (the base class) is refcounted, I guess it would be safer for the dtor to be protected, in order to catch unwanted/unexpected explicit frees. https://codereview.chromium.org/15907012/diff/1/content/browser/speech/speech... content/browser/speech/speech_recognizer_impl_android.h:33: void StartRecognition(); Shouldn't this be virtual void StartRecognition() OVERRIDE? (same for the 4 lines below) https://codereview.chromium.org/15907012/diff/1/content/browser/speech/speech... content/browser/speech/speech_recognizer_impl_android.h:39: void StartRecognitionJNI(bool continuous, bool partial_results); Shouldn't these three methods be private? Also, can you add a comment like this? // Wrappers for JNI methods. https://codereview.chromium.org/15907012/diff/1/content/browser/speech/speech... content/browser/speech/speech_recognizer_impl_android.h:42: Add a comment: // Called from Java via JNI. https://codereview.chromium.org/15907012/diff/1/content/browser/speech/speech... content/browser/speech/speech_recognizer_impl_android.h:49: void OnAudioLevelsChangeJNI(JNIEnv* env, jobject obj, jfloat rms); Looks like here you need an OnRecognitionEndJNI event coming from Java. This event should be "solemn" promise that no other callback will be called from the JNI object. Concretely, it will be the method in which you should call the listener_->OnRecognitionEnd() (C++) method, which has the meaning "This is the last event, feel free to destroy the object". (See the corresponding comment in: https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro...) https://codereview.chromium.org/15907012/diff/1/content/browser/speech/speech... content/browser/speech/speech_recognizer_impl_android.h:51: void OnAudioStart(); Similar to previous comments: I guess these 6 methods, which are wrapped by the equivalent ...JNI ones, should be private. https://codereview.chromium.org/15907012/diff/1/content/browser/speech/speech... content/browser/speech/speech_recognizer_impl_android.h:61: Nit: remove blank line. https://codereview.chromium.org/15907012/diff/1/content/browser/speech/speech... content/browser/speech/speech_recognizer_impl_android.h:62: SpeechRecognitionSessionConfig config_; See my previous comment: since it seems you're using config_ only in StartRecognition, it would be cleaner just using calling GetSessionConfig (in StartRecognition) and avoiding this field at all. https://codereview.chromium.org/15907012/diff/1/content/public/android/java/s... File content/public/android/java/src/org/chromium/content/browser/SpeechRecognition.java (right): https://codereview.chromium.org/15907012/diff/1/content/public/android/java/s... content/public/android/java/src/org/chromium/content/browser/SpeechRecognition.java:32: private int mNativeSpeechRecognizerImplAndroid = 0; You'll need a reviewer for someone more experienced than me on this .java file :). Is this the right way to issue callbacks between Java and C++? https://codereview.chromium.org/15907012/diff/1/content/public/android/java/s... content/public/android/java/src/org/chromium/content/browser/SpeechRecognition.java:39: @Override Nit: \n? (here and below) https://codereview.chromium.org/15907012/diff/1/content/public/android/java/s... content/public/android/java/src/org/chromium/content/browser/SpeechRecognition.java:62: // FIXME call recognizer.destroy() ? On 2013/05/31 17:22:24, janx wrote: > I am not sure when it makes the most sense to destroy the SpeechRecognizer. > After it has finished recognizing? When stopped? When cancelled? Just before calling the OnRecognitionEndJNI method sounds fine. However, it's more important that you clean up the mNativeSpeechRecognizerImplAndroid pointer and either check-for-null or assert before calling JNI methods. https://codereview.chromium.org/15907012/diff/1/content/public/android/java/s... content/public/android/java/src/org/chromium/content/browser/SpeechRecognition.java:66: nativeOnAudioLevelsChangeJNI(mNativeSpeechRecognizerImplAndroid, rms); See my previous comments, I think you can remove completely this call and leave onRmsChanged unimplemented. Also, please can you add a TODO to speech_recognition_event_listener.h to the OnAudioLevelsChange, mentioning that is just needed for x-webkit-speech bubble support? Thanks https://codereview.chromium.org/15907012/diff/1/content/public/android/java/s... content/public/android/java/src/org/chromium/content/browser/SpeechRecognition.java:70: String[] strings = results.toArray(new String[results.size()]); s/strings/transcripts/ https://codereview.chromium.org/15907012/diff/1/content/public/android/java/s... content/public/android/java/src/org/chromium/content/browser/SpeechRecognition.java:71: float[] floats = bundle.getFloatArray(SpeechRecognizer.CONFIDENCE_SCORES); s/floats/scores/ https://codereview.chromium.org/15907012/diff/1/content/public/android/java/s... content/public/android/java/src/org/chromium/content/browser/SpeechRecognition.java:101: // FIXME call recognizer.destroy() ? On 2013/05/31 17:22:24, janx wrote: > ditto As in my previous comment, s/CancelRecognition/AbortRecognition/. You need to be sure here the the On...End events are always correctly dispatched. The question is: will recognizer.cancel() cause further (Java) callbacks on the listener? Maybe the onEndOfSpeech() event will? In that case, you don't want to .destroy here(). https://codereview.chromium.org/15907012/diff/1/content/public/android/java/s... content/public/android/java/src/org/chromium/content/browser/SpeechRecognition.java:107: // FIXME call recognizer.destroy() ? On 2013/05/31 17:22:24, janx wrote: > ditto Definitely not here. On StopRecognition we are interested in getting (eventually) results with the audio captured so far. https://codereview.chromium.org/15907012/diff/1/content/public/android/java/s... content/public/android/java/src/org/chromium/content/browser/SpeechRecognition.java:115: private native void nativeOnAudioLevelsChangeJNI(int nativeSpeechRecognizerImplAndroid, float rms); On 2013/05/31 17:22:24, janx wrote: > Those lines are more than 80 characters and the presubmit checks complain about > it, but the code looks really bad and confusing when wrapped, and other files in > the same folder ignore the 80 char limit. Should those lines be wrapped? If so, > how? I'm fine with no wrapping if there are other similar cases.
Thank you so much Primiano for the awesome comments! I ended up changing a lot of things, and it all feels much more solid now. I will upload a new patch set with all the fixes once I have moved Android-to-Web speech error translation from CC to Java, and then ask someone to review my Java/JNI code. https://codereview.chromium.org/15907012/diff/1/content/browser/browser_main_... File content/browser/browser_main_loop.cc (right): https://codereview.chromium.org/15907012/diff/1/content/browser/browser_main_... content/browser/browser_main_loop.cc:36: #include "content/browser/speech/speech_recognizer_impl_android.h" On 2013/06/03 16:24:11, Primiano Tucci wrote: > Nit: swap lines 36 and 37. I believe the alphabetical order is correct as it is. https://codereview.chromium.org/15907012/diff/1/content/browser/speech/speech... File content/browser/speech/speech_recognition_manager_impl.cc (right): https://codereview.chromium.org/15907012/diff/1/content/browser/speech/speech... content/browser/speech/speech_recognition_manager_impl.cc:138: config, On 2013/06/03 16:24:11, Primiano Tucci wrote: > Can you just pass session_id and use > SpeechRecognitionManager.GetSessionConfig(id) to get the config inside > SpeechRecognizerImplAndroid? Since the config is used only in StartRecognition, I'll use the getter instead of saving the config as an attribute. Nice catch. https://codereview.chromium.org/15907012/diff/1/content/browser/speech/speech... File content/browser/speech/speech_recognizer_impl_android.cc (right): https://codereview.chromium.org/15907012/diff/1/content/browser/speech/speech... content/browser/speech/speech_recognizer_impl_android.cc:47: void SpeechRecognizerImplAndroid::StartRecognitionJNI(bool continuous, On 2013/06/03 16:24:11, Primiano Tucci wrote: > Nit: \n I was collating coupled functions together, but since that is not a standard thing I will add empty lines everywhere. https://codereview.chromium.org/15907012/diff/1/content/browser/speech/speech... content/browser/speech/speech_recognizer_impl_android.cc:48: bool partial_results) { On 2013/06/03 16:24:11, Primiano Tucci wrote: > Why not keeping the interim_results name? > I understand that in the Android world our "interim" results are called > "partial", but I'd prefer to not see this name translation here, while we are > still in the chromium/blink world :) Let's go with "interim" then. https://codereview.chromium.org/15907012/diff/1/content/browser/speech/speech... content/browser/speech/speech_recognizer_impl_android.cc:53: reinterpret_cast<jlong>(this))); Fixed both occurences (this CL and http://crrev.com/15793008/). https://codereview.chromium.org/15907012/diff/1/content/browser/speech/speech... content/browser/speech/speech_recognizer_impl_android.cc:66: void SpeechRecognizerImplAndroid::CancelRecognitionJNI() { On 2013/06/03 16:24:11, Primiano Tucci wrote: > Similarly to my previous comment (on partial_results). The name of the Java > methods (specifically, the Android SpeechRecognizer's API) going to be called > should not be perceived here. Done. https://codereview.chromium.org/15907012/diff/1/content/browser/speech/speech... content/browser/speech/speech_recognizer_impl_android.cc:86: return false; // FIXME Thanks for your input, I'll go ahead and reimplement IsActive() and IsCapturingAudio() correctly, using a state_ member variable. https://codereview.chromium.org/15907012/diff/1/content/browser/speech/speech... content/browser/speech/speech_recognizer_impl_android.cc:90: return false; // FIXME On 2013/06/03 16:24:11, Primiano Tucci wrote: > On 2013/05/31 17:22:24, janx wrote: > > ditto > Ditto :) > > Btw, you should be able to implement these two methods (this and IsActive) just > using two booleans (or even better a single enum), which are simply updated in > the Start/Stop/Abort methods. Done. https://codereview.chromium.org/15907012/diff/1/content/browser/speech/speech... content/browser/speech/speech_recognizer_impl_android.cc:121: // Since Android's speech API doesn't have an AudioEnd event, we emulate it. On 2013/06/03 16:24:11, Primiano Tucci wrote: > It's ok emulating the event but not here. The same principle (see my other > comments) applies here about encapsulating the Android's API. > Please do it in the .java file (so introduce an OnAudioEndJNI method here). Ok, I'll emulate the AudioEnd event in the Java file instead. https://codereview.chromium.org/15907012/diff/1/content/browser/speech/speech... content/browser/speech/speech_recognizer_impl_android.cc:156: SpeechRecognitionErrorCode code = SPEECH_RECOGNITION_ERROR_NONE; On 2013/06/03 16:24:11, Primiano Tucci wrote: > Is it actually possible to get another error code here? > Perhaps it make more sense to add a default case in the switch like this: > > default: > NOTREACHED(); > return; SPEECH_RECOGNITION_ERROR_NONE is a hack and should not be used, so "default: NOTREACHED()" is a great idea! https://codereview.chromium.org/15907012/diff/1/content/browser/speech/speech... content/browser/speech/speech_recognizer_impl_android.cc:189: if (error.code != SPEECH_RECOGNITION_ERROR_NONE) On 2013/06/03 16:24:11, Primiano Tucci wrote: > See previous comment, this shouldn't be required. Removed. https://codereview.chromium.org/15907012/diff/1/content/browser/speech/speech... content/browser/speech/speech_recognizer_impl_android.cc:201: // FIXME set audio_level_ and noise_level from rms Agreed, let's completely remove OnAudioLevelsChange{,JNI}. https://codereview.chromium.org/15907012/diff/1/content/browser/speech/speech... File content/browser/speech/speech_recognizer_impl_android.h (right): https://codereview.chromium.org/15907012/diff/1/content/browser/speech/speech... content/browser/speech/speech_recognizer_impl_android.h:13: On 2013/06/03 16:24:11, Primiano Tucci wrote: > Nit: Is this newline needed here? I can't find anything in either code style. I think new lines are needed when include order matters and you have to break alphabetical order. It is not needed here, so I'll remove it. https://codereview.chromium.org/15907012/diff/1/content/browser/speech/speech... content/browser/speech/speech_recognizer_impl_android.h:27: virtual ~SpeechRecognizerImplAndroid(); On 2013/06/03 16:24:11, Primiano Tucci wrote: > Since SpeechRecognizer (the base class) is refcounted, I guess it would be safer > for the dtor to be protected, in order to catch unwanted/unexpected explicit > frees. I'll make it private, thanks for spotting this! https://codereview.chromium.org/15907012/diff/1/content/browser/speech/speech... content/browser/speech/speech_recognizer_impl_android.h:33: void StartRecognition(); On 2013/06/03 16:24:11, Primiano Tucci wrote: > Shouldn't this be virtual void StartRecognition() OVERRIDE? > (same for the 4 lines below) Indeed, that's a leftover from before SpeechRecognizer could be derived. https://codereview.chromium.org/15907012/diff/1/content/browser/speech/speech... content/browser/speech/speech_recognizer_impl_android.h:39: void StartRecognitionJNI(bool continuous, bool partial_results); On 2013/06/03 16:24:11, Primiano Tucci wrote: > Shouldn't these three methods be private? > Also, can you add a comment like this? > // Wrappers for JNI methods. Will do. https://codereview.chromium.org/15907012/diff/1/content/browser/speech/speech... content/browser/speech/speech_recognizer_impl_android.h:42: On 2013/06/03 16:24:11, Primiano Tucci wrote: > Add a comment: // Called from Java via JNI. Done. https://codereview.chromium.org/15907012/diff/1/content/browser/speech/speech... content/browser/speech/speech_recognizer_impl_android.h:49: void OnAudioLevelsChangeJNI(JNIEnv* env, jobject obj, jfloat rms); I'll implement an OnRecognitionEndJNI(e,o) method with a wrapper calling listener_->OnRecognitionEnd(), and make sure that no events are thrown afterwards. https://codereview.chromium.org/15907012/diff/1/content/browser/speech/speech... content/browser/speech/speech_recognizer_impl_android.h:51: void OnAudioStart(); On 2013/06/03 16:24:11, Primiano Tucci wrote: > Similar to previous comments: I guess these 6 methods, which are wrapped by the > equivalent ...JNI ones, should be private. Moving to private. https://codereview.chromium.org/15907012/diff/1/content/browser/speech/speech... content/browser/speech/speech_recognizer_impl_android.h:62: SpeechRecognitionSessionConfig config_; On 2013/06/03 16:24:11, Primiano Tucci wrote: > See my previous comment: since it seems you're using config_ only in > StartRecognition, it would be cleaner just using calling GetSessionConfig (in > StartRecognition) and avoiding this field at all. Great suggestion, I'll use SpeechRecognitionManagerImpl::GetInstance()->GetSessionConfig() instead of the config_ attribute (thanks for the tip).
Recycled two comments from previous patchset for future reviewers: https://codereview.chromium.org/15907012/diff/11001/content/browser/speech/sp... File content/browser/speech/speech_recognizer_impl_android.cc (right): https://codereview.chromium.org/15907012/diff/11001/content/browser/speech/sp... content/browser/speech/speech_recognizer_impl_android.cc:43: listener_->OnRecognitionStart(session_id_); Primiano Tucci 2013/06/03 16:24:11 I'd like to have feedback by some other reviewer on this comment. I think is a bit safer posting the OnRecognitionStart (and similar below), even if the source and target threads are the same (IO), rather than calling the callback method directly. What I fear about calling callbacks synchronously is that you can end up in unpleasant nested/reentrant call stacks. For instance, imagine that for some reason the listener (i.e. the S.R. manager in our case) has a code path that calls AbortRecognition in the OnRecognitionStart event handler (e.g., in a error/not authorized code path). In such a case you would end up having a call stack like this: SRManager.Something() SRImplAndroid.StartRecognition() SRManager.OnRecognitionStart(...) SRImplAndroid.AbortRecognition() SRManager.OnRecognitionEnd() I don't know if this particular example is possible with the current S.R. manager implementation, but IMHO if some code path will change in future, a nested call stack like the one depicted above could cause serious headaches. Conversely, if you post call-backs on the same (IO) thread, you basically will break any possiblity of nesting/reentrancy (at the price of a little higher latency). If you take a look to the current speech_recognizer_impl.cc implementation, all the listener_->CallbackMethods() are always called inside the FSM, which by design is always decoupled in a separate message loop task (read: all evolution functions of the FSM are "posted"). What do you think? https://codereview.chromium.org/15907012/diff/11001/content/public/android/ja... File content/public/android/java/src/org/chromium/content/browser/SpeechRecognition.java (right): https://codereview.chromium.org/15907012/diff/11001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/SpeechRecognition.java:33: private int mNativeSpeechRecognizerImplAndroid = 0; Primiano Tucci 2013/06/03 16:24:11 You'll need a reviewer for someone more experienced than me on this .java file :). Is this the right way to issue callbacks between Java and C++?
Thanks Jan! I mostly looked at the Java file, as Primiano looked at the other files already. Can you please refer to the speech recognition bug in the commit message as well? https://codereview.chromium.org/15907012/diff/11001/content/content_browser.gypi File content/content_browser.gypi (right): https://codereview.chromium.org/15907012/diff/11001/content/content_browser.g... content/content_browser.gypi:1281: ['include', '^browser/speech/speech_recognition_dispatcher_host'], nit: I'd prefer to add \\.(cc|h)$ here to indicate that it's just about these two files, and not about any file starting with these characters. Goes for lines 1281, 1282 and 1284. https://codereview.chromium.org/15907012/diff/11001/content/public/android/ja... File content/public/android/java/src/org/chromium/content/browser/SpeechRecognition.java (right): https://codereview.chromium.org/15907012/diff/11001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/SpeechRecognition.java:13: import android.widget.Toast; I don't think you are using Toast. https://codereview.chromium.org/15907012/diff/11001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/SpeechRecognition.java:17: import org.chromium.base.ThreadUtils; I don't think you are using ThreadUtils. https://codereview.chromium.org/15907012/diff/11001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/SpeechRecognition.java:26: class SpeechRecognition { In general, I feel this class can do with a little bit more documentation. At least having some descriptions of the exposed-to-native API would be nice. https://codereview.chromium.org/15907012/diff/11001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/SpeechRecognition.java:28: private Intent intent = null; s/intent/mIntent/ https://codereview.chromium.org/15907012/diff/11001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/SpeechRecognition.java:31: private SpeechRecognizer recognizer = null; s/recognizer/mRecognizer/ https://codereview.chromium.org/15907012/diff/11001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/SpeechRecognition.java:32: private RecognitionListener listener = null; s/listener/mListener/ https://codereview.chromium.org/15907012/diff/11001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/SpeechRecognition.java:36: @Override Java really should have some kind of @Implements to complement @Override, since this technically doesn't override anything. https://codereview.chromium.org/15907012/diff/11001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/SpeechRecognition.java:45: // Since there is no AudioEnd event, we emulate it. Maybe rephrase this as something like "Android doesn't have an event similar to the speech API's onaudioend event. Fire it now as this is the most appropriate place to do so."? https://codereview.chromium.org/15907012/diff/11001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/SpeechRecognition.java:54: public void onEvent(int event, Bundle bundle) { } nit: even when empty, please have an empty line between methods. https://codereview.chromium.org/15907012/diff/11001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/SpeechRecognition.java:67: terminate(); If we don't call terminate() here, will it just restart? How are we handling recognition services which don't support continuous mode? https://codereview.chromium.org/15907012/diff/11001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/SpeechRecognition.java:71: public void handleResults(Bundle bundle, boolean provisional) { This method can be private. https://codereview.chromium.org/15907012/diff/11001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/SpeechRecognition.java:72: ArrayList<String> list = bundle.getStringArrayList(SpeechRecognizer.RESULTS_RECOGNITION); nit: line length limit in Java is a hundred characters. Elsewhere in this file too. https://codereview.chromium.org/15907012/diff/11001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/SpeechRecognition.java:84: recognizer = SpeechRecognizer.createSpeechRecognizer(mContext); While more of a follow up, have you thought about using SpeechRecognizer.isRecognitionAvailable() to gracefully disable the speech recognition feature if the system can't provide it? https://codereview.chromium.org/15907012/diff/11001/content/public/browser/sp... File content/public/browser/speech_recognition_event_listener.h (right): https://codereview.chromium.org/15907012/diff/11001/content/public/browser/sp... content/public/browser/speech_recognition_event_listener.h:56: // TODO: Is this necessary? It looks like only x-webkit-speech bubble uses it. Please (a) add attribution to any TODO you add to the code, so "TODO(janx)" in this case, and (b) file a bug about this considering it'll just get lost otherwise.
Thanks for the comments! I filed an engineering bug (crbug.com/247369) corresponding to this CL and linked it. As to the rest, see answers below: https://codereview.chromium.org/15907012/diff/11001/content/content_browser.gypi File content/content_browser.gypi (right): https://codereview.chromium.org/15907012/diff/11001/content/content_browser.g... content/content_browser.gypi:1281: ['include', '^browser/speech/speech_recognition_dispatcher_host'], On 2013/06/06 11:48:13, Peter Beverloo wrote: > nit: I'd prefer to add \\.(cc|h)$ here to indicate that it's just about these > two files, and not about any file starting with these characters. Goes for > lines 1281, 1282 and 1284. Done. https://codereview.chromium.org/15907012/diff/11001/content/public/android/ja... File content/public/android/java/src/org/chromium/content/browser/SpeechRecognition.java (right): https://codereview.chromium.org/15907012/diff/11001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/SpeechRecognition.java:13: import android.widget.Toast; On 2013/06/06 11:48:13, Peter Beverloo wrote: > I don't think you are using Toast. Nor am I using ThreadUtils or FutureTask. Nice catch, thanks. https://codereview.chromium.org/15907012/diff/11001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/SpeechRecognition.java:31: private SpeechRecognizer recognizer = null; On 2013/06/06 11:48:13, Peter Beverloo wrote: > s/recognizer/mRecognizer/ Done reluctantly. https://codereview.chromium.org/15907012/diff/11001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/SpeechRecognition.java:36: @Override On 2013/06/06 11:48:13, Peter Beverloo wrote: > Java really should have some kind of @Implements to complement @Override, since > this technically doesn't override anything. Maybe you should file a bug at http://bugreport.sun.com/bugreport/. https://codereview.chromium.org/15907012/diff/11001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/SpeechRecognition.java:45: // Since there is no AudioEnd event, we emulate it. On 2013/06/06 11:48:13, Peter Beverloo wrote: > Maybe rephrase this as something like "Android doesn't have an event similar to > the speech API's onaudioend event. Fire it now as this is the most appropriate > place to do so."? Rephrased to something similar. https://codereview.chromium.org/15907012/diff/11001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/SpeechRecognition.java:54: public void onEvent(int event, Bundle bundle) { } On 2013/06/06 11:48:13, Peter Beverloo wrote: > nit: even when empty, please have an empty line between methods. Done. https://codereview.chromium.org/15907012/diff/11001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/SpeechRecognition.java:67: terminate(); On 2013/06/06 11:48:13, Peter Beverloo wrote: > If we don't call terminate() here, will it just restart? How are we handling > recognition services which don't support continuous mode? The way it behaves now is that if mContinuous is true, and continuous mode is not supported, the recognition stalls (after yielding the first final result, it doesn't stop but doesn't restart and doesn't yield further results). https://codereview.chromium.org/15907012/diff/11001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/SpeechRecognition.java:71: public void handleResults(Bundle bundle, boolean provisional) { On 2013/06/06 11:48:13, Peter Beverloo wrote: > This method can be private. Done. https://codereview.chromium.org/15907012/diff/11001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/SpeechRecognition.java:72: ArrayList<String> list = bundle.getStringArrayList(SpeechRecognizer.RESULTS_RECOGNITION); On 2013/06/06 11:48:13, Peter Beverloo wrote: > nit: line length limit in Java is a hundred characters. Elsewhere in this file > too. Done. https://codereview.chromium.org/15907012/diff/11001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/SpeechRecognition.java:84: recognizer = SpeechRecognizer.createSpeechRecognizer(mContext); On 2013/06/06 11:48:13, Peter Beverloo wrote: > While more of a follow up, have you thought about using > SpeechRecognizer.isRecognitionAvailable() to gracefully disable the speech > recognition feature if the system can't provide it? Yes, I think that we could easily make a JNI call to a static Java method returning SpeechRecognizer.isRecognitionAvailable() when deciding whether to activate the feature in Blink (e.g. near https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r...). https://codereview.chromium.org/15907012/diff/11001/content/public/browser/sp... File content/public/browser/speech_recognition_event_listener.h (right): https://codereview.chromium.org/15907012/diff/11001/content/public/browser/sp... content/public/browser/speech_recognition_event_listener.h:56: // TODO: Is this necessary? It looks like only x-webkit-speech bubble uses it. On 2013/06/06 11:48:13, Peter Beverloo wrote: > Please (a) add attribution to any TODO you add to the code, so "TODO(janx)" in > this case, and (b) file a bug about this considering it'll just get lost > otherwise. Filed http://crbug.com/247351.
Recycled 3 comments from last patchset: https://codereview.chromium.org/15907012/diff/20001/content/browser/speech/sp... File content/browser/speech/speech_recognizer_impl_android.cc (right): https://codereview.chromium.org/15907012/diff/20001/content/browser/speech/sp... content/browser/speech/speech_recognizer_impl_android.cc:43: listener_->OnRecognitionStart(session_id_); Primiano Tucci 2013/06/03 16:24:11 I'd like to have feedback by some other reviewer on this comment. I think is a bit safer posting the OnRecognitionStart (and similar below), even if the source and target threads are the same (IO), rather than calling the callback method directly. What I fear about calling callbacks synchronously is that you can end up in unpleasant nested/reentrant call stacks. For instance, imagine that for some reason the listener (i.e. the S.R. manager in our case) has a code path that calls AbortRecognition in the OnRecognitionStart event handler (e.g., in a error/not authorized code path). In such a case you would end up having a call stack like this: SRManager.Something() SRImplAndroid.StartRecognition() SRManager.OnRecognitionStart(...) SRImplAndroid.AbortRecognition() SRManager.OnRecognitionEnd() I don't know if this particular example is possible with the current S.R. manager implementation, but IMHO if some code path will change in future, a nested call stack like the one depicted above could cause serious headaches. Conversely, if you post call-backs on the same (IO) thread, you basically will break any possiblity of nesting/reentrancy (at the price of a little higher latency). If you take a look to the current speech_recognizer_impl.cc implementation, all the listener_->CallbackMethods() are always called inside the FSM, which by design is always decoupled in a separate message loop task (read: all evolution functions of the FSM are "posted"). What do you think? https://codereview.chromium.org/15907012/diff/20001/content/public/android/ja... File content/public/android/java/src/org/chromium/content/browser/SpeechRecognition.java (right): https://codereview.chromium.org/15907012/diff/20001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/SpeechRecognition.java:26: class SpeechRecognition { Peter Beverloo 2013/06/06 11:48:13 In general, I feel this class can do with a little bit more documentation. At least having some descriptions of the exposed-to-native API would be nice. https://codereview.chromium.org/15907012/diff/20001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/SpeechRecognition.java:33: private int mNativeSpeechRecognizerImplAndroid = 0; Primiano Tucci 2013/06/03 16:24:11 You'll need a reviewer for someone more experienced than me on this .java file :). Is this the right way to issue callbacks between Java and C++?
nice, thanks janx! I have a few nits and small suggestions, the most important one about the "JNI" suffix for the functions versus just posting the task back to itself... there's one comment that is not entirely related to your patch (protected raw members), so feel free to do it in a separate patch.. https://codereview.chromium.org/15907012/diff/20001/content/browser/browser_m... File content/browser/browser_main_loop.cc (right): https://codereview.chromium.org/15907012/diff/20001/content/browser/browser_m... content/browser/browser_main_loop.cc:467: SpeechRecognizerImplAndroid::Init(base::android::AttachCurrentThread()); this is not the right place :) more below.. https://codereview.chromium.org/15907012/diff/20001/content/browser/speech/sp... File content/browser/speech/speech_recognizer.h (right): https://codereview.chromium.org/15907012/diff/20001/content/browser/speech/sp... content/browser/speech/speech_recognizer.h:35: int session_id_; ops. I understand it's not your fault, but the style-guide forbids "naked protected members".. since the new class is using this, would you mind: - create const accessors() - use that in the new class - put a TODO to change other classes to use accessors() rather than member_ ? https://codereview.chromium.org/15907012/diff/20001/content/browser/speech/sp... File content/browser/speech/speech_recognizer_impl_android.cc (right): https://codereview.chromium.org/15907012/diff/20001/content/browser/speech/sp... content/browser/speech/speech_recognizer_impl_android.cc:29: RegisterNativesImpl(env); as above, the way to do this is to expose a Register() methods, and add the call in browser/android/browser_jni_registrar.cc.. https://codereview.chromium.org/15907012/diff/20001/content/browser/speech/sp... content/browser/speech/speech_recognizer_impl_android.cc:47: base::Bind(&content::SpeechRecognizerImplAndroid::StartRecognitionJNI, ahn, right.. so there are two ways of doing this: 1. use "OnUIThread" suffix instead of "JNI". 2. since the code is simple enough, another pattern is to just test and post back to itself.. ...Foo() { if (!BrowserThread::CurrentlyOn(BrowserThread::UI)) { BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, ... Foo); return; } DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); Java_..._Foo(...); } I don't have strong feelings either way, but there's a lot of boiler plate to just separate the "OnUIThread", so perhaps it's simpler to keep in one function. https://codereview.chromium.org/15907012/diff/20001/content/browser/speech/sp... content/browser/speech/speech_recognizer_impl_android.cc:55: AttachCurrentThread(), AttachCurrentThread is a bit expensive. common pattern is to extract in a local: JNIEnv* env = AttachCurrentThread(); then use where needed. https://codereview.chromium.org/15907012/diff/20001/content/browser/speech/sp... content/browser/speech/speech_recognizer_impl_android.cc:74: j_recognition_.obj()); nit: indent https://codereview.chromium.org/15907012/diff/20001/content/browser/speech/sp... File content/browser/speech/speech_recognizer_impl_android.h (right): https://codereview.chromium.org/15907012/diff/20001/content/browser/speech/sp... content/browser/speech/speech_recognizer_impl_android.h:36: void OnAudioStartJNI(JNIEnv* env, jobject obj); remove the JNI suffix, we never use that. https://codereview.chromium.org/15907012/diff/20001/content/browser/speech/sp... content/browser/speech/speech_recognizer_impl_android.h:57: void StopRecognitionJNI(); ditto for the JNI suffix https://codereview.chromium.org/15907012/diff/20001/content/public/android/ja... File content/public/android/java/src/org/chromium/content/browser/SpeechRecognition.java (right): https://codereview.chromium.org/15907012/diff/20001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/SpeechRecognition.java:33: private int mNativeSpeechRecognizerImplAndroid = 0; I suppose almost all of these could be final and initialized only in the ctor rather than here (except probably the mRecognizer since it needs to be destroy()). https://codereview.chromium.org/15907012/diff/20001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/SpeechRecognition.java:107: public static SpeechRecognition createSpeechRecognition(Context context, nit: we tend to keep @CalledByNative private, since they aren't called by java... (that is, we only use the visibility modifiers here for java itself..) https://codereview.chromium.org/15907012/diff/20001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/SpeechRecognition.java:113: public void StartRecognition(boolean continuous, boolean interim_results) { nit:s/Start/start/, below s/Abort/abort/ and s/Stop/stop/ (i.e., java naming convention) https://codereview.chromium.org/15907012/diff/20001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/SpeechRecognition.java:132: private native void nativeOnAudioStartJNI(int nativeSpeechRecognizerImplAndroid); nit: remove all JNI suffix.
Thank you for the great comments Marcus, they were really helpful! I addressed all of them (more details below), and fixed the naked protected member in crrev.com/16012015, cherry-picking it here (I will rebase once the fix has rolled). I also recycled one comment from Primiano who is worried about nested/reentrant call stacks in speech_recognizer_impl_android.cc. https://codereview.chromium.org/15907012/diff/20001/content/browser/browser_m... File content/browser/browser_main_loop.cc (right): https://codereview.chromium.org/15907012/diff/20001/content/browser/browser_m... content/browser/browser_main_loop.cc:467: SpeechRecognizerImplAndroid::Init(base::android::AttachCurrentThread()); On 2013/06/07 15:35:10, bulach wrote: > this is not the right place :) more below.. Renamed to RegisterSpeechRecognizer and moved to content/browser/android/browser_jni_registrar.cc. https://codereview.chromium.org/15907012/diff/20001/content/browser/speech/sp... File content/browser/speech/speech_recognizer.h (right): https://codereview.chromium.org/15907012/diff/20001/content/browser/speech/sp... content/browser/speech/speech_recognizer.h:35: int session_id_; On 2013/06/07 15:35:10, bulach wrote: > ops. I understand it's not your fault, but the style-guide forbids "naked > protected members".. > since the new class is using this, would you mind: > - create const accessors() > - use that in the new class > - put a TODO to change other classes to use accessors() rather than member_ ? Thanks for pointing out the issue, it was actually my fault since I made the SpeechRecognizer class derivable with naked protected members in the first place. Moved listener_ and session_id_ to private and implemented "listener const()" and "session_id const()" in crrev.com/16012015, to which I will rebase this CL once it has rolled. I went ahead and fixed all accesses of the members, no need for TODOs. https://codereview.chromium.org/15907012/diff/20001/content/browser/speech/sp... File content/browser/speech/speech_recognizer_impl_android.cc (right): https://codereview.chromium.org/15907012/diff/20001/content/browser/speech/sp... content/browser/speech/speech_recognizer_impl_android.cc:29: RegisterNativesImpl(env); On 2013/06/07 15:35:10, bulach wrote: > as above, the way to do this is to expose a Register() methods, and add the call > in browser/android/browser_jni_registrar.cc.. Fixed. Thanks for the explanation! https://codereview.chromium.org/15907012/diff/20001/content/browser/speech/sp... content/browser/speech/speech_recognizer_impl_android.cc:47: base::Bind(&content::SpeechRecognizerImplAndroid::StartRecognitionJNI, On 2013/06/07 15:35:10, bulach wrote: > ahn, right.. so there are two ways of doing this: > 1. use "OnUIThread" suffix instead of "JNI". > 2. since the code is simple enough, another pattern is to just test and post > back to itself.. > > ...Foo() { > if (!BrowserThread::CurrentlyOn(BrowserThread::UI)) { > BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, ... Foo); > return; > } > DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); > Java_..._Foo(...); > } > > I don't have strong feelings either way, but there's a lot of boiler plate to > just separate the "OnUIThread", so perhaps it's simpler to keep in one function. I refactored most of them into single functions, except StartRecognition{,OnUIThread} and OnRecognitionResults{,OnIOThread} because they both need to perform operations on one thread and pass the result to the other (respectively SpeechRecognitionManager::GetSessionConfig() on IO thread and JavaFloatArrayToFloatVector on UI thread). https://codereview.chromium.org/15907012/diff/20001/content/browser/speech/sp... content/browser/speech/speech_recognizer_impl_android.cc:55: AttachCurrentThread(), On 2013/06/07 15:35:10, bulach wrote: > AttachCurrentThread is a bit expensive. common pattern is to extract in a local: > JNIEnv* env = AttachCurrentThread(); > > then use where needed. Done. https://codereview.chromium.org/15907012/diff/20001/content/browser/speech/sp... content/browser/speech/speech_recognizer_impl_android.cc:74: j_recognition_.obj()); On 2013/06/07 15:35:10, bulach wrote: > nit: indent Done. https://codereview.chromium.org/15907012/diff/20001/content/browser/speech/sp... File content/browser/speech/speech_recognizer_impl_android.h (right): https://codereview.chromium.org/15907012/diff/20001/content/browser/speech/sp... content/browser/speech/speech_recognizer_impl_android.h:36: void OnAudioStartJNI(JNIEnv* env, jobject obj); On 2013/06/07 15:35:10, bulach wrote: > remove the JNI suffix, we never use that. Removed and refactored to single functions. https://codereview.chromium.org/15907012/diff/20001/content/browser/speech/sp... content/browser/speech/speech_recognizer_impl_android.h:57: void StopRecognitionJNI(); On 2013/06/07 15:35:10, bulach wrote: > ditto for the JNI suffix Done. https://codereview.chromium.org/15907012/diff/20001/content/public/android/ja... File content/public/android/java/src/org/chromium/content/browser/SpeechRecognition.java (right): https://codereview.chromium.org/15907012/diff/20001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/SpeechRecognition.java:33: private int mNativeSpeechRecognizerImplAndroid = 0; On 2013/06/07 15:35:10, bulach wrote: > I suppose almost all of these could be final and initialized only in the ctor > rather than here (except probably the mRecognizer since it needs to be > destroy()). I moved all initializations into the ctor and made every member final except mRecognizer (as you pointed out, it needs to be destroyed) and mContinuous (it changes when you call startRecognition). https://codereview.chromium.org/15907012/diff/20001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/SpeechRecognition.java:107: public static SpeechRecognition createSpeechRecognition(Context context, On 2013/06/07 15:35:10, bulach wrote: > nit: we tend to keep @CalledByNative private, since they aren't called by > java... (that is, we only use the visibility modifiers here for java itself..) Ok thanks, changed to private. I wasn't sure how this would affect the JNI interface. https://codereview.chromium.org/15907012/diff/20001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/SpeechRecognition.java:113: public void StartRecognition(boolean continuous, boolean interim_results) { On 2013/06/07 15:35:10, bulach wrote: > nit:s/Start/start/, below s/Abort/abort/ and s/Stop/stop/ (i.e., java naming > convention) Fixed. https://codereview.chromium.org/15907012/diff/20001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/SpeechRecognition.java:132: private native void nativeOnAudioStartJNI(int nativeSpeechRecognizerImplAndroid); On 2013/06/07 15:35:10, bulach wrote: > nit: remove all JNI suffix. Done. https://codereview.chromium.org/15907012/diff/29001/content/browser/speech/sp... File content/browser/speech/speech_recognizer_impl_android.cc (right): https://codereview.chromium.org/15907012/diff/29001/content/browser/speech/sp... content/browser/speech/speech_recognizer_impl_android.cc:39: listener()->OnRecognitionStart(session_id()); Primiano Tucci 2013/06/03 16:24:11 I'd like to have feedback by some other reviewer on this comment. I think is a bit safer posting the OnRecognitionStart (and similar below), even if the source and target threads are the same (IO), rather than calling the callback method directly. What I fear about calling callbacks synchronously is that you can end up in unpleasant nested/reentrant call stacks. For instance, imagine that for some reason the listener (i.e. the S.R. manager in our case) has a code path that calls AbortRecognition in the OnRecognitionStart event handler (e.g., in a error/not authorized code path). In such a case you would end up having a call stack like this: SRManager.Something() SRImplAndroid.StartRecognition() SRManager.OnRecognitionStart(...) SRImplAndroid.AbortRecognition() SRManager.OnRecognitionEnd() I don't know if this particular example is possible with the current S.R. manager implementation, but IMHO if some code path will change in future, a nested call stack like the one depicted above could cause serious headaches. Conversely, if you post call-backs on the same (IO) thread, you basically will break any possiblity of nesting/reentrancy (at the price of a little higher latency). If you take a look to the current speech_recognizer_impl.cc implementation, all the listener_->CallbackMethods() are always called inside the FSM, which by design is always decoupled in a separate message loop task (read: all evolution functions of the FSM are "posted"). What do you think?
lgtm, thanks! just some nits and small details below, feel free to go ahead once peter and primiano are happy :) https://codereview.chromium.org/15907012/diff/29001/content/browser/speech/sp... File content/browser/speech/speech_recognition_manager_impl.cc (right): https://codereview.chromium.org/15907012/diff/29001/content/browser/speech/sp... content/browser/speech/speech_recognition_manager_impl.cc:145: session->recognizer = new SpeechRecognizerImplAndroid( uber nit: fits one line? https://codereview.chromium.org/15907012/diff/29001/content/browser/speech/sp... File content/browser/speech/speech_recognizer_impl.cc (right): https://codereview.chromium.org/15907012/diff/29001/content/browser/speech/sp... content/browser/speech/speech_recognizer_impl.cc:84: DCHECK(this->listener() != NULL); nit: move the DCHECK to the base class, and there just check for the param rather than the accessor. https://codereview.chromium.org/15907012/diff/29001/content/browser/speech/sp... File content/browser/speech/speech_recognizer_impl_android.cc (right): https://codereview.chromium.org/15907012/diff/29001/content/browser/speech/sp... content/browser/speech/speech_recognizer_impl_android.cc:39: listener()->OnRecognitionStart(session_id()); On 2013/06/10 16:51:19, janx wrote: > Primiano Tucci 2013/06/03 16:24:11 > I'd like to have feedback by some other reviewer on this comment. > I think is a bit safer posting the OnRecognitionStart (and similar below), even > if the source and target threads are the same (IO), rather than calling the > callback method directly. > What I fear about calling callbacks synchronously is that you can end up in > unpleasant nested/reentrant call stacks. > For instance, imagine that for some reason the listener (i.e. the S.R. manager > in our case) has a code path that calls AbortRecognition in the > OnRecognitionStart event handler (e.g., in a error/not authorized code path). > In such a case you would end up having a call stack like this: > SRManager.Something() > SRImplAndroid.StartRecognition() > SRManager.OnRecognitionStart(...) > SRImplAndroid.AbortRecognition() > SRManager.OnRecognitionEnd() > > I don't know if this particular example is possible with the current S.R. > manager implementation, but IMHO if some code path will change in future, a > nested call stack like the one depicted above could cause serious headaches. > Conversely, if you post call-backs on the same (IO) thread, you basically will > break any possiblity of nesting/reentrancy (at the price of a little higher > latency). > > > If you take a look to the current speech_recognizer_impl.cc implementation, all > the listener_->CallbackMethods() are always called inside the FSM, which by > design is always decoupled in a separate message loop task (read: all evolution > functions of the FSM are "posted"). > > What do you think? if this is asking for my opinion, I'd say go with primiano's :) if the other implementation is always posting to the callback rather than synchronously calling it, it'd be better to keep the same behavior on this platform. https://codereview.chromium.org/15907012/diff/29001/content/browser/speech/sp... content/browser/speech/speech_recognizer_impl_android.cc:48: bool interim_results) { nit: either left-align, or keep the previous ( by itself with the first param idented by 4 https://codereview.chromium.org/15907012/diff/29001/content/browser/speech/sp... content/browser/speech/speech_recognizer_impl_android.cc:62: listener()->OnRecognitionError(session_id(), as above, perhaps better to post this task after the java side has been cancelled below. https://codereview.chromium.org/15907012/diff/29001/content/browser/speech/sp... content/browser/speech/speech_recognizer_impl_android.cc:86: return state_ != STATE_IDLE; just confirming: these are always called in the IO thread, right? perhaps a comment in the .h saying this member is only ever accessed on the IO thread, and also a DCHECK here and the following method. https://codereview.chromium.org/15907012/diff/29001/content/browser/speech/sp... content/browser/speech/speech_recognizer_impl_android.cc:96: base::Bind(&SpeechRecognizerImplAndroid::OnAudioStart, this, env, oh, an important detail: neither env or obj are thread safe, and can't be used in any other thread but the one that originated from the JNI. thankfully it's not being used :) however, it'd be clearer by sending NULL for both... same for all the IO => UI posts below.. https://codereview.chromium.org/15907012/diff/29001/content/browser/speech/sp... content/browser/speech/speech_recognizer_impl_android.cc:155: SpeechRecognitionResults results; nit: is this copy-constructable? if so, you may want to do this transformation on 145 above rather than Post both vectors.. https://codereview.chromium.org/15907012/diff/29001/content/browser/speech/sp... content/browser/speech/speech_recognizer_impl_android.cc:157: SpeechRecognitionResult& result = results.back(); CHECK_EQ(options.size(), scores.size()); https://codereview.chromium.org/15907012/diff/29001/content/browser/speech/sp... content/browser/speech/speech_recognizer_impl_android.cc:158: for (unsigned int i = 0; i < options.size(); i++) { nit: s/unsigned int/size_t/ and also ++i https://codereview.chromium.org/15907012/diff/29001/content/browser/speech/sp... File content/browser/speech/speech_recognizer_impl_android.h (right): https://codereview.chromium.org/15907012/diff/29001/content/browser/speech/sp... content/browser/speech/speech_recognizer_impl_android.h:54: std::vector<float> scores, vectors should be passed by const& rather than by value (to avoid an extra copy) https://codereview.chromium.org/15907012/diff/29001/content/public/android/ja... File content/public/android/java/src/org/chromium/content/browser/SpeechRecognition.java (right): https://codereview.chromium.org/15907012/diff/29001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/SpeechRecognition.java:30: private SpeechRecognizer mRecognizer; nit: put the final members first
https://codereview.chromium.org/15907012/diff/29001/content/browser/speech/sp... File content/browser/speech/speech_recognizer.h (right): https://codereview.chromium.org/15907012/diff/29001/content/browser/speech/sp... content/browser/speech/speech_recognizer.h:35: friend class base::RefCountedThreadSafe<SpeechRecognizer>; Nit: Was the move of friend classs.... under private intentional? I think it should be in the same section of the dtor (which is the method that requires the "friendship"). In this case, since this class is derived, the dtor needs to be protected, thus the "friend " should be protected as well (not private). https://codereview.chromium.org/15907012/diff/29001/content/public/android/ja... File content/public/android/java/src/org/chromium/content/browser/SpeechRecognition.java (right): https://codereview.chromium.org/15907012/diff/29001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/SpeechRecognition.java:54: public void onError(int error) { In which order (relatively to onEndOfSpeech) is the onError event dispatched? Are we keeping the same order of events in case of errors of Chrome desktop (i.e. speech_recognizer_impl.cc), that is: onSoundEnd -> onAudioEnd -> onError -> onRecognitionEnd ? (see https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/sp...) https://codereview.chromium.org/15907012/diff/29001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/SpeechRecognition.java:135: nativeOnRecognitionEnd(mNativeSpeechRecognizerImplAndroid); At this point you should reset the native pointer (mNativeSpeechRecognizerImplAndroid). As per definition of OnRecognitionEnd event (see https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro...) you must "promise" that no more events will come, so the caller can safely destroy the object. The easiest way for respecting this contract is resetting the native pointer (i.e. mNativeSpeechRecognizerImplAndroid = 0) https://codereview.chromium.org/15907012/diff/29001/content/public/browser/sp... File content/public/browser/speech_recognition_event_listener.h (right): https://codereview.chromium.org/15907012/diff/29001/content/public/browser/sp... content/public/browser/speech_recognition_event_listener.h:56: // TODO(janx) Is this necessary? It looks like only x-webkit-speech bubble femto-nit: colon after TODO(janx)
https://codereview.chromium.org/15907012/diff/29001/content/browser/speech/sp... File content/browser/speech/speech_recognition_manager_impl.cc (right): https://codereview.chromium.org/15907012/diff/29001/content/browser/speech/sp... content/browser/speech/speech_recognition_manager_impl.cc:145: session->recognizer = new SpeechRecognizerImplAndroid( On 2013/06/11 07:24:48, bulach wrote: > uber nit: fits one line? Fits. https://codereview.chromium.org/15907012/diff/29001/content/browser/speech/sp... File content/browser/speech/speech_recognizer.h (right): https://codereview.chromium.org/15907012/diff/29001/content/browser/speech/sp... content/browser/speech/speech_recognizer.h:35: friend class base::RefCountedThreadSafe<SpeechRecognizer>; On 2013/06/11 13:34:51, Primiano Tucci wrote: > Nit: Was the move of friend classs.... under private intentional? > I think it should be in the same section of the dtor (which is the method that > requires the "friendship"). > In this case, since this class is derived, the dtor needs to be protected, thus > the "friend " should be protected as well (not private). Moved to protected. https://codereview.chromium.org/15907012/diff/29001/content/browser/speech/sp... File content/browser/speech/speech_recognizer_impl.cc (right): https://codereview.chromium.org/15907012/diff/29001/content/browser/speech/sp... content/browser/speech/speech_recognizer_impl.cc:84: DCHECK(this->listener() != NULL); On 2013/06/11 07:24:48, bulach wrote: > nit: move the DCHECK to the base class, and there just check for the param > rather than the accessor. Moved DCHECK to base class. https://codereview.chromium.org/15907012/diff/29001/content/browser/speech/sp... File content/browser/speech/speech_recognizer_impl_android.cc (right): https://codereview.chromium.org/15907012/diff/29001/content/browser/speech/sp... content/browser/speech/speech_recognizer_impl_android.cc:39: listener()->OnRecognitionStart(session_id()); Converted all "listener()->OnSomeEvent(session_id())" to "BrowserThread::PostTask(BrowserThread::IO, FROM_HERE, base::Bind( &SpeechRecognitionEventListener::OnSomeEvent, base::Unretained(listener()), session_id()))" to avoid nested/reentrant call stacks. https://codereview.chromium.org/15907012/diff/29001/content/browser/speech/sp... content/browser/speech/speech_recognizer_impl_android.cc:48: bool interim_results) { On 2013/06/11 07:24:48, bulach wrote: > nit: either left-align, or keep the previous ( by itself with the first param > idented by 4 Went for \n after ( and first param indented by 4. https://codereview.chromium.org/15907012/diff/29001/content/browser/speech/sp... content/browser/speech/speech_recognizer_impl_android.cc:62: listener()->OnRecognitionError(session_id(), On 2013/06/11 07:24:48, bulach wrote: > as above, perhaps better to post this task after the java side has been > cancelled below. Now handled in the Java side, call removed. https://codereview.chromium.org/15907012/diff/29001/content/browser/speech/sp... content/browser/speech/speech_recognizer_impl_android.cc:86: return state_ != STATE_IDLE; On 2013/06/11 07:24:48, bulach wrote: > just confirming: these are always called in the IO thread, right? perhaps a > comment in the .h saying this member is only ever accessed on the IO thread, and > also a DCHECK here and the following method. Added "DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO))". https://codereview.chromium.org/15907012/diff/29001/content/browser/speech/sp... content/browser/speech/speech_recognizer_impl_android.cc:96: base::Bind(&SpeechRecognizerImplAndroid::OnAudioStart, this, env, On 2013/06/11 07:24:48, bulach wrote: > oh, an important detail: neither env or obj are thread safe, and can't be used > in any other thread but the one that originated from the JNI. thankfully it's > not being used :) however, it'd be clearer by sending NULL for both... > same for all the IO => UI posts below.. Fixed by sending "static_cast<JNIEnv*>(NULL), static_cast<jobject>(NULL)" instead of "env, obj". https://codereview.chromium.org/15907012/diff/29001/content/browser/speech/sp... content/browser/speech/speech_recognizer_impl_android.cc:155: SpeechRecognitionResults results; On 2013/06/11 07:24:48, bulach wrote: > nit: is this copy-constructable? if so, you may want to do this transformation > on 145 above rather than Post both vectors.. SpeechRecognitionResults is just a std::vector, so it is copy-constructable. I moved the transformation to the UI thread. https://codereview.chromium.org/15907012/diff/29001/content/browser/speech/sp... content/browser/speech/speech_recognizer_impl_android.cc:157: SpeechRecognitionResult& result = results.back(); On 2013/06/11 07:24:48, bulach wrote: > CHECK_EQ(options.size(), scores.size()); Done. https://codereview.chromium.org/15907012/diff/29001/content/browser/speech/sp... content/browser/speech/speech_recognizer_impl_android.cc:158: for (unsigned int i = 0; i < options.size(); i++) { On 2013/06/11 07:24:48, bulach wrote: > nit: s/unsigned int/size_t/ and also ++i Done. https://codereview.chromium.org/15907012/diff/29001/content/browser/speech/sp... File content/browser/speech/speech_recognizer_impl_android.h (right): https://codereview.chromium.org/15907012/diff/29001/content/browser/speech/sp... content/browser/speech/speech_recognizer_impl_android.h:54: std::vector<float> scores, On 2013/06/11 07:24:48, bulach wrote: > vectors should be passed by const& rather than by value (to avoid an extra copy) Moved creation of SpeechRecognitionResults object to UI thread, now passed as a single "std::vector<X> const &x". https://codereview.chromium.org/15907012/diff/29001/content/public/android/ja... File content/public/android/java/src/org/chromium/content/browser/SpeechRecognition.java (right): https://codereview.chromium.org/15907012/diff/29001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/SpeechRecognition.java:30: private SpeechRecognizer mRecognizer; On 2013/06/11 07:24:48, bulach wrote: > nit: put the final members first Done. https://codereview.chromium.org/15907012/diff/29001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/SpeechRecognition.java:54: public void onError(int error) { On 2013/06/11 13:34:51, Primiano Tucci wrote: > In which order (relatively to onEndOfSpeech) is the onError event dispatched? > Are we keeping the same order of events in case of errors of Chrome desktop > (i.e. speech_recognizer_impl.cc), that is: > onSoundEnd -> onAudioEnd -> onError -> onRecognitionEnd ? > (see > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/sp...) The order is the same as on desktop except in one edge case: when an error happens before SoundEnd/AudioEnd, e.g. if the user calls abort() before those events. In this edge case, OnSoundEnd/OnAudioEnd are not called at all. This doesn't break anything on the client side, because the most important onRecognitionEnd is called properly, but is asymmetric to what happens on desktop. In order to guarantee that events are called in the same way on mobile as on desktop, I made terminate() state-aware so that it can throw ending events and tear down properly. https://codereview.chromium.org/15907012/diff/29001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/SpeechRecognition.java:135: nativeOnRecognitionEnd(mNativeSpeechRecognizerImplAndroid); On 2013/06/11 13:34:51, Primiano Tucci wrote: > At this point you should reset the native pointer > (mNativeSpeechRecognizerImplAndroid). > As per definition of OnRecognitionEnd event (see > https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro...) > you must "promise" that no more events will come, so the caller can safely > destroy the object. > The easiest way for respecting this contract is resetting the native pointer > (i.e. mNativeSpeechRecognizerImplAndroid = 0) Done. https://codereview.chromium.org/15907012/diff/29001/content/public/browser/sp... File content/public/browser/speech_recognition_event_listener.h (right): https://codereview.chromium.org/15907012/diff/29001/content/public/browser/sp... content/public/browser/speech_recognition_event_listener.h:56: // TODO(janx) Is this necessary? It looks like only x-webkit-speech bubble On 2013/06/11 13:34:51, Primiano Tucci wrote: > femto-nit: colon after TODO(janx) Done.
LGTM. Great work. P.S. I've some final enjoyable nits. :) https://codereview.chromium.org/15907012/diff/47001/content/browser/speech/sp... File content/browser/speech/speech_recognizer_impl_android.cc (right): https://codereview.chromium.org/15907012/diff/47001/content/browser/speech/sp... content/browser/speech/speech_recognizer_impl_android.cc:104: BrowserThread::PostTask(BrowserThread::IO, FROM_HERE, base::Bind( No need for this PostTask here, since you already posted few lines above (to swtich from UI to IO thread). Just do a listener()->OnAudioStart(session_id()) https://codereview.chromium.org/15907012/diff/47001/content/browser/speech/sp... content/browser/speech/speech_recognizer_impl_android.cc:116: DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO)); Ditto, no need to post. https://codereview.chromium.org/15907012/diff/47001/content/browser/speech/sp... content/browser/speech/speech_recognizer_impl_android.cc:130: BrowserThread::PostTask(BrowserThread::IO, FROM_HERE, base::Bind( Ditto. https://codereview.chromium.org/15907012/diff/47001/content/browser/speech/sp... content/browser/speech/speech_recognizer_impl_android.cc:145: BrowserThread::PostTask(BrowserThread::IO, FROM_HERE, base::Bind( Ditto. https://codereview.chromium.org/15907012/diff/47001/content/browser/speech/sp... content/browser/speech/speech_recognizer_impl_android.cc:153: std::vector<string16> options; s/options/transcripts/ https://codereview.chromium.org/15907012/diff/47001/content/browser/speech/sp... content/browser/speech/speech_recognizer_impl_android.cc:175: BrowserThread::PostTask(BrowserThread::IO, FROM_HERE, base::Bind( Ditto. https://codereview.chromium.org/15907012/diff/47001/content/browser/speech/sp... content/browser/speech/speech_recognizer_impl_android.cc:182: if (BrowserThread::CurrentlyOn(BrowserThread::UI)) { As above, no need to this UI->IO switching https://codereview.chromium.org/15907012/diff/47001/content/browser/speech/sp... content/browser/speech/speech_recognizer_impl_android.cc:192: &SpeechRecognitionEventListener::OnRecognitionError, Ditto. https://codereview.chromium.org/15907012/diff/47001/content/browser/speech/sp... content/browser/speech/speech_recognizer_impl_android.cc:208: BrowserThread::PostTask(BrowserThread::IO, FROM_HERE, base::Bind( Ditto. https://codereview.chromium.org/15907012/diff/47001/content/public/android/ja... File content/public/android/java/src/org/chromium/content/browser/SpeechRecognition.java (right): https://codereview.chromium.org/15907012/diff/47001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/SpeechRecognition.java:31: private final RecognitionListener mListener; Ultra-nit: can you move it to line 39, just below mRecognizer, so it is more clear that this is the listener for the mRecognizer? https://codereview.chromium.org/15907012/diff/47001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/SpeechRecognition.java:32: private final int STATE_IDLE = 0; Nit: +static (i.e. private static final int). Here and the two lines below. Also move the static constants on the top (line 29), before the other fields. https://codereview.chromium.org/15907012/diff/47001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/SpeechRecognition.java:170: mRecognizer.cancel(); I fear that if someone (i.e. the S.R. manager) calls abortRecognition twice in a row here you will get NullPointerException. In the original design the manager was engineer to be "politically correct", i.e. never call abort twice, and only make calls which make sense. However, taking a look back to the git history, it seems that this principle has been later violated, and I fear that there are corner cases in which the S.R. manager can call abortRecognition twice (or stop + abort) on the poor recognizers. In essence, would be nice if here (and similarly in start and stopRecognition) you just add a check on mRecognizer != NULL. https://codereview.chromium.org/15907012/diff/47001/content/public/browser/sp... File content/public/browser/speech_recognition_event_listener.h (right): https://codereview.chromium.org/15907012/diff/47001/content/public/browser/sp... content/public/browser/speech_recognition_event_listener.h:57: // uses it (see crbug.com/247351). I hope this will be removed soon. That argument name "noise_volume" is irritating (should be noise_level). :)
I'd like to see one more iteration, please. Thank you, looking good! https://codereview.chromium.org/15907012/diff/47001/content/browser/speech/sp... File content/browser/speech/speech_recognizer_impl_android.cc (right): https://codereview.chromium.org/15907012/diff/47001/content/browser/speech/sp... content/browser/speech/speech_recognizer_impl_android.cc:43: SpeechRecognitionSessionConfig config_ = config is a local variable, so it shouldn't have the underscore suffix (which is for member variables). https://codereview.chromium.org/15907012/diff/47001/content/content_browser.gypi File content/content_browser.gypi (right): https://codereview.chromium.org/15907012/diff/47001/content/content_browser.g... content/content_browser.gypi:1303: 'browser/speech/speech_recognizer_impl_android.cc', These should be excluded through a global _android exclusion rule for non-Android platform, right? Why are they being included again? https://codereview.chromium.org/15907012/diff/47001/content/public/android/ja... File content/public/android/java/src/org/chromium/content/browser/SpeechRecognition.java (right): https://codereview.chromium.org/15907012/diff/47001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/SpeechRecognition.java:32: private final int STATE_IDLE = 0; Agreed. Please also add a comment that they are equal to the C++ constants. https://codereview.chromium.org/15907012/diff/47001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/SpeechRecognition.java:40: class Listener implements RecognitionListener { You never replied to my comment about method in this class having a little bit more documentation (patch set 2). Can you please do that? https://codereview.chromium.org/15907012/diff/47001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/SpeechRecognition.java:65: case SpeechRecognizer.ERROR_AUDIO: nit: indent, should be four spaces per level. https://codereview.chromium.org/15907012/diff/47001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/SpeechRecognition.java:120: bundle.getStringArrayList(SpeechRecognizer.RESULTS_RECOGNITION); nit: should have eight space indent (continuation). https://codereview.chromium.org/15907012/diff/47001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/SpeechRecognition.java:124: provisional); nit: should have eight space indent (continuation).
https://codereview.chromium.org/15907012/diff/47001/content/public/android/ja... File content/public/android/java/src/org/chromium/content/browser/SpeechRecognition.java (right): https://codereview.chromium.org/15907012/diff/47001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/SpeechRecognition.java:32: private final int STATE_IDLE = 0; On 2013/06/13 11:46:23, Peter Beverloo wrote: > Agreed. Please also add a comment that they are equal to the C++ constants. Hmm I think they're not really the same and btw there is no functional dependency with them. These constants respond to a local (i.e. Java-only) need, which is closing the OnAudio... event properly in any situation. Unfortunately the Android API don't do that for us, so he has to take care of this additional level of detail here.
Thank you for the returns, all comments addressed. https://codereview.chromium.org/15907012/diff/47001/content/browser/speech/sp... File content/browser/speech/speech_recognizer_impl_android.cc (right): https://codereview.chromium.org/15907012/diff/47001/content/browser/speech/sp... content/browser/speech/speech_recognizer_impl_android.cc:43: SpeechRecognitionSessionConfig config_ = On 2013/06/13 11:46:23, Peter Beverloo wrote: > config is a local variable, so it shouldn't have the underscore suffix (which is > for member variables). Done. https://codereview.chromium.org/15907012/diff/47001/content/browser/speech/sp... content/browser/speech/speech_recognizer_impl_android.cc:104: BrowserThread::PostTask(BrowserThread::IO, FROM_HERE, base::Bind( On 2013/06/13 10:05:05, Primiano Tucci wrote: > No need for this PostTask here, since you already posted few lines above (to > swtich from UI to IO thread). > Just do a listener()->OnAudioStart(session_id()) Indeed, it seems I went berserk replacing all listener calls by posted tasks. I'll revert where it didn't make sense. https://codereview.chromium.org/15907012/diff/47001/content/browser/speech/sp... content/browser/speech/speech_recognizer_impl_android.cc:116: DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO)); On 2013/06/13 10:05:05, Primiano Tucci wrote: > Ditto, no need to post. Done. https://codereview.chromium.org/15907012/diff/47001/content/browser/speech/sp... content/browser/speech/speech_recognizer_impl_android.cc:130: BrowserThread::PostTask(BrowserThread::IO, FROM_HERE, base::Bind( On 2013/06/13 10:05:05, Primiano Tucci wrote: > Ditto. Done. https://codereview.chromium.org/15907012/diff/47001/content/browser/speech/sp... content/browser/speech/speech_recognizer_impl_android.cc:145: BrowserThread::PostTask(BrowserThread::IO, FROM_HERE, base::Bind( On 2013/06/13 10:05:05, Primiano Tucci wrote: > Ditto. Done. https://codereview.chromium.org/15907012/diff/47001/content/browser/speech/sp... content/browser/speech/speech_recognizer_impl_android.cc:153: std::vector<string16> options; On 2013/06/13 10:05:05, Primiano Tucci wrote: > s/options/transcripts/ While I agree that those are transcripts from an audio medium to a text medium, all these strings are generated from a single speech utterance, thus I prefer to emphasize the notion of choice: These are several options of recognized text associated with confidence scores, and there is a choice to make between them. https://codereview.chromium.org/15907012/diff/47001/content/browser/speech/sp... content/browser/speech/speech_recognizer_impl_android.cc:175: BrowserThread::PostTask(BrowserThread::IO, FROM_HERE, base::Bind( On 2013/06/13 10:05:05, Primiano Tucci wrote: > Ditto. Done. https://codereview.chromium.org/15907012/diff/47001/content/browser/speech/sp... content/browser/speech/speech_recognizer_impl_android.cc:192: &SpeechRecognitionEventListener::OnRecognitionError, On 2013/06/13 10:05:05, Primiano Tucci wrote: > Ditto. Done. https://codereview.chromium.org/15907012/diff/47001/content/browser/speech/sp... content/browser/speech/speech_recognizer_impl_android.cc:208: BrowserThread::PostTask(BrowserThread::IO, FROM_HERE, base::Bind( On 2013/06/13 10:05:05, Primiano Tucci wrote: > Ditto. Done. https://codereview.chromium.org/15907012/diff/47001/content/content_browser.gypi File content/content_browser.gypi (right): https://codereview.chromium.org/15907012/diff/47001/content/content_browser.g... content/content_browser.gypi:1303: 'browser/speech/speech_recognizer_impl_android.cc', On 2013/06/13 11:46:23, Peter Beverloo wrote: > These should be excluded through a global _android exclusion rule for > non-Android platform, right? Why are they being included again? I can't see any global _android exclusion rule; if it were in this file it would need to be here. These two files were included by default and caused linker errors on platforms without JNI (all non-android). We could experiment with an "['exclude', 'android']" rule to cover {pre,suf}fixed files and folders, but that might be a bit extreme. https://codereview.chromium.org/15907012/diff/47001/content/public/android/ja... File content/public/android/java/src/org/chromium/content/browser/SpeechRecognition.java (right): https://codereview.chromium.org/15907012/diff/47001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/SpeechRecognition.java:31: private final RecognitionListener mListener; On 2013/06/13 10:05:05, Primiano Tucci wrote: > Ultra-nit: can you move it to line 39, just below mRecognizer, so it is more > clear that this is the listener for the mRecognizer? This ultra-nit conflicts with: "bulach 2013/06/11 07:24:48 nit: put the final members first". https://codereview.chromium.org/15907012/diff/47001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/SpeechRecognition.java:32: private final int STATE_IDLE = 0; On 2013/06/13 10:05:05, Primiano Tucci wrote: > Nit: +static (i.e. private static final int). Here and the two lines below. > Also move the static constants on the top (line 29), before the other fields. Done. https://codereview.chromium.org/15907012/diff/47001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/SpeechRecognition.java:40: class Listener implements RecognitionListener { On 2013/06/13 11:46:23, Peter Beverloo wrote: > You never replied to my comment about method in this class having a little bit > more documentation (patch set 2). Can you please do that? I did add a little more documentation and recycled the comment a few times to see if other reviewers shared the concern. I now went ahead and added more comments and explanations. https://codereview.chromium.org/15907012/diff/47001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/SpeechRecognition.java:65: case SpeechRecognizer.ERROR_AUDIO: On 2013/06/13 11:46:23, Peter Beverloo wrote: > nit: indent, should be four spaces per level. Done. https://codereview.chromium.org/15907012/diff/47001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/SpeechRecognition.java:120: bundle.getStringArrayList(SpeechRecognizer.RESULTS_RECOGNITION); On 2013/06/13 11:46:23, Peter Beverloo wrote: > nit: should have eight space indent (continuation). Done. https://codereview.chromium.org/15907012/diff/47001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/SpeechRecognition.java:124: provisional); On 2013/06/13 11:46:23, Peter Beverloo wrote: > nit: should have eight space indent (continuation). Done. https://codereview.chromium.org/15907012/diff/47001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/SpeechRecognition.java:170: mRecognizer.cancel(); On 2013/06/13 10:05:05, Primiano Tucci wrote: > I fear that if someone (i.e. the S.R. manager) calls abortRecognition twice in a > row here you will get NullPointerException. > In the original design the manager was engineer to be "politically correct", > i.e. never call abort twice, and only make calls which make sense. > However, taking a look back to the git history, it seems that this principle has > been later violated, and I fear that there are corner cases in which the S.R. > manager can call abortRecognition twice (or stop + abort) on the poor > recognizers. > In essence, would be nice if here (and similarly in start and stopRecognition) > you just add a check on mRecognizer != NULL. Done. https://codereview.chromium.org/15907012/diff/47001/content/public/browser/sp... File content/public/browser/speech_recognition_event_listener.h (right): https://codereview.chromium.org/15907012/diff/47001/content/public/browser/sp... content/public/browser/speech_recognition_event_listener.h:57: // uses it (see crbug.com/247351). On 2013/06/13 10:05:05, Primiano Tucci wrote: > I hope this will be removed soon. That argument name "noise_volume" is > irritating (should be noise_level). :) I could fix the variable name, it is only used 25 times in the whole code base. :)
lgtm. Please go through the owners dance :-).
Thanks Peter! Avi, could you have a look please? :)
Only whiny nits from me. LGTM https://codereview.chromium.org/15907012/diff/55001/content/browser/speech/sp... File content/browser/speech/speech_recognizer.h (right): https://codereview.chromium.org/15907012/diff/55001/content/browser/speech/sp... content/browser/speech/speech_recognizer.h:23: DCHECK(listener_ != NULL); DCHECK(listener_); should work just fine. https://codereview.chromium.org/15907012/diff/55001/content/browser/speech/sp... File content/browser/speech/speech_recognizer_impl_android.cc (right): https://codereview.chromium.org/15907012/diff/55001/content/browser/speech/sp... content/browser/speech/speech_recognizer_impl_android.cc:12: #include "base/utf_string_conversions.h" base/strings/utf_string_conversions.h https://codereview.chromium.org/15907012/diff/55001/content/browser/speech/sp... File content/browser/speech/speech_recognizer_impl_android.h (right): https://codereview.chromium.org/15907012/diff/55001/content/browser/speech/sp... content/browser/speech/speech_recognizer_impl_android.h:26: // SpeechRecognizer methods nit: periods at end of comments. https://codereview.chromium.org/15907012/diff/55001/content/browser/speech/sp... content/browser/speech/speech_recognizer_impl_android.h:33: // Called from Java methods via JNI nit: periods at end of comments.
Thank you Avi, all fixed! Pushing the patch down the rabbit hole.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/janx@chromium.org/15907012/70001
Message was sent while issue was closed.
Change committed as 206398 |
