Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2888103002:
Speed up the Incognito NTP reload (Closed)
DescriptionSpeed up the Incognito NTP reload
As described in crbug.com/715042, the new Incognito NTP sometimes
flickers on reload. This is likely caused by the fact that it uses
JavaScript when computing layout, including two relatively heavy
offsetWidth operations which cause a reflow.
However, offsetWidth is constant for a given default font size, and only
needs to be computed once. It can be then stored in localStorage for
the duration of the Incognito session.
Testing on my local debug build shows that the JavaScript execution is
now sped up from ~22ms to ~5ms.
BUG=715042
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2888103002
Cr-Commit-Position: refs/heads/master@{#473176}
Committed: https://chromium.googlesource.com/chromium/src/+/24526e022f84a2a625012160b818eef0ebd0b8de
Patch Set 1 #
Total comments: 8
Patch Set 2 : Fixed flickering. #
Total comments: 3
Patch Set 3 : Use the fontSize attribute directly. #Messages
Total messages: 20 (11 generated)
Description was changed from ========== Speed up the Incognito NTP reload As described in crbug.com/715042, the new Incognito NTP sometimes flickers on reload. This is likely caused by the fact that it uses JavaScript when computing layout, including two relatively heavy offsetWidth operations which cause a reflow. However, offsetWidth is constant for a given default font size, and only needs to be computed once. It can be then stored in localStorage for the duration of the Incognito session. Testing on my local debug build shows that the JavaScript execution is now sped up from ~22ms to ~5ms. BUG=715042 ========== to ========== Speed up the Incognito NTP reload As described in crbug.com/715042, the new Incognito NTP sometimes flickers on reload. This is likely caused by the fact that it uses JavaScript when computing layout, including two relatively heavy offsetWidth operations which cause a reflow. However, offsetWidth is constant for a given default font size, and only needs to be computed once. It can be then stored in localStorage for the duration of the Incognito session. Testing on my local debug build shows that the JavaScript execution is now sped up from ~22ms to ~5ms. BUG=715042 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by msramek@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...
Patchset #1 (id:1) has been deleted
msramek@chromium.org changed reviewers: + dbeam@chromium.org
Hi Dan! Since you reviewed my previous Incognito NTP CLs, please have a look at this one as well :) Just one polishing idea I had. Thanks, Martin
https://codereview.chromium.org/2888103002/diff/20001/chrome/browser/resource... File chrome/browser/resources/ntp4/md_incognito_tab.js (right): https://codereview.chromium.org/2888103002/diff/20001/chrome/browser/resource... chrome/browser/resources/ntp4/md_incognito_tab.js:30: document.defaultView.getComputedStyle(document.body, null) i don't really see a difference between document.defaultView.getComputedStyle() and window.getComputedStyle() in this case (which is shorter and more common). i also don't think you need the "null" arg https://codereview.chromium.org/2888103002/diff/20001/chrome/browser/resource... chrome/browser/resources/ntp4/md_incognito_tab.js:31: .getPropertyValue("font-size"); does zoom affect this value? https://codereview.chromium.org/2888103002/diff/20001/chrome/browser/resource... chrome/browser/resources/ntp4/md_incognito_tab.js:32: var savedMaxWidth = window.localStorage.getItem(fontSize); can this all just be: var fontSize = window.getComputedStyle(document.body).fontSize; var maxWidth = localStorage[fontSize] || bulletpoints[0].offsetWidth + bulletpoints[1].offsetWidth + 42; ? https://codereview.chromium.org/2888103002/diff/20001/chrome/browser/resource... chrome/browser/resources/ntp4/md_incognito_tab.js:55: window.localStorage.setItem(fontSize, maxWidth); localStorage[fontSize] = maxWidth;
https://codereview.chromium.org/2888103002/diff/20001/chrome/browser/resource... File chrome/browser/resources/ntp4/md_incognito_tab.js (right): https://codereview.chromium.org/2888103002/diff/20001/chrome/browser/resource... chrome/browser/resources/ntp4/md_incognito_tab.js:30: document.defaultView.getComputedStyle(document.body, null) On 2017/05/17 18:09:04, Dan Beam wrote: > i don't really see a difference between document.defaultView.getComputedStyle() > and window.getComputedStyle() in this case (which is shorter and more common). > i also don't think you need the "null" arg Done. Simplified, thanks. https://codereview.chromium.org/2888103002/diff/20001/chrome/browser/resource... chrome/browser/resources/ntp4/md_incognito_tab.js:31: .getPropertyValue("font-size"); On 2017/05/17 18:09:04, Dan Beam wrote: > does zoom affect this value? Good point :) I tested that neither the computed style font size nor the offsetWidth are affected by zooming, so the caching should be valid. https://codereview.chromium.org/2888103002/diff/20001/chrome/browser/resource... chrome/browser/resources/ntp4/md_incognito_tab.js:32: var savedMaxWidth = window.localStorage.getItem(fontSize); On 2017/05/17 18:09:04, Dan Beam wrote: > can this all just be: > > var fontSize = window.getComputedStyle(document.body).fontSize; > var maxWidth = localStorage[fontSize] || > bulletpoints[0].offsetWidth + bulletpoints[1].offsetWidth + 42; > > ? Done. Yes, but I'd like to keep the comments to explain the constants. https://codereview.chromium.org/2888103002/diff/20001/chrome/browser/resource... chrome/browser/resources/ntp4/md_incognito_tab.js:55: window.localStorage.setItem(fontSize, maxWidth); On 2017/05/17 18:09:04, Dan Beam wrote: > localStorage[fontSize] = maxWidth; Done. https://codereview.chromium.org/2888103002/diff/40001/chrome/browser/resource... File chrome/browser/resources/ntp4/md_incognito_tab.js (right): https://codereview.chromium.org/2888103002/diff/40001/chrome/browser/resource... chrome/browser/resources/ntp4/md_incognito_tab.js:38: localStorage[fontSize] = maxWidth; I fixed the ordering here. We want to save the result to the local storage *before* capping it at 600 (line 44), so that when we restore it from the localStorage, we can again apply the class 'tooWide' (line 46).
lgtm https://codereview.chromium.org/2888103002/diff/40001/chrome/browser/resource... File chrome/browser/resources/ntp4/md_incognito_tab.js (right): https://codereview.chromium.org/2888103002/diff/40001/chrome/browser/resource... chrome/browser/resources/ntp4/md_incognito_tab.js:30: window.getComputedStyle(document.body).getPropertyValue("font-size"); don't really get why you prefer getPropertyValue('font-size') over .fontSize, but if you do stay with this (for some reason), you shouldn't be using double-quotes
Thanks, Dan! https://codereview.chromium.org/2888103002/diff/40001/chrome/browser/resource... File chrome/browser/resources/ntp4/md_incognito_tab.js (right): https://codereview.chromium.org/2888103002/diff/40001/chrome/browser/resource... chrome/browser/resources/ntp4/md_incognito_tab.js:30: window.getComputedStyle(document.body).getPropertyValue("font-size"); On 2017/05/19 01:09:32, Dan Beam wrote: > don't really get why you prefer getPropertyValue('font-size') over .fontSize, > but if you do stay with this (for some reason), you shouldn't be using > double-quotes Fixed. Sorry, I missed that in your original comment. I wrote this while reading a documentation which suggests getPropertyValue() probably for cross-browser reasons.
The CQ bit was checked by msramek@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/2888103002/#ps60001 (title: "Use the fontSize attribute directly.")
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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by msramek@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": 60001, "attempt_start_ts": 1495195389107910,
"parent_rev": "61e63ff014b59a17d7f3edf860481f4e8b756617", "commit_rev":
"24526e022f84a2a625012160b818eef0ebd0b8de"}
Message was sent while issue was closed.
Description was changed from ========== Speed up the Incognito NTP reload As described in crbug.com/715042, the new Incognito NTP sometimes flickers on reload. This is likely caused by the fact that it uses JavaScript when computing layout, including two relatively heavy offsetWidth operations which cause a reflow. However, offsetWidth is constant for a given default font size, and only needs to be computed once. It can be then stored in localStorage for the duration of the Incognito session. Testing on my local debug build shows that the JavaScript execution is now sped up from ~22ms to ~5ms. BUG=715042 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Speed up the Incognito NTP reload As described in crbug.com/715042, the new Incognito NTP sometimes flickers on reload. This is likely caused by the fact that it uses JavaScript when computing layout, including two relatively heavy offsetWidth operations which cause a reflow. However, offsetWidth is constant for a given default font size, and only needs to be computed once. It can be then stored in localStorage for the duration of the Incognito session. Testing on my local debug build shows that the JavaScript execution is now sped up from ~22ms to ~5ms. BUG=715042 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2888103002 Cr-Commit-Position: refs/heads/master@{#473176} Committed: https://chromium.googlesource.com/chromium/src/+/24526e022f84a2a625012160b818... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001) as https://chromium.googlesource.com/chromium/src/+/24526e022f84a2a625012160b818... |
