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)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: Harald, Assigned: perry)

References

Details

Attachments

(1 file, 3 obsolete files)

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
Flags: needinfo?(mstange)
Summary: Expose TIME_TO_NON_BLANK_PAINT as performance marker → Expose TIME_TO_NON_BLANK_PAINT as Performance Entry behind pref
Blocks: 1363845
Assignee: nobody → jiangperry
Status: NEW → ASSIGNED
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)
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)
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)
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)
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 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 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-
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)
Attachment #8884405 - Flags: review?(kyle) → review+
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
Depends on: 1379327
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)
Attachment #8885026 - Flags: review?(kyle) → review+
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
https://hg.mozilla.org/mozilla-central/rev/852607a17f6c
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
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.
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.
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 ...").
> 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
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.
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)
(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)
(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)
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.
Flags: needinfo?(jiangperry)
(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!
(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.
(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)
.IsNull() check will be done in TimeStampToDOM
Attachment #8885026 - Attachment is obsolete: true
Flags: needinfo?(jiangperry)
Attachment #8888442 - Flags: review?(kyle)
(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)
Attachment #8888442 - Flags: review?(kyle) → review+
(In reply to :Perry Jiang from comment #29)
> My bad, just submitted the new patch!

Thanks :perry
Is attachment 8888442 [details] [diff] [review] ready to land?
Flags: needinfo?(jiangperry)
(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)
Pushed by felipc@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a7db98b3785
Follow-up: replace timeToNonBlankPaint's duration with timestamp. r=qDot
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.