Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1257283007:
Voice input (Closed)
|
Created:
5 years, 4 months ago by shichengfeng Modified:
5 years, 4 months ago CC:
chromium-reviews, chromoting-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAndroid Chromoting: Enable voice input.
Embed Speech Recognizer so that the Cardboard desktop activity could support voice input.
Committed: https://crrev.com/fbd9604ed58e3369eb33aa4b9a3525c1116689d0
Cr-Commit-Position: refs/heads/master@{#342217}
Patch Set 1 : Enable voice input. #
Total comments: 18
Patch Set 2 : Fix Service Connection leak, add a flag to indicate whether the speech recognizer is listening or n… #
Total comments: 17
Patch Set 3 : Add a flag to indicate whether is listening to the speech recognizer or not. #
Total comments: 8
Patch Set 4 : Improve documentation. #
Messages
Total messages: 16 (4 generated)
Patchset #1 (id:1) has been deleted
shichengfeng@google.com changed reviewers: + lambroslambrou@chromium.org, sergeyu@chromium.org
Enable voice input for Cardboard desktop activity.
https://codereview.chromium.org/1257283007/diff/20001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/CardboardDesktopActivity.java (right): https://codereview.chromium.org/1257283007/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/CardboardDesktopActivity.java:58: } else if (mRenderer.isLookingRightToDesktop()) { This is OK for now, but add a TODO for a more polished UI :) https://codereview.chromium.org/1257283007/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/CardboardDesktopActivity.java:123: String text = results.getStringArrayList(SpeechRecognizer.RESULTS_RECOGNITION).get(0); I don't know if the result array is guaranteed to be non-empty? Maybe check it anyway? https://codereview.chromium.org/1257283007/diff/20001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/CardboardDesktopRenderer.java (right): https://codereview.chromium.org/1257283007/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/CardboardDesktopRenderer.java:379: // This can be called on any thread. Undo this change? https://codereview.chromium.org/1257283007/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/CardboardDesktopRenderer.java:387: // This can be called on any thread. I think Javadoc is better, since the method is public: "Returns true if user is looking at the space to the left of the desktop. Can be called on any thread." https://codereview.chromium.org/1257283007/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/CardboardDesktopRenderer.java:395: public boolean isLookingRightToDesktop() { Same here.
https://codereview.chromium.org/1257283007/diff/20001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/CardboardDesktopRenderer.java (right): https://codereview.chromium.org/1257283007/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/CardboardDesktopRenderer.java:388: public boolean isLookingLeftToDesktop() { s/To/Of/
https://codereview.chromium.org/1257283007/diff/20001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/CardboardDesktopActivity.java (right): https://codereview.chromium.org/1257283007/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/CardboardDesktopActivity.java:37: mSpeechRecognizer = SpeechRecognizer.createSpeechRecognizer(this); It might be good idea to create this lazily the first time the user tries to use speech input. https://codereview.chromium.org/1257283007/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/CardboardDesktopActivity.java:99: mSpeechRecognizer.startListening(intent); I think one problem when I tried using the feature is that there is no indication that speech input is active. It would be great to add some visual indicator for that. https://codereview.chromium.org/1257283007/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/CardboardDesktopActivity.java:99: mSpeechRecognizer.startListening(intent); Do we need to verify that the recognizer is not already listening before calling startListening()?
Fix Service Connection leak, add a flag to indicate whether the speech recognizer is listening or not and improve documentation. https://codereview.chromium.org/1257283007/diff/20001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/CardboardDesktopActivity.java (right): https://codereview.chromium.org/1257283007/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/CardboardDesktopActivity.java:37: mSpeechRecognizer = SpeechRecognizer.createSpeechRecognizer(this); On 2015/08/05 17:53:47, Sergey Ulanov wrote: > It might be good idea to create this lazily the first time the user tries to use > speech input. Done. https://codereview.chromium.org/1257283007/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/CardboardDesktopActivity.java:58: } else if (mRenderer.isLookingRightToDesktop()) { On 2015/08/05 00:25:52, Lambros wrote: > This is OK for now, but add a TODO for a more polished UI :) Done. https://codereview.chromium.org/1257283007/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/CardboardDesktopActivity.java:99: mSpeechRecognizer.startListening(intent); On 2015/08/05 17:53:47, Sergey Ulanov wrote: > Do we need to verify that the recognizer is not already listening before calling > startListening()? Yes, we have to. I added a flag to prevent calling multiple startListening at the same time. https://codereview.chromium.org/1257283007/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/CardboardDesktopActivity.java:99: mSpeechRecognizer.startListening(intent); On 2015/08/05 17:53:47, Sergey Ulanov wrote: > I think one problem when I tried using the feature is that there is no > indication that speech input is active. It would be great to add some visual > indicator for that. Sure, I will put all UI polish in another CL. Put a TODO here. https://codereview.chromium.org/1257283007/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/CardboardDesktopActivity.java:123: String text = results.getStringArrayList(SpeechRecognizer.RESULTS_RECOGNITION).get(0); On 2015/08/05 00:25:52, Lambros wrote: > I don't know if the result array is guaranteed to be non-empty? Maybe check it > anyway? Done. https://codereview.chromium.org/1257283007/diff/20001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/CardboardDesktopRenderer.java (right): https://codereview.chromium.org/1257283007/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/CardboardDesktopRenderer.java:379: // This can be called on any thread. On 2015/08/05 00:25:52, Lambros wrote: > Undo this change? Done. https://codereview.chromium.org/1257283007/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/CardboardDesktopRenderer.java:387: // This can be called on any thread. On 2015/08/05 00:25:52, Lambros wrote: > I think Javadoc is better, since the method is public: > "Returns true if user is looking at the space to the left of the desktop. Can be > called on any thread." Done. https://codereview.chromium.org/1257283007/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/CardboardDesktopRenderer.java:388: public boolean isLookingLeftToDesktop() { On 2015/08/05 00:31:14, Lambros wrote: > s/To/Of/ Done. https://codereview.chromium.org/1257283007/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/CardboardDesktopRenderer.java:395: public boolean isLookingRightToDesktop() { On 2015/08/05 00:25:52, Lambros wrote: > Same here. Done.
https://codereview.chromium.org/1257283007/diff/40001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/CardboardDesktopActivity.java (right): https://codereview.chromium.org/1257283007/diff/40001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/CardboardDesktopActivity.java:60: // TODO(shichengfeng): Give a more polished UI(including menu icons nit: space after "UI" https://codereview.chromium.org/1257283007/diff/40001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/CardboardDesktopActivity.java:64: } else if (mRenderer.isLookingRightOfDesktop() && !mIsListening) { Maybe move the !listening check inside sendVoiceInput() ? https://codereview.chromium.org/1257283007/diff/40001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/CardboardDesktopActivity.java:65: mIsListening = true; Move this inside sendVoiceInput() ? https://codereview.chromium.org/1257283007/diff/40001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/CardboardDesktopActivity.java:116: private void sendVoiceInput() { This doesn't really "send" anything, it just starts listening. Maybe rename to "listenForVoiceInput" ? https://codereview.chromium.org/1257283007/diff/40001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/CardboardDesktopActivity.java:118: mSpeechRecognizer = SpeechRecognizer.createSpeechRecognizer(this); Check to see if the service is available before creating the recognizer. See http://developer.android.com/reference/android/speech/SpeechRecognizer.html#i... https://codereview.chromium.org/1257283007/diff/40001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/CardboardDesktopActivity.java:142: public void onEndOfSpeech() { Set mIsListening = false here? The recognizer stops listening automatically when it detects the speech has ended - see http://developer.android.com/reference/android/speech/SpeechRecognizer.html#s.... In a followup CL, you could give user feedback here. https://codereview.chromium.org/1257283007/diff/40001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/CardboardDesktopActivity.java:155: mIsListening = false; I wonder if this belongs in onEndOfSpeech() ? https://codereview.chromium.org/1257283007/diff/40001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/CardboardDesktopRenderer.java (right): https://codereview.chromium.org/1257283007/diff/40001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/CardboardDesktopRenderer.java:380: * Return true if user is looking at the desktop false otherwise. "false otherwise" is not necessary in Chromium/Google style - see https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Com... https://codereview.chromium.org/1257283007/diff/40001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/CardboardDesktopRenderer.java:392: * false otherwise. This method can be called on any thread. same here https://codereview.chromium.org/1257283007/diff/40001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/CardboardDesktopRenderer.java:402: * false otherwise. This method can be called on any thread. same here
Add a flag to indicate whether is listening to the speech recognizer or not. https://codereview.chromium.org/1257283007/diff/40001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/CardboardDesktopActivity.java (right): https://codereview.chromium.org/1257283007/diff/40001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/CardboardDesktopActivity.java:60: // TODO(shichengfeng): Give a more polished UI(including menu icons On 2015/08/05 23:59:37, Lambros wrote: > nit: space after "UI" Done. https://codereview.chromium.org/1257283007/diff/40001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/CardboardDesktopActivity.java:64: } else if (mRenderer.isLookingRightOfDesktop() && !mIsListening) { On 2015/08/05 23:59:37, Lambros wrote: > Maybe move the !listening check inside sendVoiceInput() ? Done. https://codereview.chromium.org/1257283007/diff/40001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/CardboardDesktopActivity.java:65: mIsListening = true; On 2015/08/05 23:59:37, Lambros wrote: > Move this inside sendVoiceInput() ? Done. https://codereview.chromium.org/1257283007/diff/40001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/CardboardDesktopActivity.java:116: private void sendVoiceInput() { On 2015/08/05 23:59:37, Lambros wrote: > This doesn't really "send" anything, it just starts listening. Maybe rename to > "listenForVoiceInput" ? Done. https://codereview.chromium.org/1257283007/diff/40001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/CardboardDesktopActivity.java:118: mSpeechRecognizer = SpeechRecognizer.createSpeechRecognizer(this); On 2015/08/05 23:59:37, Lambros wrote: > Check to see if the service is available before creating the recognizer. See > http://developer.android.com/reference/android/speech/SpeechRecognizer.html#i... Done. https://codereview.chromium.org/1257283007/diff/40001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/CardboardDesktopActivity.java:142: public void onEndOfSpeech() { On 2015/08/05 23:59:37, Lambros wrote: > Set mIsListening = false here? The recognizer stops listening automatically when > it detects the speech has ended - see > http://developer.android.com/reference/android/speech/SpeechRecognizer.html#s.... > In a followup CL, you could give user feedback here. Done. https://codereview.chromium.org/1257283007/diff/40001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/CardboardDesktopActivity.java:155: mIsListening = false; On 2015/08/05 23:59:37, Lambros wrote: > I wonder if this belongs in onEndOfSpeech() ? Done.
lgtm when comments are addressed https://codereview.chromium.org/1257283007/diff/60001/remoting/android/java/A... File remoting/android/java/AndroidManifest.xml.jinja2 (right): https://codereview.chromium.org/1257283007/diff/60001/remoting/android/java/A... remoting/android/java/AndroidManifest.xml.jinja2:13: <uses-permission android:name="android.permission.RECORD_AUDIO" /> FYI: I think the extra permission is fine. By the time we actually enable and ship this feature, we should be using Android's new runtime permissions model, which makes the user-experience a lot better. https://codereview.chromium.org/1257283007/diff/60001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/CardboardDesktopActivity.java (right): https://codereview.chromium.org/1257283007/diff/60001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/CardboardDesktopActivity.java:83: mSpeechRecognizer.stopListening(); Will this trigger a callback that sets mIsListening to false? https://codereview.chromium.org/1257283007/diff/60001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/CardboardDesktopActivity.java:102: mSpeechRecognizer.stopListening(); Same question here. https://codereview.chromium.org/1257283007/diff/60001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/CardboardDesktopActivity.java:133: RecognizerIntent.LANGUAGE_MODEL_FREE_FORM); Can you add a comment to say why you added this extra item?
Improve documentation. https://codereview.chromium.org/1257283007/diff/60001/remoting/android/java/A... File remoting/android/java/AndroidManifest.xml.jinja2 (right): https://codereview.chromium.org/1257283007/diff/60001/remoting/android/java/A... remoting/android/java/AndroidManifest.xml.jinja2:13: <uses-permission android:name="android.permission.RECORD_AUDIO" /> On 2015/08/06 20:44:21, Lambros wrote: > FYI: I think the extra permission is fine. By the time we actually enable and > ship this feature, we should be using Android's new runtime permissions model, > which makes the user-experience a lot better. Acknowledged. https://codereview.chromium.org/1257283007/diff/60001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/CardboardDesktopActivity.java (right): https://codereview.chromium.org/1257283007/diff/60001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/CardboardDesktopActivity.java:83: mSpeechRecognizer.stopListening(); On 2015/08/06 20:44:21, Lambros wrote: > Will this trigger a callback that sets mIsListening to false? Yes it will. https://codereview.chromium.org/1257283007/diff/60001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/CardboardDesktopActivity.java:102: mSpeechRecognizer.stopListening(); On 2015/08/06 20:44:21, Lambros wrote: > Same question here. Yes it will. https://codereview.chromium.org/1257283007/diff/60001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/CardboardDesktopActivity.java:133: RecognizerIntent.LANGUAGE_MODEL_FREE_FORM); On 2015/08/06 20:44:21, Lambros wrote: > Can you add a comment to say why you added this extra item? Of course, the reason here is that LANGUAGE_MODEL_FREE_FORM is used to improve dictation accuracy for the voice keyboard.
The CQ bit was checked by shichengfeng@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from lambroslambrou@chromium.org Link to the patchset: https://codereview.chromium.org/1257283007/#ps80001 (title: "Improve documentation.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1257283007/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1257283007/80001
Message was sent while issue was closed.
Committed patchset #4 (id:80001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/fbd9604ed58e3369eb33aa4b9a3525c1116689d0 Cr-Commit-Position: refs/heads/master@{#342217} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
