Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
693433002:
Hotword Audio Verification App: update training UI on hotword trigger. (Closed)
|
Created:
6 years, 1 month ago by kcarattini Modified:
6 years, 1 month ago CC:
chromium-reviews, arv+watch_chromium.org, chrome-apps-syd-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@tp3-api Project:
chromium Visibility:
Public. |
DescriptionHotword Audio Verification App: update training UI on hotword trigger.
BUG=390086
Committed: https://crrev.com/960939ae20f289d38da08b3f943c16e725deb339
Cr-Commit-Position: refs/heads/master@{#302554}
Patch Set 1 #
Total comments: 20
Patch Set 2 : Review Comments #Patch Set 3 : Typos #
Total comments: 6
Patch Set 4 : Review comments #Patch Set 5 : Rebase #
Messages
Total messages: 11 (2 generated)
kcarattini@chromium.org changed reviewers: + dbeam@chromium.org, rlp@chromium.org
https://codereview.chromium.org/693433002/diff/1/chrome/browser/resources/hot... File chrome/browser/resources/hotword_audio_verification/flow.js (right): https://codereview.chromium.org/693433002/diff/1/chrome/browser/resources/hot... chrome/browser/resources/hotword_audio_verification/flow.js:68: Flow.prototype.advanceStep = function() { what happens if this is called while training? does that matter? is it expected? I've never seen this code in action... https://codereview.chromium.org/693433002/diff/1/chrome/browser/resources/hot... chrome/browser/resources/hotword_audio_verification/flow.js:85: Flow.prototype.startTraining = function() { should we check that !this.training_? https://codereview.chromium.org/693433002/diff/1/chrome/browser/resources/hot... chrome/browser/resources/hotword_audio_verification/flow.js:94: if (chrome.hotwordPrivate.onHotwordTriggered) curlies https://codereview.chromium.org/693433002/diff/1/chrome/browser/resources/hot... chrome/browser/resources/hotword_audio_verification/flow.js:104: Flow.prototype.stopTraining = function() { should this actually be a check to ensure this.training_? https://codereview.chromium.org/693433002/diff/1/chrome/browser/resources/hot... chrome/browser/resources/hotword_audio_verification/flow.js:132: * Completes the training process. @private, finalizeSpeakerModel_ https://codereview.chromium.org/693433002/diff/1/chrome/browser/resources/hot... chrome/browser/resources/hotword_audio_verification/flow.js:135: if (this.training_) { nit: if (!this.training_) return; https://codereview.chromium.org/693433002/diff/1/chrome/browser/resources/hot... chrome/browser/resources/hotword_audio_verification/flow.js:146: * Handles a hotword trigger event and updates the training UI. handleHotwordTrigger_, @private https://codereview.chromium.org/693433002/diff/1/chrome/browser/resources/hot... chrome/browser/resources/hotword_audio_verification/flow.js:163: } // Only the last step makes it here. (or something like that either here or above the `return;` on L162) https://codereview.chromium.org/693433002/diff/1/chrome/browser/resources/hot... chrome/browser/resources/hotword_audio_verification/flow.js:178: * Gets and starts the appropriate flow for the launch mode. @param {WhateverTypeStateIs} state Doc comment about |state|. https://codereview.chromium.org/693433002/diff/1/chrome/browser/resources/hot... chrome/browser/resources/hotword_audio_verification/flow.js:189: state.launchMode == LaunchMode.SPEECH_TRAINING) curlies
https://codereview.chromium.org/693433002/diff/1/chrome/browser/resources/hot... File chrome/browser/resources/hotword_audio_verification/flow.js (right): https://codereview.chromium.org/693433002/diff/1/chrome/browser/resources/hot... chrome/browser/resources/hotword_audio_verification/flow.js:68: Flow.prototype.advanceStep = function() { On 2014/10/30 02:57:55, Dan Beam wrote: > what happens if this is called while training? does that matter? is it > expected? I've never seen this code in action... It shouldn't matter if this is called while training in theory. But the current code does all the training in a single page (step), so it doesn't happen in practice (once you start training, there is no way to advance to the next step without completing training). Theoretically training could happen over multiple pages, so I see no reason to restrict it. https://codereview.chromium.org/693433002/diff/1/chrome/browser/resources/hot... chrome/browser/resources/hotword_audio_verification/flow.js:85: Flow.prototype.startTraining = function() { On 2014/10/30 02:57:54, Dan Beam wrote: > should we check that !this.training_? Done. https://codereview.chromium.org/693433002/diff/1/chrome/browser/resources/hot... chrome/browser/resources/hotword_audio_verification/flow.js:94: if (chrome.hotwordPrivate.onHotwordTriggered) On 2014/10/30 02:57:55, Dan Beam wrote: > curlies Done. https://codereview.chromium.org/693433002/diff/1/chrome/browser/resources/hot... chrome/browser/resources/hotword_audio_verification/flow.js:104: Flow.prototype.stopTraining = function() { On 2014/10/30 02:57:54, Dan Beam wrote: > should this actually be a check to ensure this.training_? No, since this is called from all events that close the opt-in flow, including those that aren't involved in training. Also, it's possible for a user action to try to stop the training (by closing the app window) just after it has already been automatically stopped. We don't want an error in that case. https://codereview.chromium.org/693433002/diff/1/chrome/browser/resources/hot... chrome/browser/resources/hotword_audio_verification/flow.js:132: * Completes the training process. On 2014/10/30 02:57:54, Dan Beam wrote: > @private, finalizeSpeakerModel_ Done. https://codereview.chromium.org/693433002/diff/1/chrome/browser/resources/hot... chrome/browser/resources/hotword_audio_verification/flow.js:135: if (this.training_) { On 2014/10/30 02:57:54, Dan Beam wrote: > nit: > > if (!this.training_) > return; Done. https://codereview.chromium.org/693433002/diff/1/chrome/browser/resources/hot... chrome/browser/resources/hotword_audio_verification/flow.js:146: * Handles a hotword trigger event and updates the training UI. On 2014/10/30 02:57:54, Dan Beam wrote: > handleHotwordTrigger_, @private Done. https://codereview.chromium.org/693433002/diff/1/chrome/browser/resources/hot... chrome/browser/resources/hotword_audio_verification/flow.js:163: } On 2014/10/30 02:57:54, Dan Beam wrote: > // Only the last step makes it here. > > (or something like that either here or above the `return;` on L162) Done. https://codereview.chromium.org/693433002/diff/1/chrome/browser/resources/hot... chrome/browser/resources/hotword_audio_verification/flow.js:178: * Gets and starts the appropriate flow for the launch mode. On 2014/10/30 02:57:54, Dan Beam wrote: > @param {WhateverTypeStateIs} state Doc comment about |state|. Done. https://codereview.chromium.org/693433002/diff/1/chrome/browser/resources/hot... chrome/browser/resources/hotword_audio_verification/flow.js:189: state.launchMode == LaunchMode.SPEECH_TRAINING) On 2014/10/30 02:57:54, Dan Beam wrote: > curlies Done.
lgtm with nits https://codereview.chromium.org/693433002/diff/40001/chrome/browser/resources... File chrome/browser/resources/hotword_audio_verification/flow.js (right): https://codereview.chromium.org/693433002/diff/40001/chrome/browser/resources... chrome/browser/resources/hotword_audio_verification/flow.js:119: chrome.hotwordPrivate.stopTraining(); indent https://codereview.chromium.org/693433002/diff/40001/chrome/browser/resources... chrome/browser/resources/hotword_audio_verification/flow.js:129: chrome.hotwordPrivate.setAudioLoggingEnabled(true, function() {}); indent
lgtm https://codereview.chromium.org/693433002/diff/40001/chrome/browser/resources... File chrome/browser/resources/hotword_audio_verification/flow.js (right): https://codereview.chromium.org/693433002/diff/40001/chrome/browser/resources... chrome/browser/resources/hotword_audio_verification/flow.js:96: this.trainingPagePrefix_ = 'speech-training'; maybe this code should be: switch (this.launchMode_) { case LaunchMode.HOTWORD_ONLY: case LaunchMode.RETRAIN: this.trainingPagePrefix_ = 'hotword-only'; break; case LaunchMode.HOTWORD_AND_AUDIO_HISTORY: this.trainingPagePrefix_ = 'speech-training'; break; } or if (this.launchMode_ == LaunchMode.HOTWORD_ONLY || this.launchMode_ == LaunchMode.RETRAIN) { this.trainingPagePrefix_ = 'hotword-only'; } else if (this.launchMode_ == LaunchMode.HOTWORD_AND_AUDIO_HISTORY) { this.trainingPagePrefix_ = 'speech-training'; }
https://codereview.chromium.org/693433002/diff/40001/chrome/browser/resources... File chrome/browser/resources/hotword_audio_verification/flow.js (right): https://codereview.chromium.org/693433002/diff/40001/chrome/browser/resources... chrome/browser/resources/hotword_audio_verification/flow.js:96: this.trainingPagePrefix_ = 'speech-training'; On 2014/10/30 22:25:35, Dan Beam wrote: > maybe this code should be: > > switch (this.launchMode_) { > case LaunchMode.HOTWORD_ONLY: > case LaunchMode.RETRAIN: > this.trainingPagePrefix_ = 'hotword-only'; > break; > case LaunchMode.HOTWORD_AND_AUDIO_HISTORY: > this.trainingPagePrefix_ = 'speech-training'; > break; > } > > or > > if (this.launchMode_ == LaunchMode.HOTWORD_ONLY || > this.launchMode_ == LaunchMode.RETRAIN) { > this.trainingPagePrefix_ = 'hotword-only'; > } else if (this.launchMode_ == LaunchMode.HOTWORD_AND_AUDIO_HISTORY) { > this.trainingPagePrefix_ = 'speech-training'; > } Done. https://codereview.chromium.org/693433002/diff/40001/chrome/browser/resources... chrome/browser/resources/hotword_audio_verification/flow.js:119: chrome.hotwordPrivate.stopTraining(); On 2014/10/30 17:13:13, rpetterson wrote: > indent Done. https://codereview.chromium.org/693433002/diff/40001/chrome/browser/resources... chrome/browser/resources/hotword_audio_verification/flow.js:129: chrome.hotwordPrivate.setAudioLoggingEnabled(true, function() {}); On 2014/10/30 17:13:13, rpetterson wrote: > indent Done.
The CQ bit was checked by kcarattini@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/693433002/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/960939ae20f289d38da08b3f943c16e725deb339 Cr-Commit-Position: refs/heads/master@{#302554} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
