Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
464213002:
Add a 'NaCl manager' to manage the state of the NaCl hotword detector plugin. (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, Matt Giuca Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionAdd a 'NaCl manager' to manage the state of the NaCl hotword detector plugin.
Based on the hotword extension, but heavily modified.
BUG=397019
Committed: https://crrev.com/7b7830556b3f4bbff30bf5a6c8adea241b94464d
Cr-Commit-Position: refs/heads/master@{#291597}
Patch Set 1 #
Total comments: 36
Patch Set 2 : Address review comments. #Patch Set 3 : Rebase. #
Messages
Total messages: 15 (0 generated)
So... who should I send this to?
I'll leave it to rlp to review or decide who should be reviewing this NaCl module stuff. But you will also need someone in chrome/browser/resources/OWNERS to review it.
jhawkins@chromium.org: Please review changes in everywhere.
https://codereview.chromium.org/464213002/diff/1/chrome/browser/resources/hot... File chrome/browser/resources/hotword/constants.js (right): https://codereview.chromium.org/464213002/diff/1/chrome/browser/resources/hot... chrome/browser/resources/hotword/constants.js:10: * Hotword data shared module extension's id. s/id/ID/ https://codereview.chromium.org/464213002/diff/1/chrome/browser/resources/hot... chrome/browser/resources/hotword/constants.js:28: var Timeout = { The var name should have the unit of time in it. I suggest TimeoutMs. https://codereview.chromium.org/464213002/diff/1/chrome/browser/resources/hot... File chrome/browser/resources/hotword/nacl_manager.js (right): https://codereview.chromium.org/464213002/diff/1/chrome/browser/resources/hot... chrome/browser/resources/hotword/nacl_manager.js:31: * The window.timeout Id associated with a pending message. s/Id/ID/ https://codereview.chromium.org/464213002/diff/1/chrome/browser/resources/hot... chrome/browser/resources/hotword/nacl_manager.js:32: * @type {?number} @private {?number} Here and elsewhere. https://codereview.chromium.org/464213002/diff/1/chrome/browser/resources/hot... chrome/browser/resources/hotword/nacl_manager.js:58: * Url containing hotword-model data file. s/Url/URL/ https://codereview.chromium.org/464213002/diff/1/chrome/browser/resources/hot... chrome/browser/resources/hotword/nacl_manager.js:73: * asynchronous (and potentially queued), we don't know what state the plugin is Optional nit: Consider removing the ambiguous pronoun, we. https://codereview.chromium.org/464213002/diff/1/chrome/browser/resources/hot... chrome/browser/resources/hotword/nacl_manager.js:92: nit: Remove double blank line. https://codereview.chromium.org/464213002/diff/1/chrome/browser/resources/hot... chrome/browser/resources/hotword/nacl_manager.js:118: * @param {Function} func Prefer the more-specific type 'function(param, list): return' even for simple callback functions. https://codereview.chromium.org/464213002/diff/1/chrome/browser/resources/hot... chrome/browser/resources/hotword/nacl_manager.js:150: this.waitForMessage_(hotword.constants.Timeout.LONG, nit: The start of parameter rows must align on the same column, i.e., this.waitForMessage_( one, two, three); or this.waitForMessage_(one, two, three); Here and elsewhere. https://codereview.chromium.org/464213002/diff/1/chrome/browser/resources/hot... chrome/browser/resources/hotword/nacl_manager.js:200: NaClManager.prototype.initialize = function(naclArch, stream) { This method is quite long. I suggest breaking it up into well-defined methods. https://codereview.chromium.org/464213002/diff/1/chrome/browser/resources/hot... chrome/browser/resources/hotword/nacl_manager.js:263: * Shut down the NaCl plugin and free all resources. Shuts https://codereview.chromium.org/464213002/diff/1/chrome/browser/resources/hot... chrome/browser/resources/hotword/nacl_manager.js:302: console.log('Existing wait: ' + this.expectingMessage_); Remove console logging. Here and elsewhere. https://codereview.chromium.org/464213002/diff/1/chrome/browser/resources/hot... chrome/browser/resources/hotword/nacl_manager.js:335: NaClManager.prototype.handlePluginMessage_ = function(msg) { I suggest also breaking up this method into sub-handler methods for readability.
https://codereview.chromium.org/464213002/diff/1/chrome/browser/resources/hot... File chrome/browser/resources/hotword/constants.js (right): https://codereview.chromium.org/464213002/diff/1/chrome/browser/resources/hot... chrome/browser/resources/hotword/constants.js:21: var SHARED_MODULE_ROOT = '_modules/' + SHARED_MODULE_ID; Is module a specific type of extension that we're going to be switching to? Or is this terminology from the external component extension? https://codereview.chromium.org/464213002/diff/1/chrome/browser/resources/hot... File chrome/browser/resources/hotword/nacl_manager.js (right): https://codereview.chromium.org/464213002/diff/1/chrome/browser/resources/hot... chrome/browser/resources/hotword/nacl_manager.js:147: assert(this.recognizerState_ == ManagerState_.STOPPED); Why is this assert necessary? I don't see how it would be possible to get into this if statement if this is not the case . . . https://codereview.chromium.org/464213002/diff/1/chrome/browser/resources/hot... chrome/browser/resources/hotword/nacl_manager.js:229: // If the data file exists, assume a valid NaCl nmf also exists. how expensive is it to check? https://codereview.chromium.org/464213002/diff/1/chrome/browser/resources/hot... chrome/browser/resources/hotword/nacl_manager.js:287: this.deferredPluginMessages_.push(data); Is it not possible to have deferred messages if the manager state is OFF? https://codereview.chromium.org/464213002/diff/1/chrome/browser/resources/hot... chrome/browser/resources/hotword/nacl_manager.js:301: if (this.expectingMessage_) { Is it possible for this function to get called twice such that we lose the earlier message?
https://codereview.chromium.org/464213002/diff/1/chrome/browser/resources/hot... File chrome/browser/resources/hotword/constants.js (right): https://codereview.chromium.org/464213002/diff/1/chrome/browser/resources/hot... chrome/browser/resources/hotword/constants.js:10: * Hotword data shared module extension's id. On 2014/08/13 14:13:16, James Hawkins wrote: > s/id/ID/ Done. https://codereview.chromium.org/464213002/diff/1/chrome/browser/resources/hot... chrome/browser/resources/hotword/constants.js:21: var SHARED_MODULE_ROOT = '_modules/' + SHARED_MODULE_ID; On 2014/08/15 00:41:57, rpetterson wrote: > Is module a specific type of extension that we're going to be switching to? Or > is this terminology from the external component extension? A "shared module" is a type of extension that provides data to another extension, and doesn't function by itself. In our case, it will provide the NEXEs and language models. A more public example is the cast API: http://www.chromestory.com/2014/08/chromecast-becomes-first-chrome-shared-mod... An example of usage can be seen in https://src.chromium.org/viewvc/chrome/trunk/src/chrome/test/data/extensions/... https://codereview.chromium.org/464213002/diff/1/chrome/browser/resources/hot... chrome/browser/resources/hotword/constants.js:28: var Timeout = { On 2014/08/13 14:13:16, James Hawkins wrote: > The var name should have the unit of time in it. I suggest TimeoutMs. Done. https://codereview.chromium.org/464213002/diff/1/chrome/browser/resources/hot... File chrome/browser/resources/hotword/nacl_manager.js (right): https://codereview.chromium.org/464213002/diff/1/chrome/browser/resources/hot... chrome/browser/resources/hotword/nacl_manager.js:31: * The window.timeout Id associated with a pending message. On 2014/08/13 14:13:18, James Hawkins wrote: > s/Id/ID/ Done. https://codereview.chromium.org/464213002/diff/1/chrome/browser/resources/hot... chrome/browser/resources/hotword/nacl_manager.js:32: * @type {?number} On 2014/08/13 14:13:17, James Hawkins wrote: > @private {?number} > > Here and elsewhere. Done. But I wonder, what's the different between that and: @type {?number} @private The closure reference doesn't mention having a type after @private (https://developers.google.com/closure/compiler/docs/js-for-compiler). https://codereview.chromium.org/464213002/diff/1/chrome/browser/resources/hot... chrome/browser/resources/hotword/nacl_manager.js:58: * Url containing hotword-model data file. On 2014/08/13 14:13:18, James Hawkins wrote: > s/Url/URL/ Done. https://codereview.chromium.org/464213002/diff/1/chrome/browser/resources/hot... chrome/browser/resources/hotword/nacl_manager.js:73: * asynchronous (and potentially queued), we don't know what state the plugin is On 2014/08/13 14:13:17, James Hawkins wrote: > Optional nit: Consider removing the ambiguous pronoun, we. Done. https://codereview.chromium.org/464213002/diff/1/chrome/browser/resources/hot... chrome/browser/resources/hotword/nacl_manager.js:92: On 2014/08/13 14:13:17, James Hawkins wrote: > nit: Remove double blank line. Done. Removed everywhere, which seems to be consistent with the rest of chrome. https://codereview.chromium.org/464213002/diff/1/chrome/browser/resources/hot... chrome/browser/resources/hotword/nacl_manager.js:118: * @param {Function} func On 2014/08/13 14:13:17, James Hawkins wrote: > Prefer the more-specific type 'function(param, list): return' even for simple > callback functions. Done. https://codereview.chromium.org/464213002/diff/1/chrome/browser/resources/hot... chrome/browser/resources/hotword/nacl_manager.js:147: assert(this.recognizerState_ == ManagerState_.STOPPED); On 2014/08/15 00:41:57, rpetterson wrote: > Why is this assert necessary? I don't see how it would be possible to get into > this if statement if this is not the case . . . Oops. Artifact of older iteration. https://codereview.chromium.org/464213002/diff/1/chrome/browser/resources/hot... chrome/browser/resources/hotword/nacl_manager.js:150: this.waitForMessage_(hotword.constants.Timeout.LONG, On 2014/08/13 14:13:17, James Hawkins wrote: > nit: The start of parameter rows must align on the same column, i.e., > > this.waitForMessage_( > one, two, > three); > > or > > this.waitForMessage_(one, two, > three); > > Here and elsewhere. Done. https://codereview.chromium.org/464213002/diff/1/chrome/browser/resources/hot... chrome/browser/resources/hotword/nacl_manager.js:200: NaClManager.prototype.initialize = function(naclArch, stream) { On 2014/08/13 14:13:17, James Hawkins wrote: > This method is quite long. I suggest breaking it up into well-defined methods. Done. https://codereview.chromium.org/464213002/diff/1/chrome/browser/resources/hot... chrome/browser/resources/hotword/nacl_manager.js:229: // If the data file exists, assume a valid NaCl nmf also exists. On 2014/08/15 00:41:57, rpetterson wrote: > how expensive is it to check? Cheap. I've added the check. https://codereview.chromium.org/464213002/diff/1/chrome/browser/resources/hot... chrome/browser/resources/hotword/nacl_manager.js:263: * Shut down the NaCl plugin and free all resources. On 2014/08/13 14:13:17, James Hawkins wrote: > Shuts Done. https://codereview.chromium.org/464213002/diff/1/chrome/browser/resources/hot... chrome/browser/resources/hotword/nacl_manager.js:287: this.deferredPluginMessages_.push(data); On 2014/08/15 00:41:57, rpetterson wrote: > Is it not possible to have deferred messages if the manager state is OFF? Hm. We shouldn't be trying to send messages at all if the state is OFF. Probably don't need the idea of deferred messages at all. And come to think of it, OFF is a bad state name. Should be UNINITIALIZED. https://codereview.chromium.org/464213002/diff/1/chrome/browser/resources/hot... chrome/browser/resources/hotword/nacl_manager.js:301: if (this.expectingMessage_) { On 2014/08/15 00:41:57, rpetterson wrote: > Is it possible for this function to get called twice such that we lose the > earlier message? It used to be in an earlier iteration, but I changed that. It shouldn't be any more, so I've changed this to an assert(). https://codereview.chromium.org/464213002/diff/1/chrome/browser/resources/hot... chrome/browser/resources/hotword/nacl_manager.js:302: console.log('Existing wait: ' + this.expectingMessage_); On 2014/08/13 14:13:17, James Hawkins wrote: > Remove console logging. > > Here and elsewhere. Done, but I'd like to add it back (in a future CL) to help debug. Is there a JS logging class that's frequently used for debug logging, or should I just write my own trivial logging.js? https://codereview.chromium.org/464213002/diff/1/chrome/browser/resources/hot... chrome/browser/resources/hotword/nacl_manager.js:335: NaClManager.prototype.handlePluginMessage_ = function(msg) { On 2014/08/13 14:13:18, James Hawkins wrote: > I suggest also breaking up this method into sub-handler methods for readability. Done.
Ping!
Thanks for the reminder. This addresses everything I noticed. LGTM
Ping(James): Still need an lgtm from you.
lgtm
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/464213002/40001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Message was sent while issue was closed.
Committed patchset #3 (40001) as 4c2c2d2f1b5a8c2930207b57b9c96980bec37f57
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/7b7830556b3f4bbff30bf5a6c8adea241b94464d Cr-Commit-Position: refs/heads/master@{#291597} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
