1298381 - Implement Time to First Contentful Paint (TTFCP) for Telemetry/Profiler

Status: RESOLVED FIXED

(Core :: Layout, defect, P3)

Description

[:Dominik Strohmeier [:dstrohmeier]]

Time to First Contentful Visual celebrates the first visible change to the screen that average users can observe. It can also be seen as the time when any "contentful" thing (text, image, SVG, non-empty canvas) is painted for the first time on the screen.

Comment 1

[:Dominik Strohmeier [:dstrohmeier]]

Thread on Chromium about First Contentful Visual to confirm user's "is it happening?" question when requesting a new page: https://groups.google.com/a/chromium.org/forum/#!msg/loading-dev/NzBleQ4QP6Y/XRoR0NFQBwAJ

Chromium's planning doc: https://docs.google.com/document/d/1Owfs6arciEnWgT2-8bWCcHdYRIKRKZ0Xj8UtqRx4c3k/edit#heading=h.nwfi9zag6eyn

Thread on why these metrics are called "Visual" vs "Paint": https://groups.google.com/a/chromium.org/forum/#!topic/progressive-web-metrics/_s92zvTPblk

Comment 2

[:Dominik Strohmeier [:dstrohmeier]]

This will be most likely merged with Time to First Meaningful Paint.

Comment 3

[:Dominik Strohmeier [:dstrohmeier]]

There is a proposal about a Hero Element Timing API from Chrome team that was approved by the web-perf W3C group.

Here's the current draft of the spec: https://docs.google.com/document/d/1yRYfYR1DnHtgwC4HRR04ipVVhT1h5gkI6yPmKCgJkyQ/edit

Comment 4

[Ilya Grigorik]

Related, summary from TPAC: https://github.com/w3c/charter-webperf/issues/30

Comment 5

[:Harald Kirschner :digitarald]

Clarified the minimal scope for this bug, which is only telemetry and profiler. Developer facing numbers depend on validation and spec stabilizing.

Comment 6

[Naveed Ihsanullah [:naveed]]

Is this probe needed for QF? If so please tag with [qf:p1]

Comment 7

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Implement TimeToFirstContentfulPaint behind a pref

Has not been tested or built yet.  Needs more work on the IsContentfulPaint code per discussion in #gfx

Comment 8

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Implement TimeToFirstContentfulPaint behind a pref

Comment 9

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Implement TimeToFirstContentfulPaint behind a pref

Comment 10

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Implement TimeToFirstContentfulPaint behind a pref

I'm certain the list of types getting the TYPE_IS_CONTENTFUL flag is wrong in many ways.  I need to know a) which are wrong, b) if the recursive check I have is right.  Also, should I revise NonBlankPaint to be FirstPaint?  (and what exactly are the differences at the DisplayList level).  Thanks

Comment 11

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 9019136 [details] [diff] [review]
Implement TimeToFirstContentfulPaint behind a pref

Review of attachment 9019136 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/painting/nsDisplayItemTypesList.h
@@ +22,1 @@
>  DECLARE_DISPLAY_ITEM_TYPE(CANVAS, TYPE_RENDERS_NO_IMAGES)

This usage of CANVAS refers to the HTML <canvas> element, which I assume is what is meant by "non-white canvas". You might want manual code in DisplayListIsContentful to query the actual element and figure out the 'non-white' bit (which it doesn't appear we have code for).

The other 3 CANVAS_ types refer to the CSS conceptual canvas (see CSS2.3.1), and the flags look correct.

@@ +64,1 @@
>  DECLARE_DISPLAY_ITEM_TYPE(SVG_WRAPPER, 0)

I think all the SVG types in this block should probably be considered contentful.

@@ +66,1 @@
>  DECLARE_DISPLAY_ITEM_TYPE(TABLE_CELL_BACKGROUND, 0)

It looks like table cells don't have separate items for the colour and set of images, so this item represents whatever we have.

You may want to query the item itself to find out if it's going to paint images.

@@ +66,3 @@
>  DECLARE_DISPLAY_ITEM_TYPE(TABLE_CELL_BACKGROUND, 0)
>  DECLARE_DISPLAY_ITEM_TYPE(TABLE_CELL_SELECTION, TYPE_RENDERS_NO_IMAGES)
>  DECLARE_DISPLAY_ITEM_TYPE(TABLE_BORDER_COLLAPSE, 0)

This represents all the backgrounds for an entire border-collapse table, including colours and images for each cell/row/column.

Again, may be worth trying to dig deeper to decide if it's contentful.

Comment 12

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Implement TimeToFirstContentfulPaint behind a pref

disabled the actual check of the canvas MaybeModified state for now (also for some reason my simple test doesn't seem to get GetContext() when I ask for canvas.getContext('2d'))

Comment 13

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Implement TimeToFirstContentfulPaint behind a pref

Works; tested with a simple canvas-only testcase

Comment 14

[Bobby Holley (:bholley)]

Can you confirm what I'm reviewing? Just the WebIDL changes?

Comment 15

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 9021963 [details] [diff] [review]
Implement TimeToFirstContentfulPaint behind a pref

Review of attachment 9021963 [details] [diff] [review]:
-----------------------------------------------------------------

Display list side looks good!

I'm sure there will be more tweaking required with the CONTENTFUL flags, and getting the canvas override and spec to match etc, but landing the infrastructure sounds like a good idea!

::: layout/painting/nsDisplayItemTypesList.h
@@ +57,5 @@
>  DECLARE_DISPLAY_ITEM_TYPE(SUBDOCUMENT, TYPE_RENDERS_NO_IMAGES)
>  DECLARE_DISPLAY_ITEM_TYPE(MASK, 0)
>  DECLARE_DISPLAY_ITEM_TYPE(FILTER, TYPE_RENDERS_NO_IMAGES)
>  DECLARE_DISPLAY_ITEM_TYPE(SVG_OUTER_SVG, TYPE_RENDERS_NO_IMAGES)
>  DECLARE_DISPLAY_ITEM_TYPE(SVG_GEOMETRY, 0)

SVG_GEOMETRY is the standard svg display item, we should probably consider that CONTENTFUL too.

Comment 16

[Bobby Holley (:bholley)]

Comment on attachment 9021963 [details] [diff] [review]
Implement TimeToFirstContentfulPaint behind a pref

Canceling review pending comment 4.

Comment 17

[Randell Jesup [:jesup] (needinfo me)]

(In reply to Bobby Holley (:bholley) from comment #14)
> Can you confirm what I'm reviewing? Just the WebIDL changes?

Yes - though if you're willing/able to review any more of the DOM/layout changes, that's great.  If not I'll hit up appropriate other peers for those.

Comment 18

[Bobby Holley (:bholley)]

Comment on attachment 9021963 [details] [diff] [review]
Implement TimeToFirstContentfulPaint behind a pref

Review of attachment 9021963 [details] [diff] [review]:
-----------------------------------------------------------------

I'll defer to smaug, who can presumably also stamp the webidl changes.

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 9021963 [details] [diff] [review]
Implement TimeToFirstContentfulPaint behind a pref

>+    nsPrintfCString marker("Contentful paint after %dms for URL %s, %s",
>+                           int(elapsed.ToMilliseconds()), spec.get(),
>+                           mDocShellHasBeenActiveSinceNavigationStart ? "foreground tab" : "this tab was inactive some of the time between navigation start and first non-blank paint");
overlong line

> void
>+nsPresContext::NotifyContentfulPaint()
>+{
>+  if (!mHadContentfulPaint) {
>+    mHadContentfulPaint = true;
>+    if (IsRootContentDocument()) {
>+      RefPtr<nsDOMNavigationTiming> timing = mDocument->GetNavigationTiming();
>+      if (timing) {
>+        timing->NotifyContentfulPaintForRootContentDocument();
>+      }
>+
>+      mFirstContentfulPaintTime = TimeStamp::Now();
You just assign to this variable once but never use the value for anything.
Drop mFirstContentfulPaintTime from nsPresContext.

>-
>+  bool HadContentfulPaint() const {
Nit, { should be on its own line


> nsDisplayListBuilder::LeavePresShell(nsIFrame* aReferenceFrame,
>                                      nsDisplayList* aPaintedContents)
> {
>   NS_ASSERTION(CurrentPresShellState()->mPresShell ==
>                  aReferenceFrame->PresShell(),
>                "Presshell mismatch");
> 
>-  if (mIsPaintingToWindow) {
>+  if (mIsPaintingToWindow && aPaintedContents) {
>     nsPresContext* pc = aReferenceFrame->PresContext();
>     if (!pc->HadNonBlankPaint()) {
>       if (!CurrentPresShellState()->mIsBackgroundOnly &&
>           DisplayListIsNonBlank(aPaintedContents)) {
>         pc->NotifyNonBlankPaint();
>       }
>     }
>+    if (!pc->HadContentfulPaint()) {
>+      if (!CurrentPresShellState()->mIsBackgroundOnly &&
>+          DisplayListIsContentful(aPaintedContents)) {
>+        pc->NotifyContentfulPaint();
>+      }
>+    }
>   }
I'm not super happy with checking both non-blank and contentful-paint, even if the pref isn't, but I guess it isn't too slow.

Comment 20

[Philip Tellis]

Minor request, but could we change the bug title to say FCP rather than TCP to avoid confusion and showing up in search results for TCP? Thanks.

Comment 21

[Daniel Holbert [:dholbert]]

Looks like the acronym is TTFCP per https://github.com/paulirish/pwmetrics/blame/cc8d5e7fb009573cc1991c029faf21d5d9e09093/readme.md#L317 -- updating summary to use that acronym.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/91659baa6ddb
Implement TimeToFirstContentfulPaint behind a pref r=mattwoodrow,smaug

Comment 23

[Andrei Ciure[:aciure]]

Backed out changeset 91659baa6ddb (bug 1298381) for nsDisplayList.h perma failures

push that caused the backout: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&selectedJob=211784719&revision=91659baa6ddbb00282b9a3f9b9748cb25597c13e

failure: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&fromchange=c0bef417dc8e17d6a2661075ae8db9df50480b2c&selectedJob=211767221&searchStr=windows%2C10%2Cx64%2Cdebug%2Creftests%2Cwith%2Ce10s%2Ctest-windows10-64%2Fdebug-crashtest-e10s%2Cr-e10s%28c%29

backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/e88e746154edd216d2e315ead34c4d0c6205429c

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a10cbfd5f411
Implement TimeToFirstContentfulPaint behind a pref r=mattwoodrow,smaug

Comment 25

[Randell Jesup [:jesup] (needinfo me)]

Needed a bugfix to RDL first.  Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bf0cc200405f09cd32dcf6ac157d93fbc390cf5a

Comment 26

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/a10cbfd5f411

Comment 27

[Henri]

(In reply to Daniel Holbert [:dholbert] (PTO Fri Nov 16) from comment #21)
> Looks like the acronym is TTFCP per
> https://github.com/paulirish/pwmetrics/blame/
> cc8d5e7fb009573cc1991c029faf21d5d9e09093/readme.md#L317 -- updating summary
> to use that acronym.

I too was going to side w/ Philip, as the spec has it listed as FCP as well. https://w3c.github.io/paint-timing/ 
But I see that Paul's comment was pretty recent.