Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
493203004:
Add a StateManager for managing hotwording state based on various factors (Closed)
|
Created:
6 years, 4 months ago by Anand Mistry (off Chromium) Modified:
6 years, 3 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, rlp+watch_chromium.org, arv+watch_chromium.org, extensions-reviews_chromium.org, chrome-apps-syd-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@hotword-nacl-manager Project:
chromium Visibility:
Public. |
DescriptionAdd a StateManager for managing hotwording state based on various factors
such as user settings.
At the same time, hook up the StateManager to some events.
BUG=397019
Committed: https://crrev.com/fff0b9adc25f6f297c875e293825ca41cbead657
Cr-Commit-Position: refs/heads/master@{#293876}
Patch Set 1 #
Total comments: 33
Patch Set 2 : Rebase. #Patch Set 3 : Address review comments. #
Total comments: 18
Patch Set 4 : Address more review comments. #Patch Set 5 : Minor comment change. #Patch Set 6 : Rebase. #
Messages
Total messages: 11 (1 generated)
https://codereview.chromium.org/493203004/diff/1/chrome/browser/resources/hot... File chrome/browser/resources/hotword/manager.js (right): https://codereview.chromium.org/493203004/diff/1/chrome/browser/resources/hot... chrome/browser/resources/hotword/manager.js:22: }.bind(this)); remove .bind(this) https://codereview.chromium.org/493203004/diff/1/chrome/browser/resources/hot... chrome/browser/resources/hotword/manager.js:27: }.bind(this)); remove .bind(this) https://codereview.chromium.org/493203004/diff/1/chrome/browser/resources/hot... chrome/browser/resources/hotword/manager.js:32: if (info.id == hotword.constants.SHARED_MODULE_ID) { nit: no curlies https://codereview.chromium.org/493203004/diff/1/chrome/browser/resources/hot... File chrome/browser/resources/hotword/state_manager.js (right): https://codereview.chromium.org/493203004/diff/1/chrome/browser/resources/hot... chrome/browser/resources/hotword/state_manager.js:10: 'use strict'; this code should probably be indented because it's inside a function. i can see the rationale of treating this like a C++ namespace (which we don't indent), but all the rest of webui code currently idents cr.define()s. https://codereview.chromium.org/493203004/diff/1/chrome/browser/resources/hot... chrome/browser/resources/hotword/state_manager.js:31: * @private {?hotword.NaClManager} add ? to |this.hotwordStatus_|'s type or remove from here https://codereview.chromium.org/493203004/diff/1/chrome/browser/resources/hot... chrome/browser/resources/hotword/state_manager.js:38: /** * @enum {number} * @private */ https://codereview.chromium.org/493203004/diff/1/chrome/browser/resources/hot... chrome/browser/resources/hotword/state_manager.js:70: if (this.hotwordStatus_.enabled) { indent off (needs 1 more space) https://codereview.chromium.org/493203004/diff/1/chrome/browser/resources/hot... chrome/browser/resources/hotword/state_manager.js:89: // if we error too often. is there some code you meant to put here? https://codereview.chromium.org/493203004/diff/1/chrome/browser/resources/hot... chrome/browser/resources/hotword/state_manager.js:92: if (this.pluginManager_ == null) { if (!this.pluginManager_) { https://codereview.chromium.org/493203004/diff/1/chrome/browser/resources/hot... chrome/browser/resources/hotword/state_manager.js:96: this.onReady_.bind(this)); indent off x 3 https://codereview.chromium.org/493203004/diff/1/chrome/browser/resources/hot... chrome/browser/resources/hotword/state_manager.js:106: // src.chromium.org/svn/trunk/src/content/common/media/media_stream_options.cc use bit.ly or just put local path. https://codereview.chromium.org/493203004/diff/1/chrome/browser/resources/hot... chrome/browser/resources/hotword/state_manager.js:108: ({audio: {optional: [{ googDucking: false}] }}); no \s after { or } https://codereview.chromium.org/493203004/diff/1/chrome/browser/resources/hot... chrome/browser/resources/hotword/state_manager.js:115: this.pluginManager_ = null; nit: this code seems to be repeated a lot: this.pluginManager_.shutdown(); this.pluginManager_ = null; how about: /** @private */ shutdownPluginManager_: function() { if (this.pluginManager_) this.pluginManager_.shutdown(); this.pluginManager_ = null; }, https://codereview.chromium.org/493203004/diff/1/chrome/browser/resources/hot... chrome/browser/resources/hotword/state_manager.js:173: assert(this.pluginManager_ != null); assert(this.pluginManager_)
LGTM for overall functionality with a nit and a general comment: In the past we've had a lot of issues with the hotword helper popping up even when it shouldn't. I see you're adding a lot of listeners. It seems that those should be fine -- most are one time events like startup or install -- but please check and double check so we're not dealing with this issue again. https://codereview.chromium.org/493203004/diff/1/chrome/browser/resources/hot... File chrome/browser/resources/hotword/manifest.json (right): https://codereview.chromium.org/493203004/diff/1/chrome/browser/resources/hot... chrome/browser/resources/hotword/manifest.json:6: "version": "0.0.1.0", This should be incremented for the new events to take effect in previous installs.
https://codereview.chromium.org/493203004/diff/1/chrome/browser/resources/hot... File chrome/browser/resources/hotword/manager.js (right): https://codereview.chromium.org/493203004/diff/1/chrome/browser/resources/hot... chrome/browser/resources/hotword/manager.js:22: }.bind(this)); On 2014/08/26 18:40:51, Dan Beam wrote: > remove .bind(this) Done. https://codereview.chromium.org/493203004/diff/1/chrome/browser/resources/hot... chrome/browser/resources/hotword/manager.js:27: }.bind(this)); On 2014/08/26 18:40:51, Dan Beam wrote: > remove .bind(this) Done. https://codereview.chromium.org/493203004/diff/1/chrome/browser/resources/hot... chrome/browser/resources/hotword/manager.js:32: if (info.id == hotword.constants.SHARED_MODULE_ID) { On 2014/08/26 18:40:51, Dan Beam wrote: > nit: no curlies Looking into this, the style guide gives no preference either way. The Chrome code base also doesn't consistently enforce this, and I've seen cases where both styles exist in a single file. Is this preference documented somewhere? If not, I'd like to continue using braces for single line blocks. There's already one existing file in this extension with that style... and it's the one I prefer. https://codereview.chromium.org/493203004/diff/1/chrome/browser/resources/hot... File chrome/browser/resources/hotword/manifest.json (right): https://codereview.chromium.org/493203004/diff/1/chrome/browser/resources/hot... chrome/browser/resources/hotword/manifest.json:6: "version": "0.0.1.0", On 2014/08/26 19:16:59, rpetterson wrote: > This should be incremented for the new events to take effect in previous > installs. Done. https://codereview.chromium.org/493203004/diff/1/chrome/browser/resources/hot... File chrome/browser/resources/hotword/state_manager.js (right): https://codereview.chromium.org/493203004/diff/1/chrome/browser/resources/hot... chrome/browser/resources/hotword/state_manager.js:10: 'use strict'; On 2014/08/26 18:40:51, Dan Beam wrote: > this code should probably be indented because it's inside a function. > > i can see the rationale of treating this like a C++ namespace (which we don't > indent), but all the rest of webui code currently idents cr.define()s. Done. https://codereview.chromium.org/493203004/diff/1/chrome/browser/resources/hot... chrome/browser/resources/hotword/state_manager.js:31: * @private {?hotword.NaClManager} On 2014/08/26 18:40:51, Dan Beam wrote: > add ? to |this.hotwordStatus_|'s type or remove from here Done. https://codereview.chromium.org/493203004/diff/1/chrome/browser/resources/hot... chrome/browser/resources/hotword/state_manager.js:38: On 2014/08/26 18:40:51, Dan Beam wrote: > /** > * @enum {number} > * @private > */ Done. https://codereview.chromium.org/493203004/diff/1/chrome/browser/resources/hot... chrome/browser/resources/hotword/state_manager.js:70: if (this.hotwordStatus_.enabled) { On 2014/08/26 18:40:52, Dan Beam wrote: > indent off (needs 1 more space) WTF? How did that end up there? Done. https://codereview.chromium.org/493203004/diff/1/chrome/browser/resources/hot... chrome/browser/resources/hotword/state_manager.js:89: // if we error too often. On 2014/08/26 18:40:52, Dan Beam wrote: > is there some code you meant to put here? Not yet. I'm not ready to fill in that kind of detail. Also, I think that logic is more complicated than I want to put in this CL. https://codereview.chromium.org/493203004/diff/1/chrome/browser/resources/hot... chrome/browser/resources/hotword/state_manager.js:92: if (this.pluginManager_ == null) { On 2014/08/26 18:40:52, Dan Beam wrote: > if (!this.pluginManager_) { Done. https://codereview.chromium.org/493203004/diff/1/chrome/browser/resources/hot... chrome/browser/resources/hotword/state_manager.js:96: this.onReady_.bind(this)); On 2014/08/26 18:40:52, Dan Beam wrote: > indent off x 3 Done. https://codereview.chromium.org/493203004/diff/1/chrome/browser/resources/hot... chrome/browser/resources/hotword/state_manager.js:106: // src.chromium.org/svn/trunk/src/content/common/media/media_stream_options.cc On 2014/08/26 18:40:52, Dan Beam wrote: > use bit.ly or just put local path. Done. https://codereview.chromium.org/493203004/diff/1/chrome/browser/resources/hot... chrome/browser/resources/hotword/state_manager.js:108: ({audio: {optional: [{ googDucking: false}] }}); On 2014/08/26 18:40:52, Dan Beam wrote: > no \s after { or } Done. https://codereview.chromium.org/493203004/diff/1/chrome/browser/resources/hot... chrome/browser/resources/hotword/state_manager.js:115: this.pluginManager_ = null; On 2014/08/26 18:40:51, Dan Beam wrote: > nit: this code seems to be repeated a lot: > > this.pluginManager_.shutdown(); > this.pluginManager_ = null; > > how about: > > /** @private */ > shutdownPluginManager_: function() { > if (this.pluginManager_) > this.pluginManager_.shutdown(); > this.pluginManager_ = null; > }, Done. https://codereview.chromium.org/493203004/diff/1/chrome/browser/resources/hot... chrome/browser/resources/hotword/state_manager.js:173: assert(this.pluginManager_ != null); On 2014/08/26 18:40:52, Dan Beam wrote: > assert(this.pluginManager_) Done.
https://codereview.chromium.org/493203004/diff/1/chrome/browser/resources/hot... File chrome/browser/resources/hotword/manager.js (right): https://codereview.chromium.org/493203004/diff/1/chrome/browser/resources/hot... chrome/browser/resources/hotword/manager.js:5: 'use strict'; we generally scope 'use strict' just in case files are combined together https://codereview.chromium.org/493203004/diff/1/chrome/browser/resources/hot... chrome/browser/resources/hotword/manager.js:32: if (info.id == hotword.constants.SHARED_MODULE_ID) { On 2014/08/27 07:10:55, Anand Mistry wrote: > On 2014/08/26 18:40:51, Dan Beam wrote: > > nit: no curlies > > Looking into this, the style guide gives no preference either way. The Chrome > code base also doesn't consistently enforce this, chrome is quite consistent on this rule. > and I've seen cases where both styles exist in a single file. where? point me at the code and i'll be happy to fix this for you :). > Is this preference documented somewhere? If not, > I'd like to continue using braces for single line blocks. There's already one > existing file in this extension with that style... and it's the one I prefer. """Do not use braces for single-line logic blocks.""" http://www.chromium.org/developers/web-development-style-guide additionally, http://www.chromium.org/developers/coding-style discourages use of extra { } for conditional 1-liners. one could also interpret "Do not needlessly surround the return expression with parentheses" [1] as another in opposition of superfluous characters. [1] http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Return... https://codereview.chromium.org/493203004/diff/40001/chrome/browser/resources... File chrome/browser/resources/hotword/manager.js (right): https://codereview.chromium.org/493203004/diff/40001/chrome/browser/resources... chrome/browser/resources/hotword/manager.js:4: (function() { https://codereview.chromium.org/493203004/diff/40001/chrome/browser/resources... chrome/browser/resources/hotword/manager.js:35: if (info.id == hotword.constants.SHARED_MODULE_ID) { no curlies https://codereview.chromium.org/493203004/diff/40001/chrome/browser/resources... chrome/browser/resources/hotword/manager.js:52: }()); https://codereview.chromium.org/493203004/diff/40001/chrome/browser/resources... File chrome/browser/resources/hotword/state_manager.js (right): https://codereview.chromium.org/493203004/diff/40001/chrome/browser/resources... chrome/browser/resources/hotword/state_manager.js:6: * @fileoverview Class to manage hotwording state. nit: if it fits, one-liner -- /** ... */ https://codereview.chromium.org/493203004/diff/40001/chrome/browser/resources... chrome/browser/resources/hotword/state_manager.js:6: * @fileoverview Class to manage hotwording state. this isn't a particularly useful comment and should probably be close to @constructor for StateManager if it's class-specific https://codereview.chromium.org/493203004/diff/40001/chrome/browser/resources... chrome/browser/resources/hotword/state_manager.js:8: remove \n https://codereview.chromium.org/493203004/diff/40001/chrome/browser/resources... chrome/browser/resources/hotword/state_manager.js:55: StateManager.prototype.updateStatus = function() { nit: just assign whole prototype StateManager.prototype = { ... updateState: function() { ... }, ... }; https://codereview.chromium.org/493203004/diff/40001/chrome/browser/resources... chrome/browser/resources/hotword/state_manager.js:159: // Shouldn't be running while starting. this comment is confusing because you say the code is starting inside a state != STARTING block https://codereview.chromium.org/493203004/diff/40001/chrome/browser/resources... chrome/browser/resources/hotword/state_manager.js:192: nit: remove \n
https://codereview.chromium.org/493203004/diff/1/chrome/browser/resources/hot... File chrome/browser/resources/hotword/manager.js (right): https://codereview.chromium.org/493203004/diff/1/chrome/browser/resources/hot... chrome/browser/resources/hotword/manager.js:32: if (info.id == hotword.constants.SHARED_MODULE_ID) { On 2014/08/27 20:08:36, Dan Beam wrote: > On 2014/08/27 07:10:55, Anand Mistry wrote: > > On 2014/08/26 18:40:51, Dan Beam wrote: > > > nit: no curlies > > > > Looking into this, the style guide gives no preference either way. The Chrome > > code base also doesn't consistently enforce this, > > chrome is quite consistent on this rule. > > > and I've seen cases where both styles exist in a single file. > > where? point me at the code and i'll be happy to fix this for you :). > > > Is this preference documented somewhere? If not, > > I'd like to continue using braces for single line blocks. There's already one > > existing file in this extension with that style... and it's the one I prefer. > > """Do not use braces for single-line logic blocks.""" > http://www.chromium.org/developers/web-development-style-guide > > additionally, http://www.chromium.org/developers/coding-style discourages use of > extra { } for conditional 1-liners. > > one could also interpret "Do not needlessly surround the return expression with > parentheses" [1] as another in opposition of superfluous characters. > > [1] > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Return... Thanks. Both those pages point to the google style guides as reference, which is what I was following. I guess I should have read those pages carefully instead of skimming them. https://codereview.chromium.org/493203004/diff/40001/chrome/browser/resources... File chrome/browser/resources/hotword/manager.js (right): https://codereview.chromium.org/493203004/diff/40001/chrome/browser/resources... chrome/browser/resources/hotword/manager.js:4: On 2014/08/27 20:08:36, Dan Beam wrote: > (function() { Done. But as a JS noob, why? Is it avoid leaking variables into the 'global' scope? https://codereview.chromium.org/493203004/diff/40001/chrome/browser/resources... chrome/browser/resources/hotword/manager.js:35: if (info.id == hotword.constants.SHARED_MODULE_ID) { On 2014/08/27 20:08:36, Dan Beam wrote: > no curlies Done. https://codereview.chromium.org/493203004/diff/40001/chrome/browser/resources... chrome/browser/resources/hotword/manager.js:52: On 2014/08/27 20:08:36, Dan Beam wrote: > }()); Done. https://codereview.chromium.org/493203004/diff/40001/chrome/browser/resources... File chrome/browser/resources/hotword/state_manager.js (right): https://codereview.chromium.org/493203004/diff/40001/chrome/browser/resources... chrome/browser/resources/hotword/state_manager.js:6: * @fileoverview Class to manage hotwording state. On 2014/08/27 20:08:37, Dan Beam wrote: > this isn't a particularly useful comment and should probably be close to > @constructor for StateManager if it's class-specific Done. https://codereview.chromium.org/493203004/diff/40001/chrome/browser/resources... chrome/browser/resources/hotword/state_manager.js:8: On 2014/08/27 20:08:37, Dan Beam wrote: > remove \n Done. https://codereview.chromium.org/493203004/diff/40001/chrome/browser/resources... chrome/browser/resources/hotword/state_manager.js:55: StateManager.prototype.updateStatus = function() { On 2014/08/27 20:08:37, Dan Beam wrote: > nit: just assign whole prototype > > StateManager.prototype = { > ... > updateState: function() { > ... > }, > ... > }; Done. https://codereview.chromium.org/493203004/diff/40001/chrome/browser/resources... chrome/browser/resources/hotword/state_manager.js:159: // Shouldn't be running while starting. On 2014/08/27 20:08:37, Dan Beam wrote: > this comment is confusing because you say the code is starting inside a state != > STARTING block Done. https://codereview.chromium.org/493203004/diff/40001/chrome/browser/resources... chrome/browser/resources/hotword/state_manager.js:192: On 2014/08/27 20:08:37, Dan Beam wrote: > nit: remove \n Done.
lgtm https://codereview.chromium.org/493203004/diff/40001/chrome/browser/resources... File chrome/browser/resources/hotword/manager.js (right): https://codereview.chromium.org/493203004/diff/40001/chrome/browser/resources... chrome/browser/resources/hotword/manager.js:4: On 2014/08/28 00:59:25, Anand Mistry wrote: > On 2014/08/27 20:08:36, Dan Beam wrote: > > (function() { > > Done. But as a JS noob, why? Is it avoid leaking variables into the 'global' > scope? http://jslinterrors.com/use-the-function-form-of-use-strict
The CQ bit was checked by amistry@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/amistry@chromium.org/493203004/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as e3508bf760974d05c6a73862127f1100036904e9
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/fff0b9adc25f6f297c875e293825ca41cbead657 Cr-Commit-Position: refs/heads/master@{#293876} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
