Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2751873002:
chromeos: Avoid NOTREACHED for MD settings power page. (Closed)
|
Created:
3 years, 9 months ago by Daniel Erat Modified:
3 years, 9 months ago CC:
chromium-reviews, arv+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionchromeos: Avoid NOTREACHED for MD settings power page.
Avoid hitting a NOTREACHED() when searching for "power" or
"device" at chrome://md-settings when power settings are
disabled.
BUG=633484
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2751873002
Cr-Commit-Position: refs/heads/master@{#456966}
Committed: https://chromium.googlesource.com/chromium/src/+/382a85f9de9bff0c3ec98dc2ba37d665f5848e70
Patch Set 1 #
Total comments: 3
Patch Set 2 : move no-search$ #Messages
Total messages: 23 (11 generated)
Description was changed from ========== chromeos: Avoid NOTREACHED for MD settings power page. Avoid hitting a NOTREACHED() when searching for "power" or "device" at chrome://md-settings when power settings are disabled. Apparently the settings-power element is still attached in this case. BUG=633484 ========== to ========== chromeos: Avoid NOTREACHED for MD settings power page. Avoid hitting a NOTREACHED() when searching for "power" or "device" at chrome://md-settings when power settings are disabled. Apparently the settings-power element is still attached in this case. BUG=633484 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
derat@chromium.org changed reviewers: + dbeam@chromium.org, michaelpg@chromium.org, stevenjb@chromium.org
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...
you could also potentially slap a no-search higher up our search is only guaranteed to find UI strings (as in labels and things), not dynamic content. if most of this power stuff is dynamic, it may not need to be searched anyways https://codereview.chromium.org/2751873002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/device_page/power.js (right): https://codereview.chromium.org/2751873002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/device_page/power.js:79: settings.DevicePageBrowserProxyImpl.getInstance().updatePowerStatus(); i think you only want to wrap this line, but i'm not sure
https://codereview.chromium.org/2751873002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/device_page/power.js (right): https://codereview.chromium.org/2751873002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/device_page/power.js:79: settings.DevicePageBrowserProxyImpl.getInstance().updatePowerStatus(); On 2017/03/15 00:30:58, Dan Beam wrote: > i think you only want to wrap this line, but i'm not sure hopefully one of the other reviewers will have a strong opinion about this, because i don't. :-)
This is just a temporary workaround so I have no preference how we solve this; lgtm. If you really want an opinion I would do something like this: https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/device...
On 2017/03/15 00:30:58, Dan Beam wrote: > you could also potentially slap a no-search higher up > > our search is only guaranteed to find UI strings (as in labels and things), not > dynamic content. if most of this power stuff is dynamic, it may not need to be > searched anyways Well, it would be nice to show power source in Search when it's actually available, but we shouldn't be showing this UI at all when it isn't available. Wait... we already have no-search$="[[!enablePowerSettings_]]". Why isn't that working? https://codereview.chromium.org/2751873002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/device_page/power.js (right): https://codereview.chromium.org/2751873002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/device_page/power.js:79: settings.DevicePageBrowserProxyImpl.getInstance().updatePowerStatus(); On 2017/03/15 00:33:44, Daniel Erat wrote: > On 2017/03/15 00:30:58, Dan Beam wrote: > > i think you only want to wrap this line, but i'm not sure Nah, we should return early. Hopefully the comment in ready() helps. If !enablePowerSettings, there shouldn't be any updates to listen to. But that's another reason we shouldn't let this get attached in the first place. > > hopefully one of the other reviewers will have a strong opinion about this, > because i don't. :-)
On 2017/03/15 01:13:19, michaelpg wrote: > On 2017/03/15 00:30:58, Dan Beam wrote: > > you could also potentially slap a no-search higher up > > > > our search is only guaranteed to find UI strings (as in labels and things), > not > > dynamic content. if most of this power stuff is dynamic, it may not need to > be > > searched anyways > > Well, it would be nice to show power source in Search when it's actually > available, but we shouldn't be showing this UI at all when it isn't available. > > Wait... we already have no-search$="[[!enablePowerSettings_]]". Why isn't that > working? the existing no-search$ is on the settings-subpage element. moving it up to the template appears to fix this.
The CQ bit was checked by derat@chromium.org to run a CQ dry run
Description was changed from ========== chromeos: Avoid NOTREACHED for MD settings power page. Avoid hitting a NOTREACHED() when searching for "power" or "device" at chrome://md-settings when power settings are disabled. Apparently the settings-power element is still attached in this case. BUG=633484 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== chromeos: Avoid NOTREACHED for MD settings power page. Avoid hitting a NOTREACHED() when searching for "power" or "device" at chrome://md-settings when power settings are disabled. BUG=633484 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Aha! Cheers. LGTM
The CQ bit was unchecked by derat@chromium.org
The CQ bit was checked by derat@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm2
On 2017/03/15 01:33:50, michaelpg wrote: > lgtm2 lgtm 2
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1489541134023500,
"parent_rev": "b7d677fad92b5e6065bc3ea374425101074b6a4b", "commit_rev":
"382a85f9de9bff0c3ec98dc2ba37d665f5848e70"}
Message was sent while issue was closed.
Description was changed from ========== chromeos: Avoid NOTREACHED for MD settings power page. Avoid hitting a NOTREACHED() when searching for "power" or "device" at chrome://md-settings when power settings are disabled. BUG=633484 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== chromeos: Avoid NOTREACHED for MD settings power page. Avoid hitting a NOTREACHED() when searching for "power" or "device" at chrome://md-settings when power settings are disabled. BUG=633484 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2751873002 Cr-Commit-Position: refs/heads/master@{#456966} Committed: https://chromium.googlesource.com/chromium/src/+/382a85f9de9bff0c3ec98dc2ba37... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/382a85f9de9bff0c3ec98dc2ba37...
Message was sent while issue was closed.
lgtm, thanks for fixing! |
