Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(9)

Issue 2888103002: Speed up the Incognito NTP reload (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 month, 1 week ago by msramek (slow)
Modified:
1 month, 1 week ago
CC:
chromium-reviews, dbeam+watch-ntp_chromium.org, arv+watch_chromium.org, pedrosimonetti+watch_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

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/+/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)
msramek (slow)
Hi Dan! Since you reviewed my previous Incognito NTP CLs, please have a look at ...
1 month, 1 week ago (2017-05-17 12:06:49 UTC) #6
Dan Beam (no longer on Chrome)
https://codereview.chromium.org/2888103002/diff/20001/chrome/browser/resources/ntp4/md_incognito_tab.js File chrome/browser/resources/ntp4/md_incognito_tab.js (right): https://codereview.chromium.org/2888103002/diff/20001/chrome/browser/resources/ntp4/md_incognito_tab.js#newcode30 chrome/browser/resources/ntp4/md_incognito_tab.js:30: document.defaultView.getComputedStyle(document.body, null) i don't really see a difference between ...
1 month, 1 week ago (2017-05-17 18:09:04 UTC) #7
msramek (slow)
https://codereview.chromium.org/2888103002/diff/20001/chrome/browser/resources/ntp4/md_incognito_tab.js File chrome/browser/resources/ntp4/md_incognito_tab.js (right): https://codereview.chromium.org/2888103002/diff/20001/chrome/browser/resources/ntp4/md_incognito_tab.js#newcode30 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: > ...
1 month, 1 week ago (2017-05-18 15:16:29 UTC) #8
Dan Beam (no longer on Chrome)
lgtm https://codereview.chromium.org/2888103002/diff/40001/chrome/browser/resources/ntp4/md_incognito_tab.js File chrome/browser/resources/ntp4/md_incognito_tab.js (right): https://codereview.chromium.org/2888103002/diff/40001/chrome/browser/resources/ntp4/md_incognito_tab.js#newcode30 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') ...
1 month, 1 week ago (2017-05-19 01:09:33 UTC) #9
msramek (slow)
Thanks, Dan! https://codereview.chromium.org/2888103002/diff/40001/chrome/browser/resources/ntp4/md_incognito_tab.js File chrome/browser/resources/ntp4/md_incognito_tab.js (right): https://codereview.chromium.org/2888103002/diff/40001/chrome/browser/resources/ntp4/md_incognito_tab.js#newcode30 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: ...
1 month, 1 week ago (2017-05-19 09:50:59 UTC) #10
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2888103002/60001
1 month, 1 week ago (2017-05-19 09:51:25 UTC) #13
commit-bot: I haz the power
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_ng/builds/449749)
1 month, 1 week ago (2017-05-19 12:01:13 UTC) #15
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2888103002/60001
1 month, 1 week ago (2017-05-19 12:03:29 UTC) #17
commit-bot: I haz the power
1 month, 1 week ago (2017-05-19 13:27:53 UTC) #20
Message was sent while issue was closed.
Committed patchset #3 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/24526e022f84a2a625012160b818...
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld cb946e318