Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2853113004:
chromeos: Add settings to control power management prefs. (Closed)
|
Created:
3 years, 7 months ago by Daniel Erat Modified:
3 years, 6 months ago CC:
chromium-reviews, arv+watch_chromium.org, oshima+watch_chromium.org, dbeam+watch-settings_chromium.org, srahim+watch_chromium.org, stevenjb+watch-md-settings_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Descriptionchromeos: Add settings to control power management prefs.
Add settings allowing users to control the idle and
lid-closed power management behavior.
BUG=633455
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2853113004
Cr-Commit-Position: refs/heads/master@{#480902}
Committed: https://chromium.googlesource.com/chromium/src/+/b4400a28e45066f77cc87adc60247396f4b3875a
Patch Set 1 #Patch Set 2 : re-add some code #
Total comments: 20
Patch Set 3 : address review comments and display more lid-closed actions #
Total comments: 20
Patch Set 4 : address more review comments #Patch Set 5 : hide lid setting when lid not present #Patch Set 6 : add c++ and js tests #
Total comments: 38
Patch Set 7 : apply review comments (js tests fail) #Patch Set 8 : correct a typo and use promises to fix js tests #Patch Set 9 : fix async call #
Total comments: 8
Patch Set 10 : address comments (a11y checks fail, though) #Patch Set 11 : switch back from aria-labelledby to aria-label #Messages
Total messages: 45 (26 generated)
Description was changed from ========== chromeos: Add settings to control power management prefs. Add settings allowing users to control the idle and lid-closed power management behavior. BUG=633455 ========== to ========== chromeos: Add settings to control power management prefs. Add settings allowing users to control the idle and lid-closed power management behavior. BUG=633455 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
derat@chromium.org changed reviewers: + michaelpg@chromium.org
just wanted to get a basic sanity check of the approach that i'm taking here before i bring this through UI review and write tests for it. no rush... https://codereview.chromium.org/2853113004/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/device_page/power.js (right): https://codereview.chromium.org/2853113004/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/device_page/power.js:176: var behavior = /** @type {settings.IdleBehavior} */ are these casts bogus, or okay? i'm not even sure if i need the parseInt calls... https://codereview.chromium.org/2853113004/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/chromeos/device_power_handler.cc (right): https://codereview.chromium.org/2853113004/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/chromeos/device_power_handler.cc:265: // unmanaged case. let me know if i'm being appropriately or inappropriately lazy here. :-P i could update the JS side to support displaying additional actions (sign out, shut down), but i don't want to let users choose those, so i'd probably only include them in the <select> when the setting is managed. that might be even more confusing...
Yeah, this looks fine, LMK when you want a full review. I went ahead and mentioned a few style nits anyway. https://codereview.chromium.org/2853113004/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/device_page/power.js (right): https://codereview.chromium.org/2853113004/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/device_page/power.js:176: var behavior = /** @type {settings.IdleBehavior} */ On 2017/05/02 00:58:42, Daniel Erat wrote: > are these casts bogus, or okay? i'm not even sure if i need the parseInt > calls... I think this is fine. parseInt makes sense. The casts are probably necessary for closure. https://codereview.chromium.org/2853113004/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/device_page/power.js:177: (parseInt(this.$$('#idleSelect').value, 10)); nit: this.$.idleSelect: https://www.polymer-project.org/1.0/docs/devguide/local-dom#node-finding https://codereview.chromium.org/2853113004/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/device_page/power.js:184: (parseInt(this.$$('#lidClosedSelect').value, 10)); this.$.foo https://codereview.chromium.org/2853113004/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/device_page/power.js:211: this.$$('#lidClosedSelect').value = settings.lidClosedBehavior; this.$.foo x2 https://codereview.chromium.org/2853113004/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/chromeos/device_power_handler.cc (right): https://codereview.chromium.org/2853113004/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/chromeos/device_power_handler.cc:84: pref_change_registrar_.reset(new PrefChangeRegistrar); base::MakeUnique? https://codereview.chromium.org/2853113004/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/chromeos/device_power_handler.cc:246: prefs_->GetInteger(prefs::kPowerAcIdleAction) == could you unpack this into at least 1 if statement, so it's easier to modify in the future? https://codereview.chromium.org/2853113004/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/chromeos/device_power_handler.cc:265: // unmanaged case. On 2017/05/02 00:58:42, Daniel Erat wrote: > let me know if i'm being appropriately or inappropriately lazy here. :-P > > i could update the JS side to support displaying additional actions (sign out, > shut down), but i don't want to let users choose those, so i'd probably only > include them in the <select> when the setting is managed. that might be even > more confusing... Up to you. This may confuse debugging efforts. "I set the policy to shut down on close, but this enrolled device shows Sleep, that must be why the battery drains too fast!" https://codereview.chromium.org/2853113004/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/chromeos/device_power_handler.cc:278: lid_closed_managed == last_lid_closed_managed_) style guide mandates { } around if body, since the condition is >1 line https://codereview.chromium.org/2853113004/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/chromeos/device_power_handler.h (right): https://codereview.chromium.org/2853113004/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/chromeos/device_power_handler.h:45: DISPLAY_OFF_SLEEP = 0, Do these have to be defined in the .h file? (versus, I guess, forward declaring the enum class, defining it in .cc and setting the default values in the constructor... is that normal?)
thanks for all the help and comments here; no rush in looking at this again. https://codereview.chromium.org/2853113004/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/device_page/power.js (right): https://codereview.chromium.org/2853113004/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/device_page/power.js:176: var behavior = /** @type {settings.IdleBehavior} */ On 2017/05/04 17:36:26, michaelpg wrote: > On 2017/05/02 00:58:42, Daniel Erat wrote: > > are these casts bogus, or okay? i'm not even sure if i need the parseInt > > calls... > > I think this is fine. parseInt makes sense. The casts are probably necessary for > closure. Acknowledged. https://codereview.chromium.org/2853113004/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/device_page/power.js:177: (parseInt(this.$$('#idleSelect').value, 10)); On 2017/05/04 17:36:26, michaelpg wrote: > nit: this.$.idleSelect: > https://www.polymer-project.org/1.0/docs/devguide/local-dom#node-finding Done. https://codereview.chromium.org/2853113004/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/device_page/power.js:184: (parseInt(this.$$('#lidClosedSelect').value, 10)); On 2017/05/04 17:36:26, michaelpg wrote: > this.$.foo Done. https://codereview.chromium.org/2853113004/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/device_page/power.js:211: this.$$('#lidClosedSelect').value = settings.lidClosedBehavior; On 2017/05/04 17:36:26, michaelpg wrote: > this.$.foo x2 Done. https://codereview.chromium.org/2853113004/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/chromeos/device_power_handler.cc (right): https://codereview.chromium.org/2853113004/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/chromeos/device_power_handler.cc:84: pref_change_registrar_.reset(new PrefChangeRegistrar); On 2017/05/04 17:36:26, michaelpg wrote: > base::MakeUnique? Done. https://codereview.chromium.org/2853113004/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/chromeos/device_power_handler.cc:246: prefs_->GetInteger(prefs::kPowerAcIdleAction) == On 2017/05/04 17:36:26, michaelpg wrote: > could you unpack this into at least 1 if statement, so it's easier to modify in > the future? Done. https://codereview.chromium.org/2853113004/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/chromeos/device_power_handler.cc:265: // unmanaged case. On 2017/05/04 17:36:26, michaelpg wrote: > On 2017/05/02 00:58:42, Daniel Erat wrote: > > let me know if i'm being appropriately or inappropriately lazy here. :-P > > > > i could update the JS side to support displaying additional actions (sign out, > > shut down), but i don't want to let users choose those, so i'd probably only > > include them in the <select> when the setting is managed. that might be even > > more confusing... > > Up to you. This may confuse debugging efforts. "I set the policy to shut down on > close, but this enrolled device shows Sleep, that must be why the battery drains > too fast!" as mentioned on the .h file, i've changed things around so we show the actual lid-closed behavior and use "Other" for unhandled idle behaviors. https://codereview.chromium.org/2853113004/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/chromeos/device_power_handler.cc:278: lid_closed_managed == last_lid_closed_managed_) On 2017/05/04 17:36:26, michaelpg wrote: > style guide mandates { } around if body, since the condition is >1 line not mandated by either the google or chromium c++ guides, and i think we use both forms in chromium. after squinting at both, i don't think the brackets help with readability here, so i've left it like this. https://codereview.chromium.org/2853113004/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/chromeos/device_power_handler.h (right): https://codereview.chromium.org/2853113004/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/chromeos/device_power_handler.h:45: DISPLAY_OFF_SLEEP = 0, On 2017/05/04 17:36:26, michaelpg wrote: > Do these have to be defined in the .h file? (versus, I guess, forward declaring > the enum class, defining it in .cc and setting the default values in the > constructor... is that normal?) i've shuffled this around a bit. LidClosedBehavior is no more; i'm just using the chromeos::PowerPolicyController::Action enum in its place (it didn't make sense to add an "other" option vs. just being more specific and adding the remaining two options, "shut down" and "sign out"). IdleBehavior could presumably be defined in the .cc file as you suggest, but as you also mentioned, i'd need to initialize the member there as well. i'd prefer to leave it like this (particularly since we refer to the enum name from javascript) unless you feel strongly.
Looking good. Some small things. https://codereview.chromium.org/2853113004/diff/40001/chrome/app/settings_str... File chrome/app/settings_strings.grdp (right): https://codereview.chromium.org/2853113004/diff/40001/chrome/app/settings_str... chrome/app/settings_strings.grdp:3069: Turn off display and sleep "that turns off the screen and puts the device to sleep" https://codereview.chromium.org/2853113004/diff/40001/chrome/app/settings_str... chrome/app/settings_strings.grdp:3071: <message name="IDS_SETTINGS_POWER_IDLE_DISPLAY_OFF_STAY_AWAKE" desc="In Device Settings > Power, menu item for idle behavior."> "that turns off the screen but prevents the device from sleeping" https://codereview.chromium.org/2853113004/diff/40001/chrome/app/settings_str... chrome/app/settings_strings.grdp:3074: <message name="IDS_SETTINGS_POWER_IDLE_DISPLAY_ON" desc="In Device Settings > Power, menu item for idle behavior."> "that prevents the screen from turning off" https://codereview.chromium.org/2853113004/diff/40001/chrome/app/settings_str... chrome/app/settings_strings.grdp:3077: <message name="IDS_SETTINGS_POWER_IDLE_OTHER" desc="In Device Settings > Power, menu item for idle behavior."> "for custom idle behavior." https://codereview.chromium.org/2853113004/diff/40001/chrome/app/settings_str... chrome/app/settings_strings.grdp:3081: When lid is closed (you get the idea) https://codereview.chromium.org/2853113004/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/device_page/power.html (right): https://codereview.chromium.org/2853113004/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/device_page/power.html:6: <link rel="import" href="chrome://resources/cr_elements/policy/cr_policy_indicator.html"> place alphabetically -- yes, that puts it before polymer.html; this is normally okay because our polymer elements should also import polymer.html https://codereview.chromium.org/2853113004/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/device_page/power.js (right): https://codereview.chromium.org/2853113004/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/device_page/power.js:67: readOnly: true, remove readOnly (|computed| properties are read-only by definition) https://codereview.chromium.org/2853113004/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/device_page/power.js:74: readOnly: true, ditto https://codereview.chromium.org/2853113004/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/device_page/power.js:144: * @return {!Array<>} Options to display in idle-behavior select. !Array<!{value: settings.IdleBehavior, name: string}> https://codereview.chromium.org/2853113004/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/device_page/power.js:173: * @return {!Array<>} Options to display in lid-closed-behavior select. similarly
derat@google.com changed reviewers: + derat@google.com
thanks again. i'll push on the launch bug reviews and then look into adding tests to this change. https://codereview.chromium.org/2853113004/diff/40001/chrome/app/settings_str... File chrome/app/settings_strings.grdp (right): https://codereview.chromium.org/2853113004/diff/40001/chrome/app/settings_str... chrome/app/settings_strings.grdp:3069: Turn off display and sleep On 2017/05/08 18:48:55, michaelpg wrote: > "that turns off the screen and puts the device to sleep" Done. https://codereview.chromium.org/2853113004/diff/40001/chrome/app/settings_str... chrome/app/settings_strings.grdp:3071: <message name="IDS_SETTINGS_POWER_IDLE_DISPLAY_OFF_STAY_AWAKE" desc="In Device Settings > Power, menu item for idle behavior."> On 2017/05/08 18:48:54, michaelpg wrote: > "that turns off the screen but prevents the device from sleeping" Done. https://codereview.chromium.org/2853113004/diff/40001/chrome/app/settings_str... chrome/app/settings_strings.grdp:3074: <message name="IDS_SETTINGS_POWER_IDLE_DISPLAY_ON" desc="In Device Settings > Power, menu item for idle behavior."> On 2017/05/08 18:48:54, michaelpg wrote: > "that prevents the screen from turning off" Done. https://codereview.chromium.org/2853113004/diff/40001/chrome/app/settings_str... chrome/app/settings_strings.grdp:3077: <message name="IDS_SETTINGS_POWER_IDLE_OTHER" desc="In Device Settings > Power, menu item for idle behavior."> On 2017/05/08 18:48:54, michaelpg wrote: > "for custom idle behavior." Done. https://codereview.chromium.org/2853113004/diff/40001/chrome/app/settings_str... chrome/app/settings_strings.grdp:3081: When lid is closed On 2017/05/08 18:48:54, michaelpg wrote: > (you get the idea) Done. https://codereview.chromium.org/2853113004/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/device_page/power.html (right): https://codereview.chromium.org/2853113004/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/device_page/power.html:6: <link rel="import" href="chrome://resources/cr_elements/policy/cr_policy_indicator.html"> On 2017/05/08 18:48:55, michaelpg wrote: > place alphabetically -- yes, that puts it before polymer.html; this is normally > okay because our polymer elements should also import polymer.html Done. https://codereview.chromium.org/2853113004/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/device_page/power.js (right): https://codereview.chromium.org/2853113004/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/device_page/power.js:67: readOnly: true, On 2017/05/08 18:48:55, michaelpg wrote: > remove readOnly (|computed| properties are read-only by definition) Done. https://codereview.chromium.org/2853113004/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/device_page/power.js:74: readOnly: true, On 2017/05/08 18:48:55, michaelpg wrote: > ditto Done. https://codereview.chromium.org/2853113004/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/device_page/power.js:144: * @return {!Array<>} Options to display in idle-behavior select. On 2017/05/08 18:48:55, michaelpg wrote: > !Array<!{value: settings.IdleBehavior, name: string}> whoops, forgot to fill these in. thanks for giving me the proper syntax. :-) https://codereview.chromium.org/2853113004/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/device_page/power.js:173: * @return {!Array<>} Options to display in lid-closed-behavior select. On 2017/05/08 18:48:55, michaelpg wrote: > similarly Done.
big note to self: i need to find some way to not display the "when lid is closed" setting on devices that don't have lids. i think i can do this by calling my newly-added chromeos::PowerManagerClient::GetSwitchStates() method.
if you don't mind taking another look while this is still fresh, patch set 5 hides the lid setting when appropriate (but no rush to review if you'd rather look at it later).
Description was changed from ========== chromeos: Add settings to control power management prefs. Add settings allowing users to control the idle and lid-closed power management behavior. BUG=633455 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== chromeos: Add settings to control power management prefs. Add settings allowing users to control the idle and lid-closed power management behavior. BUG=633455 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
derat@chromium.org changed reviewers: - derat@google.com
i think i'm ready for the final push to try to get this in for M61. sorry, the tests added another 400 or so lines beyond what you reviewed earlier, and i suspect that i'm not doing some things quite right on the js side. also, i'm getting some a11y errors: --- Accessibility issues found on chrome://md-settings/power *** Begin accessibility audit results *** An accessibility audit found Errors: Error: AX_ARIA_04 (ARIA state and property values must be valid) failed on the following elements (1 - 2 of 2): #indicator #indicator See https://github.com/GoogleChrome/accessibility-developer-tools/wiki/Audit-Rule... for more information. Error: AX_ARIA_02 (ARIA attributes which refer to other elements by ID should refer to elements which exist in the DOM) failed on the following elements (1 - 2 of 2): #indicator #indicator See https://github.com/GoogleChrome/accessibility-developer-tools/wiki/Audit-Rule... for more information. Error: AX_TOOLTIP_01 (Elements with role=tooltip should have a corresponding element with aria-describedby) failed on the following elements (1 - 2 of 2): #tooltip #tooltip See https://github.com/GoogleChrome/accessibility-developer-tools/wiki/Audit-Rule... for more information. *** End accessibility audit results ***", source: test_api.js (375) --- they look like they're coming from <cr-policy-indicator>, which i would've expected to be doing the right thing with regard to aria stuff. i also noticed that the a11y checker seemed to get confused when i used aria-labelledby; it gave me "ARIA attributes which refer to other elements by ID should refer to elements which exist in the DOM". i suspect it might have trouble with elements that are dynamically added and removed.
The CQ bit was checked by derat@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
+hcarmona to take a look at a11y test errors. Mostly nits. I don't see major issues with the JS, just one note on Polymer.dom.flush usage. https://codereview.chromium.org/2853113004/diff/100001/chrome/app/settings_st... File chrome/app/settings_strings.grdp (right): https://codereview.chromium.org/2853113004/diff/100001/chrome/app/settings_st... chrome/app/settings_strings.grdp:3152: <message name="IDS_SETTINGS_POWER_IDLE_LABEL" desc="In Device Settings > Power, label for idle behavior."> "label for behavior when device is idle" to reduce likelihood of clarification question from translators https://codereview.chromium.org/2853113004/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/device_page/device_page_browser_proxy.js (right): https://codereview.chromium.org/2853113004/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/device_page/device_page_browser_proxy.js:37: * Mirrors chromeos::settings::PowerHandler::IdleBehavior nit: ending period https://codereview.chromium.org/2853113004/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/device_page/device_page_browser_proxy.js:48: * Mirrors chromeos::PowerPolicyController::Action ditto https://codereview.chromium.org/2853113004/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/device_page/device_page_browser_proxy.js:61: * idleManaged: boolean, nit: You may want to use "Controlled" for these booleans to disambiguate with power "management". https://codereview.chromium.org/2853113004/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/device_page/power.html (right): https://codereview.chromium.org/2853113004/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/device_page/power.html:43: <cr-policy-indicator id="idleManagedIndicator" Ask hcarmona@ what the correct aria use is here: * set icon-aria-label with the same "powerIdleLabel" that describes the <select>? (code search shows this is popular) * do nothing and ignore the AX errors? (settings-dropdown-menu itself does nothing here) * something else? https://codereview.chromium.org/2853113004/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/device_page/power.html:62: <cr-policy-indicator id="lidClosedManagedIndicator" (ditto) https://codereview.chromium.org/2853113004/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/device_page/power.js (right): https://codereview.chromium.org/2853113004/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/device_page/power.js:250: Polymer.dom.flush(); use this.async(function() {...}); instead of forcing the flush, because forcing the flush could force a bunch of other updates we don't want to block on https://codereview.chromium.org/2853113004/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/chromeos/device_power_handler.cc (right): https://codereview.chromium.org/2853113004/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/chromeos/device_power_handler.cc:60: PowerHandler::TestAPI::~TestAPI() = default; just for my benefit: what's the advantage of a default destructor instead of an empty destructor ({})? https://codereview.chromium.org/2853113004/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/chromeos/device_power_handler.cc:119: // Register to be notified about changes to power management prefs that are optional nit: less wordy: "Observe power management prefs used in the UI." https://codereview.chromium.org/2853113004/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/chromeos/device_power_handler.cc:183: case IdleBehavior::DISPLAY_OFF_SLEEP: could you add a brief comment about each of these cases? eg, are we clearing these prefs because DISPLAY_OFF_SLEEP is the default? Or replace with appropriately named helper function(s). https://codereview.chromium.org/2853113004/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/chromeos/device_power_handler.cc:184: prefs_->ClearPref(prefs::kPowerAcIdleAction); I'm not sure clearing the pref is the right move. If the user has changed the setting, we normally want to reflect that (even if it means setting the pref to its default value). https://codereview.chromium.org/2853113004/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/chromeos/device_power_handler.cc:208: prefs_->SetInteger(prefs::kPowerAcScreenDimDelayMs, 0); does 0 mean infinite (never dim, etc)? https://codereview.chromium.org/2853113004/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/chromeos/device_power_handler.h (right): https://codereview.chromium.org/2853113004/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/chromeos/device_power_handler.h:15: #include "base/time/time.h" can you forward declare TimeTicks instead? https://codereview.chromium.org/2853113004/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/chromeos/device_power_handler_unittest.cc (right): https://codereview.chromium.org/2853113004/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/chromeos/device_power_handler_unittest.cc:43: dbus_setter->SetPowerManagerClient(base::WrapUnique(power_manager_client_)); nit: base::MakeUnique<>() then set power_manager_client_ to GetPowerManageerClient() https://codereview.chromium.org/2853113004/diff/100001/chrome/test/data/webui... File chrome/test/data/webui/settings/device_page_tests.js (right): https://codereview.chromium.org/2853113004/diff/100001/chrome/test/data/webui... chrome/test/data/webui/settings/device_page_tests.js:278: }); after changing the handler to this.async (prior comment), add a Polymer.dom.flush here, or use Polymer.dom.flush after calling from test methods https://codereview.chromium.org/2853113004/diff/100001/chrome/test/data/webui... chrome/test/data/webui/settings/device_page_tests.js:682: .requestPowerManagementSettingsCalled_); nit: make this not private (no underscore), or add a getter. But I see you're using an existing pattern sooo meh. https://codereview.chromium.org/2853113004/diff/100001/chrome/test/data/webui... chrome/test/data/webui/settings/device_page_tests.js:684: settings.IdleBehavior.DISPLAY_OFF_SLEEP, false, nit: add comments for parameter names for these bools as in the c++ unit test https://codereview.chromium.org/2853113004/diff/100001/chrome/test/data/webui... chrome/test/data/webui/settings/device_page_tests.js:763: idleSelect.dispatchEvent(new CustomEvent('change')); maybe make a helper function selectValue(selectId, value) https://codereview.chromium.org/2853113004/diff/100001/chrome/test/data/webui... chrome/test/data/webui/settings/device_page_tests.js:831: expectTrue(powerPage.$$('#idleManagedIndicator') != null); expectNotEquals (or often we just expectTrue(!!foo))
thanks for the review! https://codereview.chromium.org/2853113004/diff/100001/chrome/app/settings_st... File chrome/app/settings_strings.grdp (right): https://codereview.chromium.org/2853113004/diff/100001/chrome/app/settings_st... chrome/app/settings_strings.grdp:3152: <message name="IDS_SETTINGS_POWER_IDLE_LABEL" desc="In Device Settings > Power, label for idle behavior."> On 2017/06/16 01:34:59, michaelpg wrote: > "label for behavior when device is idle" to reduce likelihood of clarification > question from translators Done. https://codereview.chromium.org/2853113004/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/device_page/device_page_browser_proxy.js (right): https://codereview.chromium.org/2853113004/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/device_page/device_page_browser_proxy.js:37: * Mirrors chromeos::settings::PowerHandler::IdleBehavior On 2017/06/16 01:35:00, michaelpg wrote: > nit: ending period Done. https://codereview.chromium.org/2853113004/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/device_page/device_page_browser_proxy.js:48: * Mirrors chromeos::PowerPolicyController::Action On 2017/06/16 01:35:00, michaelpg wrote: > ditto Done. https://codereview.chromium.org/2853113004/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/device_page/device_page_browser_proxy.js:61: * idleManaged: boolean, On 2017/06/16 01:35:00, michaelpg wrote: > nit: You may want to use "Controlled" for these booleans to disambiguate with > power "management". done (throughout) https://codereview.chromium.org/2853113004/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/device_page/power.html (right): https://codereview.chromium.org/2853113004/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/device_page/power.html:43: <cr-policy-indicator id="idleManagedIndicator" On 2017/06/16 01:35:00, michaelpg wrote: > Ask hcarmona@ what the correct aria use is here: > * set icon-aria-label with the same "powerIdleLabel" that describes the > <select>? (code search shows this is popular) i noticed this pattern too (and think i had this in an earlier version of the change), but it doesn't make much sense to me. i would've expected this to describe the icon, not to be yet another duplicated copy of the main select label. > * do nothing and ignore the AX errors? (settings-dropdown-menu itself does > nothing here) > * something else? https://codereview.chromium.org/2853113004/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/device_page/power.html:62: <cr-policy-indicator id="lidClosedManagedIndicator" On 2017/06/16 01:35:00, michaelpg wrote: > (ditto) Acknowledged. https://codereview.chromium.org/2853113004/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/device_page/power.js (right): https://codereview.chromium.org/2853113004/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/device_page/power.js:250: Polymer.dom.flush(); On 2017/06/16 01:35:00, michaelpg wrote: > use this.async(function() {...}); instead of forcing the flush, because forcing > the flush could force a bunch of other updates we don't want to block on hmm. i had this.async earlier (after you suggested it, i think), but it doesn't seem to get along well with the tests that i added. here's what i see after using async() here and flushing in the test; it looks like the function passed to async() isn't running before the subsequent assertions: [4693:4693:0615/203055.092960:ERROR:web_ui_test_handler.cc(77)] Failed: RUN_TEST_F("CrSettingsDevicePageTest","PowerTest") AssertionError: expected '0' to equal '2' at Function.assert.strictEqual (chai.js:2277:32) at assertEquals (test_api.js:892:17) at test_api.js:1019:20 at Context.<anonymous> (device_page_tests.js:803:11) at callFn (mocha.js:4471:21) at Test.Runnable.run (mocha.js:4464:7) at Runner.runTest (mocha.js:4934:10) at mocha.js:5040:12 at next (mocha.js:4854:14) at mocha.js:4864:7Failed: RUN_TEST_F("CrSettingsDevicePageTest","PowerTest") AssertionError: expected '0' to equal '3' at Function.assert.strictEqual (chai.js:2277:32) at assertEquals (test_api.js:892:17) at test_api.js:1019:20 at Context.<anonymous> (device_page_tests.js:807:11) at callFn (mocha.js:4471:21) at Test.Runnable.run (mocha.js:4464:7) at Runner.runTest (mocha.js:4934:10) at mocha.js:5040:12 at next (mocha.js:4854:14) at mocha.js:4864:7Failed: RUN_TEST_F("CrSettingsDevicePageTest","PowerTest") AssertionError: expected '0' to equal '1' at Function.assert.strictEqual (chai.js:2277:32) at assertEquals (test_api.js:892:17) at test_api.js:1019:20 at Context.<anonymous> (device_page_tests.js:817:11) at callFn (mocha.js:4471:21) at Test.Runnable.run (mocha.js:4464:7) at Runner.runTest (mocha.js:4934:10) at mocha.js:5040:12 at next (mocha.js:4854:14) at mocha.js:4864:7Failed: RUN_TEST_F("CrSettingsDevicePageTest","PowerTest") AssertionError: expected '0' to equal '3' at Function.assert.strictEqual (chai.js:2277:32) at assertEquals (test_api.js:892:17) at test_api.js:1019:20 at Context.<anonymous> (device_page_tests.js:834:11) at callFn (mocha.js:4471:21) at Test.Runnable.run (mocha.js:4464:7) at Runner.runTest (mocha.js:4934:10) at mocha.js:5040:12 at next (mocha.js:4854:14) at mocha.js:4864:7Failed: RUN_TEST_F("CrSettingsDevicePageTest","PowerTest") AssertionError: expected null to not equal null at Function.assert.notStrictEqual (chai.js:2295:36) at assertNotEquals (test_api.js:932:17) at test_api.js:1019:20 at Context.<anonymous> (device_page_tests.js:837:11) at callFn (mocha.js:4471:21) at Test.Runnable.run (mocha.js:4464:7) at Runner.runTest (mocha.js:4934:10) at mocha.js:5040:12 at next (mocha.js:4854:14) at mocha.js:4864:7 gen/chrome/test/data/webui/settings/cr_settings_browsertest-gen.cc:1706: Failure Value of: RunJavascriptTestF( true, "CrSettingsDevicePageTest", "PowerTest") Actual: false Expected: true i've seen some async() calls with non-zero delays in test code, but that feels pretty hacky. https://codereview.chromium.org/2853113004/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/chromeos/device_power_handler.cc (right): https://codereview.chromium.org/2853113004/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/chromeos/device_power_handler.cc:60: PowerHandler::TestAPI::~TestAPI() = default; On 2017/06/16 01:35:00, michaelpg wrote: > just for my benefit: what's the advantage of a default destructor instead of an > empty destructor ({})? i'm not sure that there's a real advantage, or that one is recommended over the other, but i've seen a shift to move "= default" lately. discussion is at https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/qgU4mh_MpGA. per https://stackoverflow.com/questions/823935/whats-the-point-in-defaulting-func..., i think that the main benefit may just be alignment with "= delete" (which seems more generally useful). https://codereview.chromium.org/2853113004/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/chromeos/device_power_handler.cc:119: // Register to be notified about changes to power management prefs that are On 2017/06/16 01:35:00, michaelpg wrote: > optional nit: less wordy: "Observe power management prefs used in the UI." Done. https://codereview.chromium.org/2853113004/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/chromeos/device_power_handler.cc:183: case IdleBehavior::DISPLAY_OFF_SLEEP: On 2017/06/16 01:35:00, michaelpg wrote: > could you add a brief comment about each of these cases? eg, are we clearing > these prefs because DISPLAY_OFF_SLEEP is the default? Or replace with > appropriately named helper function(s). Done. https://codereview.chromium.org/2853113004/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/chromeos/device_power_handler.cc:184: prefs_->ClearPref(prefs::kPowerAcIdleAction); On 2017/06/16 01:35:00, michaelpg wrote: > I'm not sure clearing the pref is the right move. If the user has changed the > setting, we normally want to reflect that (even if it means setting the pref to > its default value). hmm, i'd like to keep the possibility of changing the defaults open, since we've done it before in the past. there's no way other way for users to set these prefs right now. https://codereview.chromium.org/2853113004/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/chromeos/device_power_handler.cc:208: prefs_->SetInteger(prefs::kPowerAcScreenDimDelayMs, 0); On 2017/06/16 01:35:00, michaelpg wrote: > does 0 mean infinite (never dim, etc)? yes, 0 disables the delays. they're documented at https://chromium.googlesource.com/chromiumos/platform/system_api/+/master/dbu..., but also in the location where the pref names are defined at chrome/common/pref_names.cc. https://codereview.chromium.org/2853113004/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/chromeos/device_power_handler.h (right): https://codereview.chromium.org/2853113004/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/chromeos/device_power_handler.h:15: #include "base/time/time.h" On 2017/06/16 01:35:00, michaelpg wrote: > can you forward declare TimeTicks instead? Done. https://codereview.chromium.org/2853113004/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/chromeos/device_power_handler_unittest.cc (right): https://codereview.chromium.org/2853113004/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/chromeos/device_power_handler_unittest.cc:43: dbus_setter->SetPowerManagerClient(base::WrapUnique(power_manager_client_)); On 2017/06/16 01:35:00, michaelpg wrote: > nit: base::MakeUnique<>() then set power_manager_client_ to > GetPowerManageerClient() done (although the downside is that this requires a downcast to get the fake object out) https://codereview.chromium.org/2853113004/diff/100001/chrome/test/data/webui... File chrome/test/data/webui/settings/device_page_tests.js (right): https://codereview.chromium.org/2853113004/diff/100001/chrome/test/data/webui... chrome/test/data/webui/settings/device_page_tests.js:682: .requestPowerManagementSettingsCalled_); On 2017/06/16 01:35:00, michaelpg wrote: > nit: make this not private (no underscore), or add a getter. But I see you're > using an existing pattern sooo meh. Acknowledged. https://codereview.chromium.org/2853113004/diff/100001/chrome/test/data/webui... chrome/test/data/webui/settings/device_page_tests.js:684: settings.IdleBehavior.DISPLAY_OFF_SLEEP, false, On 2017/06/16 01:35:00, michaelpg wrote: > nit: add comments for parameter names for these bools as in the c++ unit test Done. https://codereview.chromium.org/2853113004/diff/100001/chrome/test/data/webui... chrome/test/data/webui/settings/device_page_tests.js:763: idleSelect.dispatchEvent(new CustomEvent('change')); On 2017/06/16 01:35:00, michaelpg wrote: > maybe make a helper function selectValue(selectId, value) Done. https://codereview.chromium.org/2853113004/diff/100001/chrome/test/data/webui... chrome/test/data/webui/settings/device_page_tests.js:831: expectTrue(powerPage.$$('#idleManagedIndicator') != null); On 2017/06/16 01:35:00, michaelpg wrote: > expectNotEquals (or often we just expectTrue(!!foo)) Done.
with some help from steven, i got the tests working by adding a bunch of ugly promises. :-P
On 2017/06/16 20:01:03, Daniel Erat wrote: > with some help from steven, i got the tests working by adding a bunch of ugly > promises. :-P and somehow the a11y audit errors disappeared too. hector, i'm still curious about the right way of labeling elements, in case you don't mind taking a look at power.html.
The CQ bit was checked by derat@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: closure_compilation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)
The CQ bit was checked by derat@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
hcarmona@chromium.org changed reviewers: + hcarmona@chromium.org
Unfortunately our automated a11y testing is not in a great state. go/a11y-chrome-settings-design for details https://codereview.chromium.org/2853113004/diff/160001/chrome/browser/resourc... File chrome/browser/resources/settings/device_page/power.html (right): https://codereview.chromium.org/2853113004/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/device_page/power.html:41: <div class="start">$i18n{powerIdleLabel}</div> Consider giving this element and ID and using aria-labelledby in #idleSelect. This will use the same label for both elements giving better context for screen readers. https://codereview.chromium.org/2853113004/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/device_page/power.html:43: <cr-policy-indicator id="idleControlledIndicator" Please set the icon-aria-label property for a11y. Reusing $i18n{powerIdleLabel} should be good. Unfortunately aria-lablelledby doesn't go across the shadow dom, so it can't be used here and the label text must be reused. https://codereview.chromium.org/2853113004/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/device_page/power.html:59: <div id="lidClosedRow" class="settings-box continuation" Same a11y suggestions for the children of this element as above.
lgtm https://codereview.chromium.org/2853113004/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/chromeos/device_power_handler.cc (right): https://codereview.chromium.org/2853113004/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/chromeos/device_power_handler.cc:184: prefs_->ClearPref(prefs::kPowerAcIdleAction); On 2017/06/16 02:34:24, Daniel Erat wrote: > On 2017/06/16 01:35:00, michaelpg wrote: > > I'm not sure clearing the pref is the right move. If the user has changed the > > setting, we normally want to reflect that (even if it means setting the pref > to > > its default value). > > hmm, i'd like to keep the possibility of changing the defaults open, since we've > done it before in the past. there's no way other way for users to set these > prefs right now. A meta-pref would let us tweak the default delay without overriding user selections, but I'm not sure it's worth the complexity (and you probably thought about it already) https://codereview.chromium.org/2853113004/diff/160001/chrome/browser/resourc... File chrome/browser/resources/settings/device_page/power.html (right): https://codereview.chromium.org/2853113004/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/device_page/power.html:60: hidden$="[[!hasLid_]]"> nit: 4-space indent
thanks! i'm still having problems with aria-labelledby, though. https://codereview.chromium.org/2853113004/diff/160001/chrome/browser/resourc... File chrome/browser/resources/settings/device_page/power.html (right): https://codereview.chromium.org/2853113004/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/device_page/power.html:41: <div class="start">$i18n{powerIdleLabel}</div> On 2017/06/19 22:18:11, hcarmona wrote: > Consider giving this element and ID and using aria-labelledby in #idleSelect. > This will use the same label for both elements giving better context for screen > readers. i've tried this (see next patch set), but i always get errors from the checker: --- Accessibility issues found on chrome://md-settings/power *** Begin accessibility audit results *** An accessibility audit found Errors: Error: AX_ARIA_04 (ARIA state and property values must be valid) failed on the following elements (1 - 2 of 2): #idleSelect #lidClosedSelect See https://github.com/GoogleChrome/accessibility-developer-tools/wiki/Audit-Rule... for more information. Error: AX_ARIA_02 (ARIA attributes which refer to other elements by ID should refer to elements which exist in the DOM) failed on the following elements (1 - 2 of 2): #idleSelect #lidClosedSelect See https://github.com/GoogleChrome/accessibility-developer-tools/wiki/Audit-Rule... for more information. --- any ideas? the errors disappear if i use aria-label on the two <select>s instead of aria-labelledby. https://codereview.chromium.org/2853113004/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/device_page/power.html:43: <cr-policy-indicator id="idleControlledIndicator" On 2017/06/19 22:18:11, hcarmona wrote: > Please set the icon-aria-label property for a11y. > Reusing $i18n{powerIdleLabel} should be good. > > Unfortunately aria-lablelledby doesn't go across the shadow dom, so it can't be > used here and the label text must be reused. Done. https://codereview.chromium.org/2853113004/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/device_page/power.html:59: <div id="lidClosedRow" class="settings-box continuation" On 2017/06/19 22:18:11, hcarmona wrote: > Same a11y suggestions for the children of this element as above. Done. https://codereview.chromium.org/2853113004/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/device_page/power.html:60: hidden$="[[!hasLid_]]"> On 2017/06/19 22:38:06, michaelpg wrote: > nit: 4-space indent Done.
The CQ bit was checked by derat@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
i've switched the <select>s back from aria-labelledby to aria-label, since that doesn't trigger a warning and is what i see us using elsewhere (including in power.html). i'm planning to check the change in in this state. i'm happy to upload a followup change using aria-labelledby if someone can tell me how to do that without triggering warnings (or if someone tells me i should just ignore the warnings because they're wrong). :-)
On 2017/06/20 17:46:07, Daniel Erat wrote: > i've switched the <select>s back from aria-labelledby to aria-label, since that > doesn't trigger a warning and is what i see us using elsewhere (including in > power.html). i'm planning to check the change in in this state. > > i'm happy to upload a followup change using aria-labelledby if someone can tell > me how to do that without triggering warnings (or if someone tells me i should > just ignore the warnings because they're wrong). :-) LGTM, we are working on improving our a11y tests and hope to minimize false errors.
The CQ bit was unchecked by derat@chromium.org
The CQ bit was checked by derat@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from michaelpg@chromium.org Link to the patchset: https://codereview.chromium.org/2853113004/#ps200001 (title: "switch back from aria-labelledby to aria-label")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 200001, "attempt_start_ts": 1497985059527740,
"parent_rev": "18697c83b41fc147f21b373a55da451f0fd44117", "commit_rev":
"5a1191d63dd372b98e85db5800132693535e51a1"}
CQ is committing da patch.
Bot data: {"patchset_id": 200001, "attempt_start_ts": 1497985059527740,
"parent_rev": "4b8cc92116dae0026850d39bddaeba803dd581d1", "commit_rev":
"b4400a28e45066f77cc87adc60247396f4b3875a"}
Message was sent while issue was closed.
Description was changed from ========== chromeos: Add settings to control power management prefs. Add settings allowing users to control the idle and lid-closed power management behavior. BUG=633455 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== chromeos: Add settings to control power management prefs. Add settings allowing users to control the idle and lid-closed power management behavior. BUG=633455 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2853113004 Cr-Commit-Position: refs/heads/master@{#480902} Committed: https://chromium.googlesource.com/chromium/src/+/b4400a28e45066f77cc87adc6024... ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001) as https://chromium.googlesource.com/chromium/src/+/b4400a28e45066f77cc87adc6024... |
