Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1256303002:
Creating Ui for Battery State Option in Chrome Os Emulator (Closed)
|
Created:
5 years, 4 months ago by mozartalouis Modified:
5 years, 4 months ago CC:
chromium-reviews, 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. |
DescriptionCreating Ui for Battery State Option in Chrome Os Emulator
BUG=514276
R=oshima@chromium.org
Committed: https://crrev.com/c70ee70b0311ebd4f3c14a64aa2082d2ac766ba8
Cr-Commit-Position: refs/heads/master@{#341203}
Patch Set 1 : Added radio group for battery state with functionality #
Total comments: 13
Patch Set 2 : fixed nits #
Total comments: 4
Patch Set 3 : Updated nits and fixed device_emulator undefined #
Total comments: 4
Patch Set 4 : Updated method to use scoped_ptr #
Total comments: 10
Patch Set 5 : Changed scoped_ptr to dictionary #
Total comments: 4
Patch Set 6 : Fixed nits #
Total comments: 9
Patch Set 7 : Fixed JS Nits #
Total comments: 4
Patch Set 8 : Indents #
Messages
Total messages: 28 (6 generated)
mozartalouis@google.com changed reviewers: + afakhry@chromium.org, michaelpg@chromium.org
Ready for review
https://codereview.chromium.org/1256303002/diff/1/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/emulator/battery_settings.html (right): https://codereview.chromium.org/1256303002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/emulator/battery_settings.html:19: id="battery-percent-text" type="number" min="0" max="100"> min and max aren't supported, are they? https://codereview.chromium.org/1256303002/diff/1/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/emulator/battery_settings.js (right): https://codereview.chromium.org/1256303002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/emulator/battery_settings.js:17: * A string representing the value of an nit: "of a" https://codereview.chromium.org/1256303002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/emulator/battery_settings.js:25: * An array representing the battery state options. fix indent (-1) https://codereview.chromium.org/1256303002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/emulator/battery_settings.js:37: * A string representing the value of an nit: "of a" https://codereview.chromium.org/1256303002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/emulator/battery_settings.js:97: for (var i = 0; i < this.batteryStateOptions.length; i++) { var index = this.batteryStateOptions.indexOf(state); https://codereview.chromium.org/1256303002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/emulator/battery_settings.js:105: chrome.send('updateBatteryState', [index]); indent
Ready for review https://codereview.chromium.org/1256303002/diff/1/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/emulator/battery_settings.html (right): https://codereview.chromium.org/1256303002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/emulator/battery_settings.html:19: id="battery-percent-text" type="number" min="0" max="100"> On 2015/07/27 20:46:22, michaelpg wrote: > min and max aren't supported, are they? Done. https://codereview.chromium.org/1256303002/diff/1/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/emulator/battery_settings.js (right): https://codereview.chromium.org/1256303002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/emulator/battery_settings.js:17: * A string representing the value of an On 2015/07/27 20:46:22, michaelpg wrote: > nit: "of a" Done. https://codereview.chromium.org/1256303002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/emulator/battery_settings.js:25: * An array representing the battery state options. On 2015/07/27 20:46:22, michaelpg wrote: > fix indent (-1) Done. https://codereview.chromium.org/1256303002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/emulator/battery_settings.js:37: * A string representing the value of an On 2015/07/27 20:46:22, michaelpg wrote: > nit: "of a" Done. https://codereview.chromium.org/1256303002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/emulator/battery_settings.js:97: for (var i = 0; i < this.batteryStateOptions.length; i++) { On 2015/07/27 20:46:22, michaelpg wrote: > var index = this.batteryStateOptions.indexOf(state); Done. https://codereview.chromium.org/1256303002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/emulator/battery_settings.js:105: chrome.send('updateBatteryState', [index]); On 2015/07/27 20:46:22, michaelpg wrote: > indent Done.
https://codereview.chromium.org/1256303002/diff/1/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/emulator/battery_settings.js (right): https://codereview.chromium.org/1256303002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/emulator/battery_settings.js:97: for (var i = 0; i < this.batteryStateOptions.length; i++) { On 2015/07/27 23:58:41, mozartalouis wrote: > On 2015/07/27 20:46:22, michaelpg wrote: > > var index = this.batteryStateOptions.indexOf(state); > > Done. Take another look at how indexOf works -- you don't need the loop. https://codereview.chromium.org/1256303002/diff/20001/chrome/browser/resource... File chrome/browser/resources/chromeos/emulator/battery_settings.js (right): https://codereview.chromium.org/1256303002/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/emulator/battery_settings.js:25: * An array representing the battery state options nit: add the period back
https://codereview.chromium.org/1256303002/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc (right): https://codereview.chromium.org/1256303002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc:85: args->GetInteger(0, &battery_state); you should check if it was successful, and update the value only when it's updated, like line 76 above. If this should never fail, adding DCHECK is also ok, but you should initialize the battery_state to avoid using uninitialized value.
Ready for Review https://codereview.chromium.org/1256303002/diff/20001/chrome/browser/resource... File chrome/browser/resources/chromeos/emulator/battery_settings.js (right): https://codereview.chromium.org/1256303002/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/emulator/battery_settings.js:25: * An array representing the battery state options On 2015/07/28 00:05:39, michaelpg wrote: > nit: add the period back Done. https://codereview.chromium.org/1256303002/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc (right): https://codereview.chromium.org/1256303002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc:85: args->GetInteger(0, &battery_state); On 2015/07/28 16:53:16, oshima wrote: > you should check if it was successful, and update the value only when it's > updated, like line 76 above. > > If this should never fail, adding DCHECK is also ok, but you should initialize > the battery_state to avoid using uninitialized value. Done.
https://codereview.chromium.org/1256303002/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc (right): https://codereview.chromium.org/1256303002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc:139: base::FundamentalValue(proto.battery_state())), does this compile? https://codereview.chromium.org/1256303002/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/emulator/device_emulator_ui.cc (left): https://codereview.chromium.org/1256303002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/emulator/device_emulator_ui.cc:48: revert this file?
https://codereview.chromium.org/1256303002/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc (right): https://codereview.chromium.org/1256303002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc:139: base::FundamentalValue(proto.battery_state())), On 2015/07/29 00:50:48, oshima wrote: > does this compile? Done. https://codereview.chromium.org/1256303002/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/emulator/device_emulator_ui.cc (left): https://codereview.chromium.org/1256303002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/emulator/device_emulator_ui.cc:48: On 2015/07/29 00:50:48, oshima wrote: > revert this file? Done.
The latest patch should fix everything with the battery and should be fully functional. let me know about any nits so I can fix them quickly
https://codereview.chromium.org/1256303002/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc (right): https://codereview.chromium.org/1256303002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc:88: if (args->GetInteger(0, &battery_state)) nit: you need {} in this case https://codereview.chromium.org/1256303002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc:103: props.set_external_power( nit: indent and {} https://codereview.chromium.org/1256303002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc:137: // when this function is called. This comment isn't much useful. You can omit it. https://codereview.chromium.org/1256303002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc:138: scoped_ptr<base::ListValue> power_properties(new base::ListValue); you can just create an object on stack like base::ListValue power_properties; https://codereview.chromium.org/1256303002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc:149: *power_properties); I thought you wanted to use CallJavascriptFunction( const std::string& function_name, const std::vector<const base::Value*>& args) ? If you want to just pass as one argument (which is probably better IMO), using dictionary with the same key names with proto buffers will make the code more readable, instead of using list, and access using index.
https://codereview.chromium.org/1256303002/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc (right): https://codereview.chromium.org/1256303002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc:88: if (args->GetInteger(0, &battery_state)) On 2015/07/29 21:25:12, oshima wrote: > nit: you need {} in this case Done. https://codereview.chromium.org/1256303002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc:103: props.set_external_power( On 2015/07/29 21:25:12, oshima wrote: > nit: indent and {} Done. https://codereview.chromium.org/1256303002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc:137: // when this function is called. On 2015/07/29 21:25:13, oshima wrote: > This comment isn't much useful. You can omit it. Done. https://codereview.chromium.org/1256303002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc:138: scoped_ptr<base::ListValue> power_properties(new base::ListValue); On 2015/07/29 21:25:13, oshima wrote: > you can just create an object on stack like > > base::ListValue power_properties; Done. https://codereview.chromium.org/1256303002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc:149: *power_properties); On 2015/07/29 21:25:13, oshima wrote: > I thought you wanted to use > > CallJavascriptFunction( > const std::string& function_name, > const std::vector<const base::Value*>& args) > > ? > > If you want to just pass as one argument (which is probably better IMO), using > dictionary with the same key names with proto buffers will make the code more > readable, instead of using list, and access using index. Done.
lgtm my bits if you address following comments. Please make sure you get approval from michaelpg@. https://codereview.chromium.org/1256303002/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc (right): https://codereview.chromium.org/1256303002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc:73: fake_power_manager_client_->props(); move this copy to if block. same for the rest. https://codereview.chromium.org/1256303002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc:138: power_properties.SetInteger("percent", proto.battery_percent()); can you use the same name as proto buffer? (battery_percent, battey_state etc)
Ready for Review michaelpg@
https://codereview.chromium.org/1256303002/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc (right): https://codereview.chromium.org/1256303002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc:73: fake_power_manager_client_->props(); On 2015/07/29 22:40:59, oshima wrote: > move this copy to if block. same for the rest. Done. https://codereview.chromium.org/1256303002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc:138: power_properties.SetInteger("percent", proto.battery_percent()); On 2015/07/29 22:40:59, oshima wrote: > can you use the same name as proto buffer? > (battery_percent, battey_state etc) Done.
lgtm w/ formatting nits https://codereview.chromium.org/1256303002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/chromeos/emulator/battery_settings.js (right): https://codereview.chromium.org/1256303002/diff/100001/chrome/browser/resourc... chrome/browser/resources/chromeos/emulator/battery_settings.js:35: 'Not Present']; }, nit: indent (preferably to align with 'Full') https://codereview.chromium.org/1256303002/diff/100001/chrome/browser/resourc... chrome/browser/resources/chromeos/emulator/battery_settings.js:88: chrome.send('updateBatteryPercent', [parseInt(percent)]); indent https://codereview.chromium.org/1256303002/diff/100001/chrome/browser/resourc... chrome/browser/resources/chromeos/emulator/battery_settings.js:93: if (oldState != undefined) { This check is to ignore default values set by paper elements, right? Please be sure to test that each of these properties actually gets set on load (as opposed to being set by updatePowerProperties). For example, checking "oldState != undefined" is unnecessary if a <paper-radio-group> starts out as selected == undefined, which (at first glance) is what it looks like. https://codereview.chromium.org/1256303002/diff/100001/chrome/browser/resourc... chrome/browser/resources/chromeos/emulator/battery_settings.js:122: this.batteryStateOptions[power_properties.battery_state]; indent 4 spaces https://codereview.chromium.org/1256303002/diff/100001/chrome/browser/resourc... chrome/browser/resources/chromeos/emulator/battery_settings.js:124: this.externalPowerOptions[power_properties.external_power]; same
https://codereview.chromium.org/1256303002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/chromeos/emulator/battery_settings.js (right): https://codereview.chromium.org/1256303002/diff/100001/chrome/browser/resourc... chrome/browser/resources/chromeos/emulator/battery_settings.js:35: 'Not Present']; }, On 2015/07/30 03:02:09, michaelpg (OOO til Mon.) wrote: > nit: indent (preferably to align with 'Full') Done. https://codereview.chromium.org/1256303002/diff/100001/chrome/browser/resourc... chrome/browser/resources/chromeos/emulator/battery_settings.js:93: if (oldState != undefined) { On 2015/07/30 03:02:09, michaelpg (OOO til Mon.) wrote: > This check is to ignore default values set by paper elements, right? Please be > sure to test that each of these properties actually gets set on load (as opposed > to being set by updatePowerProperties). > > For example, checking "oldState != undefined" is unnecessary if a > <paper-radio-group> starts out as selected == undefined, which (at first glance) > is what it looks like. Done. https://codereview.chromium.org/1256303002/diff/100001/chrome/browser/resourc... chrome/browser/resources/chromeos/emulator/battery_settings.js:122: this.batteryStateOptions[power_properties.battery_state]; On 2015/07/30 03:02:09, michaelpg (OOO til Mon.) wrote: > indent 4 spaces Done. https://codereview.chromium.org/1256303002/diff/100001/chrome/browser/resourc... chrome/browser/resources/chromeos/emulator/battery_settings.js:124: this.externalPowerOptions[power_properties.external_power]; On 2015/07/30 03:02:09, michaelpg (OOO til Mon.) wrote: > same Done.
The CQ bit was checked by mozartalouis@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from oshima@chromium.org, michaelpg@chromium.org Link to the patchset: https://codereview.chromium.org/1256303002/#ps120001 (title: "Fixed JS Nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1256303002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1256303002/120001
https://codereview.chromium.org/1256303002/diff/120001/chrome/browser/resourc... File chrome/browser/resources/chromeos/emulator/battery_settings.js (right): https://codereview.chromium.org/1256303002/diff/120001/chrome/browser/resourc... chrome/browser/resources/chromeos/emulator/battery_settings.js:88: chrome.send('updateBatteryPercent', [parseInt(percent)]); indent 2 spaces https://codereview.chromium.org/1256303002/diff/120001/chrome/browser/resourc... chrome/browser/resources/chromeos/emulator/battery_settings.js:95: chrome.send('updateBatteryState', [index]); ditto
The CQ bit was unchecked by mozartalouis@google.com
https://codereview.chromium.org/1256303002/diff/120001/chrome/browser/resourc... File chrome/browser/resources/chromeos/emulator/battery_settings.js (right): https://codereview.chromium.org/1256303002/diff/120001/chrome/browser/resourc... chrome/browser/resources/chromeos/emulator/battery_settings.js:88: chrome.send('updateBatteryPercent', [parseInt(percent)]); On 2015/07/30 21:24:31, oshima wrote: > indent 2 spaces Done. https://codereview.chromium.org/1256303002/diff/120001/chrome/browser/resourc... chrome/browser/resources/chromeos/emulator/battery_settings.js:95: chrome.send('updateBatteryState', [index]); On 2015/07/30 21:24:31, oshima wrote: > ditto Done.
The CQ bit was checked by mozartalouis@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from oshima@chromium.org, michaelpg@chromium.org Link to the patchset: https://codereview.chromium.org/1256303002/#ps140001 (title: "Indents")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1256303002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1256303002/140001
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/c70ee70b0311ebd4f3c14a64aa2082d2ac766ba8 Cr-Commit-Position: refs/heads/master@{#341203} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
