Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2421753002:
MD Settings: Eliminate Relaunch button flicker at chrome://md-settings/help. (Closed)
|
Created:
4 years, 2 months ago by dpapad Modified:
4 years, 1 month ago Reviewers:
Dan Beam 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/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMD Settings: Eliminate Relaunch button flicker at chrome://md-settings/help.
BUG=653068
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/dfda11a0d6b97ee2c4da83f2e5add03dcea6189c
Cr-Commit-Position: refs/heads/master@{#429034}
Patch Set 1 #Patch Set 2 : Fix test. #
Total comments: 2
Patch Set 3 : Fix Polymer missing deps. #
Total comments: 2
Patch Set 4 : Nits #
Messages
Total messages: 27 (17 generated)
Description was changed from ========== MD Settings: Reduce flicker in chrome://md-settings/help page. BUG=653068 ========== to ========== MD Settings: Reduce flicker in chrome://md-settings/help page. BUG=653068 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by dpapad@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...
Description was changed from ========== MD Settings: Reduce flicker in chrome://md-settings/help page. BUG=653068 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Reduce flicker when loading chrome://md-settings/help. BUG=653068 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== MD Settings: Reduce flicker when loading chrome://md-settings/help. BUG=653068 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Remove/reduce flicker when loading chrome://md-settings/help. BUG=653068 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Patchset #3 (id:30001) has been deleted
The CQ bit was checked by dpapad@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...
Description was changed from ========== MD Settings: Remove/reduce flicker when loading chrome://md-settings/help. BUG=653068 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Remove Relaunch button flicker in chrome://md-settings/help page. BUG=653068 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== MD Settings: Remove Relaunch button flicker in chrome://md-settings/help page. BUG=653068 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Eliminate Relaunch button flicker in chrome://md-settings/help page. BUG=653068 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
dpapad@chromium.org changed reviewers: + dbeam@chromium.org
Description was changed from ========== MD Settings: Eliminate Relaunch button flicker in chrome://md-settings/help page. BUG=653068 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Eliminate Relaunch button flicker at chrome://md-settings/help. BUG=653068 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
whatever came of this? did we decide this was the best, or did we figure out some hidden$= or <template is="dom-if"> way that was better?
On 2016/10/27 at 01:51:00, dbeam wrote: > whatever came of this? did we decide this was the best, or did we figure out some hidden$= or <template is="dom-if"> way that was better? template dom-if ended up complicating things. Essentially, instead of optimizing for the most common case (no updates available) and showing the UI quickly, it makes every case wait until the information is known (updates/no-updates), so eliminating the flicker that way, just makes the entire page load later. So my suggestion is to go with the existing approach.
https://codereview.chromium.org/2421753002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/about_page/about_page.js (right): https://codereview.chromium.org/2421753002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/about_page/about_page.js:24: }, i really think we should fix whatever made you move this above currentUpdateStatusEvent_, or at minimum add a comment explaining why the order of these properties matters
https://codereview.chromium.org/2421753002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/about_page/about_page.js (right): https://codereview.chromium.org/2421753002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/about_page/about_page.js:24: }, On 2016/10/27 at 19:28:24, Dan Beam wrote: > i really think we should fix whatever made you move this above currentUpdateStatusEvent_, or at minimum add a comment explaining why the order of these properties matters Done. Fixed the polymer properties dependencies and moved this back to where it was.
lgtm https://codereview.chromium.org/2421753002/diff/50001/chrome/browser/resource... File chrome/browser/resources/settings/about_page/about_page.html (right): https://codereview.chromium.org/2421753002/diff/50001/chrome/browser/resource... chrome/browser/resources/settings/about_page/about_page.html:88: src="[[getIconSrc_(obsoleteSystemInfo_, currentUpdateStatusEvent_)]]"> nit: 80 col wrap (you can wrap in a C++ or JS-like way now in binding)
https://codereview.chromium.org/2421753002/diff/50001/chrome/browser/resource... File chrome/browser/resources/settings/about_page/about_page.html (right): https://codereview.chromium.org/2421753002/diff/50001/chrome/browser/resource... chrome/browser/resources/settings/about_page/about_page.html:88: src="[[getIconSrc_(obsoleteSystemInfo_, currentUpdateStatusEvent_)]]"> On 2016/11/01 at 06:39:48, Dan Beam wrote: > nit: 80 col wrap (you can wrap in a C++ or JS-like way now in binding) Done.
The CQ bit was checked by dpapad@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org Link to the patchset: https://codereview.chromium.org/2421753002/#ps70001 (title: "Nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #4 (id:70001)
Message was sent while issue was closed.
Description was changed from ========== MD Settings: Eliminate Relaunch button flicker at chrome://md-settings/help. BUG=653068 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Eliminate Relaunch button flicker at chrome://md-settings/help. BUG=653068 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/dfda11a0d6b97ee2c4da83f2e5add03dcea6189c Cr-Commit-Position: refs/heads/master@{#429034} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/dfda11a0d6b97ee2c4da83f2e5add03dcea6189c Cr-Commit-Position: refs/heads/master@{#429034} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
