Closed
Bug 1377251
Opened 7 years ago
Closed 7 years ago
Expose TIME_TO_NON_BLANK_PAINT as Performance Entry behind pref
Categories
(Core :: DOM: Core & HTML, enhancement)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: Harald, Assigned: perry)
References
Details
Attachments
(1 file, 3 obsolete files)
(deleted),
patch
|
qdot
:
review+
|
Details | Diff | Splinter Review |
Bug 1307242 landed the telemetry probe and needs to be exposed to content behind a flag for page load testing in Talos.
Related, bug 1210906 asked to implement Chrome's proprietary performance.timing.firstPaint. As better reference, PerformancePaintTiming is implemented by Chrome and exposes paint timings using PerformanceEntry [2].
Markus, is it easy to add an entry to the performance timeline or should we go with a custom property, like https://bugzilla.mozilla.org/show_bug.cgi?id=1299117#c31 ?
[1]: https://github.com/wicg/paint-timing
[2]: https://w3c.github.io/performance-timeline/#the-performanceentry-interface
Reporter | ||
Updated•7 years ago
|
Flags: needinfo?(mstange)
Reporter | ||
Updated•7 years ago
|
Summary: Expose TIME_TO_NON_BLANK_PAINT as performance marker → Expose TIME_TO_NON_BLANK_PAINT as Performance Entry behind pref
Updated•7 years ago
|
Assignee: nobody → jiangperry
Status: NEW → ASSIGNED
Comment 2•7 years ago
|
||
Hey Harald, can you clarify what's being asked here? It looks like the patch from https://bugzilla.mozilla.org/show_bug.cgi?id=1299117#c31 implements performance.timing.firstMeaningfulPaint. Are you asking for something else different (what's the difference between "first meaningful paint" and "TIME_TO_NON_BLANK_PAINT" that you ask here?). Or is that the same information, and you're asking for it to be exposed in a different place too?
Flags: needinfo?(hkirschner)
Comment 3•7 years ago
|
||
The original "first meaningful paint" is not especially useful, because there is no definition of "primary content", and we can't compare Firefox and Chrome. And in fact I'm thinking we should probably WONTFIX bug 1299117.
The thing being asked here is first *non-blank* paint. That is, on page load the first time any content is loaded which isn't the page background. On facebook this is pretty much always the blue bar. This is already measured in bug 1307242 and needs to be exposed to content in at least a way that Talos can grab it.
Future followup will be time to paint particular hero elements, but that is separate and more complex than this bug.
Flags: needinfo?(hkirschner)
Comment 4•7 years ago
|
||
The non-blank paint timestamp is stored here: http://searchfox.org/mozilla-central/rev/5e1e8d2f244bd8c210a578ff1f65c3b720efe34e/dom/base/nsDOMNavigationTiming.h#135
Comment 5•7 years ago
|
||
Thanks for the explanation! So how does it sound if follow the patch from bug 1299117 as an example, and expose this as performance.timing.MozFirstNonBlankPaint behind a pref?
Flags: needinfo?(benjamin)
Comment 6•7 years ago
|
||
I think that's reasonable at a high level. At the detail level a DOM peer should review; I'm not qualified on the particulars.
Flags: needinfo?(benjamin)
Assignee | ||
Comment 7•7 years ago
|
||
Used nsDOMNavigationTiming::TimeStampToDOMHighRes rather than nsDOMNavigationTiming::TimeStampToDOM because because it appeared to give the actual time difference between mNonBlankPaintTimeStamp and mNavigationStartTimeStamp.
Attachment #8884101 -
Flags: review?(bugs)
Comment 8•7 years ago
|
||
Comment on attachment 8884101 [details] [diff] [review]
Bug 1377251 - Enable window.performance.timing.timeToNonBlankPaint behind pref
Perhaps kyle could review this.
Attachment #8884101 -
Flags: review?(bugs) → review?(kyle)
Comment 9•7 years ago
|
||
Comment on attachment 8884101 [details] [diff] [review]
Bug 1377251 - Enable window.performance.timing.timeToNonBlankPaint behind pref
Review of attachment 8884101 [details] [diff] [review]:
-----------------------------------------------------------------
Just got a couple of small but important things.
::: dom/performance/PerformanceTiming.h
@@ +245,5 @@
> }
>
> + DOMTimeMilliSec TimeToNonBlankPaint() const
> + {
> + if (!nsContentUtils::IsPerformanceTimingEnabled()) {
Add a ShouldResistFingerprinting() check here.
::: dom/webidl/PerformanceTiming.webidl
@@ +34,5 @@
> readonly attribute unsigned long long loadEventStart;
> readonly attribute unsigned long long loadEventEnd;
>
> + [Pref="dom.performance.time_to_non_blank_paint.enabled"]
> + readonly attribute unsigned long long timeToNonBlankPaint;
Can you add a comment here specifying that this is a Chrome proprietary extension, and not part of the performance timing/navigation timing spec?
Attachment #8884101 -
Flags: review?(kyle) → review-
Assignee | ||
Comment 10•7 years ago
|
||
Added ShouldResistFingerprinting() check and comment to specific that timeToNonBlankPaint is a Chrome proprietary extension.
Attachment #8884101 -
Attachment is obsolete: true
Attachment #8884405 -
Flags: review?(kyle)
Updated•7 years ago
|
Attachment #8884405 -
Flags: review?(kyle) → review+
Comment 11•7 years ago
|
||
Pushed by felipc@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5f7d4b62f6e4
Expose TIME_TO_NON_BLANK_PAINT as Performance Entry behind pref. r=qdot
I had to back this out for failures like https://treeherder.mozilla.org/logviewer.html#?job_id=112695679&repo=mozilla-inbound
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7a348bef4cd7047482e1f46018d532e912eecca
Flags: needinfo?(jiangperry)
Assignee | ||
Comment 13•7 years ago
|
||
In the case that there hasn't been a non-blank paint, window.performance.timing.timeToNonBlankPaint will be 0. This avoids the assertion failure in bug 1379327 and is consistent with Chrome [0] and IE/Edge [1].
[0] https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/timing/PerformanceTiming.cpp?l=312
[1] https://msdn.microsoft.com/en-us/library/ff974719(v=vs.85).aspx
Attachment #8884405 -
Attachment is obsolete: true
Flags: needinfo?(jiangperry)
Attachment #8885026 -
Flags: review?(kyle)
Updated•7 years ago
|
Attachment #8885026 -
Flags: review?(kyle) → review+
Updated•7 years ago
|
Keywords: checkin-needed
Comment 14•7 years ago
|
||
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/852607a17f6c
Expose TIME_TO_NON_BLANK_PAINT as Performance Entry behind pref. r=qDot
Keywords: checkin-needed
Comment 15•7 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Comment 16•7 years ago
|
||
performance.timing.timeToNonBlankPaint returns a duration (e.g. 1822 ms) while IE/Edge's performance.timing.msFirstPaint and Chrome's chrome.loadTimes().firstPaintTime return timestamps (e.g. 1499878475965 and 1499878479.305). While a duration seems easier to work with, we should probably change timeToNonBlankPaint to a timestamp to match IE/Edge and Chrome and all of the other measurements in performance.timing.*.
Perry said on IRC that he could fix this easily.
Reporter | ||
Comment 17•7 years ago
|
||
msFirstPaint and chrome.loadTimes().firstPaintTime are all non-standard. Are there benefits in matching them? All modern performance timings, including https://github.com/wicg/paint-timing, are DOMHighResTimeStamp, relative to navigation start.
Comment 18•7 years ago
|
||
MDN says the PerformanceTiming properties like `navigationStart` or `domComplete` are milliseconds since the UNIX epoch:
https://developer.mozilla.org/en-US/docs/Web/API/PerformanceTiming
The wicg/paint-timing proposal says:
> Entries will have a name of "first-paint" and "first-contentful-paint" respectively, and an entryType of "paint". startTime is the DOMHighResTimeStamp indicating when the paint occurred, and the duration will always be 0.
> "first-paint" entries contain a DOMHighResTimeStamp reporting the time when the browser first rendered after navigation.
So it looks like they are proposing absolute timestamps because they say "the time when" and duration is always 0. That said, our property name `performance.timing.timeToNonBlankPaint` does sound like a duration ("time to ...").
Reporter | ||
Comment 19•7 years ago
|
||
> MDN says the PerformanceTiming properties like `navigationStart` or `domComplete` are milliseconds since the UNIX epoch:
Good catch on PerformanceTiming. The same page load timings will be available as DOMHighResTimeStamp, part of Navigation Timing Level 2: https://developer.mozilla.org/en-US/docs/Web/API/PerformanceNavigationTiming
Comment 20•7 years ago
|
||
Let's please use the timestamp version for these. That will align much better with future work about in-page navigation.
I don't care as much about the moz* private name but I do care about avoiding much more churn. Pick something and let's move on.
Comment 21•7 years ago
|
||
Is there a corresponding event that I can have talos set an eventListener for, so talos will know when 'window.performance.timing.timeToNonBlankPaint' is available?
Flags: needinfo?(jiangperry)
Assignee | ||
Comment 22•7 years ago
|
||
(In reply to Robert Wood [:rwood] from comment #21)
> Is there a corresponding event that I can have talos set an eventListener
> for, so talos will know when 'window.performance.timing.timeToNonBlankPaint'
> is available?
There isn't - the pref would need to be enabled (or as far as I can tell with the changes I've made).
Flags: needinfo?(jiangperry)
Comment 23•7 years ago
|
||
(In reply to :Perry Jiang from comment #22)
> (In reply to Robert Wood [:rwood] from comment #21)
> > Is there a corresponding event that I can have talos set an eventListener
> > for, so talos will know when 'window.performance.timing.timeToNonBlankPaint'
> > is available?
>
> There isn't - the pref would need to be enabled (or as far as I can tell
> with the changes I've made).
Sorry, I meant when the pref is already enabled, is there an event talos can listen for, to indicate that 'window.performance.timing.timeToNonBlankPaint' now has a non-zero value.
Flags: needinfo?(jiangperry)
Comment 24•7 years ago
|
||
onload should work for that. The chances of onload firing before we paint seem pretty remote to me, although I guess it's technically possible.
Updated•7 years ago
|
Flags: needinfo?(jiangperry)
Comment 25•7 years ago
|
||
(In reply to Benjamin Smedberg [:bsmedberg] from comment #24)
> onload should work for that. The chances of onload firing before we paint
> seem pretty remote to me, although I guess it's technically possible.
Ok, thanks!
Comment 26•7 years ago
|
||
(In reply to Robert Wood [:rwood] from comment #25)
> (In reply to Benjamin Smedberg [:bsmedberg] from comment #24)
> > onload should work for that. The chances of onload firing before we paint
> > seem pretty remote to me, although I guess it's technically possible.
>
> Ok, thanks!
If that becomes a problem, you could keep listening for MozAfterPaint events until you see timeToNonBlankPaint to be non-zero, then stop listening to it.
Comment 27•7 years ago
|
||
(In reply to Benjamin Smedberg [:bsmedberg] from comment #20)
> Let's please use the timestamp version for these. That will align much
> better with future work about in-page navigation.
>
> I don't care as much about the moz* private name but I do care about
> avoiding much more churn. Pick something and let's move on.
Hi :perry, window.performance.timing.timeToNonBlankPaint is currently returning a duration, not a timestamp, is that going to be changed?
Flags: needinfo?(jiangperry)
Assignee | ||
Comment 28•7 years ago
|
||
.IsNull() check will be done in TimeStampToDOM
Attachment #8885026 -
Attachment is obsolete: true
Flags: needinfo?(jiangperry)
Attachment #8888442 -
Flags: review?(kyle)
Assignee | ||
Comment 29•7 years ago
|
||
(In reply to Robert Wood [:rwood] from comment #27)
> Hi :perry, window.performance.timing.timeToNonBlankPaint is currently
> returning a duration, not a timestamp, is that going to be changed?
My bad, just submitted the new patch!
Flags: needinfo?(mstange)
Updated•7 years ago
|
Attachment #8888442 -
Flags: review?(kyle) → review+
Comment 30•7 years ago
|
||
(In reply to :Perry Jiang from comment #29)
> My bad, just submitted the new patch!
Thanks :perry
Comment 31•7 years ago
|
||
Is attachment 8888442 [details] [diff] [review] ready to land?
Flags: needinfo?(jiangperry)
Assignee | ||
Comment 32•7 years ago
|
||
(In reply to Mike Conley (:mconley) - Buried in needinfo / review backlog, eating my way out from comment #31)
> Is attachment 8888442 [details] [diff] [review] ready to land?
Yep, it is.
Flags: needinfo?(jiangperry)
Comment 33•7 years ago
|
||
Pushed by felipc@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a7db98b3785
Follow-up: replace timeToNonBlankPaint's duration with timestamp. r=qDot
Comment 34•7 years ago
|
||
bugherder |
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•