Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1205753002:
Create chrome://device-emulator and add the ability to get a battery percentage and send an updated… (Closed)
|
Created:
5 years, 6 months ago by rfrappier Modified:
5 years, 5 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. |
DescriptionCreate chrome://device-emulator and add the ability to get a battery percentage and send an updated one to the WebUIController
BUG=499097
R=oshima@chromium.org
Committed: https://crrev.com/0a529c99776cf30c0223899cd65862fca6a09cce
Cr-Commit-Position: refs/heads/master@{#336455}
Patch Set 1 #
Total comments: 44
Patch Set 2 : Create chrome://device-emulator and add the ability to get a battery percentage and send an updated… #
Total comments: 9
Patch Set 3 : #
Total comments: 2
Patch Set 4 : #
Total comments: 11
Patch Set 5 : #
Total comments: 14
Patch Set 6 : #Messages
Total messages: 47 (8 generated)
Ready for review
oshima@chromium.org changed reviewers: + michaelpg@chromium.org
please make sure all new files have copy right. michaelpg@, can you review webui part? The UI itself is preliminary, but it still should confirm the chromium style. https://codereview.chromium.org/1205753002/diff/1/chrome/browser/ui/webui/chr... File chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc (right): https://codereview.chromium.org/1205753002/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc:440: return &NewWebUI<chromeos::CryptohomeUI>; Can you enable this only when running on Linux Desktop? You can use https://code.google.com/p/chromium/codesearch#chromium/src/base/sys_info.h&l=118 https://codereview.chromium.org/1205753002/diff/1/chrome/browser/ui/webui/chr... File chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.h (right): https://codereview.chromium.org/1205753002/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.h:10: namespace chromeos { No namespace under chrome/browser. Some files still have chromeos namespace, but it's just for historical reason. https://codereview.chromium.org/1205753002/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.h:16: document the class. https://codereview.chromium.org/1205753002/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.h:22: // content::WebUIMessageHandler // content::WebUIMessageHandler: https://codereview.chromium.org/1205753002/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.h:27: virtual void HandleUpdatePowerMode(const base::ListValue* args); do they have to be virtual? https://codereview.chromium.org/1205753002/diff/1/chrome/browser/ui/webui/chr... File chrome/browser/ui/webui/chromeos/emulator/device_emulator_ui.cc (right): https://codereview.chromium.org/1205753002/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/emulator/device_emulator_ui.cc:24: // Variables. Clean up comments. If you want to write a comment about future plan, use TODO(name): format.
On 2015/06/24 22:33:56, oshima wrote: > please make sure all new files have copy right. (Except the HTML file. HTML files do not have copyright headers for whatever reason.) > > michaelpg@, can you review webui part? The UI itself is preliminary, but it > still should confirm the chromium style. > > https://codereview.chromium.org/1205753002/diff/1/chrome/browser/ui/webui/chr... > File chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc (right): > > https://codereview.chromium.org/1205753002/diff/1/chrome/browser/ui/webui/chr... > chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc:440: return > &NewWebUI<chromeos::CryptohomeUI>; > Can you enable this only when running on Linux Desktop? You can use > > https://code.google.com/p/chromium/codesearch#chromium/src/base/sys_info.h&l=118 > > https://codereview.chromium.org/1205753002/diff/1/chrome/browser/ui/webui/chr... > File chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.h > (right): > > https://codereview.chromium.org/1205753002/diff/1/chrome/browser/ui/webui/chr... > chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.h:10: > namespace chromeos { > No namespace under chrome/browser. > > Some files still have chromeos namespace, but it's just for historical reason. > > https://codereview.chromium.org/1205753002/diff/1/chrome/browser/ui/webui/chr... > chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.h:16: > document the class. > > https://codereview.chromium.org/1205753002/diff/1/chrome/browser/ui/webui/chr... > chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.h:22: > // content::WebUIMessageHandler > // content::WebUIMessageHandler: > > https://codereview.chromium.org/1205753002/diff/1/chrome/browser/ui/webui/chr... > chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.h:27: > virtual void HandleUpdatePowerMode(const base::ListValue* args); > do they have to be virtual? > > https://codereview.chromium.org/1205753002/diff/1/chrome/browser/ui/webui/chr... > File chrome/browser/ui/webui/chromeos/emulator/device_emulator_ui.cc (right): > > https://codereview.chromium.org/1205753002/diff/1/chrome/browser/ui/webui/chr... > chrome/browser/ui/webui/chromeos/emulator/device_emulator_ui.cc:24: // > Variables. > Clean up comments. If you want to write a comment about future plan, use > TODO(name): format.
https://codereview.chromium.org/1205753002/diff/1/chrome/browser/ui/webui/chr... File chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc (right): https://codereview.chromium.org/1205753002/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc:440: return &NewWebUI<chromeos::CryptohomeUI>; On 2015/06/24 22:33:55, oshima wrote: > Can you enable this only when running on Linux Desktop? You can use > > https://code.google.com/p/chromium/codesearch#chromium/src/base/sys_info.h&l=118 So I basically just need to check if (!IsRunningOnChromeOS)? https://codereview.chromium.org/1205753002/diff/1/chrome/browser/ui/webui/chr... File chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.h (right): https://codereview.chromium.org/1205753002/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.h:10: namespace chromeos { On 2015/06/24 22:33:55, oshima wrote: > No namespace under chrome/browser. > > Some files still have chromeos namespace, but it's just for historical reason. Done. https://codereview.chromium.org/1205753002/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.h:16: On 2015/06/24 22:33:55, oshima wrote: > document the class. Done. https://codereview.chromium.org/1205753002/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.h:22: // content::WebUIMessageHandler On 2015/06/24 22:33:55, oshima wrote: > // content::WebUIMessageHandler: Done. https://codereview.chromium.org/1205753002/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.h:27: virtual void HandleUpdatePowerMode(const base::ListValue* args); On 2015/06/24 22:33:55, oshima wrote: > do they have to be virtual? No, they don't. I just had it like that because I tried to replicate an example. https://codereview.chromium.org/1205753002/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.h:27: virtual void HandleUpdatePowerMode(const base::ListValue* args); On 2015/06/24 22:33:55, oshima wrote: > do they have to be virtual? Done. https://codereview.chromium.org/1205753002/diff/1/chrome/browser/ui/webui/chr... File chrome/browser/ui/webui/chromeos/emulator/device_emulator_ui.cc (right): https://codereview.chromium.org/1205753002/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/emulator/device_emulator_ui.cc:24: // Variables. On 2015/06/24 22:33:55, oshima wrote: > Clean up comments. If you want to write a comment about future plan, use > TODO(name): format. Done.
https://codereview.chromium.org/1205753002/diff/1/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/emulator/device_emulator.html (right): https://codereview.chromium.org/1205753002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/emulator/device_emulator.html:5: <!-- <title i18n-content="deviceEmulatorTitle"></title> --> Don't comment things out. Just use placeholder strings and add i18n-stuff later. https://codereview.chromium.org/1205753002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/emulator/device_emulator.html:14: <div class="container"> throughout: don't use "class" until you need it (when you add more CSS) https://codereview.chromium.org/1205753002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/emulator/device_emulator.html:27: <input id="batteryPercentSlider" type="range" min="0" max="100"> Per the style guide, element IDs use-hyphen-form. https://www.chromium.org/developers/web-development-style-guide https://codereview.chromium.org/1205753002/diff/1/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/emulator/device_emulator.js (right): https://codereview.chromium.org/1205753002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/emulator/device_emulator.js:4: function returnBatteryInfo(percent) { Annotate for Closure (we don't closure-compile yet, but we plan to): https://developers.google.com/closure/compiler/docs/js-for-compiler e.g. /** * Updates the UI with the battery status. * @param {number} percent Battery percentage (out of 100). */ https://codereview.chromium.org/1205753002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/emulator/device_emulator.js:4: function returnBatteryInfo(percent) { setBatteryInfo makes more sense I think https://codereview.chromium.org/1205753002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/emulator/device_emulator.js:18: chrome.send('updateBatteryInfo', [parseInt(slider.value)]); [slider.valueAsNumber] https://codereview.chromium.org/1205753002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/emulator/device_emulator.js:24: var percent = (text.value !== '') ? parseInt(text.value) : 0; parseInt will return NaN if given something unparseable: var percent = parseInt(text.value) || 0; Or, because |text| is <input type="number"> and the default step is 1: var percent = text.valueAsNumber; if (isNaN(percent)) { percent = 0; text.valueAsNumber = 0; } Or something like that. https://codereview.chromium.org/1205753002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/emulator/device_emulator.js:42: slider.addEventListener('input', onBatterySliderChange); I'd suggest listening to 'change' instead, so we don't generate hundreds of IPC calls by wiggling the mouse. https://codereview.chromium.org/1205753002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/emulator/device_emulator.js:50: onBatterySliderChange: onBatterySliderChange, Why export on*Change? https://codereview.chromium.org/1205753002/diff/1/chrome/browser/ui/webui/chr... File chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc (right): https://codereview.chromium.org/1205753002/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc:46: base::FundamentalValue(rand() % 101)); #include <stdlib.h> https://codereview.chromium.org/1205753002/diff/1/chrome/browser/ui/webui/chr... File chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.h (right): https://codereview.chromium.org/1205753002/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.h:12: extern const char kBatteryPercentFunction[]; I would say "don't add needless externs; if you must add them, incorporate them in a namespace like device_emulator, but prefer adding these constants to an anonymous namespace in the implementation file if they're only needed in that file". But oshima didn't say that, so... oshima, I thought we avoided doing this, is this actually ok? https://codereview.chromium.org/1205753002/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.h:25: virtual void HandleRequestBatteryInfo(const base::ListValue* args); add fwd declaration: namespace base { class ListValue; } https://codereview.chromium.org/1205753002/diff/1/chrome/browser/ui/webui/chr... File chrome/browser/ui/webui/chromeos/emulator/device_emulator_ui.cc (right): https://codereview.chromium.org/1205753002/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/emulator/device_emulator_ui.cc:18: content::WebUIDataSource* CreateDeviceEmulatorUIDataSource() { Put functions local to your implementation inside an anonymous namespace https://codereview.chromium.org/1205753002/diff/1/chrome/chrome_browser_ui.gypi File chrome/chrome_browser_ui.gypi (right): https://codereview.chromium.org/1205753002/diff/1/chrome/chrome_browser_ui.gy... chrome/chrome_browser_ui.gypi:313: 'browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc', alphabetize https://codereview.chromium.org/1205753002/diff/1/chrome/common/url_constants.h File chrome/common/url_constants.h (right): https://codereview.chromium.org/1205753002/diff/1/chrome/common/url_constants... chrome/common/url_constants.h:262: extern const char kChromeUIDeviceEmulatorURL[]; move URL to the URL section (around line 100)
https://codereview.chromium.org/1205753002/diff/1/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/emulator/device_emulator.html (right): https://codereview.chromium.org/1205753002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/emulator/device_emulator.html:5: <!-- <title i18n-content="deviceEmulatorTitle"></title> --> On 2015/06/24 23:33:48, michaelpg wrote: > Don't comment things out. Just use placeholder strings and add i18n-stuff later. Done. https://codereview.chromium.org/1205753002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/emulator/device_emulator.html:14: <div class="container"> On 2015/06/24 23:33:48, michaelpg wrote: > throughout: don't use "class" until you need it (when you add more CSS) Done. https://codereview.chromium.org/1205753002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/emulator/device_emulator.html:27: <input id="batteryPercentSlider" type="range" min="0" max="100"> On 2015/06/24 23:33:48, michaelpg wrote: > Per the style guide, element IDs use-hyphen-form. > > https://www.chromium.org/developers/web-development-style-guide Done. https://codereview.chromium.org/1205753002/diff/1/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/emulator/device_emulator.js (right): https://codereview.chromium.org/1205753002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/emulator/device_emulator.js:4: function returnBatteryInfo(percent) { On 2015/06/24 23:33:48, michaelpg wrote: > setBatteryInfo makes more sense I think Done. https://codereview.chromium.org/1205753002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/emulator/device_emulator.js:4: function returnBatteryInfo(percent) { On 2015/06/24 23:33:48, michaelpg wrote: > Annotate for Closure (we don't closure-compile yet, but we plan to): > > https://developers.google.com/closure/compiler/docs/js-for-compiler > > e.g. > /** > * Updates the UI with the battery status. > * @param {number} percent Battery percentage (out of 100). > */ Done. https://codereview.chromium.org/1205753002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/emulator/device_emulator.js:18: chrome.send('updateBatteryInfo', [parseInt(slider.value)]); On 2015/06/24 23:33:48, michaelpg wrote: > [slider.valueAsNumber] Done. https://codereview.chromium.org/1205753002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/emulator/device_emulator.js:24: var percent = (text.value !== '') ? parseInt(text.value) : 0; On 2015/06/24 23:33:48, michaelpg wrote: > parseInt will return NaN if given something unparseable: > > var percent = parseInt(text.value) || 0; > > Or, because |text| is <input type="number"> and the default step is 1: > > var percent = text.valueAsNumber; > if (isNaN(percent)) { > percent = 0; > text.valueAsNumber = 0; > } > > Or something like that. Done. https://codereview.chromium.org/1205753002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/emulator/device_emulator.js:42: slider.addEventListener('input', onBatterySliderChange); On 2015/06/24 23:33:48, michaelpg wrote: > I'd suggest listening to 'change' instead, so we don't generate hundreds of IPC > calls by wiggling the mouse. Done. https://codereview.chromium.org/1205753002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/emulator/device_emulator.js:42: slider.addEventListener('input', onBatterySliderChange); On 2015/06/24 23:33:48, michaelpg wrote: > I'd suggest listening to 'change' instead, so we don't generate hundreds of IPC > calls by wiggling the mouse. How about I listen for 'change' for sending the percent to ChromeOS, and listen to 'input' so the user can see which percent they are setting while the slider is moving? https://codereview.chromium.org/1205753002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/emulator/device_emulator.js:50: onBatterySliderChange: onBatterySliderChange, On 2015/06/24 23:33:48, michaelpg wrote: > Why export on*Change? It's not necessary. Not sure why I put them there, actually. Will remove https://codereview.chromium.org/1205753002/diff/1/chrome/browser/ui/webui/chr... File chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc (right): https://codereview.chromium.org/1205753002/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc:46: base::FundamentalValue(rand() % 101)); On 2015/06/24 23:33:48, michaelpg wrote: > #include <stdlib.h> Done. https://codereview.chromium.org/1205753002/diff/1/chrome/browser/ui/webui/chr... File chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.h (right): https://codereview.chromium.org/1205753002/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.h:25: virtual void HandleRequestBatteryInfo(const base::ListValue* args); On 2015/06/24 23:33:48, michaelpg wrote: > add fwd declaration: > namespace base { > class ListValue; > } Done. https://codereview.chromium.org/1205753002/diff/1/chrome/browser/ui/webui/chr... File chrome/browser/ui/webui/chromeos/emulator/device_emulator_ui.cc (right): https://codereview.chromium.org/1205753002/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/emulator/device_emulator_ui.cc:18: content::WebUIDataSource* CreateDeviceEmulatorUIDataSource() { On 2015/06/24 23:33:49, michaelpg wrote: > Put functions local to your implementation inside an anonymous namespace Done. https://codereview.chromium.org/1205753002/diff/1/chrome/chrome_browser_ui.gypi File chrome/chrome_browser_ui.gypi (right): https://codereview.chromium.org/1205753002/diff/1/chrome/chrome_browser_ui.gy... chrome/chrome_browser_ui.gypi:313: 'browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc', On 2015/06/24 23:33:49, michaelpg wrote: > alphabetize Done. https://codereview.chromium.org/1205753002/diff/1/chrome/common/url_constants.h File chrome/common/url_constants.h (right): https://codereview.chromium.org/1205753002/diff/1/chrome/common/url_constants... chrome/common/url_constants.h:262: extern const char kChromeUIDeviceEmulatorURL[]; On 2015/06/24 23:33:49, michaelpg wrote: > move URL to the URL section (around line 100) Done.
Can you upload new patch? On 2015/06/25 17:09:21, rfrappier wrote: > https://codereview.chromium.org/1205753002/diff/1/chrome/browser/resources/ch... > File chrome/browser/resources/chromeos/emulator/device_emulator.html (right): > > https://codereview.chromium.org/1205753002/diff/1/chrome/browser/resources/ch... > chrome/browser/resources/chromeos/emulator/device_emulator.html:5: <!-- <title > i18n-content="deviceEmulatorTitle"></title> --> > On 2015/06/24 23:33:48, michaelpg wrote: > > Don't comment things out. Just use placeholder strings and add i18n-stuff > later. > > Done. > > https://codereview.chromium.org/1205753002/diff/1/chrome/browser/resources/ch... > chrome/browser/resources/chromeos/emulator/device_emulator.html:14: <div > class="container"> > On 2015/06/24 23:33:48, michaelpg wrote: > > throughout: don't use "class" until you need it (when you add more CSS) > > Done. > > https://codereview.chromium.org/1205753002/diff/1/chrome/browser/resources/ch... > chrome/browser/resources/chromeos/emulator/device_emulator.html:27: <input > id="batteryPercentSlider" type="range" min="0" max="100"> > On 2015/06/24 23:33:48, michaelpg wrote: > > Per the style guide, element IDs use-hyphen-form. > > > > https://www.chromium.org/developers/web-development-style-guide > > Done. > > https://codereview.chromium.org/1205753002/diff/1/chrome/browser/resources/ch... > File chrome/browser/resources/chromeos/emulator/device_emulator.js (right): > > https://codereview.chromium.org/1205753002/diff/1/chrome/browser/resources/ch... > chrome/browser/resources/chromeos/emulator/device_emulator.js:4: function > returnBatteryInfo(percent) { > On 2015/06/24 23:33:48, michaelpg wrote: > > setBatteryInfo makes more sense I think > > Done. > > https://codereview.chromium.org/1205753002/diff/1/chrome/browser/resources/ch... > chrome/browser/resources/chromeos/emulator/device_emulator.js:4: function > returnBatteryInfo(percent) { > On 2015/06/24 23:33:48, michaelpg wrote: > > Annotate for Closure (we don't closure-compile yet, but we plan to): > > > > https://developers.google.com/closure/compiler/docs/js-for-compiler > > > > e.g. > > /** > > * Updates the UI with the battery status. > > * @param {number} percent Battery percentage (out of 100). > > */ > > Done. > > https://codereview.chromium.org/1205753002/diff/1/chrome/browser/resources/ch... > chrome/browser/resources/chromeos/emulator/device_emulator.js:18: > chrome.send('updateBatteryInfo', [parseInt(slider.value)]); > On 2015/06/24 23:33:48, michaelpg wrote: > > [slider.valueAsNumber] > > Done. > > https://codereview.chromium.org/1205753002/diff/1/chrome/browser/resources/ch... > chrome/browser/resources/chromeos/emulator/device_emulator.js:24: var percent = > (text.value !== '') ? parseInt(text.value) : 0; > On 2015/06/24 23:33:48, michaelpg wrote: > > parseInt will return NaN if given something unparseable: > > > > var percent = parseInt(text.value) || 0; > > > > Or, because |text| is <input type="number"> and the default step is 1: > > > > var percent = text.valueAsNumber; > > if (isNaN(percent)) { > > percent = 0; > > text.valueAsNumber = 0; > > } > > > > Or something like that. > > Done. > > https://codereview.chromium.org/1205753002/diff/1/chrome/browser/resources/ch... > chrome/browser/resources/chromeos/emulator/device_emulator.js:42: > slider.addEventListener('input', onBatterySliderChange); > On 2015/06/24 23:33:48, michaelpg wrote: > > I'd suggest listening to 'change' instead, so we don't generate hundreds of > IPC > > calls by wiggling the mouse. > > Done. > > https://codereview.chromium.org/1205753002/diff/1/chrome/browser/resources/ch... > chrome/browser/resources/chromeos/emulator/device_emulator.js:42: > slider.addEventListener('input', onBatterySliderChange); > On 2015/06/24 23:33:48, michaelpg wrote: > > I'd suggest listening to 'change' instead, so we don't generate hundreds of > IPC > > calls by wiggling the mouse. > > How about I listen for 'change' for sending the percent to ChromeOS, and listen > to 'input' so the user can see which percent they are setting while the slider > is moving? > > https://codereview.chromium.org/1205753002/diff/1/chrome/browser/resources/ch... > chrome/browser/resources/chromeos/emulator/device_emulator.js:50: > onBatterySliderChange: onBatterySliderChange, > On 2015/06/24 23:33:48, michaelpg wrote: > > Why export on*Change? > > It's not necessary. Not sure why I put them there, actually. Will remove > > https://codereview.chromium.org/1205753002/diff/1/chrome/browser/ui/webui/chr... > File > chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc > (right): > > https://codereview.chromium.org/1205753002/diff/1/chrome/browser/ui/webui/chr... > chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc:46: > base::FundamentalValue(rand() % 101)); > On 2015/06/24 23:33:48, michaelpg wrote: > > #include <stdlib.h> > > Done. > > https://codereview.chromium.org/1205753002/diff/1/chrome/browser/ui/webui/chr... > File chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.h > (right): > > https://codereview.chromium.org/1205753002/diff/1/chrome/browser/ui/webui/chr... > chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.h:25: > virtual void HandleRequestBatteryInfo(const base::ListValue* args); > On 2015/06/24 23:33:48, michaelpg wrote: > > add fwd declaration: > > namespace base { > > class ListValue; > > } > > Done. > > https://codereview.chromium.org/1205753002/diff/1/chrome/browser/ui/webui/chr... > File chrome/browser/ui/webui/chromeos/emulator/device_emulator_ui.cc (right): > > https://codereview.chromium.org/1205753002/diff/1/chrome/browser/ui/webui/chr... > chrome/browser/ui/webui/chromeos/emulator/device_emulator_ui.cc:18: > content::WebUIDataSource* CreateDeviceEmulatorUIDataSource() { > On 2015/06/24 23:33:49, michaelpg wrote: > > Put functions local to your implementation inside an anonymous namespace > > Done. > > https://codereview.chromium.org/1205753002/diff/1/chrome/chrome_browser_ui.gypi > File chrome/chrome_browser_ui.gypi (right): > > https://codereview.chromium.org/1205753002/diff/1/chrome/chrome_browser_ui.gy... > chrome/chrome_browser_ui.gypi:313: > 'browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc', > On 2015/06/24 23:33:49, michaelpg wrote: > > alphabetize > > Done. > > https://codereview.chromium.org/1205753002/diff/1/chrome/common/url_constants.h > File chrome/common/url_constants.h (right): > > https://codereview.chromium.org/1205753002/diff/1/chrome/common/url_constants... > chrome/common/url_constants.h:262: extern const char > kChromeUIDeviceEmulatorURL[]; > On 2015/06/24 23:33:49, michaelpg wrote: > > move URL to the URL section (around line 100) > > Done.
Okay, but there's one of michaelpg@'s comments that I have yet to address.
Looking good. I defer to oshima on the externs question. https://codereview.chromium.org/1205753002/diff/1/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/emulator/device_emulator.js (right): https://codereview.chromium.org/1205753002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/emulator/device_emulator.js:42: slider.addEventListener('input', onBatterySliderChange); On 2015/06/25 17:09:21, rfrappier wrote: > On 2015/06/24 23:33:48, michaelpg wrote: > > I'd suggest listening to 'change' instead, so we don't generate hundreds of > IPC > > calls by wiggling the mouse. > > How about I listen for 'change' for sending the percent to ChromeOS, and listen > to 'input' so the user can see which percent they are setting while the slider > is moving? Good idea. https://codereview.chromium.org/1205753002/diff/20001/chrome/browser/resource... File chrome/browser/resources/chromeos/emulator/device_emulator.js (right): https://codereview.chromium.org/1205753002/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/emulator/device_emulator.js:9: * Updates the UI with the battery status nit: end comments/sentences with a period https://codereview.chromium.org/1205753002/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/emulator/device_emulator.js:23: * @param {event} event Contains information about the event which was fired. {Event}
On 2015/06/25 17:51:59, rfrappier wrote: > Okay, but there's one of michaelpg@'s comments that I have yet to address. Reviewers would expect the CL will have the patch that has the changes you said "Done". You can wait until CL is ready for upload, or you can just say "I'm still working".
looks good. just a few more comments https://codereview.chromium.org/1205753002/diff/20001/chrome/browser/browser_... File chrome/browser/browser_resources.grd (right): https://codereview.chromium.org/1205753002/diff/20001/chrome/browser/browser_... chrome/browser/browser_resources.grd:405: nit: remove empty new line https://codereview.chromium.org/1205753002/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.h (right): https://codereview.chromium.org/1205753002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.h:13: extern const char kBatteryPercentFunctionJSCallback[]; do you need to expose these constants? Or can they be in .cc file?
https://codereview.chromium.org/1205753002/diff/20001/chrome/browser/resource... File chrome/browser/resources/chromeos/emulator/device_emulator.js (right): https://codereview.chromium.org/1205753002/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/emulator/device_emulator.js:9: * Updates the UI with the battery status On 2015/06/25 18:18:19, michaelpg wrote: > nit: end comments/sentences with a period Done. https://codereview.chromium.org/1205753002/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/emulator/device_emulator.js:23: * @param {event} event Contains information about the event which was fired. On 2015/06/25 18:18:19, michaelpg wrote: > {Event} Done. https://codereview.chromium.org/1205753002/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.h (right): https://codereview.chromium.org/1205753002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.h:13: extern const char kBatteryPercentFunctionJSCallback[]; On 2015/06/25 20:02:15, oshima wrote: > do you need to expose these constants? Or can they be in .cc file? They can be in the .cc file. I should probably put them in an anonymous namespace, correct?
https://codereview.chromium.org/1205753002/diff/20001/chrome/browser/browser_... File chrome/browser/browser_resources.grd (right): https://codereview.chromium.org/1205753002/diff/20001/chrome/browser/browser_... chrome/browser/browser_resources.grd:405: On 2015/06/25 20:02:15, oshima wrote: > nit: remove empty new line Done.
On 2015/06/25 21:43:24, rfrappier wrote: > https://codereview.chromium.org/1205753002/diff/20001/chrome/browser/resource... > File chrome/browser/resources/chromeos/emulator/device_emulator.js (right): > > https://codereview.chromium.org/1205753002/diff/20001/chrome/browser/resource... > chrome/browser/resources/chromeos/emulator/device_emulator.js:9: * Updates the > UI with the battery status > On 2015/06/25 18:18:19, michaelpg wrote: > > nit: end comments/sentences with a period > > Done. > > https://codereview.chromium.org/1205753002/diff/20001/chrome/browser/resource... > chrome/browser/resources/chromeos/emulator/device_emulator.js:23: * @param > {event} event Contains information about the event which was fired. > On 2015/06/25 18:18:19, michaelpg wrote: > > {Event} > > Done. > > https://codereview.chromium.org/1205753002/diff/20001/chrome/browser/ui/webui... > File chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.h > (right): > > https://codereview.chromium.org/1205753002/diff/20001/chrome/browser/ui/webui... > chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.h:13: > extern const char kBatteryPercentFunctionJSCallback[]; > On 2015/06/25 20:02:15, oshima wrote: > > do you need to expose these constants? Or can they be in .cc file? > > They can be in the .cc file. I should probably put them in an anonymous > namespace, correct? yes
Just uploaded a new version. I added a couple UI elements to the webpage. There is one control in particular that I am debating on how to handle: the select box for the Power Source. Currently, I created an enumeration in the DeviceEmulatorMessageHandler which is meant to hold the values for each <option> element. I then make those values available to the JavaScript in the DeviceEmulatorUI, load them after the DOM is loaded, and create the select options. The purpose of having the enum is so that the values for the <option> elements are not hard-coded in the HTML. Let me know what you both think of this approach. https://codereview.chromium.org/1205753002/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.h (right): https://codereview.chromium.org/1205753002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.h:13: extern const char kBatteryPercentFunctionJSCallback[]; On 2015/06/25 20:02:15, oshima wrote: > do you need to expose these constants? Or can they be in .cc file? Done.
It's better to avoid adding more features during review process. Reviewers has to review new code again and will slow down the process. https://codereview.chromium.org/1205753002/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.h (right): https://codereview.chromium.org/1205753002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.h:24: }; What is this for? Can you document this?
https://codereview.chromium.org/1205753002/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.h (right): https://codereview.chromium.org/1205753002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.h:24: }; On 2015/06/25 23:52:26, oshima wrote: > What is this for? Can you document this? I've updated this to use the enumeration declared in src/third_party/cros_system_api/dbus/power_manager/power_supply_properties.proto. I also updated the language to be more consistent (i.e. changing "Battery Power" to "Disconnected")
c++ lgtm if you addressed comments below. https://codereview.chromium.org/1205753002/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc (right): https://codereview.chromium.org/1205753002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc:7: #include <stdlib.h> you don't need this, do you? https://codereview.chromium.org/1205753002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc:11: #include "chromeos/dbus/power_manager/power_supply_properties.pb.h" and this? https://codereview.chromium.org/1205753002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc:16: // Define the name of the callback functions that will be used by JavaScript . at the end
c++ lgtm if you addressed comments below. https://codereview.chromium.org/1205753002/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc (right): https://codereview.chromium.org/1205753002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc:7: #include <stdlib.h> you don't need this, do you? https://codereview.chromium.org/1205753002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc:11: #include "chromeos/dbus/power_manager/power_supply_properties.pb.h" and this? https://codereview.chromium.org/1205753002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc:16: // Define the name of the callback functions that will be used by JavaScript . at the end
forgot to mention. please wait for michaelpg@'s approval for webui code.
https://codereview.chromium.org/1205753002/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc (right): https://codereview.chromium.org/1205753002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc:7: #include <stdlib.h> On 2015/06/26 16:22:07, oshima wrote: > you don't need this, do you? It worked without it, but comment #6 indicated that it should be there. Should I remove it? https://codereview.chromium.org/1205753002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc:11: #include "chromeos/dbus/power_manager/power_supply_properties.pb.h" On 2015/06/26 16:22:07, oshima wrote: > and this? This will be used later for handling the power supply change. I can remove it for now if you'd prefer. https://codereview.chromium.org/1205753002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc:16: // Define the name of the callback functions that will be used by JavaScript On 2015/06/26 16:22:07, oshima wrote: > . at the end Done.
https://codereview.chromium.org/1205753002/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc (right): https://codereview.chromium.org/1205753002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc:7: #include <stdlib.h> On 2015/06/26 16:33:08, rfrappier wrote: > On 2015/06/26 16:22:07, oshima wrote: > > you don't need this, do you? > > It worked without it, but comment #6 indicated that it should be there. Should I > remove it? oh, I missed rand() below. Yes, this should be here. https://codereview.chromium.org/1205753002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc:11: #include "chromeos/dbus/power_manager/power_supply_properties.pb.h" On 2015/06/26 16:33:08, rfrappier wrote: > On 2015/06/26 16:22:07, oshima wrote: > > and this? > > This will be used later for handling the power supply change. I can remove it > for now if you'd prefer. Please add when necessary. It can confuse reviewers, and you may end up having different design.
https://codereview.chromium.org/1205753002/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc (right): https://codereview.chromium.org/1205753002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc:7: #include <stdlib.h> On 2015/06/26 17:19:49, oshima wrote: > On 2015/06/26 16:33:08, rfrappier wrote: > > On 2015/06/26 16:22:07, oshima wrote: > > > you don't need this, do you? > > > > It worked without it, but comment #6 indicated that it should be there. Should > I > > remove it? > > oh, I missed rand() below. Yes, this should be here. Done. https://codereview.chromium.org/1205753002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc:11: #include "chromeos/dbus/power_manager/power_supply_properties.pb.h" On 2015/06/26 17:19:49, oshima wrote: > On 2015/06/26 16:33:08, rfrappier wrote: > > On 2015/06/26 16:22:07, oshima wrote: > > > and this? > > > > This will be used later for handling the power supply change. I can remove it > > for now if you'd prefer. > > Please add when necessary. It can confuse reviewers, and you may end up having > different design. Okay, will remove for now. https://codereview.chromium.org/1205753002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc:11: #include "chromeos/dbus/power_manager/power_supply_properties.pb.h" On 2015/06/26 17:19:49, oshima wrote: > On 2015/06/26 16:33:08, rfrappier wrote: > > On 2015/06/26 16:22:07, oshima wrote: > > > and this? > > > > This will be used later for handling the power supply change. I can remove it > > for now if you'd prefer. > > Please add when necessary. It can confuse reviewers, and you may end up having > different design. Done.
I only looked at the html/js/css this time. You'll need to add an OWNER for WebUI -- try stevenjb@. https://codereview.chromium.org/1205753002/diff/80001/chrome/browser/resource... File chrome/browser/resources/chromeos/emulator/device_emulator.css (right): https://codereview.chromium.org/1205753002/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/emulator/device_emulator.css:24: float: left; redundant https://codereview.chromium.org/1205753002/diff/80001/chrome/browser/resource... File chrome/browser/resources/chromeos/emulator/device_emulator.html (right): https://codereview.chromium.org/1205753002/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/emulator/device_emulator.html:52: <input id="time-until-empty-radio" type="radio" name="time-until-radio"> Stick to 80-column limit: <input id="time-until-empty-radio" type="radio" name="time-until-radio"> https://codereview.chromium.org/1205753002/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/emulator/device_emulator.html:59: <input id="time-until-full-radio" type="radio" name="time-until-radio"> and here https://codereview.chromium.org/1205753002/diff/80001/chrome/browser/resource... File chrome/browser/resources/chromeos/emulator/device_emulator.js (right): https://codereview.chromium.org/1205753002/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/emulator/device_emulator.js:17: text.value = percent.toString(); just use slider.valueAsNumber/text.valueAsNumber https://codereview.chromium.org/1205753002/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/emulator/device_emulator.js:22: * is released. Updates the ChromeOS UI. uber nit: Chrome OS is two words! :-P https://codereview.chromium.org/1205753002/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/emulator/device_emulator.js:75: var disconnectedOptionValue = loadTimeData.getString('DISCONNECTED'); use camelCase for localized string keys: 'disconnected', 'usbPower' https://codereview.chromium.org/1205753002/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/emulator/device_emulator.js:112: text.addEventListener('input', onBatteryTextChange); name should match event ('input' is probably fine)
On 2015/06/26 17:46:00, michaelpg wrote: > I only looked at the html/js/css this time. > > You'll need to add an OWNER for WebUI -- try stevenjb@. Isn't c/b/ui/webui/chromeos/OWNERS sufficient? > > https://codereview.chromium.org/1205753002/diff/80001/chrome/browser/resource... > File chrome/browser/resources/chromeos/emulator/device_emulator.css (right): > > https://codereview.chromium.org/1205753002/diff/80001/chrome/browser/resource... > chrome/browser/resources/chromeos/emulator/device_emulator.css:24: float: left; > redundant > > https://codereview.chromium.org/1205753002/diff/80001/chrome/browser/resource... > File chrome/browser/resources/chromeos/emulator/device_emulator.html (right): > > https://codereview.chromium.org/1205753002/diff/80001/chrome/browser/resource... > chrome/browser/resources/chromeos/emulator/device_emulator.html:52: <input > id="time-until-empty-radio" type="radio" name="time-until-radio"> > Stick to 80-column limit: > > <input id="time-until-empty-radio" type="radio" > name="time-until-radio"> > > https://codereview.chromium.org/1205753002/diff/80001/chrome/browser/resource... > chrome/browser/resources/chromeos/emulator/device_emulator.html:59: <input > id="time-until-full-radio" type="radio" name="time-until-radio"> > and here > > https://codereview.chromium.org/1205753002/diff/80001/chrome/browser/resource... > File chrome/browser/resources/chromeos/emulator/device_emulator.js (right): > > https://codereview.chromium.org/1205753002/diff/80001/chrome/browser/resource... > chrome/browser/resources/chromeos/emulator/device_emulator.js:17: text.value = > percent.toString(); > just use slider.valueAsNumber/text.valueAsNumber > > https://codereview.chromium.org/1205753002/diff/80001/chrome/browser/resource... > chrome/browser/resources/chromeos/emulator/device_emulator.js:22: * is released. > Updates the ChromeOS UI. > uber nit: Chrome OS is two words! :-P > > https://codereview.chromium.org/1205753002/diff/80001/chrome/browser/resource... > chrome/browser/resources/chromeos/emulator/device_emulator.js:75: var > disconnectedOptionValue = loadTimeData.getString('DISCONNECTED'); > use camelCase for localized string keys: 'disconnected', 'usbPower' > > https://codereview.chromium.org/1205753002/diff/80001/chrome/browser/resource... > chrome/browser/resources/chromeos/emulator/device_emulator.js:112: > text.addEventListener('input', onBatteryTextChange); > name should match event ('input' is probably fine)
On 2015/06/26 18:11:33, oshima wrote: > On 2015/06/26 17:46:00, michaelpg wrote: > > I only looked at the html/js/css this time. > > > > You'll need to add an OWNER for WebUI -- try stevenjb@. > > Isn't c/b/ui/webui/chromeos/OWNERS sufficient? > > > > > > https://codereview.chromium.org/1205753002/diff/80001/chrome/browser/resource... > > File chrome/browser/resources/chromeos/emulator/device_emulator.css (right): > > > > > https://codereview.chromium.org/1205753002/diff/80001/chrome/browser/resource... > > chrome/browser/resources/chromeos/emulator/device_emulator.css:24: float: > left; > > redundant > > > > > https://codereview.chromium.org/1205753002/diff/80001/chrome/browser/resource... > > File chrome/browser/resources/chromeos/emulator/device_emulator.html (right): > > > > > https://codereview.chromium.org/1205753002/diff/80001/chrome/browser/resource... > > chrome/browser/resources/chromeos/emulator/device_emulator.html:52: <input > > id="time-until-empty-radio" type="radio" name="time-until-radio"> > > Stick to 80-column limit: > > > > <input id="time-until-empty-radio" type="radio" > > name="time-until-radio"> > > > > > https://codereview.chromium.org/1205753002/diff/80001/chrome/browser/resource... > > chrome/browser/resources/chromeos/emulator/device_emulator.html:59: <input > > id="time-until-full-radio" type="radio" name="time-until-radio"> > > and here > > > > > https://codereview.chromium.org/1205753002/diff/80001/chrome/browser/resource... > > File chrome/browser/resources/chromeos/emulator/device_emulator.js (right): > > > > > https://codereview.chromium.org/1205753002/diff/80001/chrome/browser/resource... > > chrome/browser/resources/chromeos/emulator/device_emulator.js:17: text.value = > > percent.toString(); > > just use slider.valueAsNumber/text.valueAsNumber > > > > > https://codereview.chromium.org/1205753002/diff/80001/chrome/browser/resource... > > chrome/browser/resources/chromeos/emulator/device_emulator.js:22: * is > released. > > Updates the ChromeOS UI. > > uber nit: Chrome OS is two words! :-P > > > > > https://codereview.chromium.org/1205753002/diff/80001/chrome/browser/resource... > > chrome/browser/resources/chromeos/emulator/device_emulator.js:75: var > > disconnectedOptionValue = loadTimeData.getString('DISCONNECTED'); > > use camelCase for localized string keys: 'disconnected', 'usbPower' > > > > > https://codereview.chromium.org/1205753002/diff/80001/chrome/browser/resource... > > chrome/browser/resources/chromeos/emulator/device_emulator.js:112: > > text.addEventListener('input', onBatteryTextChange); > > name should match event ('input' is probably fine)
On 2015/06/26 18:15:54, michaelpg wrote: > On 2015/06/26 18:11:33, oshima wrote: > > On 2015/06/26 17:46:00, michaelpg wrote: > > > I only looked at the html/js/css this time. > > > > > > You'll need to add an OWNER for WebUI -- try stevenjb@. > > > > Isn't c/b/ui/webui/chromeos/OWNERS sufficient? yes (along with c/b/resources/chromeos) -- but you and i aren't in either :-( > > > > > > > > > > > https://codereview.chromium.org/1205753002/diff/80001/chrome/browser/resource... > > > File chrome/browser/resources/chromeos/emulator/device_emulator.css (right): > > > > > > > > > https://codereview.chromium.org/1205753002/diff/80001/chrome/browser/resource... > > > chrome/browser/resources/chromeos/emulator/device_emulator.css:24: float: > > left; > > > redundant > > > > > > > > > https://codereview.chromium.org/1205753002/diff/80001/chrome/browser/resource... > > > File chrome/browser/resources/chromeos/emulator/device_emulator.html > (right): > > > > > > > > > https://codereview.chromium.org/1205753002/diff/80001/chrome/browser/resource... > > > chrome/browser/resources/chromeos/emulator/device_emulator.html:52: <input > > > id="time-until-empty-radio" type="radio" name="time-until-radio"> > > > Stick to 80-column limit: > > > > > > <input id="time-until-empty-radio" type="radio" > > > name="time-until-radio"> > > > > > > > > > https://codereview.chromium.org/1205753002/diff/80001/chrome/browser/resource... > > > chrome/browser/resources/chromeos/emulator/device_emulator.html:59: <input > > > id="time-until-full-radio" type="radio" name="time-until-radio"> > > > and here > > > > > > > > > https://codereview.chromium.org/1205753002/diff/80001/chrome/browser/resource... > > > File chrome/browser/resources/chromeos/emulator/device_emulator.js (right): > > > > > > > > > https://codereview.chromium.org/1205753002/diff/80001/chrome/browser/resource... > > > chrome/browser/resources/chromeos/emulator/device_emulator.js:17: text.value > = > > > percent.toString(); > > > just use slider.valueAsNumber/text.valueAsNumber > > > > > > > > > https://codereview.chromium.org/1205753002/diff/80001/chrome/browser/resource... > > > chrome/browser/resources/chromeos/emulator/device_emulator.js:22: * is > > released. > > > Updates the ChromeOS UI. > > > uber nit: Chrome OS is two words! :-P > > > > > > > > > https://codereview.chromium.org/1205753002/diff/80001/chrome/browser/resource... > > > chrome/browser/resources/chromeos/emulator/device_emulator.js:75: var > > > disconnectedOptionValue = loadTimeData.getString('DISCONNECTED'); > > > use camelCase for localized string keys: 'disconnected', 'usbPower' > > > > > > > > > https://codereview.chromium.org/1205753002/diff/80001/chrome/browser/resource... > > > chrome/browser/resources/chromeos/emulator/device_emulator.js:112: > > > text.addEventListener('input', onBatteryTextChange); > > > name should match event ('input' is probably fine)
On Fri, Jun 26, 2015 at 11:16 AM, <michaelpg@chromium.org> wrote: > On 2015/06/26 18:15:54, michaelpg wrote: > >> On 2015/06/26 18:11:33, oshima wrote: >> > On 2015/06/26 17:46:00, michaelpg wrote: >> > > I only looked at the html/js/css this time. >> > > >> > > You'll need to add an OWNER for WebUI -- try stevenjb@. >> > >> > Isn't c/b/ui/webui/chromeos/OWNERS sufficient? >> > > yes (along with c/b/resources/chromeos) -- but you and i aren't in either > :-( I'm not qualified, but you should be in there now. I'll send CL. > > > > >> > > >> > > >> > >> > > > https://codereview.chromium.org/1205753002/diff/80001/chrome/browser/resource... > >> > > File chrome/browser/resources/chromeos/emulator/device_emulator.css >> > (right): > >> > > >> > > >> > >> > > > https://codereview.chromium.org/1205753002/diff/80001/chrome/browser/resource... > >> > > chrome/browser/resources/chromeos/emulator/device_emulator.css:24: >> float: >> > left; >> > > redundant >> > > >> > > >> > >> > > > https://codereview.chromium.org/1205753002/diff/80001/chrome/browser/resource... > >> > > File chrome/browser/resources/chromeos/emulator/device_emulator.html >> (right): >> > > >> > > >> > >> > > > https://codereview.chromium.org/1205753002/diff/80001/chrome/browser/resource... > >> > > chrome/browser/resources/chromeos/emulator/device_emulator.html:52: >> <input >> > > id="time-until-empty-radio" type="radio" name="time-until-radio"> >> > > Stick to 80-column limit: >> > > >> > > <input id="time-until-empty-radio" type="radio" >> > > name="time-until-radio"> >> > > >> > > >> > >> > > > https://codereview.chromium.org/1205753002/diff/80001/chrome/browser/resource... > >> > > chrome/browser/resources/chromeos/emulator/device_emulator.html:59: >> <input >> > > id="time-until-full-radio" type="radio" name="time-until-radio"> >> > > and here >> > > >> > > >> > >> > > > https://codereview.chromium.org/1205753002/diff/80001/chrome/browser/resource... > >> > > File chrome/browser/resources/chromeos/emulator/device_emulator.js >> > (right): > >> > > >> > > >> > >> > > > https://codereview.chromium.org/1205753002/diff/80001/chrome/browser/resource... > >> > > chrome/browser/resources/chromeos/emulator/device_emulator.js:17: >> > text.value > >> = >> > > percent.toString(); >> > > just use slider.valueAsNumber/text.valueAsNumber >> > > >> > > >> > >> > > > https://codereview.chromium.org/1205753002/diff/80001/chrome/browser/resource... > >> > > chrome/browser/resources/chromeos/emulator/device_emulator.js:22: * is >> > released. >> > > Updates the ChromeOS UI. >> > > uber nit: Chrome OS is two words! :-P >> > > >> > > >> > >> > > > https://codereview.chromium.org/1205753002/diff/80001/chrome/browser/resource... > >> > > chrome/browser/resources/chromeos/emulator/device_emulator.js:75: var >> > > disconnectedOptionValue = loadTimeData.getString('DISCONNECTED'); >> > > use camelCase for localized string keys: 'disconnected', 'usbPower' >> > > >> > > >> > >> > > > https://codereview.chromium.org/1205753002/diff/80001/chrome/browser/resource... > >> > > chrome/browser/resources/chromeos/emulator/device_emulator.js:112: >> > > text.addEventListener('input', onBatteryTextChange); >> > > name should match event ('input' is probably fine) >> > > > > https://codereview.chromium.org/1205753002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Just did! On Fri, Jun 26, 2015 at 11:40 AM, oshima <oshima@chromium.org> wrote: > > > On Fri, Jun 26, 2015 at 11:16 AM, <michaelpg@chromium.org> wrote: > >> On 2015/06/26 18:15:54, michaelpg wrote: >> >>> On 2015/06/26 18:11:33, oshima wrote: >>> > On 2015/06/26 17:46:00, michaelpg wrote: >>> > > I only looked at the html/js/css this time. >>> > > >>> > > You'll need to add an OWNER for WebUI -- try stevenjb@. >>> > >>> > Isn't c/b/ui/webui/chromeos/OWNERS sufficient? >>> >> >> yes (along with c/b/resources/chromeos) -- but you and i aren't in either >> :-( > > > I'm not qualified, but you should be in there now. I'll send CL. > > >> >> >> > >>> > > >>> > > >>> > >>> >> >> >> https://codereview.chromium.org/1205753002/diff/80001/chrome/browser/resource... >> >>> > > File chrome/browser/resources/chromeos/emulator/device_emulator.css >>> >> (right): >> >>> > > >>> > > >>> > >>> >> >> >> https://codereview.chromium.org/1205753002/diff/80001/chrome/browser/resource... >> >>> > > chrome/browser/resources/chromeos/emulator/device_emulator.css:24: >>> float: >>> > left; >>> > > redundant >>> > > >>> > > >>> > >>> >> >> >> https://codereview.chromium.org/1205753002/diff/80001/chrome/browser/resource... >> >>> > > File chrome/browser/resources/chromeos/emulator/device_emulator.html >>> (right): >>> > > >>> > > >>> > >>> >> >> >> https://codereview.chromium.org/1205753002/diff/80001/chrome/browser/resource... >> >>> > > chrome/browser/resources/chromeos/emulator/device_emulator.html:52: >>> <input >>> > > id="time-until-empty-radio" type="radio" name="time-until-radio"> >>> > > Stick to 80-column limit: >>> > > >>> > > <input id="time-until-empty-radio" type="radio" >>> > > name="time-until-radio"> >>> > > >>> > > >>> > >>> >> >> >> https://codereview.chromium.org/1205753002/diff/80001/chrome/browser/resource... >> >>> > > chrome/browser/resources/chromeos/emulator/device_emulator.html:59: >>> <input >>> > > id="time-until-full-radio" type="radio" name="time-until-radio"> >>> > > and here >>> > > >>> > > >>> > >>> >> >> >> https://codereview.chromium.org/1205753002/diff/80001/chrome/browser/resource... >> >>> > > File chrome/browser/resources/chromeos/emulator/device_emulator.js >>> >> (right): >> >>> > > >>> > > >>> > >>> >> >> >> https://codereview.chromium.org/1205753002/diff/80001/chrome/browser/resource... >> >>> > > chrome/browser/resources/chromeos/emulator/device_emulator.js:17: >>> >> text.value >> >>> = >>> > > percent.toString(); >>> > > just use slider.valueAsNumber/text.valueAsNumber >>> > > >>> > > >>> > >>> >> >> >> https://codereview.chromium.org/1205753002/diff/80001/chrome/browser/resource... >> >>> > > chrome/browser/resources/chromeos/emulator/device_emulator.js:22: * >>> is >>> > released. >>> > > Updates the ChromeOS UI. >>> > > uber nit: Chrome OS is two words! :-P >>> > > >>> > > >>> > >>> >> >> >> https://codereview.chromium.org/1205753002/diff/80001/chrome/browser/resource... >> >>> > > chrome/browser/resources/chromeos/emulator/device_emulator.js:75: var >>> > > disconnectedOptionValue = loadTimeData.getString('DISCONNECTED'); >>> > > use camelCase for localized string keys: 'disconnected', 'usbPower' >>> > > >>> > > >>> > >>> >> >> >> https://codereview.chromium.org/1205753002/diff/80001/chrome/browser/resource... >> >>> > > chrome/browser/resources/chromeos/emulator/device_emulator.js:112: >>> > > text.addEventListener('input', onBatteryTextChange); >>> > > name should match event ('input' is probably fine) >>> >> >> >> >> https://codereview.chromium.org/1205753002/ >> > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/1205753002/diff/80001/chrome/browser/resource... File chrome/browser/resources/chromeos/emulator/device_emulator.css (right): https://codereview.chromium.org/1205753002/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/emulator/device_emulator.css:24: float: left; On 2015/06/26 17:45:59, michaelpg wrote: > redundant Done. https://codereview.chromium.org/1205753002/diff/80001/chrome/browser/resource... File chrome/browser/resources/chromeos/emulator/device_emulator.html (right): https://codereview.chromium.org/1205753002/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/emulator/device_emulator.html:52: <input id="time-until-empty-radio" type="radio" name="time-until-radio"> On 2015/06/26 17:45:59, michaelpg wrote: > Stick to 80-column limit: > > <input id="time-until-empty-radio" type="radio" > name="time-until-radio"> Done. https://codereview.chromium.org/1205753002/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/emulator/device_emulator.html:59: <input id="time-until-full-radio" type="radio" name="time-until-radio"> On 2015/06/26 17:45:59, michaelpg wrote: > and here Done. https://codereview.chromium.org/1205753002/diff/80001/chrome/browser/resource... File chrome/browser/resources/chromeos/emulator/device_emulator.js (right): https://codereview.chromium.org/1205753002/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/emulator/device_emulator.js:17: text.value = percent.toString(); On 2015/06/26 17:45:59, michaelpg wrote: > just use slider.valueAsNumber/text.valueAsNumber Done. https://codereview.chromium.org/1205753002/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/emulator/device_emulator.js:22: * is released. Updates the ChromeOS UI. On 2015/06/26 17:46:00, michaelpg wrote: > uber nit: Chrome OS is two words! :-P Done. https://codereview.chromium.org/1205753002/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/emulator/device_emulator.js:75: var disconnectedOptionValue = loadTimeData.getString('DISCONNECTED'); On 2015/06/26 17:46:00, michaelpg wrote: > use camelCase for localized string keys: 'disconnected', 'usbPower' Done. https://codereview.chromium.org/1205753002/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/emulator/device_emulator.js:112: text.addEventListener('input', onBatteryTextChange); On 2015/06/26 17:45:59, michaelpg wrote: > name should match event ('input' is probably fine) Done.
Cool, lgtm! Looking forward to being able to use this. I just added myself as an OWNER so you should be good to go.
The CQ bit was checked by rfrappier@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from oshima@chromium.org Link to the patchset: https://codereview.chromium.org/1205753002/#ps100001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1205753002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
rfrappier@google.com changed reviewers: + xiyuan@chromium.org
xiyuan@, I have a couple files that need to be reviewed by you before I can land this patch. The following files are the ones I need you to have a look at: chrome/browser/browser_resources.grd chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc
lgtm
The CQ bit was checked by rfrappier@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1205753002/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/0a529c99776cf30c0223899cd65862fca6a09cce Cr-Commit-Position: refs/heads/master@{#336455}
Message was sent while issue was closed.
Patchset #7 (id:120001) has been deleted
Message was sent while issue was closed.
Patchset #7 (id:140001) has been deleted |
