Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2916383002:
Settings: About: Fix restart and powerwash button visibility (Closed)
|
Created:
3 years, 6 months ago by stevenjb Modified:
3 years, 6 months ago Reviewers:
dpapad CC:
arv+watch_chromium.org, chromium-reviews, dbeam+watch-settings_chromium.org, michaelpg+watch-md-settings_chromium.org, michaelpg+watch-md-ui_chromium.org, stevenjb+watch-md-settings_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionSettings: About: Fix restart and powerwash button visibility
Currently if |currentChannel_| changes after |targetChannel_|
computeShowRelaunchAndPowerwash_() will not get called and the
button visibility may be incorrect. Fix the
computeShowRelaunchAndPowerwash_() params so that they share the same logic.
BUG=724518
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2916383002
Cr-Commit-Position: refs/heads/master@{#476867}
Committed: https://chromium.googlesource.com/chromium/src/+/038795765171270a268fde65453f5af66d914061
Patch Set 1 #Patch Set 2 : Smaller delta #Messages
Total messages: 20 (9 generated)
Description was changed from ========== Settings: About: Update restart button visibility consistently Currently if |currentChannel_| changes after |targetChannel_| computeShowRelaunchAndPowerwash_() will not get called and the button visibility may be incorrect. Instead of fixing the computeShowRelaunchAndPowerwash_() params, update visibity for both in updateShowRelaunch_() so that they stay in sync. BUG=724518 ========== to ========== Settings: About: Update restart button visibility consistently Currently if |currentChannel_| changes after |targetChannel_| computeShowRelaunchAndPowerwash_() will not get called and the button visibility may be incorrect. Instead of fixing the computeShowRelaunchAndPowerwash_() params, update visibity for both in updateShowRelaunch_() so that they stay in sync. BUG=724518 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
stevenjb@chromium.org changed reviewers: + dpapad@chromium.org
Where you able to reproduce this locally? Can we complement our existing tests at https://cs.chromium.org/chromium/src/chrome/test/data/webui/settings/about_pa... to cover this case? Also at first glance, it seems an smaller fix (and less risky IMO), to simply add the missing |currentChannel| dependency at https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/about_..., than the existing fix proposal. From the code it is clear that computeShowRelaunchAndPowerwash_ calls isTargetChannelMoreStable_ which depends on both |currentChannel| and |targetChannel|, and therefore reveals the missing dependency.
On 2017/06/02 19:45:45, dpapad wrote: > Where you able to reproduce this locally? Can we complement our existing tests > at > https://cs.chromium.org/chromium/src/chrome/test/data/webui/settings/about_pa... > to cover this case? > > Also at first glance, it seems an smaller fix (and less risky IMO), to simply > add the missing |currentChannel| dependency at > https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/about_..., > than the existing fix proposal. From the code it is clear that > computeShowRelaunchAndPowerwash_ calls isTargetChannelMoreStable_ which depends > on both |currentChannel| and |targetChannel|, and therefore reveals the missing > dependency. We don't have a repro. Probably related to changing channel, but also appears to be an edge case. While I agree that adding currentChannel to computeShowRelaunchAndPowerwash_ is arguably a simpler fix, I feel that having it set separately is more error prone (and probably how the bug regressed in the first place). That's why I prefer this approach. If you think fixing computeShowRelaunchAndPowerwash_ is better, sure, I can do that. It hardly seems worth the time to discuss to me.
PTAL
Patch #2 LGTM. Please update the CL description since it is obsolete as of patch #2. Having said that, I generally suggest reproducing a bug before speculatively fixing it (even though the fix seems correct), because this can reveal more issues with the code. For example, based on what the CL description says "if |currentChannel_| changes after |targetChannel_|", the only way this can happen in the code is if the getChannelInfo() callback executes after a 'target-channel-changed' callback has already fired (see [1]), which would be unfortunate, and perhaps we should handle this in a better way if indeed happens (by registering the listener only after the initial call to getChannelInfo() has returned). [1] https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/about_...
I was unable to reproduce this. You may be right. There is clearly a bug in the code. Speculatively fixing that seems like a much better use of both our time. On Fri, Jun 2, 2017 at 2:03 PM, dpapad@chromium.org via codereview.chromium.org <reply@chromiumcodereview-hr.appspotmail.com> wrote: > Patch #2 LGTM. Please update the CL description since it is obsolete as of > patch > #2. > > Having said that, I generally suggest reproducing a bug before > speculatively > fixing it (even though the fix seems correct), because this can reveal more > issues with the code. > > For example, based on what the CL description says "if |currentChannel_| > changes > after |targetChannel_|", the only way this can happen in the code is if the > getChannelInfo() callback executes after a 'target-channel-changed' > callback has > already fired (see [1]), which would be unfortunate, and perhaps we should > handle this in a better way if indeed happens (by registering the listener > only > after the initial call to getChannelInfo() has returned). > > [1] > https://cs.chromium.org/chromium/src/chrome/browser/ > resources/settings/about_page/about_page.js?l=138,142 > > https://codereview.chromium.org/2916383002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Description was changed from ========== Settings: About: Update restart button visibility consistently Currently if |currentChannel_| changes after |targetChannel_| computeShowRelaunchAndPowerwash_() will not get called and the button visibility may be incorrect. Instead of fixing the computeShowRelaunchAndPowerwash_() params, update visibity for both in updateShowRelaunch_() so that they stay in sync. BUG=724518 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Settings: About: Fix restart and powerwash button visibility Currently if |currentChannel_| changes after |targetChannel_| computeShowRelaunchAndPowerwash_() will not get called and the button visibility may be incorrect. Instead of fixing the computeShowRelaunchAndPowerwash_() params, update visibity for both in updateShowRelaunch_() so that they stay in sync. BUG=724518 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by stevenjb@chromium.org
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
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by stevenjb@chromium.org
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": 20001, "attempt_start_ts": 1496445604183560,
"parent_rev": "3ad08a4885eb537aa4c071fa356f6636ae71d9f8", "commit_rev":
"038795765171270a268fde65453f5af66d914061"}
Message was sent while issue was closed.
Description was changed from ========== Settings: About: Fix restart and powerwash button visibility Currently if |currentChannel_| changes after |targetChannel_| computeShowRelaunchAndPowerwash_() will not get called and the button visibility may be incorrect. Instead of fixing the computeShowRelaunchAndPowerwash_() params, update visibity for both in updateShowRelaunch_() so that they stay in sync. BUG=724518 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Settings: About: Fix restart and powerwash button visibility Currently if |currentChannel_| changes after |targetChannel_| computeShowRelaunchAndPowerwash_() will not get called and the button visibility may be incorrect. Instead of fixing the computeShowRelaunchAndPowerwash_() params, update visibity for both in updateShowRelaunch_() so that they stay in sync. BUG=724518 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2916383002 Cr-Commit-Position: refs/heads/master@{#476867} Committed: https://chromium.googlesource.com/chromium/src/+/038795765171270a268fde65453f... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/038795765171270a268fde65453f...
Message was sent while issue was closed.
On 2017/06/02 at 21:03:59, dpapad wrote: > Patch #2 LGTM. Please update the CL description since it is obsolete as of patch #2. ^ CL description is misleading, as it was reflecting patch #1 and not patch #2. Not a huge deal, but please pay attention to reviewers comments besides L-G-T-M, before CQ-ing.
Message was sent while issue was closed.
Description was changed from ========== Settings: About: Fix restart and powerwash button visibility Currently if |currentChannel_| changes after |targetChannel_| computeShowRelaunchAndPowerwash_() will not get called and the button visibility may be incorrect. Instead of fixing the computeShowRelaunchAndPowerwash_() params, update visibity for both in updateShowRelaunch_() so that they stay in sync. BUG=724518 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2916383002 Cr-Commit-Position: refs/heads/master@{#476867} Committed: https://chromium.googlesource.com/chromium/src/+/038795765171270a268fde65453f... ========== to ========== Settings: About: Fix restart and powerwash button visibility Currently if |currentChannel_| changes after |targetChannel_| computeShowRelaunchAndPowerwash_() will not get called and the button visibility may be incorrect. Fix the computeShowRelaunchAndPowerwash_() params so that they share the same logic. BUG=724518 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2916383002 Cr-Commit-Position: refs/heads/master@{#476867} Committed: https://chromium.googlesource.com/chromium/src/+/038795765171270a268fde65453f... ========== |
