Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
600523004:
Support hotwording on google.com and the new tab page. (Closed)
|
Created:
6 years, 3 months ago by Anand Mistry (off Chromium) Modified:
6 years, 2 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@master Project:
chromium Visibility:
Public. |
DescriptionSupport hotwording on google.com and the new tab page.
The audio_client.js content script is copied from the existing hotword_helper
extesion, with the only change being the hotword extension ID to point to this
new extension.
BUG=397019
Committed: https://crrev.com/69ec447836330c3e1fc62ee2deb7027fdbd2e4a0
Cr-Commit-Position: refs/heads/master@{#299394}
Patch Set 1 #Patch Set 2 : Cleanup for review. #
Total comments: 7
Patch Set 3 : Move started callback into session object. #
Total comments: 4
Patch Set 4 : Rebase. #Patch Set 5 : Rebase. #Patch Set 6 : Rebase. #
Total comments: 29
Patch Set 7 : Rebase and fix copyright lines. #Patch Set 8 : Rebase. #Patch Set 9 : Address review comments. #Patch Set 10 : Remove extra blank lines. #
Total comments: 23
Patch Set 11 : Address more review comments. #Patch Set 12 : Remove unnecessary hasListener check. #
Total comments: 4
Patch Set 13 : More review comments. #
Messages
Total messages: 20 (2 generated)
amistry@chromium.org changed reviewers: + dbeam@chromium.org, rlp@chromium.org
https://codereview.chromium.org/600523004/diff/20001/chrome/browser/resources... File chrome/browser/resources/hotword/manager.js (right): https://codereview.chromium.org/600523004/diff/20001/chrome/browser/resources... chrome/browser/resources/hotword/manager.js:53: chrome.hotwordPrivate.notifyHotwordRecognition('search', Based on hotword_private_api.cc, the type -- 'search' in this case -- parameter isn't used. What will this eventually do? https://codereview.chromium.org/600523004/diff/20001/chrome/browser/resources... File chrome/browser/resources/hotword/page_audio_manager.js (right): https://codereview.chromium.org/600523004/diff/20001/chrome/browser/resources... chrome/browser/resources/hotword/page_audio_manager.js:38: this.tabActivatedListener_ = this.handleActivatedTab_.bind(this); So I guess for always on hotwording, we do need to make sure the extension is running all the time . . . . and that's why we do it on created and updated, yes? This is just my paranoia since we had so many bugs with the helper :-/ https://codereview.chromium.org/600523004/diff/20001/chrome/browser/resources... File chrome/browser/resources/hotword/state_manager.js (right): https://codereview.chromium.org/600523004/diff/20001/chrome/browser/resources... chrome/browser/resources/hotword/state_manager.js:55: this.sessionStartedCbs_ = []; why aren't these stored in the Session_ type?
https://codereview.chromium.org/600523004/diff/20001/chrome/browser/resources... File chrome/browser/resources/hotword/manager.js (right): https://codereview.chromium.org/600523004/diff/20001/chrome/browser/resources... chrome/browser/resources/hotword/manager.js:53: chrome.hotwordPrivate.notifyHotwordRecognition('search', On 2014/09/25 02:23:37, rpetterson wrote: > Based on hotword_private_api.cc, the type -- 'search' in this case -- parameter > isn't used. What will this eventually do? Honestly, no idea. I have no plans for it. It already existed in the API. https://codereview.chromium.org/600523004/diff/20001/chrome/browser/resources... File chrome/browser/resources/hotword/page_audio_manager.js (right): https://codereview.chromium.org/600523004/diff/20001/chrome/browser/resources... chrome/browser/resources/hotword/page_audio_manager.js:38: this.tabActivatedListener_ = this.handleActivatedTab_.bind(this); On 2014/09/25 02:23:38, rpetterson wrote: > So I guess for always on hotwording, we do need to make sure the extension is > running all the time . . . . and that's why we do it on created and updated, > yes? With always-on, as long as the NaCl module is receiving audio, the extension will keep running. These event handlers are unrelated to always-on. > This is just my paranoia since we had so many bugs with the helper :-/ If you look at the bottom of this file, if you have always-on enabled, all these listeners are unregistered. The magic I've discovered with event handlers is that you can stop receiving those events by unregistering all handlers. However, on a new event page, you can't unregister anything since nothing is registered, so you first register and then unregister. https://codereview.chromium.org/600523004/diff/20001/chrome/browser/resources... File chrome/browser/resources/hotword/state_manager.js (right): https://codereview.chromium.org/600523004/diff/20001/chrome/browser/resources... chrome/browser/resources/hotword/state_manager.js:55: this.sessionStartedCbs_ = []; On 2014/09/25 02:23:38, rpetterson wrote: > why aren't these stored in the Session_ type? There could be cases where you have several of these outstanding for a single session, and managing them separately would be better. Thinking about it a bit more, it doesn't matter. Having a single one per session type would be clearer, and allow it to be canceled when stopping a session.
LGTM https://codereview.chromium.org/600523004/diff/20001/chrome/browser/resources... File chrome/browser/resources/hotword/manager.js (right): https://codereview.chromium.org/600523004/diff/20001/chrome/browser/resources... chrome/browser/resources/hotword/manager.js:53: chrome.hotwordPrivate.notifyHotwordRecognition('search', On 2014/09/25 05:23:42, Anand Mistry wrote: > On 2014/09/25 02:23:37, rpetterson wrote: > > Based on hotword_private_api.cc, the type -- 'search' in this case -- > parameter > > isn't used. What will this eventually do? > > Honestly, no idea. I have no plans for it. It already existed in the API. I think Mukai originally wrote it . . . looking more at the code, it's not used at all. I've added crbug.com/418847 to make sure we clean it up at some point.
Dan: Ping! ...or re-assign :P
https://codereview.chromium.org/600523004/diff/40001/chrome/browser/resources... File chrome/browser/resources/hotword/audio_client.js (right): https://codereview.chromium.org/600523004/diff/40001/chrome/browser/resources... chrome/browser/resources/hotword/audio_client.js:93: AudioClient.HOTWORD_EXTENSION_ID_ = 'nbpagnldghgfoolbancepceaanlmhfmd'; nit: eventually makes sense to only have 1 line between top-level lines, here https://codereview.chromium.org/600523004/diff/40001/chrome/browser/resources... File chrome/browser/resources/hotword/manager.js (left): https://codereview.chromium.org/600523004/diff/40001/chrome/browser/resources... chrome/browser/resources/hotword/manager.js:37: chrome.runtime.reload(); revert https://codereview.chromium.org/600523004/diff/40001/chrome/browser/resources... File chrome/browser/resources/hotword/page_audio_manager.js (right): https://codereview.chromium.org/600523004/diff/40001/chrome/browser/resources... chrome/browser/resources/hotword/page_audio_manager.js:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. nit: no (c) https://codereview.chromium.org/600523004/diff/100001/chrome/browser/resource... File chrome/browser/resources/hotword/constants.js (right): https://codereview.chromium.org/600523004/diff/100001/chrome/browser/resource... chrome/browser/resources/hotword/constants.js:137: COMMAND_FIELD_NAME: COMMAND_FIELD_NAME, nit: alphabetize https://codereview.chromium.org/600523004/diff/100001/chrome/browser/resource... File chrome/browser/resources/hotword/page_audio_manager.js (right): https://codereview.chromium.org/600523004/diff/100001/chrome/browser/resource... chrome/browser/resources/hotword/page_audio_manager.js:27: * @private {!Object.<integer, chrome.runtime.Port>} s/integer/number https://codereview.chromium.org/600523004/diff/100001/chrome/browser/resource... chrome/browser/resources/hotword/page_audio_manager.js:56: * extension. It's used by isEligibleUrl to make that function clearer. i don't think having both a "checkEligibleUrl_" and "isEligibleUrl_" is clearer https://codereview.chromium.org/600523004/diff/100001/chrome/browser/resource... chrome/browser/resources/hotword/page_audio_manager.js:66: if (url === base || prefer == to === https://codereview.chromium.org/600523004/diff/100001/chrome/browser/resource... chrome/browser/resources/hotword/page_audio_manager.js:92: ]; don't use an array here until it has more than 1 item and/or isn't used only by `baseUrls[0]` https://codereview.chromium.org/600523004/diff/100001/chrome/browser/resource... chrome/browser/resources/hotword/page_audio_manager.js:133: if (windows[i].focused) { nit: reduce nesting by switching all for (...) { if (...) { ... } } to for (...) { if (!...) continue; ... } https://codereview.chromium.org/600523004/diff/100001/chrome/browser/resource... chrome/browser/resources/hotword/page_audio_manager.js:137: callback(tab); nit: calback.call(this, tab); so you can remove a lot of ".bind(this)" to all the passed in callbacks? https://codereview.chromium.org/600523004/diff/100001/chrome/browser/resource... chrome/browser/resources/hotword/page_audio_manager.js:204: if ('loading' !== info['status']) like yoda code should not read info['status'] != 'loading' https://codereview.chromium.org/600523004/diff/100001/chrome/browser/resource... chrome/browser/resources/hotword/page_audio_manager.js:236: var tab = /** @type {!Tab} */ (port.sender.tab); */ ( => */( https://codereview.chromium.org/600523004/diff/100001/chrome/browser/resource... chrome/browser/resources/hotword/page_audio_manager.js:298: var num = 0; nit: better var name https://codereview.chromium.org/600523004/diff/100001/chrome/browser/resource... chrome/browser/resources/hotword/page_audio_manager.js:301: if (isNaN(id)) when should this happen? maybe assert(!isNaN(id)) instead? https://codereview.chromium.org/600523004/diff/100001/chrome/browser/resource... chrome/browser/resources/hotword/page_audio_manager.js:393: break; nit: combine case? e.g. case CommandFromPage.SPEECH_END: case CommandFromPage.SPEECH_RESET: this.startHotwording_(); break; https://codereview.chromium.org/600523004/diff/100001/chrome/browser/resource... chrome/browser/resources/hotword/page_audio_manager.js:440: if (enabled) curlies because bottom has 'em
On 2014/10/03 03:40:20, Anand Mistry wrote: > Dan: Ping! ...or re-assign :P btw, feel free to re-assign when I'm taking too long (sorry)
https://codereview.chromium.org/600523004/diff/100001/chrome/browser/resource... File chrome/browser/resources/hotword/constants.js (right): https://codereview.chromium.org/600523004/diff/100001/chrome/browser/resource... chrome/browser/resources/hotword/constants.js:137: COMMAND_FIELD_NAME: COMMAND_FIELD_NAME, On 2014/10/09 22:43:58, Dan Beam wrote: > nit: alphabetize Done. https://codereview.chromium.org/600523004/diff/100001/chrome/browser/resource... File chrome/browser/resources/hotword/page_audio_manager.js (right): https://codereview.chromium.org/600523004/diff/100001/chrome/browser/resource... chrome/browser/resources/hotword/page_audio_manager.js:27: * @private {!Object.<integer, chrome.runtime.Port>} On 2014/10/09 22:43:58, Dan Beam wrote: > s/integer/number Done. https://codereview.chromium.org/600523004/diff/100001/chrome/browser/resource... chrome/browser/resources/hotword/page_audio_manager.js:56: * extension. It's used by isEligibleUrl to make that function clearer. On 2014/10/09 22:43:58, Dan Beam wrote: > i don't think having both a "checkEligibleUrl_" and "isEligibleUrl_" is clearer Deleted that comment, and also renamed to make it clearer what it does (it only checks the path component). https://codereview.chromium.org/600523004/diff/100001/chrome/browser/resource... chrome/browser/resources/hotword/page_audio_manager.js:66: if (url === base || On 2014/10/09 22:43:58, Dan Beam wrote: > prefer == to === Done. https://codereview.chromium.org/600523004/diff/100001/chrome/browser/resource... chrome/browser/resources/hotword/page_audio_manager.js:92: ]; On 2014/10/09 22:43:58, Dan Beam wrote: > don't use an array here until it has more than 1 item and/or isn't used only by > `baseUrls[0]` Done. https://codereview.chromium.org/600523004/diff/100001/chrome/browser/resource... chrome/browser/resources/hotword/page_audio_manager.js:133: if (windows[i].focused) { On 2014/10/09 22:43:58, Dan Beam wrote: > nit: reduce nesting by switching all > > for (...) { > if (...) { > ... > } > } > > to > > for (...) { > if (!...) > continue; > ... > } Done. https://codereview.chromium.org/600523004/diff/100001/chrome/browser/resource... chrome/browser/resources/hotword/page_audio_manager.js:137: callback(tab); On 2014/10/09 22:43:58, Dan Beam wrote: > nit: calback.call(this, tab); so you can remove a lot of ".bind(this)" to all > the passed in callbacks? Done. I'm curious about this one. As a JS newbie, explicitly having the .bind on callback functions makes it very clear how the function is executed. Why remove it? https://codereview.chromium.org/600523004/diff/100001/chrome/browser/resource... chrome/browser/resources/hotword/page_audio_manager.js:204: if ('loading' !== info['status']) On 2014/10/09 22:43:58, Dan Beam wrote: > like yoda code should not read > > info['status'] != 'loading' Done. https://codereview.chromium.org/600523004/diff/100001/chrome/browser/resource... chrome/browser/resources/hotword/page_audio_manager.js:236: var tab = /** @type {!Tab} */ (port.sender.tab); On 2014/10/09 22:43:58, Dan Beam wrote: > */ ( => */( Done. https://codereview.chromium.org/600523004/diff/100001/chrome/browser/resource... chrome/browser/resources/hotword/page_audio_manager.js:298: var num = 0; On 2014/10/09 22:43:58, Dan Beam wrote: > nit: better var name Deleted. Variable is unnecessary. https://codereview.chromium.org/600523004/diff/100001/chrome/browser/resource... chrome/browser/resources/hotword/page_audio_manager.js:301: if (isNaN(id)) On 2014/10/09 22:43:58, Dan Beam wrote: > when should this happen? As far as I can tell, never. > maybe assert(!isNaN(id)) instead? Done. https://codereview.chromium.org/600523004/diff/100001/chrome/browser/resource... chrome/browser/resources/hotword/page_audio_manager.js:393: break; On 2014/10/09 22:43:58, Dan Beam wrote: > nit: combine case? e.g. > > case CommandFromPage.SPEECH_END: > case CommandFromPage.SPEECH_RESET: > this.startHotwording_(); > break; Done. https://codereview.chromium.org/600523004/diff/100001/chrome/browser/resource... chrome/browser/resources/hotword/page_audio_manager.js:440: if (enabled) On 2014/10/09 22:43:58, Dan Beam wrote: > curlies because bottom has 'em Done.
https://codereview.chromium.org/600523004/diff/40001/chrome/browser/resources... File chrome/browser/resources/hotword/audio_client.js (right): https://codereview.chromium.org/600523004/diff/40001/chrome/browser/resources... chrome/browser/resources/hotword/audio_client.js:93: AudioClient.HOTWORD_EXTENSION_ID_ = 'nbpagnldghgfoolbancepceaanlmhfmd'; On 2014/10/09 22:43:58, Dan Beam wrote: > nit: eventually makes sense to only have 1 line between top-level lines, here Done.
just waiting on PageAudioManager#disconnectAllClients_ https://codereview.chromium.org/600523004/diff/100001/chrome/browser/resource... File chrome/browser/resources/hotword/constants.js (right): https://codereview.chromium.org/600523004/diff/100001/chrome/browser/resource... chrome/browser/resources/hotword/constants.js:137: COMMAND_FIELD_NAME: COMMAND_FIELD_NAME, On 2014/10/10 23:11:44, Anand Mistry wrote: > On 2014/10/09 22:43:58, Dan Beam wrote: > > nit: alphabetize > > Done. thanks for doing this. what you have works, but I meant: return { ... CONSTANTS_ALPHABETIZED_WITHIN_THEIR_OWN_LIST ... ... EnumsAlphabetizedWithinTheirOwnList ... }; sorry, you had absolutely 0 way of knowing that :/. also, ideally the initialization/declaration order of each value in the file would also match the order in the return {...};, e.g., /** Spooky. */ var HALLOWEEN = 1; /** Oooky. */ var SCARES_SMALL_CHILDREN = 2; /** Very. */ var Candy = {BabyRuth: 'no', MilkyWay: 'teeth'}; /** Scary. */ var Costumes = {Frankenstein: 1, Ghost: 2}; return { HALLOWEEN: HALLOWEEN, SCARES_SMALL_CHILDREN: SCARES_SMALL_CHILDREN, Candy: Candy, Costumes: Costumes, } feel free to do this separately. it's not a huge deal. https://codereview.chromium.org/600523004/diff/100001/chrome/browser/resource... File chrome/browser/resources/hotword/page_audio_manager.js (right): https://codereview.chromium.org/600523004/diff/100001/chrome/browser/resource... chrome/browser/resources/hotword/page_audio_manager.js:137: callback(tab); On 2014/10/10 23:11:44, Anand Mistry wrote: > On 2014/10/09 22:43:58, Dan Beam wrote: > > nit: calback.call(this, tab); so you can remove a lot of ".bind(this)" to all > > the passed in callbacks? > > Done. I'm curious about this one. As a JS newbie, explicitly having the .bind > on callback functions makes it very clear how the function is executed. Why > remove it? that's the only drawback. you could also make a /** * ... * @param {?} opt_scope An optional scope to call |callback| with. */ and then callback.call(opt_scope, ...); // Works if |opt_scope| is undefined or null. https://codereview.chromium.org/600523004/diff/410001/chrome/browser/resource... File chrome/browser/resources/hotword/page_audio_manager.js (right): https://codereview.chromium.org/600523004/diff/410001/chrome/browser/resource... chrome/browser/resources/hotword/page_audio_manager.js:230: var tab = /** @type {!Tab} */(port.sender.tab); nit: lower |tab| right above where it's used https://codereview.chromium.org/600523004/diff/410001/chrome/browser/resource... chrome/browser/resources/hotword/page_audio_manager.js:231: if (port.name === hotword.constants.CLIENT_PORT_NAME) { == https://codereview.chromium.org/600523004/diff/410001/chrome/browser/resource... chrome/browser/resources/hotword/page_audio_manager.js:231: if (port.name === hotword.constants.CLIENT_PORT_NAME) { nit: if (port.name != hotword.constants.CLIENT_PORT_NAME) return; ... https://codereview.chromium.org/600523004/diff/410001/chrome/browser/resource... chrome/browser/resources/hotword/page_audio_manager.js:265: for (var id in tabIds.portMap_) { shouldn't this be for (var id in this.portMap_) { looks like this is doing very little currently :( https://codereview.chromium.org/600523004/diff/410001/chrome/browser/resource... chrome/browser/resources/hotword/page_audio_manager.js:374: var command = request[hotword.constants.COMMAND_FIELD_NAME]; nit: this should work fine switch (request[hotword.constants.COMMAND_FIELD_NAME]) {...} or if you don't like doing that: var command = request[hotword.constants.COMMAND_FIELD_NAME]; if (!command) return; it just seems silly to have if (obj[really.long.prop_name])) var ref = obj[same.long.prop_name]; https://codereview.chromium.org/600523004/diff/410001/chrome/browser/resource... chrome/browser/resources/hotword/page_audio_manager.js:411: return; is this needed? https://codereview.chromium.org/600523004/diff/410001/chrome/browser/resource... chrome/browser/resources/hotword/page_audio_manager.js:427: !this.stateManager_.isAlwaysOnEnabled(); just isAlwaysOnEnabled() (which checks isEnabled() anyways) https://codereview.chromium.org/600523004/diff/410001/chrome/browser/resource... File chrome/browser/resources/hotword/state_manager.js (right): https://codereview.chromium.org/600523004/diff/410001/chrome/browser/resource... chrome/browser/resources/hotword/state_manager.js:9: * Trivial container class for session information. @param x 3 https://codereview.chromium.org/600523004/diff/410001/chrome/browser/resource... chrome/browser/resources/hotword/state_manager.js:17: * @protected {!hotword.constants.SessionSource} nit: ehhh, I'd just make this @private if it's only used in this file. closure compiler does file-level access control, not class-level. https://codereview.chromium.org/600523004/diff/410001/chrome/browser/resource... chrome/browser/resources/hotword/state_manager.js:67: * Event that fires when the hotwording status has changed. @type
https://codereview.chromium.org/600523004/diff/100001/chrome/browser/resource... File chrome/browser/resources/hotword/constants.js (right): https://codereview.chromium.org/600523004/diff/100001/chrome/browser/resource... chrome/browser/resources/hotword/constants.js:137: COMMAND_FIELD_NAME: COMMAND_FIELD_NAME, On 2014/10/11 00:54:35, Dan Beam wrote: > On 2014/10/10 23:11:44, Anand Mistry wrote: > > On 2014/10/09 22:43:58, Dan Beam wrote: > > > nit: alphabetize > > > > Done. > > thanks for doing this. > > what you have works, but I meant: > > return { > ... CONSTANTS_ALPHABETIZED_WITHIN_THEIR_OWN_LIST ... > ... EnumsAlphabetizedWithinTheirOwnList ... > }; > > sorry, you had absolutely 0 way of knowing that :/. Gotcha! Fixed that. > > also, ideally the initialization/declaration order of each value in the file > would also match the order in the return {...};, e.g., > > /** Spooky. */ > var HALLOWEEN = 1; > > /** Oooky. */ > var SCARES_SMALL_CHILDREN = 2; > > /** Very. */ > var Candy = {BabyRuth: 'no', MilkyWay: 'teeth'}; > > /** Scary. */ > var Costumes = {Frankenstein: 1, Ghost: 2}; > > return { > HALLOWEEN: HALLOWEEN, > SCARES_SMALL_CHILDREN: SCARES_SMALL_CHILDREN, > Candy: Candy, > Costumes: Costumes, > } > > feel free to do this separately. it's not a huge deal. Yeah, I'll do this in a separate CL. https://codereview.chromium.org/600523004/diff/410001/chrome/browser/resource... File chrome/browser/resources/hotword/page_audio_manager.js (right): https://codereview.chromium.org/600523004/diff/410001/chrome/browser/resource... chrome/browser/resources/hotword/page_audio_manager.js:230: var tab = /** @type {!Tab} */(port.sender.tab); On 2014/10/11 00:54:35, Dan Beam wrote: > nit: lower |tab| right above where it's used Done. https://codereview.chromium.org/600523004/diff/410001/chrome/browser/resource... chrome/browser/resources/hotword/page_audio_manager.js:231: if (port.name === hotword.constants.CLIENT_PORT_NAME) { On 2014/10/11 00:54:35, Dan Beam wrote: > nit: > > if (port.name != hotword.constants.CLIENT_PORT_NAME) > return; > > ... Done. https://codereview.chromium.org/600523004/diff/410001/chrome/browser/resource... chrome/browser/resources/hotword/page_audio_manager.js:231: if (port.name === hotword.constants.CLIENT_PORT_NAME) { On 2014/10/11 00:54:35, Dan Beam wrote: > == Done. https://codereview.chromium.org/600523004/diff/410001/chrome/browser/resource... chrome/browser/resources/hotword/page_audio_manager.js:265: for (var id in tabIds.portMap_) { On 2014/10/11 00:54:35, Dan Beam wrote: > shouldn't this be > > for (var id in this.portMap_) { > > looks like this is doing very little currently :( Oops. https://codereview.chromium.org/600523004/diff/410001/chrome/browser/resource... chrome/browser/resources/hotword/page_audio_manager.js:374: var command = request[hotword.constants.COMMAND_FIELD_NAME]; On 2014/10/11 00:54:35, Dan Beam wrote: > nit: this should work fine > > switch (request[hotword.constants.COMMAND_FIELD_NAME]) {...} > > or if you don't like doing that: > > var command = request[hotword.constants.COMMAND_FIELD_NAME]; > if (!command) > return; > > it just seems silly to have > > if (obj[really.long.prop_name])) > var ref = obj[same.long.prop_name]; Done. https://codereview.chromium.org/600523004/diff/410001/chrome/browser/resource... chrome/browser/resources/hotword/page_audio_manager.js:411: return; On 2014/10/11 00:54:35, Dan Beam wrote: > is this needed? Unless I'm misreading the event implementation, addListener() doesn't check that the function hasn't already been added. In that case, this prevents duplicate additions of the same function which messes with the logic here. https://codereview.chromium.org/600523004/diff/410001/chrome/browser/resource... chrome/browser/resources/hotword/page_audio_manager.js:427: !this.stateManager_.isAlwaysOnEnabled(); On 2014/10/11 00:54:35, Dan Beam wrote: > just isAlwaysOnEnabled() (which checks isEnabled() anyways) That's not enough. isAlwaysOnEnabled() also returns false when isEnabled() returns false. The goal is that this code runs when the user wants the not-always-on hotwording. https://codereview.chromium.org/600523004/diff/410001/chrome/browser/resource... File chrome/browser/resources/hotword/state_manager.js (right): https://codereview.chromium.org/600523004/diff/410001/chrome/browser/resource... chrome/browser/resources/hotword/state_manager.js:9: * Trivial container class for session information. On 2014/10/11 00:54:36, Dan Beam wrote: > @param x 3 Done. https://codereview.chromium.org/600523004/diff/410001/chrome/browser/resource... chrome/browser/resources/hotword/state_manager.js:17: * @protected {!hotword.constants.SessionSource} On 2014/10/11 00:54:35, Dan Beam wrote: > nit: ehhh, I'd just make this @private if it's only used in this file. closure > compiler does file-level access control, not class-level. Done. https://codereview.chromium.org/600523004/diff/410001/chrome/browser/resource... chrome/browser/resources/hotword/state_manager.js:67: * Event that fires when the hotwording status has changed. On 2014/10/11 00:54:36, Dan Beam wrote: > @type Done.
https://codereview.chromium.org/600523004/diff/410001/chrome/browser/resource... File chrome/browser/resources/hotword/page_audio_manager.js (right): https://codereview.chromium.org/600523004/diff/410001/chrome/browser/resource... chrome/browser/resources/hotword/page_audio_manager.js:411: return; On 2014/10/13 19:30:21, Anand Mistry wrote: > On 2014/10/11 00:54:35, Dan Beam wrote: > > is this needed? > > Unless I'm misreading the event implementation, addListener() doesn't check that > the function hasn't already been added. In that case, this prevents duplicate > additions of the same function which messes with the logic here. this is removeListener() https://codereview.chromium.org/600523004/diff/410001/chrome/browser/resource... chrome/browser/resources/hotword/page_audio_manager.js:427: !this.stateManager_.isAlwaysOnEnabled(); On 2014/10/13 19:30:21, Anand Mistry wrote: > On 2014/10/11 00:54:35, Dan Beam wrote: > > just isAlwaysOnEnabled() (which checks isEnabled() anyways) > > That's not enough. isAlwaysOnEnabled() also returns false when isEnabled() > returns false. > > The goal is that this code runs when the user wants the not-always-on > hotwording. ah, yeah, sorry -- missed a !
https://codereview.chromium.org/600523004/diff/410001/chrome/browser/resource... File chrome/browser/resources/hotword/page_audio_manager.js (right): https://codereview.chromium.org/600523004/diff/410001/chrome/browser/resource... chrome/browser/resources/hotword/page_audio_manager.js:411: return; On 2014/10/13 19:33:09, Dan Beam wrote: > On 2014/10/13 19:30:21, Anand Mistry wrote: > > On 2014/10/11 00:54:35, Dan Beam wrote: > > > is this needed? > > > > Unless I'm misreading the event implementation, addListener() doesn't check > that > > the function hasn't already been added. In that case, this prevents duplicate > > additions of the same function which messes with the logic here. > > this is removeListener() Oops. Sorry. Yeah, it is unnecessary.
lgtm https://codereview.chromium.org/600523004/diff/620001/chrome/browser/resource... File chrome/browser/resources/hotword/page_audio_manager.js (right): https://codereview.chromium.org/600523004/diff/620001/chrome/browser/resource... chrome/browser/resources/hotword/page_audio_manager.js:55: * Helper function to test if a URL path is elibible for hotwording. eligible https://codereview.chromium.org/600523004/diff/620001/chrome/browser/resource... File chrome/browser/resources/hotword/state_manager.js (right): https://codereview.chromium.org/600523004/diff/620001/chrome/browser/resource... chrome/browser/resources/hotword/state_manager.js:31: this.triggerCb = triggerCb; add _ if @private
Thanks. https://codereview.chromium.org/600523004/diff/620001/chrome/browser/resource... File chrome/browser/resources/hotword/page_audio_manager.js (right): https://codereview.chromium.org/600523004/diff/620001/chrome/browser/resource... chrome/browser/resources/hotword/page_audio_manager.js:55: * Helper function to test if a URL path is elibible for hotwording. On 2014/10/13 21:58:03, Dan Beam wrote: > eligible Done. https://codereview.chromium.org/600523004/diff/620001/chrome/browser/resource... File chrome/browser/resources/hotword/state_manager.js (right): https://codereview.chromium.org/600523004/diff/620001/chrome/browser/resource... chrome/browser/resources/hotword/state_manager.js:31: this.triggerCb = triggerCb; On 2014/10/13 21:58:03, Dan Beam wrote: > add _ if @private Done.
The CQ bit was checked by amistry@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/600523004/710001
Message was sent while issue was closed.
Committed patchset #13 (id:710001)
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/69ec447836330c3e1fc62ee2deb7027fdbd2e4a0 Cr-Commit-Position: refs/heads/master@{#299394} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
