Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2629573006:
chromeos: Add Power device page to chrome://md-settings. (Closed)
|
Created:
3 years, 11 months ago by Daniel Erat Modified:
3 years, 9 months ago CC:
chromium-reviews, michaelpg+watch-md-settings_chromium.org, michaelpg+watch-md-ui_chromium.org, arv+watch_chromium.org, vitalyp+closure_chromium.org, dbeam+watch-settings_chromium.org, dbeam+watch-closure_chromium.org, stevenjb+watch-md-settings_chromium.org, jlklein+watch-closure_chromium.org, stevenjb Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionchromeos: Add Power device page to chrome://md-settings.
Move the power row into its own subpage in anticipation of
adding additional settings.
BUG=633484, 633455
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2629573006
Cr-Commit-Position: refs/heads/master@{#454136}
Committed: https://chromium.googlesource.com/chromium/src/+/8f2dc13d423abd8414e6bd5624aa516fcb1e683d
Patch Set 1 #Patch Set 2 : update copyright year #
Total comments: 4
Patch Set 3 : update now that power source settings exist #
Total comments: 24
Patch Set 4 : apply review feedback #
Total comments: 2
Patch Set 5 : merge #
Total comments: 8
Messages
Total messages: 55 (28 generated)
Description was changed from ========== chromeos: Add empty Power subpage to chrome://md-settings. Add a (currently-empty) power management subpage to chrome://md-settings. BUG=633484 ========== to ========== chromeos: Add empty Power subpage to chrome://md-settings. Add a (currently-empty) power management subpage to chrome://md-settings. BUG=633484 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
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 derat@chromium.org
derat@chromium.org changed reviewers: + michaelpg@chromium.org
i've pulled out part of my existing code to at least add an empty subpage (to make sure i'm not blocking any of the type-c work).
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...
stevenjb@: do you want to add this right now?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
stevenjb@chromium.org changed reviewers: + stevenjb@chromium.org
derat@ - Let's hide this by default for now like we do with touch calibration (showTouchCalibrationSetting).
On 2017/01/13 02:36:31, Dan Beam wrote: > stevenjb@: do you want to add this right now? I'd vote to not add this yet. We don't have mocks AFAIK. I've put up a CL that adds the USB Type-C selection as a row in Device, instead of a sub-page: https://codereview.chromium.org/2629173004/ Only shows on Chromebooks that support dual-role devices (samus and a few newer ones). It should be trivial to move that into a Power sub-page when the Power sub-page is ready.
https://codereview.chromium.org/2629573006/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/device_page/power.js (right): https://codereview.chromium.org/2629573006/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/device_page/power.js:9: nit: remove blank line https://codereview.chromium.org/2629573006/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/device_page/power.js:15: prefs: { will we need this?
we don't have any top-level non-subpage rows in device right now, do we? https://codereview.chromium.org/2629573006/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/device_page/power.js (right): https://codereview.chromium.org/2629573006/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/device_page/power.js:9: On 2017/01/13 17:15:35, michaelpg (NYC) wrote: > nit: remove blank line done (locally) https://codereview.chromium.org/2629573006/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/device_page/power.js:15: prefs: { On 2017/01/13 17:15:35, michaelpg (NYC) wrote: > will we need this? yes, i believe that we will for http://crbug.com/633455. the subpage will be displayed unconditionally at that point.
On 2017/01/13 17:34:08, Daniel Erat wrote: > we don't have any top-level non-subpage rows in device right now, do we? No. If you'd prefer we put the Type-C power settings in a sub-page right now, we can do that. But either way, we shouldn't show this row unconditionally until it has content, since we're actually enabling md-settings for dev channel on CrOS. > > https://codereview.chromium.org/2629573006/diff/20001/chrome/browser/resource... > File chrome/browser/resources/settings/device_page/power.js (right): > > https://codereview.chromium.org/2629573006/diff/20001/chrome/browser/resource... > chrome/browser/resources/settings/device_page/power.js:9: > On 2017/01/13 17:15:35, michaelpg (NYC) wrote: > > nit: remove blank line > > done (locally) > > https://codereview.chromium.org/2629573006/diff/20001/chrome/browser/resource... > chrome/browser/resources/settings/device_page/power.js:15: prefs: { > On 2017/01/13 17:15:35, michaelpg (NYC) wrote: > > will we need this? > > yes, i believe that we will for http://crbug.com/633455. the subpage will be > displayed unconditionally at that point.
On 2017/01/13 21:15:04, michaelpg (NYC) wrote: > On 2017/01/13 17:34:08, Daniel Erat wrote: > > we don't have any top-level non-subpage rows in device right now, do we? > > No. If you'd prefer we put the Type-C power settings in a sub-page right now, we > can do that. But either way, we shouldn't show this row unconditionally until it > has content, since we're actually enabling md-settings for dev channel on CrOS. ah, i didn't realize it was finally happening. :-) if i were adding a new setting that doesn't have a firm deadline, would i be wise to just work on adding it to md-settings instead of additionally to the current settings page? if you're willing to pull this into your type-c change, that's great, but i'm also okay with doing the merge if yours is expected to go in soon. > > > > > https://codereview.chromium.org/2629573006/diff/20001/chrome/browser/resource... > > File chrome/browser/resources/settings/device_page/power.js (right): > > > > > https://codereview.chromium.org/2629573006/diff/20001/chrome/browser/resource... > > chrome/browser/resources/settings/device_page/power.js:9: > > On 2017/01/13 17:15:35, michaelpg (NYC) wrote: > > > nit: remove blank line > > > > done (locally) > > > > > https://codereview.chromium.org/2629573006/diff/20001/chrome/browser/resource... > > chrome/browser/resources/settings/device_page/power.js:15: prefs: { > > On 2017/01/13 17:15:35, michaelpg (NYC) wrote: > > > will we need this? > > > > yes, i believe that we will for http://crbug.com/633455. the subpage will be > > displayed unconditionally at that point.
On 2017/01/13 21:19:40, Daniel Erat wrote: > On 2017/01/13 21:15:04, michaelpg (NYC) wrote: > > On 2017/01/13 17:34:08, Daniel Erat wrote: > > > we don't have any top-level non-subpage rows in device right now, do we? > > > > No. If you'd prefer we put the Type-C power settings in a sub-page right now, > we > > can do that. But either way, we shouldn't show this row unconditionally until > it > > has content, since we're actually enabling md-settings for dev channel on > CrOS. > > ah, i didn't realize it was finally happening. :-) if i were adding a new > setting that doesn't have a firm deadline, would i be wise to just work on > adding it to md-settings instead of additionally to the current settings page? Yes, I think so. Just keep it behind a flag if the feature isn't ready. > > if you're willing to pull this into your type-c change, that's great, but i'm > also okay with doing the merge if yours is expected to go in soon. Trying to land it today/tuesday. > > > > > > > > > > https://codereview.chromium.org/2629573006/diff/20001/chrome/browser/resource... > > > File chrome/browser/resources/settings/device_page/power.js (right): > > > > > > > > > https://codereview.chromium.org/2629573006/diff/20001/chrome/browser/resource... > > > chrome/browser/resources/settings/device_page/power.js:9: > > > On 2017/01/13 17:15:35, michaelpg (NYC) wrote: > > > > nit: remove blank line > > > > > > done (locally) > > > > > > > > > https://codereview.chromium.org/2629573006/diff/20001/chrome/browser/resource... > > > chrome/browser/resources/settings/device_page/power.js:15: prefs: { > > > On 2017/01/13 17:15:35, michaelpg (NYC) wrote: > > > > will we need this? > > > > > > yes, i believe that we will for http://crbug.com/633455. the subpage will be > > > displayed unconditionally at that point.
On 2017/01/13 22:18:36, michaelpg (NYC) wrote: > On 2017/01/13 21:19:40, Daniel Erat wrote: > > if you're willing to pull this into your type-c change, that's great, but i'm > > also okay with doing the merge if yours is expected to go in soon. > > Trying to land it today/tuesday. got it. going ahead with your change as-is sounds fine to me.
Description was changed from ========== chromeos: Add empty Power subpage to chrome://md-settings. Add a (currently-empty) power management subpage to chrome://md-settings. BUG=633484 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== chromeos: Add Power device page to chrome://md-settings. Move the power row into its own subpage in anticipation of adding additional settings. BUG=633484 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== chromeos: Add Power device page to chrome://md-settings. Move the power row into its own subpage in anticipation of adding additional settings. BUG=633484 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== chromeos: Add Power device page to chrome://md-settings. Move the power row into its own subpage in anticipation of adding additional settings. BUG=633484,633455 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
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 finally merged this with the power row. mind taking another look? https://codereview.chromium.org/2629573006/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/device_page/device_page.js (right): https://codereview.chromium.org/2629573006/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/device_page/device_page.js:29: hasMouse_: {type: Boolean, value: false}, "git cl format --js" did this. let me know if you'd prefer that i revert the parts that are irrelevant to my change. https://codereview.chromium.org/2629573006/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/route.js (right): https://codereview.chromium.org/2629573006/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/route.js:141: r.POWER = r.DEVICE.createChild('/power'); should i reorder this listing to match the order of the rows in the device page?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
overall looks good, some clean-up to do after the move though. https://codereview.chromium.org/2629573006/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/device_page/device_page.html (right): https://codereview.chromium.org/2629573006/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/device_page/device_page.html:2: <link rel="import" href="chrome://resources/html/md_select_css.html"> unused, move to power.html https://codereview.chromium.org/2629573006/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/device_page/device_page.html:22: <style include="settings-shared md-select"></style> "md-select" styles no longer apply here https://codereview.chromium.org/2629573006/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/device_page/device_page.js (right): https://codereview.chromium.org/2629573006/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/device_page/device_page.js:29: hasMouse_: {type: Boolean, value: false}, On 2017/03/01 02:43:31, Daniel Erat wrote: > "git cl format --js" did this. let me know if you'd prefer that i revert the > parts that are irrelevant to my change. no worries, but you'll actually need to rebase two of these on stevenjb's fix: https://codereview.chromium.org/2727443003/ https://codereview.chromium.org/2629573006/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/device_page/device_page.js:117: onPowerTap_: function() { opt. nit: place in the same order as the HTML elements that reference these (after onStorageTap_, looks like) https://codereview.chromium.org/2629573006/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/device_page/power.html (right): https://codereview.chromium.org/2629573006/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/device_page/power.html:4: <link rel="import" href="../settings_shared_css.html"> import what you use (eg md_select_css.html, i18n_behavior.html). basically several of the imports in device_page.html. https://codereview.chromium.org/2629573006/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/device_page/power.js (right): https://codereview.chromium.org/2629573006/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/device_page/power.js:18: enablePowerSettings: Boolean, if this is false, we probably should bail out in ready() as this UI would be unusable (see navigateToPreviousRoute()) https://codereview.chromium.org/2629573006/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/device_page/power.js:30: * List of available dual-role power sources, if enablePowerSettings_ is on. enablePowerSettings (no underscore) https://codereview.chromium.org/2629573006/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/route.js (right): https://codereview.chromium.org/2629573006/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/route.js:141: r.POWER = r.DEVICE.createChild('/power'); On 2017/03/01 02:43:31, Daniel Erat wrote: > should i reorder this listing to match the order of the rows in the device page? Yeah, I think that's roughly what we were going for here.
https://codereview.chromium.org/2629573006/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/device_page/device_page.html (right): https://codereview.chromium.org/2629573006/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/device_page/device_page.html:106: <template is="dom-if" route-path="/power"> This is a subtle issue I just learned about: Since #powerRow is conditional on if="[[enablePowerSettings_]], we need to set the following here: no-search$="[[!enablePowerSettings_]] Otherwise the search code will look for #powerRow and throw an error when it fails to find it. It would also be possible to put this subpage (including the route-path dom-if) inside a dom-if with if="[[enablePowerSettings_]], however that may break direct navigation, depending on how enablePowerSettings_ is set, so I would recommend just setting no-search. https://codereview.chromium.org/2629573006/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/device_page/device_page.js (right): https://codereview.chromium.org/2629573006/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/device_page/device_page.js:29: hasMouse_: {type: Boolean, value: false}, On 2017/03/01 05:20:38, michaelpg wrote: > On 2017/03/01 02:43:31, Daniel Erat wrote: > > "git cl format --js" did this. let me know if you'd prefer that i revert the > > parts that are irrelevant to my change. > > no worries, but you'll actually need to rebase two of these on stevenjb's fix: > https://codereview.chromium.org/2727443003/ Also, if you want this on multuple lines (and we generally prefer that, at least I do), add a trailing comma after 'value: false'. e.g. see 'prefs' above. https://codereview.chromium.org/2629573006/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/device_page/power.js (right): https://codereview.chromium.org/2629573006/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/device_page/power.js:61: if (this.enablePowerSettings) { In practice this is fine since enablePowerSettings comes from loadTimeData which will always be set before this is attached. However since this is provided by device-page it would be nice to add a comment here that enablePowerSettings comes from loadTimeData. (Alternately this could be moved to an enablePowerSettings listener). Also: invert and early exit and, as Michael suggests, close the subpage if !enablePowerSettings (or do that in ready() which gets called first: https://www.polymer-project.org/1.0/docs/devguide/registering-elements).
https://codereview.chromium.org/2629573006/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/device_page/device_page.html (right): https://codereview.chromium.org/2629573006/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/device_page/device_page.html:2: <link rel="import" href="chrome://resources/html/md_select_css.html"> On 2017/03/01 05:20:38, michaelpg wrote: > unused, move to power.html Done. https://codereview.chromium.org/2629573006/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/device_page/device_page.html:22: <style include="settings-shared md-select"></style> On 2017/03/01 05:20:38, michaelpg wrote: > "md-select" styles no longer apply here Done. https://codereview.chromium.org/2629573006/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/device_page/device_page.html:106: <template is="dom-if" route-path="/power"> On 2017/03/01 17:19:43, stevenjb wrote: > This is a subtle issue I just learned about: > Since #powerRow is conditional on if="[[enablePowerSettings_]], we need to set > the following here: > > no-search$="[[!enablePowerSettings_]] > > Otherwise the search code will look for #powerRow and throw an error when it > fails to find it. > > It would also be possible to put this subpage (including the route-path dom-if) > inside a dom-if with if="[[enablePowerSettings_]], however that may break direct > navigation, depending on how enablePowerSettings_ is set, so I would recommend > just setting no-search. Done. https://codereview.chromium.org/2629573006/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/device_page/device_page.js (right): https://codereview.chromium.org/2629573006/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/device_page/device_page.js:29: hasMouse_: {type: Boolean, value: false}, On 2017/03/01 17:19:43, stevenjb wrote: > On 2017/03/01 05:20:38, michaelpg wrote: > > On 2017/03/01 02:43:31, Daniel Erat wrote: > > > "git cl format --js" did this. let me know if you'd prefer that i revert the > > > parts that are irrelevant to my change. > > > > no worries, but you'll actually need to rebase two of these on stevenjb's fix: > > https://codereview.chromium.org/2727443003/ > > Also, if you want this on multuple lines (and we generally prefer that, at least > I do), add a trailing comma after 'value: false'. e.g. see 'prefs' above. > done (added trailing commas... it's going to be tough to train myself to stop avoiding those in JS) https://codereview.chromium.org/2629573006/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/device_page/device_page.js:117: onPowerTap_: function() { On 2017/03/01 05:20:38, michaelpg wrote: > opt. nit: place in the same order as the HTML elements that reference these > (after onStorageTap_, looks like) Done. https://codereview.chromium.org/2629573006/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/device_page/power.html (right): https://codereview.chromium.org/2629573006/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/device_page/power.html:4: <link rel="import" href="../settings_shared_css.html"> On 2017/03/01 05:20:38, michaelpg wrote: > import what you use (eg md_select_css.html, i18n_behavior.html). basically > several of the imports in device_page.html. Done. https://codereview.chromium.org/2629573006/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/device_page/power.js (right): https://codereview.chromium.org/2629573006/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/device_page/power.js:18: enablePowerSettings: Boolean, On 2017/03/01 05:20:39, michaelpg wrote: > if this is false, we probably should bail out in ready() as this UI would be > unusable (see navigateToPreviousRoute()) done (i think; let me know if i got it wrong) https://codereview.chromium.org/2629573006/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/device_page/power.js:30: * List of available dual-role power sources, if enablePowerSettings_ is on. On 2017/03/01 05:20:39, michaelpg wrote: > enablePowerSettings (no underscore) Done. https://codereview.chromium.org/2629573006/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/device_page/power.js:61: if (this.enablePowerSettings) { On 2017/03/01 17:19:43, stevenjb wrote: > In practice this is fine since enablePowerSettings comes from loadTimeData which > will always be set before this is attached. However since this is provided by > device-page it would be nice to add a comment here that enablePowerSettings > comes from loadTimeData. (Alternately this could be moved to an > enablePowerSettings listener). > Also: invert and early exit and, as Michael suggests, close the subpage if > !enablePowerSettings (or do that in ready() which gets called first: > https://www.polymer-project.org/1.0/docs/devguide/registering-elements). Done. https://codereview.chromium.org/2629573006/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/route.js (right): https://codereview.chromium.org/2629573006/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/route.js:141: r.POWER = r.DEVICE.createChild('/power'); On 2017/03/01 05:20:39, michaelpg wrote: > On 2017/03/01 02:43:31, Daniel Erat wrote: > > should i reorder this listing to match the order of the rows in the device > page? > > Yeah, I think that's roughly what we were going for here. 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
lgtm https://codereview.chromium.org/2629573006/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/device_page/device_page.js (right): https://codereview.chromium.org/2629573006/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/device_page/device_page.js:29: hasMouse_: {type: Boolean, value: false}, On 2017/03/01 23:01:53, Daniel Erat wrote: > On 2017/03/01 17:19:43, stevenjb wrote: > > On 2017/03/01 05:20:38, michaelpg wrote: > > > On 2017/03/01 02:43:31, Daniel Erat wrote: > > > > "git cl format --js" did this. let me know if you'd prefer that i revert > the > > > > parts that are irrelevant to my change. > > > > > > no worries, but you'll actually need to rebase two of these on stevenjb's > fix: > > > https://codereview.chromium.org/2727443003/ > > > > Also, if you want this on multuple lines (and we generally prefer that, at > least > > I do), add a trailing comma after 'value: false'. e.g. see 'prefs' above. > > > > done (added trailing commas... it's going to be tough to train myself to stop > avoiding those in JS) Oh, BTW, this changed in ToT, you'll need to rebase. It has the trailing ',' where it matters now FWIW.
lgtm https://codereview.chromium.org/2629573006/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/device_page/device_page.js (right): https://codereview.chromium.org/2629573006/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/device_page/device_page.js:31: value: false, (just make sure this gets rebased correctly to avoid inadvertently reverting https://codereview.chromium.org/2727443003/)
https://codereview.chromium.org/2629573006/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/device_page/device_page.js (right): https://codereview.chromium.org/2629573006/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/device_page/device_page.js:31: value: false, On 2017/03/01 23:39:12, michaelpg wrote: > (just make sure this gets rebased correctly to avoid inadvertently reverting > https://codereview.chromium.org/2727443003/) thanks for the heads-up.
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 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 stevenjb@chromium.org, michaelpg@chromium.org Link to the patchset: https://codereview.chromium.org/2629573006/#ps80001 (title: "merge")
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": 80001, "attempt_start_ts": 1488416945495140,
"parent_rev": "6e7ba20e77bee4d77a97d817c2cbde507bd16211", "commit_rev":
"8f2dc13d423abd8414e6bd5624aa516fcb1e683d"}
Message was sent while issue was closed.
Description was changed from ========== chromeos: Add Power device page to chrome://md-settings. Move the power row into its own subpage in anticipation of adding additional settings. BUG=633484,633455 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== chromeos: Add Power device page to chrome://md-settings. Move the power row into its own subpage in anticipation of adding additional settings. BUG=633484,633455 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2629573006 Cr-Commit-Position: refs/heads/master@{#454136} Committed: https://chromium.googlesource.com/chromium/src/+/8f2dc13d423abd8414e6bd5624aa... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/8f2dc13d423abd8414e6bd5624aa...
Message was sent while issue was closed.
dbeam@chromium.org changed reviewers: + dbeam@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2629573006/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/device_page/power.js (right): https://codereview.chromium.org/2629573006/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/device_page/power.js:74: settings.DevicePageBrowserProxyImpl.getInstance().updatePowerStatus(); this should only be called if (this.enablePowerSettings) this is causing an unhandled chrome.send() messages (and hitting NOTREACHED() in web_ui_impl.cc)
Message was sent while issue was closed.
https://codereview.chromium.org/2629573006/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/device_page/power.js (right): https://codereview.chromium.org/2629573006/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/device_page/power.js:74: settings.DevicePageBrowserProxyImpl.getInstance().updatePowerStatus(); On 2017/03/14 21:50:09, Dan Beam wrote: > this should only be called if (this.enablePowerSettings) > > this is causing an unhandled chrome.send() messages (and hitting NOTREACHED() in > web_ui_impl.cc) thanks for pointing that out. i'll send a fix. but i'm also a bit confused, since it sounded (from what steven said, i think) like this would never be attached if the settings were disabled.
Message was sent while issue was closed.
https://codereview.chromium.org/2629573006/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/device_page/power.js (right): https://codereview.chromium.org/2629573006/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/device_page/power.js:74: settings.DevicePageBrowserProxyImpl.getInstance().updatePowerStatus(); On 2017/03/14 22:08:16, Daniel Erat wrote: > On 2017/03/14 21:50:09, Dan Beam wrote: > > this should only be called if (this.enablePowerSettings) > > > > this is causing an unhandled chrome.send() messages (and hitting NOTREACHED() > in > > web_ui_impl.cc) > > thanks for pointing that out. i'll send a fix. > > but i'm also a bit confused, since it sounded (from what steven said, i think) > like this would never be attached if the settings were disabled. dbeam@ - This should be protected by a dom-if, in device_page.html?
Message was sent while issue was closed.
https://codereview.chromium.org/2629573006/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/device_page/power.js (right): https://codereview.chromium.org/2629573006/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/device_page/power.js:74: settings.DevicePageBrowserProxyImpl.getInstance().updatePowerStatus(); On 2017/03/14 22:17:44, stevenjb wrote: > On 2017/03/14 22:08:16, Daniel Erat wrote: > > On 2017/03/14 21:50:09, Dan Beam wrote: > > > this should only be called if (this.enablePowerSettings) > > > > > > this is causing an unhandled chrome.send() messages (and hitting > NOTREACHED() > > in > > > web_ui_impl.cc) > > > > thanks for pointing that out. i'll send a fix. > > > > but i'm also a bit confused, since it sounded (from what steven said, i think) > > like this would never be attached if the settings were disabled. > > dbeam@ - This should be protected by a dom-if, in device_page.html? search for device -> asplode
Message was sent while issue was closed.
https://codereview.chromium.org/2629573006/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/device_page/power.js (right): https://codereview.chromium.org/2629573006/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/device_page/power.js:74: settings.DevicePageBrowserProxyImpl.getInstance().updatePowerStatus(); On 2017/03/14 22:27:24, Dan Beam wrote: > On 2017/03/14 22:17:44, stevenjb wrote: > > On 2017/03/14 22:08:16, Daniel Erat wrote: > > > On 2017/03/14 21:50:09, Dan Beam wrote: > > > > this should only be called if (this.enablePowerSettings) > > > > > > > > this is causing an unhandled chrome.send() messages (and hitting > > NOTREACHED() > > > in > > > > web_ui_impl.cc) > > > > > > thanks for pointing that out. i'll send a fix. > > > > > > but i'm also a bit confused, since it sounded (from what steven said, i > think) > > > like this would never be attached if the settings were disabled. > > > > dbeam@ - This should be protected by a dom-if, in device_page.html? > > search for device -> asplode yep, i'm able to reproduce this.
Message was sent while issue was closed.
https://codereview.chromium.org/2629573006/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/device_page/power.js (right): https://codereview.chromium.org/2629573006/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/device_page/power.js:74: settings.DevicePageBrowserProxyImpl.getInstance().updatePowerStatus(); On 2017/03/14 22:27:24, Dan Beam wrote: > On 2017/03/14 22:17:44, stevenjb wrote: > > On 2017/03/14 22:08:16, Daniel Erat wrote: > > > On 2017/03/14 21:50:09, Dan Beam wrote: > > > > this should only be called if (this.enablePowerSettings) > > > > > > > > this is causing an unhandled chrome.send() messages (and hitting > > NOTREACHED() > > > in > > > > web_ui_impl.cc) > > > > > > thanks for pointing that out. i'll send a fix. > > > > > > but i'm also a bit confused, since it sounded (from what steven said, i > think) > > > like this would never be attached if the settings were disabled. > > > > dbeam@ - This should be protected by a dom-if, in device_page.html? > > search for device -> asplode I wasn't so much looking for a repro as for an understanding as to why this occurs so that we can avoid it in the future, although mentioning 'search' triggers a vague recollection that we expand all templates not marked as no search? So if that is the case, a quick fix might be to mark this page as no search for now, and/or create a compound if for the section that includes enablePowerSettings_.
Message was sent while issue was closed.
https://codereview.chromium.org/2629573006/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/device_page/power.js (right): https://codereview.chromium.org/2629573006/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/device_page/power.js:74: settings.DevicePageBrowserProxyImpl.getInstance().updatePowerStatus(); On 2017/03/14 23:08:14, stevenjb wrote: > On 2017/03/14 22:27:24, Dan Beam wrote: > > On 2017/03/14 22:17:44, stevenjb wrote: > > > On 2017/03/14 22:08:16, Daniel Erat wrote: > > > > On 2017/03/14 21:50:09, Dan Beam wrote: > > > > > this should only be called if (this.enablePowerSettings) > > > > > > > > > > this is causing an unhandled chrome.send() messages (and hitting > > > NOTREACHED() > > > > in > > > > > web_ui_impl.cc) > > > > > > > > thanks for pointing that out. i'll send a fix. > > > > > > > > but i'm also a bit confused, since it sounded (from what steven said, i > > think) > > > > like this would never be attached if the settings were disabled. > > > > > > dbeam@ - This should be protected by a dom-if, in device_page.html? > > > > search for device -> asplode > > I wasn't so much looking for a repro as for an understanding as to why this > occurs so that we can avoid it in the future, although mentioning 'search' > triggers a vague recollection that we expand all templates not marked as no > search? So if that is the case, a quick fix might be to mark this page as no > search for now, and/or create a compound if for the section that includes > enablePowerSettings_. > > or just wrap this code in an if (this.enablePowerSettings) again
Message was sent while issue was closed.
https://codereview.chromium.org/2629573006/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/device_page/power.js (right): https://codereview.chromium.org/2629573006/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/device_page/power.js:74: settings.DevicePageBrowserProxyImpl.getInstance().updatePowerStatus(); On 2017/03/14 23:17:39, Dan Beam wrote: > On 2017/03/14 23:08:14, stevenjb wrote: > > On 2017/03/14 22:27:24, Dan Beam wrote: > > > On 2017/03/14 22:17:44, stevenjb wrote: > > > > On 2017/03/14 22:08:16, Daniel Erat wrote: > > > > > On 2017/03/14 21:50:09, Dan Beam wrote: > > > > > > this should only be called if (this.enablePowerSettings) > > > > > > > > > > > > this is causing an unhandled chrome.send() messages (and hitting > > > > NOTREACHED() > > > > > in > > > > > > web_ui_impl.cc) > > > > > > > > > > thanks for pointing that out. i'll send a fix. > > > > > > > > > > but i'm also a bit confused, since it sounded (from what steven said, i > > > think) > > > > > like this would never be attached if the settings were disabled. > > > > > > > > dbeam@ - This should be protected by a dom-if, in device_page.html? > > > > > > search for device -> asplode > > > > I wasn't so much looking for a repro as for an understanding as to why this > > occurs so that we can avoid it in the future, although mentioning 'search' > > triggers a vague recollection that we expand all templates not marked as no > > search? So if that is the case, a quick fix might be to mark this page as no > > search for now, and/or create a compound if for the section that includes > > enablePowerSettings_. > > > > > > or just wrap this code in an if (this.enablePowerSettings) again uploaded https://codereview.chromium.org/2751873002/ |
