fix(telemetry): #3118 workaround 500s apparent topsites_first_paint_ts #3128
Conversation
system-addon/lib/TelemetryFeed.jsm
Outdated
@@ -266,6 +265,13 @@ this.TelemetryFeed = class TelemetryFeed { | |||
// get blows up. | |||
let session = this.sessions.get(port); | |||
|
|||
// Partial workaround for #3118; avoids the worst incorrect associations of | |||
// times with browsers, and should result in no longer having apparent load | |||
// times of 500,000 ms. Better fix for more cases forthcoming. |
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.
The 500sec comment doesn't really make any sense by itself. Probably better to just say to associate the load trigger with the visibility event as the user is most likely associating the trigger to the tab just shown. Could maybe note that this is to not associate with a preloaded browser as those don't get the event until shown.
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.
Good catch, the 500sec was pretty context-free. :-)
assert.calledOnce(instance.setLoadTriggerInfo); | ||
assert.calledWithExactly(instance.setLoadTriggerInfo, "port123"); | ||
assert.include(instance.sessions.get("port123").perf, data); | ||
}); |
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.
A negative test would be good sanity check. I.e., non-visibility_event_rcvd
_ts doesn't call setLoadTriggerInfo
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.
Good idea; done.
c03f842
to
80565ac
Compare
Fix #3118. r?Mardak This should be the exact fix we already discussed.