Perf-Dashboard.Render sync phase sets location.hash, but the actual DOM modifications happen asynchronously in the onhashchange() callback.#488
Conversation
✅ Deploy Preview for webkit-speedometer ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
| return; | ||
| this._previousHash = unescapedHash; | ||
| this.route(); | ||
| this._hash = null; |
There was a problem hiding this comment.
it's not clear to me why this._hash is set to null here. In _updateURLState above it looks like we want to keep it synced with location.hash.
Then if we don't reset it, we probably don't need _previousHash and the check above would work, right?
There was a problem hiding this comment.
That's a good point, much simpler to do that. Fixed :)
modifications happen asynchronously in the onhashchange() callback. This results in nothing interesting being measured during the sync phase, and the resulting rendering update (during the async phase) not including the change. Change the test to manually and synchronously trigger the onhashchange() callback, and change the page content to detect and omit duplicate events.
mattwoodrow
force-pushed
the
perf-dashboard-sync-hashchange
branch
from
f8884de to
f7cb31b
Compare
julienw
left a comment
julienw
left a comment
There was a problem hiding this comment.
Thanks, this looks better to me.
I also looked with a firefox profiler, this clearly captures more work, which sounds good to me.
It would be too to have a sign-off from somebody at Apple who initially brought this in speedometer.
There's just one small correctness issue left to fix, I believe.
Thanks for the fix!
| _hashDidChange() | ||
| { | ||
| if (unescape(location.hash) == this._hash) | ||
| let unescapedHash = unescape(location.hash); |
There was a problem hiding this comment.
I think you want decodeURI
There was a problem hiding this comment.
Possibly? That's not new to this change though, I just added a local var to cache the result.
There was a problem hiding this comment.
mmm right
If the hash doesn't have any encoded char this is OK, but I think this will be buggy as soon as you start having some. Probably we don't have any at the moment.
|
do we also want to merge the same changes into the 3.1 release branch? |
|
Yeah, we should probably do that. |
…OM (#488) modifications happen asynchronously in the onhashchange() callback. This results in nothing interesting being measured during the sync phase, and the resulting rendering update (during the async phase) not including the change. Change the test to manually and synchronously trigger the onhashchange() callback, and change the page content to detect and omit duplicate events. Co-authored-by: Matt Woodrow <m_woodrow@apple.com>
|
Here's a PR to merge the fix into release/3.1: #496 |
…OM (#488) (#496) modifications happen asynchronously in the onhashchange() callback. This results in nothing interesting being measured during the sync phase, and the resulting rendering update (during the async phase) not including the change. Change the test to manually and synchronously trigger the onhashchange() callback, and change the page content to detect and omit duplicate events. Co-authored-by: Matt Woodrow <m_woodrow@apple.com>
b475f2d Perf-Dashboard.Render sync phase sets location.hash, but the actual DOM (WebKit#488) (WebKit#496) b270564 (origin/3.1, 3.1) Release/3.1 428 - add id to input (NEW - with correct base to merge into) (WebKit#494) aba1d5e (thor/release/3.1, flashd/release/3.1) Deconfuse Geomean Metric and geomean aggregate value (WebKit#408) (WebKit#480) ce03cfb Don't display invalid results (WebKit#406) (WebKit#479) 6f25345 Update Version Text from 3.0 to 3.1 (WebKit#485) cc9ee08 Release/3.1 - merge 400 (rename className -> class) (WebKit#483) e12e84c Release/3.1 - merge 448 (news site article ids nuxt) (WebKit#482) 8a4a566 Release/3.1 - merge 447 (news site article ids next) (WebKit#481) Change-Id: If0b19391128b82389eda1a61d33dc4e2be2cd180
5 participants
This results in nothing interesting being measured during the sync phase, and the resulting rendering update (during the async phase) not including the change.
Change the test to manually and synchronously trigger the onhashchange() callback, and change the page content to detect and omit duplicate events.
Crossbench results before/after the change: