Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1274403003:
Full Implementation of Audio for the Chrome Os Device Emulator (Closed)
|
Created:
5 years, 4 months ago by mozartalouis Modified:
5 years, 4 months ago CC:
chromium-reviews, hashimoto+watch_chromium.org, stevenjb+watch_chromium.org, arv+watch_chromium.org, oshima+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplementation of Audio for the Chrome Os Device Emulator
BUG=516545
Committed: https://crrev.com/a79e8ad477bb753fbed0bb2e00864419915b44e7
Cr-Commit-Position: refs/heads/master@{#342849}
Patch Set 1 : Initial Patch #
Total comments: 35
Patch Set 2 : Updated with merge #
Total comments: 12
Patch Set 3 : More nits #
Total comments: 4
Patch Set 4 : Fixed AudioType in JS/HTML #
Total comments: 3
Patch Set 5 : Comments #Messages
Total messages: 27 (8 generated)
Ready for Review
xiyuan@chromium.org changed reviewers: + xiyuan@chromium.org - xiyuan@google.com
LGTM with nits Please wait for stevenjb@ and michaelpg@ as well. Thanks. https://codereview.chromium.org/1274403003/diff/1/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/emulator/audio_settings.html (right): https://codereview.chromium.org/1274403003/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/emulator/audio_settings.html:21: <paper-input value="{{item.deviceName}}" label="Name"></paper-input> nit: wrap? https://codereview.chromium.org/1274403003/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/emulator/audio_settings.html:23: error-message="Invalid ID: Must be 5 digits"> nit: 4-space indent. https://codereview.chromium.org/1274403003/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/emulator/audio_settings.html:26: <paper-toggle-button checked="{{item.isInput}}"></paper-toggle-button> nit: wrap https://codereview.chromium.org/1274403003/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/emulator/audio_settings.html:28: <paper-toggle-button checked="{{item.active}}"></paper-toggle-button> nit: wrap https://codereview.chromium.org/1274403003/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/emulator/audio_settings.html:36: >[[option.name]]</paper-radio-button> nit: ">" should stay with previous line. https://codereview.chromium.org/1274403003/diff/1/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/emulator/audio_settings.js (right): https://codereview.chromium.org/1274403003/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/emulator/audio_settings.js:5: /** @enum {string} */ AudioNodeType = { nit: var AudioNodeType = {... "var" was missing. https://codereview.chromium.org/1274403003/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/emulator/audio_settings.js:164: }, nit: remove "," for the last property https://codereview.chromium.org/1274403003/diff/1/chrome/browser/ui/webui/chr... File chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc (right): https://codereview.chromium.org/1274403003/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc:32: "device_emulator.audioSettings.updateAudioNodes"; nit: Move this with other JS callback names around line 46-53 (i.e. where kAddBluetoothDeviceJSCallback etc is defined). https://codereview.chromium.org/1274403003/diff/1/chromeos/dbus/fake_cras_aud... File chromeos/dbus/fake_cras_audio_client.cc (right): https://codereview.chromium.org/1274403003/diff/1/chromeos/dbus/fake_cras_aud... chromeos/dbus/fake_cras_audio_client.cc:191: FOR_EACH_OBSERVER(Observer, observers_, NodesChanged()); Move this with node_list_.erase() so that we only notify when the removal actually happens.
Ready for Review michaelpg@ stevenjb@ https://codereview.chromium.org/1274403003/diff/1/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/emulator/audio_settings.html (right): https://codereview.chromium.org/1274403003/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/emulator/audio_settings.html:21: <paper-input value="{{item.deviceName}}" label="Name"></paper-input> On 2015/08/07 20:17:12, xiyuan wrote: > nit: wrap? Done. https://codereview.chromium.org/1274403003/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/emulator/audio_settings.html:23: error-message="Invalid ID: Must be 5 digits"> On 2015/08/07 20:17:12, xiyuan wrote: > nit: 4-space indent. Done. https://codereview.chromium.org/1274403003/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/emulator/audio_settings.html:26: <paper-toggle-button checked="{{item.isInput}}"></paper-toggle-button> On 2015/08/07 20:17:12, xiyuan wrote: > nit: wrap Done. https://codereview.chromium.org/1274403003/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/emulator/audio_settings.html:28: <paper-toggle-button checked="{{item.active}}"></paper-toggle-button> On 2015/08/07 20:17:12, xiyuan wrote: > nit: wrap Done. https://codereview.chromium.org/1274403003/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/emulator/audio_settings.html:36: >[[option.name]]</paper-radio-button> On 2015/08/07 20:17:12, xiyuan wrote: > nit: ">" should stay with previous line. Done. https://codereview.chromium.org/1274403003/diff/1/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/emulator/audio_settings.js (right): https://codereview.chromium.org/1274403003/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/emulator/audio_settings.js:5: /** @enum {string} */ AudioNodeType = { On 2015/08/07 20:17:12, xiyuan wrote: > nit: var AudioNodeType = {... > > "var" was missing. Done. https://codereview.chromium.org/1274403003/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/emulator/audio_settings.js:164: }, On 2015/08/07 20:17:12, xiyuan wrote: > nit: remove "," for the last property Done. https://codereview.chromium.org/1274403003/diff/1/chrome/browser/ui/webui/chr... File chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc (right): https://codereview.chromium.org/1274403003/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc:32: "device_emulator.audioSettings.updateAudioNodes"; On 2015/08/07 20:17:12, xiyuan wrote: > nit: Move this with other JS callback names around line 46-53 (i.e. where > kAddBluetoothDeviceJSCallback etc is defined). Done. https://codereview.chromium.org/1274403003/diff/1/chromeos/dbus/fake_cras_aud... File chromeos/dbus/fake_cras_audio_client.cc (right): https://codereview.chromium.org/1274403003/diff/1/chromeos/dbus/fake_cras_aud... chromeos/dbus/fake_cras_audio_client.cc:191: FOR_EACH_OBSERVER(Observer, observers_, NodesChanged()); On 2015/08/07 20:17:12, xiyuan wrote: > Move this with node_list_.erase() so that we only notify when the removal > actually happens. Done.
https://codereview.chromium.org/1274403003/diff/1/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/emulator/audio_settings.html (right): https://codereview.chromium.org/1274403003/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/emulator/audio_settings.html:6: <link rel="import" href="chrome://resources/polymer/v1_0/paper-icon-button/paper-icon-button.html"> nit: aphabetize https://codereview.chromium.org/1274403003/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/emulator/audio_settings.html:36: >[[option.name]]</paper-radio-button> On 2015/08/07 20:17:12, xiyuan wrote: > nit: ">" should stay with previous line. actually, this is an acceptable hacky workaround: Polymer [[binding.annotations]] need to span the entire tag :-( https://codereview.chromium.org/1274403003/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/emulator/audio_settings.html:42: data-index="[[index]]">Add Node</paper-button> Instead of data-index, just use event.model.index. https://codereview.chromium.org/1274403003/diff/1/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/emulator/audio_settings.js (right): https://codereview.chromium.org/1274403003/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/emulator/audio_settings.js:95: appendNewNode: function() { this.push('nodes', new AudioNode()); }, nit: don't put non-trivial functions all on one line https://codereview.chromium.org/1274403003/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/emulator/audio_settings.js:98: * This adds or modifes an audio node to the AudioNodeList. nit: modifies https://codereview.chromium.org/1274403003/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/emulator/audio_settings.js:99: * @param {event} node. event -> Event https://codereview.chromium.org/1274403003/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/emulator/audio_settings.js:102: // Create a new audio node and add all the properties from |nodes[i]| nit: end w/ period https://codereview.chromium.org/1274403003/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/emulator/audio_settings.js:103: var info = this.nodes[node.path[2].dataIndex]; node is an event, right? Rename the parameter to event, and use event.target instead of event.path[2]. event.target gives you the Polymer element in your local DOM, while event.path[i] depends on the internal implementation of the element. https://codereview.chromium.org/1274403003/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/emulator/audio_settings.js:164: }, On 2015/08/07 20:17:12, xiyuan wrote: > nit: remove "," for the last property Some people like this as it makes it easier to add new properties -- the diff and blames look nicer.
Patchset #2 (id:20001) has been deleted
Ready for Review https://codereview.chromium.org/1274403003/diff/1/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/emulator/audio_settings.html (right): https://codereview.chromium.org/1274403003/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/emulator/audio_settings.html:36: >[[option.name]]</paper-radio-button> On 2015/08/07 22:47:42, michaelpg wrote: > On 2015/08/07 20:17:12, xiyuan wrote: > > nit: ">" should stay with previous line. > > actually, this is an acceptable hacky workaround: Polymer > [[binding.annotations]] need to span the entire tag :-( Acknowledged. https://codereview.chromium.org/1274403003/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/emulator/audio_settings.html:42: data-index="[[index]]">Add Node</paper-button> On 2015/08/07 22:47:42, michaelpg wrote: > Instead of data-index, just use event.model.index. Done. https://codereview.chromium.org/1274403003/diff/1/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/emulator/audio_settings.js (right): https://codereview.chromium.org/1274403003/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/emulator/audio_settings.js:95: appendNewNode: function() { this.push('nodes', new AudioNode()); }, On 2015/08/07 22:47:42, michaelpg wrote: > nit: don't put non-trivial functions all on one line i think this was the work of clang-format. :/ won't use it again https://codereview.chromium.org/1274403003/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/emulator/audio_settings.js:98: * This adds or modifes an audio node to the AudioNodeList. On 2015/08/07 22:47:42, michaelpg wrote: > nit: modifies Done. https://codereview.chromium.org/1274403003/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/emulator/audio_settings.js:99: * @param {event} node. On 2015/08/07 22:47:42, michaelpg wrote: > event -> Event Done. https://codereview.chromium.org/1274403003/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/emulator/audio_settings.js:102: // Create a new audio node and add all the properties from |nodes[i]| On 2015/08/07 22:47:42, michaelpg wrote: > nit: end w/ period Done. https://codereview.chromium.org/1274403003/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/emulator/audio_settings.js:103: var info = this.nodes[node.path[2].dataIndex]; On 2015/08/07 22:47:42, michaelpg wrote: > node is an event, right? Rename the parameter to event, and use event.target > instead of event.path[2]. event.target gives you the Polymer element in your > local DOM, while event.path[i] depends on the internal implementation of the > element. Done.
https://codereview.chromium.org/1274403003/diff/1/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/emulator/audio_settings.html (right): https://codereview.chromium.org/1274403003/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/emulator/audio_settings.html:42: data-index="[[index]]">Add Node</paper-button> On 2015/08/07 23:16:50, mozartalouis wrote: > On 2015/08/07 22:47:42, michaelpg wrote: > > Instead of data-index, just use event.model.index. > > Done. Ah, I meant that in insertAudioNode, the event parameter has the index already defined at event.model.index. So you don't need to include anything in the HTML here. https://codereview.chromium.org/1274403003/diff/40001/chrome/browser/resource... File chrome/browser/resources/chromeos/emulator/audio_settings.html (right): https://codereview.chromium.org/1274403003/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/emulator/audio_settings.html:7: <link rel="import" href="chrome://resources/polymer/v1_0/polymer/polymer.html"> it's OK to include Polymer first since other imports depend on it (although the other imports also import Polymer themselves, so you don' really need it at all, it's just good practice to include-what-you-use) https://codereview.chromium.org/1274403003/diff/40001/chrome/browser/resource... File chrome/browser/resources/chromeos/emulator/audio_settings.js (right): https://codereview.chromium.org/1274403003/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/emulator/audio_settings.js:105: var info = this.nodes[e.model.index]; yep, this is what I meant https://codereview.chromium.org/1274403003/diff/40001/chrome/browser/resource... File chrome/browser/resources/chromeos/emulator/battery_settings.css (right): https://codereview.chromium.org/1274403003/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/emulator/battery_settings.css:32: gg vim artifact?
https://codereview.chromium.org/1274403003/diff/40001/chrome/browser/resource... File chrome/browser/resources/chromeos/emulator/audio_settings.js (right): https://codereview.chromium.org/1274403003/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/emulator/audio_settings.js:30: this.id = '30001'; Document why '30001' is the default (even if it's 'Set to arbitrary default.'). https://codereview.chromium.org/1274403003/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/emulator/audio_settings.js:67: * @type !Array<! {name: string, type: string} > Elim ' ' before { and after } https://codereview.chromium.org/1274403003/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/emulator/audio_settings.js:101: * @param {Event} e. Document the parts of the object we care about instead of using Event: @param {model: {index: number}} e Event with a model containing the index in |nodes| to add. https://codereview.chromium.org/1274403003/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/emulator/audio_settings.js:106: info.type = this.getAudioTypeForName(info.type); This will modify this.nodes[e.model.index], which I don't think is what you want. The code is also confusing, why are we passing info.type to getAudioTypeForName? If we are passing a different type when sending 'insertAudioNode', we need to typedef that separately (even if it is mostly the same). https://codereview.chromium.org/1274403003/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/emulator/audio_settings.js:112: * @param {Event} e. Document 'e' https://codereview.chromium.org/1274403003/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/emulator/audio_settings.js:123: * @return {string} The label which represents |type|. Clarify, e.g. 'The display name corresponding to |type|.' https://codereview.chromium.org/1274403003/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/emulator/audio_settings.js:135: * of a nodes name. Please clarify 'an AudioType of a nodes name' https://codereview.chromium.org/1274403003/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/emulator/audio_settings.js:136: * @return {string} The label which represents |name|. Clarify https://codereview.chromium.org/1274403003/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc (right): https://codereview.chromium.org/1274403003/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc:231: audio_node->SetBoolean("is_input", node.is_input); Any reason not to use the same property names here as in the JS, i.e. "isInput" ? Then we can avoid the extra translation in the JS.
Ready for Review https://codereview.chromium.org/1274403003/diff/40001/chrome/browser/resource... File chrome/browser/resources/chromeos/emulator/audio_settings.html (right): https://codereview.chromium.org/1274403003/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/emulator/audio_settings.html:7: <link rel="import" href="chrome://resources/polymer/v1_0/polymer/polymer.html"> On 2015/08/10 16:37:23, michaelpg wrote: > it's OK to include Polymer first since other imports depend on it (although the > other imports also import Polymer themselves, so you don' really need it at all, > it's just good practice to include-what-you-use) Done. https://codereview.chromium.org/1274403003/diff/40001/chrome/browser/resource... File chrome/browser/resources/chromeos/emulator/audio_settings.js (right): https://codereview.chromium.org/1274403003/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/emulator/audio_settings.js:30: this.id = '30001'; On 2015/08/10 16:46:28, stevenjb wrote: > Document why '30001' is the default (even if it's 'Set to arbitrary default.'). Done. https://codereview.chromium.org/1274403003/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/emulator/audio_settings.js:67: * @type !Array<! {name: string, type: string} > On 2015/08/10 16:46:27, stevenjb wrote: > Elim ' ' before { and after } Done. https://codereview.chromium.org/1274403003/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/emulator/audio_settings.js:67: * @type !Array<! {name: string, type: string} > On 2015/08/10 16:46:27, stevenjb wrote: > Elim ' ' before { and after } Pre-submit is complaining about this. I will just ignore it though https://codereview.chromium.org/1274403003/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/emulator/audio_settings.js:101: * @param {Event} e. On 2015/08/10 16:46:28, stevenjb wrote: > Document the parts of the object we care about instead of using Event: > > @param {model: {index: number}} e Event with a model containing the index in > |nodes| to add. Done. https://codereview.chromium.org/1274403003/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/emulator/audio_settings.js:105: var info = this.nodes[e.model.index]; On 2015/08/10 16:37:23, michaelpg wrote: > yep, this is what I meant Done. https://codereview.chromium.org/1274403003/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/emulator/audio_settings.js:106: info.type = this.getAudioTypeForName(info.type); On 2015/08/10 16:46:27, stevenjb wrote: > This will modify this.nodes[e.model.index], which I don't think is what you > want. The code is also confusing, why are we passing info.type to > getAudioTypeForName? > > If we are passing a different type when sending 'insertAudioNode', we need to > typedef that separately (even if it is mostly the same). Done. https://codereview.chromium.org/1274403003/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/emulator/audio_settings.js:112: * @param {Event} e. On 2015/08/10 16:46:28, stevenjb wrote: > Document 'e' Done. https://codereview.chromium.org/1274403003/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/emulator/audio_settings.js:123: * @return {string} The label which represents |type|. On 2015/08/10 16:46:27, stevenjb wrote: > Clarify, e.g. 'The display name corresponding to |type|.' > Done. https://codereview.chromium.org/1274403003/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/emulator/audio_settings.js:135: * of a nodes name. On 2015/08/10 16:46:28, stevenjb wrote: > Please clarify 'an AudioType of a nodes name' Done. https://codereview.chromium.org/1274403003/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/emulator/audio_settings.js:136: * @return {string} The label which represents |name|. On 2015/08/10 16:46:28, stevenjb wrote: > Clarify Done. https://codereview.chromium.org/1274403003/diff/40001/chrome/browser/resource... File chrome/browser/resources/chromeos/emulator/battery_settings.css (right): https://codereview.chromium.org/1274403003/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/emulator/battery_settings.css:32: gg On 2015/08/10 16:37:23, michaelpg wrote: > vim artifact? Not even sure :/ https://codereview.chromium.org/1274403003/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc (right): https://codereview.chromium.org/1274403003/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc:231: audio_node->SetBoolean("is_input", node.is_input); On 2015/08/10 16:46:28, stevenjb wrote: > Any reason not to use the same property names here as in the JS, i.e. "isInput" > ? Then we can avoid the extra translation in the JS. I was trying to follow each languages naming scheme but you are correct. fixed in the next patch
https://codereview.chromium.org/1274403003/diff/40001/chrome/browser/resource... File chrome/browser/resources/chromeos/emulator/audio_settings.js (right): https://codereview.chromium.org/1274403003/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/emulator/audio_settings.js:67: * @type !Array<! {name: string, type: string} > On 2015/08/10 17:42:37, mozartalouis wrote: > On 2015/08/10 16:46:27, stevenjb wrote: > > Elim ' ' before { and after } > > Pre-submit is complaining about this. I will just ignore it though Surrounding the whole type in {} should resolve this. https://codereview.chromium.org/1274403003/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/emulator/audio_settings.js:106: info.type = this.getAudioTypeForName(info.type); On 2015/08/10 17:42:38, mozartalouis wrote: > On 2015/08/10 16:46:27, stevenjb wrote: > > This will modify this.nodes[e.model.index], which I don't think is what you > > want. The code is also confusing, why are we passing info.type to > > getAudioTypeForName? > > > > If we are passing a different type when sending 'insertAudioNode', we need to > > typedef that separately (even if it is mostly the same). > > Done. The new code actually does the same thing as the old code, just using a temporary; stevenjb's point is that you don't want to modify |info|.
Patchset #3 (id:60001) has been deleted
https://codereview.chromium.org/1274403003/diff/40001/chrome/browser/resource... File chrome/browser/resources/chromeos/emulator/audio_settings.js (right): https://codereview.chromium.org/1274403003/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/emulator/audio_settings.js:67: * @type !Array<! {name: string, type: string} > On 2015/08/10 21:21:36, michaelpg wrote: > On 2015/08/10 17:42:37, mozartalouis wrote: > > On 2015/08/10 16:46:27, stevenjb wrote: > > > Elim ' ' before { and after } > > > > Pre-submit is complaining about this. I will just ignore it though > > Surrounding the whole type in {} should resolve this. Done. https://codereview.chromium.org/1274403003/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/emulator/audio_settings.js:106: info.type = this.getAudioTypeForName(info.type); On 2015/08/10 21:21:36, michaelpg wrote: > On 2015/08/10 17:42:38, mozartalouis wrote: > > On 2015/08/10 16:46:27, stevenjb wrote: > > > This will modify this.nodes[e.model.index], which I don't think is what you > > > want. The code is also confusing, why are we passing info.type to > > > getAudioTypeForName? > > > > > > If we are passing a different type when sending 'insertAudioNode', we need > to > > > typedef that separately (even if it is mostly the same). > > > > Done. > > The new code actually does the same thing as the old code, just using a > temporary; stevenjb's point is that you don't want to modify |info|. Done.
Patchset #3 (id:80001) has been deleted
https://codereview.chromium.org/1274403003/diff/100001/chrome/browser/resourc... File chrome/browser/resources/chromeos/emulator/audio_settings.js (right): https://codereview.chromium.org/1274403003/diff/100001/chrome/browser/resourc... chrome/browser/resources/chromeos/emulator/audio_settings.js:67: * {@type !Array<!{name: string, type: string}>} Shouldn't this be !Array<!{name: string, type: AudioNodeType}> ? https://codereview.chromium.org/1274403003/diff/100001/chrome/browser/resourc... chrome/browser/resources/chromeos/emulator/audio_settings.js:108: chrome.send('insertAudioNode', [info, this.getAudioTypeForName(info.type)]); I still don't understand why we are passing info.type, which is of type AudioNodeType to getAudioTypeForName() which takes a 'name' parameter which is a string. https://codereview.chromium.org/1274403003/diff/100001/chrome/browser/resourc... chrome/browser/resources/chromeos/emulator/audio_settings.js:124: * of a node. Shouldn't |type| be of type {AudioNodeType} ? https://codereview.chromium.org/1274403003/diff/100001/chrome/browser/resourc... chrome/browser/resources/chromeos/emulator/audio_settings.js:138: * @return {string} The label which represents |name|. Sill unclear. This returns this.nodeTypeOptions[i].type which looks like it should be AudioNodeType, not a label. https://codereview.chromium.org/1274403003/diff/100001/chrome/browser/resourc... chrome/browser/resources/chromeos/emulator/audio_settings.js:162: node.active = nodeList[i]['active']; Now we can do: Object.assign(node, nodeList[i]);
Patchset #2 (id:40001) has been deleted
Ready for review https://codereview.chromium.org/1274403003/diff/100001/chrome/browser/resourc... File chrome/browser/resources/chromeos/emulator/audio_settings.js (right): https://codereview.chromium.org/1274403003/diff/100001/chrome/browser/resourc... chrome/browser/resources/chromeos/emulator/audio_settings.js:67: * {@type !Array<!{name: string, type: string}>} On 2015/08/10 21:47:34, stevenjb wrote: > Shouldn't this be !Array<!{name: string, type: AudioNodeType}> ? michaelpg@ suggested that in order for the pre-submit to to stop complaining, to wrap the whole thing in {} but will remove https://codereview.chromium.org/1274403003/diff/100001/chrome/browser/resourc... chrome/browser/resources/chromeos/emulator/audio_settings.js:108: chrome.send('insertAudioNode', [info, this.getAudioTypeForName(info.type)]); On 2015/08/10 21:47:34, stevenjb wrote: > I still don't understand why we are passing info.type, which is of type > AudioNodeType to getAudioTypeForName() which takes a 'name' parameter which is a > string. You have to pass info.type because with insertAudioNode is called from HTML, |info| doesn't get the enum representation of type meaning if the type it headphone I receive the name but not the type. ex. info.type will return 'Headphones', this.getAudioTypeForName(info.type) will return 'HEADPHONE' which is needed in the C++ to determine the correct enum value https://codereview.chromium.org/1274403003/diff/100001/chrome/browser/resourc... chrome/browser/resources/chromeos/emulator/audio_settings.js:124: * of a node. On 2015/08/10 21:47:34, stevenjb wrote: > Shouldn't |type| be of type {AudioNodeType} ? Done. should i rename them getAudioTypeName getAudioType? to make more sense https://codereview.chromium.org/1274403003/diff/100001/chrome/browser/resourc... chrome/browser/resources/chromeos/emulator/audio_settings.js:138: * @return {string} The label which represents |name|. On 2015/08/10 21:47:34, stevenjb wrote: > Sill unclear. This returns this.nodeTypeOptions[i].type which looks like it > should be AudioNodeType, not a label. Done. https://codereview.chromium.org/1274403003/diff/100001/chrome/browser/resourc... chrome/browser/resources/chromeos/emulator/audio_settings.js:162: node.active = nodeList[i]['active']; On 2015/08/10 21:47:34, stevenjb wrote: > Now we can do: > > Object.assign(node, nodeList[i]); Done.
https://codereview.chromium.org/1274403003/diff/100001/chrome/browser/resourc... File chrome/browser/resources/chromeos/emulator/audio_settings.js (right): https://codereview.chromium.org/1274403003/diff/100001/chrome/browser/resourc... chrome/browser/resources/chromeos/emulator/audio_settings.js:108: chrome.send('insertAudioNode', [info, this.getAudioTypeForName(info.type)]); On 2015/08/10 23:02:07, mozartalouis wrote: > On 2015/08/10 21:47:34, stevenjb wrote: > > I still don't understand why we are passing info.type, which is of type > > AudioNodeType to getAudioTypeForName() which takes a 'name' parameter which is > a > > string. > > You have to pass info.type because with insertAudioNode is called from HTML, > |info| doesn't get the enum representation of type meaning if the type it > headphone I receive the name but not the type. > > ex. info.type will return 'Headphones', > this.getAudioTypeForName(info.type) will return 'HEADPHONE' which is needed in > the C++ to determine the correct enum value This is super confusing. At minimum this needs to be commented in the code, but see my suggestion in the .html comment. https://codereview.chromium.org/1274403003/diff/120001/chrome/browser/resourc... File chrome/browser/resources/chromeos/emulator/audio_settings.html (right): https://codereview.chromium.org/1274403003/diff/120001/chrome/browser/resourc... chrome/browser/resources/chromeos/emulator/audio_settings.html:35: <paper-radio-group selected="{{item.type}}"> If this is causing item.type to become the name instead, that is super confusing. https://codereview.chromium.org/1274403003/diff/120001/chrome/browser/resourc... chrome/browser/resources/chromeos/emulator/audio_settings.html:39: >[[option.name]]</paper-radio-button> Can't we set the value of the radio to be option.type so that {{item.type}} gets set correctly when selected?
It was just an HTML typo that was causing me to do extra :/ https://codereview.chromium.org/1274403003/diff/120001/chrome/browser/resourc... File chrome/browser/resources/chromeos/emulator/audio_settings.html (right): https://codereview.chromium.org/1274403003/diff/120001/chrome/browser/resourc... chrome/browser/resources/chromeos/emulator/audio_settings.html:35: <paper-radio-group selected="{{item.type}}"> On 2015/08/10 23:14:09, stevenjb wrote: > If this is causing item.type to become the name instead, that is super > confusing. I Understand now. the problem was that below it should have been option.type for the name and the actual value as option.name for the display. with that, i can now remove those 2 methods https://codereview.chromium.org/1274403003/diff/120001/chrome/browser/resourc... chrome/browser/resources/chromeos/emulator/audio_settings.html:39: >[[option.name]]</paper-radio-button> On 2015/08/10 23:14:09, stevenjb wrote: > Can't we set the value of the radio to be option.type so that {{item.type}} gets > set correctly when selected? Done.
lgtm https://codereview.chromium.org/1274403003/diff/100001/chrome/browser/resourc... File chrome/browser/resources/chromeos/emulator/audio_settings.js (right): https://codereview.chromium.org/1274403003/diff/100001/chrome/browser/resourc... chrome/browser/resources/chromeos/emulator/audio_settings.js:67: * {@type !Array<!{name: string, type: string}>} On 2015/08/10 21:47:34, stevenjb wrote: > Shouldn't this be !Array<!{name: string, type: AudioNodeType}> ? We've been following the @type {...} convention from the closure docs in most WebUI. This code doesn't include the outer braces, which I don't care too much about right now, but that's what is causing the presubmit warning on this particular line. https://codereview.chromium.org/1274403003/diff/140001/chrome/browser/resourc... File chrome/browser/resources/chromeos/emulator/audio_settings.html (right): https://codereview.chromium.org/1274403003/diff/140001/chrome/browser/resourc... chrome/browser/resources/chromeos/emulator/audio_settings.html:38: <paper-radio-button name="[[option.type]]" Much better! https://codereview.chromium.org/1274403003/diff/140001/chrome/browser/resourc... File chrome/browser/resources/chromeos/emulator/audio_settings.js (right): https://codereview.chromium.org/1274403003/diff/140001/chrome/browser/resourc... chrome/browser/resources/chromeos/emulator/audio_settings.js:130: // create a new audio node and add all the properties from |nodeList[i]| nit: capitalize sentence & end w/ period
Thanks for the cleanup, much more readable now. LGTM https://codereview.chromium.org/1274403003/diff/140001/chrome/browser/resourc... File chrome/browser/resources/chromeos/emulator/audio_settings.html (right): https://codereview.chromium.org/1274403003/diff/140001/chrome/browser/resourc... chrome/browser/resources/chromeos/emulator/audio_settings.html:38: <paper-radio-button name="[[option.type]]" On 2015/08/11 00:25:09, michaelpg wrote: > Much better! +1
The CQ bit was checked by mozartalouis@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org, stevenjb@chromium.org, michaelpg@chromium.org Link to the patchset: https://codereview.chromium.org/1274403003/#ps160001 (title: "Comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1274403003/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1274403003/160001
Message was sent while issue was closed.
Committed patchset #5 (id:160001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/a79e8ad477bb753fbed0bb2e00864419915b44e7 Cr-Commit-Position: refs/heads/master@{#342849} |
