Topsites telemetry fixes (#3105) #3108
Conversation
Mardak
changed the title
| mockRaf.step({count: 1}); | ||
| assert.calledOnce(callback); |
There was a problem hiding this comment.
This was a little confusing to figure out as there's no explicit check that the callback was called except that if it wasn't called, the test would have timed out.
| // have to do it here, so that other calls to this method | ||
| // while waiting for the next frame won't also try to handle | ||
| // handle it. | ||
| this._timestampHandled = true; |
There was a problem hiding this comment.
nit: this narrow comment block was bothering me more than it probably should ;)
// And if we haven't, we're doing so now, so remember that. Even if
// something goes wrong in the callback, we can't try again, as we'd be
// sending back the wrong data, and we have to do it here, so that other
// calls to this method while waiting for the next frame won't also try to
// handle handle it.There was a problem hiding this comment.
Yeah, I dunno how it ended up that narrow. As we all know, 80 columns is the One True Width. :-D
| * (not the upcoming one) paints. | ||
| * Call the given callback after the upcoming frame paints. We're using | ||
| * a Promise rather than a setTimeout or double rFA, as Promise resolution | ||
| * is faster, as of this writing (Firefox 57 Nightly) - see #3105 for |
There was a problem hiding this comment.
probably note that it's also consistent not just faster in particular when in a not visible browser
There was a problem hiding this comment.
I'll update the stuff referring to Promises to talk about setTimeout (and maybe postMessage).
|
@Mardak, can you have another quick look, since I've swapped out the promise for setTimeout... |
This does two things:
The part that is easily testable is the extra marks:
There should be exactly two topsites_first_painted_ts.
r? Mardak