-
Notifications
You must be signed in to change notification settings - Fork 9.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fMP: Use last fMPCandidate if no FMP is marked. #1632
Conversation
if (candidates.length === 0) { | ||
throw new Error('No usable `firstMeaningfulPaint(Candidate)` events found in trace'); | ||
} | ||
tabTrace.firstMeaningfulPaintEvt = candidates.pop(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you think any other audits would need to use this inferred fmp event? or is this it?
@@ -148,7 +148,7 @@ class Metrics { | |||
* @param {number} navStartTs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
update this?
const navStartTs = metrics.find(e => e.id === 'navstart').ts; | ||
if (this.getNavigationStartEvt().ts !== navStartTs) { | ||
const navStartEvt = metrics.find(e => e.id === 'navstart'); | ||
if (!navStartEvt || this.getNavigationStartEvt().ts !== navStartEvt.ts) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this.getNavigationStartEvt()
always find something or is this another candidate for TypeError too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yup it should always find something.
The getNavigationStartEvt
method could live on top of trace-of-tab as well, though...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
^ filed that over here #1568
9cafae6
to
9e6bc39
Compare
PTAL |
log.verbose('trace-of-tab', `No firstMeaningfulPaint found, falling back to last ${fmpCand}`); | ||
const lastCandidate = frameEvents.filter(e => e.name === fmpCand).pop(); | ||
if (!lastCandidate) { | ||
log.verbose('No `firstMeaningfulPaintCandidate` events found in trace'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'trace-of-tab' for log title?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
Travis is confused, but the tests are in good shape. :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, LGTM2!
Good test page for this is https://madeby.google.com/phone/ which we always time out after 30s
fixes #1564
--
fmpCandidate comes from
loading
trace category (for whatever reason).The changes in pwmetrics are for handling -1 failures a little better.