Bug 208499 - Implement FCP (first contentful paint)
Summary: Implement FCP (first contentful paint)
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Noam Rosenthal
URL:
Keywords: InRadar, WebExposed
Depends on:
Blocks: 78011
  Show dependency treegraph
 
Reported: 2020-03-02 23:28 PST by Noam Rosenthal
Modified: 2021-02-19 11:56 PST (History)
32 users (show)

See Also:


Attachments
Patch (129.99 KB, patch)
2020-03-20 06:17 PDT, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch (140.43 KB, patch)
2020-03-23 11:33 PDT, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch (140.94 KB, patch)
2020-03-23 11:51 PDT, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch (140.94 KB, patch)
2020-03-24 09:23 PDT, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch (136.41 KB, patch)
2020-03-30 04:11 PDT, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch (132.83 KB, patch)
2020-03-30 04:51 PDT, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch (134.09 KB, patch)
2020-03-30 05:29 PDT, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch (133.60 KB, patch)
2020-03-30 05:49 PDT, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch (133.63 KB, patch)
2020-03-30 06:25 PDT, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch (134.42 KB, patch)
2020-03-30 07:12 PDT, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch (135.46 KB, patch)
2020-03-30 07:54 PDT, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch (136.75 KB, patch)
2020-03-30 08:25 PDT, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch (134.60 KB, patch)
2020-03-30 11:51 PDT, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch (142.81 KB, patch)
2020-04-03 00:19 PDT, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch (142.73 KB, patch)
2020-04-03 00:44 PDT, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch (137.42 KB, patch)
2020-04-05 23:08 PDT, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch (140.68 KB, patch)
2020-04-10 11:24 PDT, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch (133.82 KB, patch)
2020-04-13 10:47 PDT, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch (144.52 KB, patch)
2020-04-15 02:03 PDT, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch (142.83 KB, patch)
2020-04-16 04:48 PDT, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch (142.87 KB, patch)
2020-04-16 06:56 PDT, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch (142.87 KB, patch)
2020-04-16 08:15 PDT, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch (172.46 KB, patch)
2020-04-21 04:21 PDT, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch (172.46 KB, patch)
2020-04-21 06:52 PDT, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch: using fake paint (141.16 KB, patch)
2020-04-21 09:20 PDT, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch: using fake paint (141.16 KB, patch)
2020-04-21 09:21 PDT, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch (145.83 KB, patch)
2020-04-23 01:48 PDT, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch (144.72 KB, patch)
2020-04-27 12:26 PDT, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch (135.57 KB, patch)
2020-04-27 13:47 PDT, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch (135.82 KB, patch)
2020-04-27 21:45 PDT, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch (136.13 KB, patch)
2020-04-27 22:27 PDT, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch for landing (135.28 KB, patch)
2020-04-28 13:48 PDT, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch for landing (135.30 KB, patch)
2020-04-28 14:29 PDT, Noam Rosenthal
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Noam Rosenthal 2020-03-02 23:28:21 PST
Spec: https://w3c.github.io/paint-timing/
FCP gives web developers an indication of when painting was actually performed and when it's contentful (contains text/canvas/image/svg).
This is part of the painting API, and a part where WebKit is more simiilar to other browsers so is less ambiguous than the other entry of paint-API, first-paint
Comment 1 Ryosuke Niwa 2020-03-03 00:21:18 PST
See my comment on webkit.org/78011. Before implementing this API, we should adjust our heuristics for the first meaningful paint.
Comment 2 Noam Rosenthal 2020-03-03 00:33:36 PST
(In reply to Ryosuke Niwa from comment #1)
> See my comment on webkit.org/78011. Before implementing this API, we should
> adjust our heuristics for the first meaningful paint.

Great! I created a bug to track the change in heuristic: https://bugs.webkit.org/show_bug.cgi?id=208500
Comment 3 Noam Rosenthal 2020-03-20 06:17:01 PDT
Created attachment 394078 [details]
Patch
Comment 4 Noam Rosenthal 2020-03-20 06:24:41 PDT
I made a crack at the API adhering to the current version of the spec.
I know it's not currently adjusted to VNE heuristic, for the following reasons:

- It's behind a flag, we can adjust VNE/FCP as we go along, and also as the spec evolves, before enabling it in Safari etc.
- in many cases VNE would happen before FCP, as VNE (unlike FCP) is bound to happen after all resources load, and unlike FCP, VNE is not layout bound.
- VNE currently has no tests. The new FCP tests from W3C could serve as infrastructure for VNE tests.
- I wanted to move forward, and it's been difficult to contribute to VNE as it's not something that has a spec.

It currently covers 23/24 of the W3C tests, and more could be added incrementally.
If we still want to adjust VNE before moving forward with it it's of course OK, but I'd be happy to get some guidance on how to do that (See https://bugs.webkit.org/show_bug.cgi?id=208501 for example, the discussion is a bit opaque).
Comment 5 Ryosuke Niwa 2020-03-20 20:00:57 PDT
I don't think we ever want to FCP that's totally different from VNE since then FCP would become a fictional metric.
Comment 6 Ryosuke Niwa 2020-03-20 20:01:12 PDT
Comment on attachment 394078 [details]
Patch

r- on that basis
Comment 7 Noam Rosenthal 2020-03-21 08:49:04 PDT
(In reply to Ryosuke Niwa from comment #5)
> I don't think we ever want to FCP that's totally different from VNE since
> then FCP would become a fictional metric.

Oops, I think I've used wrong wording to describe this work!

FCP here is not totally different from VNE - what I meant is that it doesn't count directly on VNE code because VNE is a pre-layout heuristic metric and FCP is a post-layout accurate metric. For example, the characters of text that has an empty bounding rect would count for VNE but not for FCP, because the bounding box can only be calculated after layout.

It's not a fictional metric in this patch - it reports the first contentful paint as defined in the spec. Since VNE delays paint, that delay is indirectly taken into account, but not directly in the code path.

If this is still unclear I'd love to chat about this on Slack :)
Comment 8 Noam Rosenthal 2020-03-23 11:33:06 PDT
Created attachment 394280 [details]
Patch
Comment 9 Noam Rosenthal 2020-03-23 11:35:55 PDT
A new patch. explicity referring to VNE.
Comment 10 Noam Rosenthal 2020-03-23 11:51:28 PDT
Created attachment 394284 [details]
Patch
Comment 11 Noam Rosenthal 2020-03-24 09:23:32 PDT
Created attachment 394370 [details]
Patch
Comment 12 Noam Rosenthal 2020-03-30 04:11:16 PDT
Created attachment 394899 [details]
Patch
Comment 13 Noam Rosenthal 2020-03-30 04:51:06 PDT
Created attachment 394904 [details]
Patch
Comment 14 Noam Rosenthal 2020-03-30 05:29:12 PDT
Created attachment 394907 [details]
Patch
Comment 15 Noam Rosenthal 2020-03-30 05:49:06 PDT
Created attachment 394909 [details]
Patch
Comment 16 Noam Rosenthal 2020-03-30 06:25:53 PDT
Created attachment 394911 [details]
Patch
Comment 17 Noam Rosenthal 2020-03-30 07:12:57 PDT
Created attachment 394913 [details]
Patch
Comment 18 Noam Rosenthal 2020-03-30 07:54:30 PDT
Created attachment 394917 [details]
Patch
Comment 19 Noam Rosenthal 2020-03-30 08:25:40 PDT
Created attachment 394919 [details]
Patch
Comment 20 Noam Rosenthal 2020-03-30 11:51:10 PDT
Created attachment 394942 [details]
Patch
Comment 21 Noam Rosenthal 2020-03-31 02:01:33 PDT
OK, the build failure is same as https://bugs.webkit.org/show_bug.cgi?id=201641
Hope we get the WIN bots to run a clean build, they currently can't handle adding preference keys.
Comment 22 Simon Fraser (smfr) 2020-04-01 12:28:28 PDT
Comment on attachment 394942 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=394942&action=review

> Source/WebCore/page/FrameView.cpp:4202
> +    || frame().document()->securityOrigin().canAccess(frame().mainFrame().document()->securityOrigin());

Weird indentation. I'm surprised we don't have a helper on Frame for this.

> Source/WebCore/page/FrameView.cpp:4205
> +void FrameView::markPaintTimingIfNeeded()

Does this really belong here, or on Document?

Does FCP fire when a page comes out of the page cache? How does it relate to the page visibility API?

> Source/WebCore/page/FrameView.h:828
> +    bool m_didMarkFirstContentfulPaint { false };

Please don't put this member variable here before all the others. For one thing, you've introduced 3 bytes of padding before m_frame. Group it with other bools lower down.

> Source/WebCore/page/PerformancePaintTiming.h:35
> +    static Ref<PerformancePaintTiming> createFirstContentfulPaint(double timeStamp)

Should that be a DOMHighResTimeStamp or a MonotonicTime? We try to avoid raw 'doubles' because they are ambiguous.

> Source/WebCore/rendering/RenderElement.cpp:334
> +    if (!opacity() || isSVGHiddenContainer())

We don't care about opacity on ancestors?

> Source/WebCore/rendering/RenderElement.cpp:338
> +        if (style().visibility() != Visibility::Visible)

We don't care about visibility on ancestors?

> Source/WebCore/rendering/RenderElement.cpp:344
> +            return current->boundingClientRect().intersects(documentRect);

Are we only calling this when layout is up-to-date, so that boundingClientRect() gives a valid answer?
Seems odd to bounce to Element to call boundingClientRect() which is going to call right back into rendering code.

> Source/WebCore/rendering/RenderElement.cpp:355
> +    for (const RenderObject* renderObject = firstChild(); renderObject; renderObject = renderObject->nextSibling()) {
> +        if (renderObject->containsPaintableContent())
> +            return true;
> +    }

It would be much nicer to hoist the recursion out of the function that does the actual work. This should use render tree traversal helpers, and it should be really obvious that this is a massive, expensive full render tree traversal, with boundingClientRect() (an ancestor tree walk) for each renderer.

In fact, I think we need to do this more efficiently, and be able to clip out non-visible areas, which makes it more like paint traversal. I'm going to r- because I think the perf implications here are a showstopper.
Comment 23 zalan 2020-04-01 12:39:05 PDT
Comment on attachment 394942 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=394942&action=review

>> Source/WebCore/rendering/RenderElement.cpp:355
>> +    }
> 
> It would be much nicer to hoist the recursion out of the function that does the actual work. This should use render tree traversal helpers, and it should be really obvious that this is a massive, expensive full render tree traversal, with boundingClientRect() (an ancestor tree walk) for each renderer.
> 
> In fact, I think we need to do this more efficiently, and be able to clip out non-visible areas, which makes it more like paint traversal. I'm going to r- because I think the perf implications here are a showstopper.

Actually this function should not even be on the RenderElement (the only non-recursive entry point is the RenderView). It looks more like a helper function that collects "paintable information" about the render tree.
Comment 24 Noam Rosenthal 2020-04-01 12:49:01 PDT
Comment on attachment 394942 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=394942&action=review

>> Source/WebCore/page/FrameView.cpp:4202
>> +    || frame().document()->securityOrigin().canAccess(frame().mainFrame().document()->securityOrigin());
> 
> Weird indentation. I'm surprised we don't have a helper on Frame for this.

Yea, actually check-webkit-style gave me trouble when I tried to do anything different. Will find a way.

>> Source/WebCore/page/FrameView.cpp:4205
>> +void FrameView::markPaintTimingIfNeeded()
> 
> Does this really belong here, or on Document?
> 
> Does FCP fire when a page comes out of the page cache? How does it relate to the page visibility API?

It can be in Document, maybe that's better.
By the spec, if the page comes out of the page cache before FCP was called, it might still be called. The mark of whether it was called or not is associated with Document.

>> Source/WebCore/page/FrameView.h:828
>> +    bool m_didMarkFirstContentfulPaint { false };
> 
> Please don't put this member variable here before all the others. For one thing, you've introduced 3 bytes of padding before m_frame. Group it with other bools lower down.

Sure. I think it needs to move to Document. though as per previous comment

>> Source/WebCore/page/PerformancePaintTiming.h:35
>> +    static Ref<PerformancePaintTiming> createFirstContentfulPaint(double timeStamp)
> 
> Should that be a DOMHighResTimeStamp or a MonotonicTime? We try to avoid raw 'doubles' because they are ambiguous.

Aye.

>> Source/WebCore/rendering/RenderElement.cpp:334
>> +    if (!opacity() || isSVGHiddenContainer())
> 
> We don't care about opacity on ancestors?

No, because this is called recursively, and this early return exits before we check descendant opacity.

>> Source/WebCore/rendering/RenderElement.cpp:338
>> +        if (style().visibility() != Visibility::Visible)
> 
> We don't care about visibility on ancestors?

Isn't visibility inherited rather than affected by ancestors?

>> Source/WebCore/rendering/RenderElement.cpp:344
>> +            return current->boundingClientRect().intersects(documentRect);
> 
> Are we only calling this when layout is up-to-date, so that boundingClientRect() gives a valid answer?
> Seems odd to bounce to Element to call boundingClientRect() which is going to call right back into rendering code.

Yes - we are only calling this when layout is up to date. I'll add an assert for that.
Maybe boundingClientRect can move into here, and Element would call it?

>>> Source/WebCore/rendering/RenderElement.cpp:355
>>> +    }
>> 
>> It would be much nicer to hoist the recursion out of the function that does the actual work. This should use render tree traversal helpers, and it should be really obvious that this is a massive, expensive full render tree traversal, with boundingClientRect() (an ancestor tree walk) for each renderer.
>> 
>> In fact, I think we need to do this more efficiently, and be able to clip out non-visible areas, which makes it more like paint traversal. I'm going to r- because I think the perf implications here are a showstopper.
> 
> Actually this function should not even be on the RenderElement (the only non-recursive entry point is the RenderView). It looks more like a helper function that collects "paintable information" about the render tree.

@smfr: Regarding the performance hit - note that this doesn't called during render etc - only in the beginning when FCP has not been reported yet. The performance hit would only be there in edge cases where there is a very deep tree of non-paintable things. But I'll try to think of alternatives.

@zalan: sure, will revise.
Comment 25 Noam Rosenthal 2020-04-03 00:19:28 PDT
Created attachment 395353 [details]
Patch
Comment 26 Noam Rosenthal 2020-04-03 00:44:57 PDT
Created attachment 395354 [details]
Patch
Comment 27 Noam Rosenthal 2020-04-03 00:47:58 PDT
Revised based on comments from @zalan and @smfr:
- Moved paintability checks to PaintTimingRenderSupport (other name suggestions welcome)
- Moved eligibility logic to Document, made more succinct.
- Used more efficient methods for recursion and geometry checks
- Added comments and asserts to make sure the expensive path would only very rarely be used.
Comment 28 Noam Rosenthal 2020-04-05 23:08:52 PDT
Created attachment 395549 [details]
Patch
Comment 29 Noam Rosenthal 2020-04-10 11:24:03 PDT
Created attachment 396106 [details]
Patch
Comment 30 Noam Rosenthal 2020-04-10 11:37:39 PDT
Added a test that makes sure VNE blocks FCP - a delayed stylesheet loading would take place before FCP.
Comment 31 Noam Rosenthal 2020-04-13 10:47:24 PDT
Created attachment 396308 [details]
Patch
Comment 32 zalan 2020-04-13 13:41:34 PDT
Comment on attachment 396308 [details]
Patch

The rendering bits look good. Great test coverage!
Comment 33 Simon Fraser (smfr) 2020-04-13 14:24:09 PDT
Comment on attachment 396308 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=396308&action=review

> Source/WebCore/dom/Document.cpp:3152
> +void Document::markPaintTimingIfNeeded()

Would it be accurate to call this reportPaintTimingIfNeeded? I'm not really a fan of the "mark" terminology.

> Source/WebCore/page/FrameView.cpp:985
> +    if (auto* document = frame().document())
> +        document->markPaintTimingIfNeeded();

This is the wrong place for this.It should go in Page::updateRendering().

> Source/WebCore/page/PerformanceEntry.h:55
>          Mark = 1 << 1,
>          Measure = 1 << 2,
>          Resource = 1 << 3,
> +        Paint = 1 << 4

I like these lined up (adjust spaces before the =)

> Source/WebCore/rendering/ContentfulPaintChecker.cpp:40
> +static bool intersectsWithDocumentRect(const RenderObject& object, const IntRect& documentRect)

intersectsDocumentRect()

> Source/WebCore/rendering/ContentfulPaintChecker.cpp:47
> +            if (textRenderer.localToAbsoluteQuad(FloatQuad(box.rect())).boundingBox().intersects(documentRect))

This localToAbsolute() is another ancestor tree walk.

> Source/WebCore/rendering/ContentfulPaintChecker.cpp:109
> +    for (auto& ancestor : lineageOfType<RenderObject>(object)) {
> +        if (!ancestor.style().opacity())
> +            return false;
> +    }

Ouch. For each renderer, you're doing an ancestor tree walk. Starts getting > O(n)

> Source/WebCore/rendering/ContentfulPaintChecker.cpp:114
> +bool qualifiesForContentfulPaint(RenderView& view)

Might be nicer to pass a FrameView here and assert !frameView.needsLayout().

> Source/WebCore/rendering/ContentfulPaintChecker.cpp:120
> +    for (auto& object : descendantsOfType<RenderObject>(view)) {
> +        if (isContentful(object) && isPaintable(object, documentRect))
> +            return true;
> +    }

Potentially a full tree walk, which we need to avoid.

Why doesn't this make use of didMarkVisuallyNonEmptyPixels() which you're adding in the other patch? If we know which renderers have been marked, can't we remember them and ask them directly?

> Source/WebCore/rendering/ContentfulPaintChecker.h:27
> +bool qualifiesForContentfulPaint(RenderView&);

Maybe make this a class with static member functions. I suspect we'll want a real controller object with more state one day.
Comment 34 Noam Rosenthal 2020-04-13 14:33:39 PDT
Comment on attachment 396308 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=396308&action=review

>> Source/WebCore/dom/Document.cpp:3152
>> +void Document::markPaintTimingIfNeeded()
> 
> Would it be accurate to call this reportPaintTimingIfNeeded? I'm not really a fan of the "mark" terminology.

Sure, I tried to follow the same language as the spec.

>> Source/WebCore/page/FrameView.cpp:985
>> +        document->markPaintTimingIfNeeded();
> 
> This is the wrong place for this.It should go in Page::updateRendering().

Gotcha.

>> Source/WebCore/page/PerformanceEntry.h:55
>> +        Paint = 1 << 4
> 
> I like these lined up (adjust spaces before the =)

ok

>> Source/WebCore/rendering/ContentfulPaintChecker.cpp:47
>> +            if (textRenderer.localToAbsoluteQuad(FloatQuad(box.rect())).boundingBox().intersects(documentRect))
> 
> This localToAbsolute() is another ancestor tree walk.

I guess an alternative would be to pass the matrix going down the tree, using a child-walk rather than ancestor walk, or to save the potentially-contentful elements somewhere.

>> Source/WebCore/rendering/ContentfulPaintChecker.cpp:109
>> +    }
> 
> Ouch. For each renderer, you're doing an ancestor tree walk. Starts getting > O(n)

The other option is to do a child walk and stop when I get to zero opacity, like I did in a previous patch. would that work better?

>> Source/WebCore/rendering/ContentfulPaintChecker.cpp:114
>> +bool qualifiesForContentfulPaint(RenderView& view)
> 
> Might be nicer to pass a FrameView here and assert !frameView.needsLayout().

ok

>> Source/WebCore/rendering/ContentfulPaintChecker.cpp:120
>> +    }
> 
> Potentially a full tree walk, which we need to avoid.
> 
> Why doesn't this make use of didMarkVisuallyNonEmptyPixels() which you're adding in the other patch? If we know which renderers have been marked, can't we remember them and ask them directly?

We can do that, but then we would need to save a set of them somewhere (in FrameView?) Would that be preferrable? I guess we can clear that set once we report paint timing.

>> Source/WebCore/rendering/ContentfulPaintChecker.h:27
>> +bool qualifiesForContentfulPaint(RenderView&);
> 
> Maybe make this a class with static member functions. I suspect we'll want a real controller object with more state one day.

sure
Comment 35 Noam Rosenthal 2020-04-15 02:03:27 PDT
Created attachment 396515 [details]
Patch
Comment 36 Noam Rosenthal 2020-04-15 02:06:08 PDT
(In reply to Noam Rosenthal from comment #35)
> Created attachment 396515 [details]
> Patch

Riding on existing walker determineNonLayerDescendantsPaintedContent, which is capped at 200... so FCP will be reported if we've passed 200 non-layer elements and still not sure if we're contentful or not.
Comment 37 Noam Rosenthal 2020-04-16 04:48:45 PDT
Created attachment 396634 [details]
Patch
Comment 38 Noam Rosenthal 2020-04-16 06:56:07 PDT
Created attachment 396645 [details]
Patch
Comment 39 Noam Rosenthal 2020-04-16 08:15:33 PDT
Created attachment 396651 [details]
Patch
Comment 40 Simon Fraser (smfr) 2020-04-20 11:14:47 PDT
Comment on attachment 396651 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=396651&action=review

> Source/WebCore/page/PerformancePaintTiming.h:33
> +class PerformancePaintTiming final : public PerformanceEntry {

I would have called this PerformancePaintTimingEntry but I guess the web API precludes this.

> Source/WebCore/rendering/ContentfulPaintChecker.cpp:44
> +    TransformationMatrix transform = parentTransform * layer.currentTransform();

This manual transform math is almost certainly wrong, and I've no idea how you can test it to make sure it's correct here. Think about preserves3D, flattening, perspective....
Comment 41 Noam Rosenthal 2020-04-20 11:19:01 PDT
Comment on attachment 396651 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=396651&action=review

>> Source/WebCore/rendering/ContentfulPaintChecker.cpp:44
>> +    TransformationMatrix transform = parentTransform * layer.currentTransform();
> 
> This manual transform math is almost certainly wrong, and I've no idea how you can test it to make sure it's correct here. Think about preserves3D, flattening, perspective....

any suggestions on where to get this right without doing another ancestor walk? All I'm really looking for is to see that the layer is not transformed to nothing (e.g. by a 3d rotate) or to some place outside the document rect (e.g. a negative y).
Comment 42 Noam Rosenthal 2020-04-21 04:21:43 PDT
Created attachment 397073 [details]
Patch
Comment 43 Noam Rosenthal 2020-04-21 04:22:32 PDT
New patch uses fake-paint mechanism instead of custom tree walking.
Comment 44 Noam Rosenthal 2020-04-21 06:52:25 PDT
Created attachment 397077 [details]
Patch
Comment 45 Noam Rosenthal 2020-04-21 09:20:45 PDT
Created attachment 397087 [details]
Patch: using fake paint
Comment 46 Noam Rosenthal 2020-04-21 09:21:41 PDT
Created attachment 397088 [details]
Patch: using fake paint
Comment 47 Noam Rosenthal 2020-04-23 01:48:03 PDT
Created attachment 397330 [details]
Patch
Comment 48 Noam Rosenthal 2020-04-27 12:26:45 PDT
Created attachment 397716 [details]
Patch
Comment 49 Noam Rosenthal 2020-04-27 13:47:33 PDT
Created attachment 397730 [details]
Patch
Comment 50 Noam Rosenthal 2020-04-27 21:45:28 PDT
Created attachment 397802 [details]
Patch
Comment 51 Noam Rosenthal 2020-04-27 22:27:02 PDT
Created attachment 397808 [details]
Patch
Comment 52 Simon Fraser (smfr) 2020-04-28 12:31:35 PDT
Comment on attachment 397808 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=397808&action=review

> Source/WebCore/dom/Document.cpp:3155
> +void Document::reportPaintTimingIfNeeded()

I'd prefer this not be called "report". It doesn't synchronously call out to JS (which is good); it enqueues a perf entry, which is dispatched via the task queue (a zero-delay timer).

> Source/WebCore/dom/Document.h:2050
> +    bool m_didReportFirstContentfulPaint { false };

didEnqueue?

> Source/WebCore/page/Page.cpp:1399
> +        document.reportPaintTimingIfNeeded();

This is where "report" terminology is bad. It should be clear that this function does not trigger script synchronously.

> Source/WebCore/platform/graphics/GraphicsContext.h:519
> +    void setContentfulPaintDetected() { m_contenfulPaintDetected = true; }
> +    bool contenfulPaintDetected() const { return m_contenfulPaintDetected; }

We should probably change PaintInvalidationReasons to be less about invalidation, and more general. Then this could just be a value of PaintInvalidationReasons. Same for event region painting.

Not necessary for this patch though.

> Source/WebCore/rendering/TextPainter.cpp:111
> +        if (!textRun.text().toStringWithoutCopying().isAllSpecialCharacters<isHTMLSpace>())

Not sure you need the toStringWithoutCopying() there.

> Source/WebKit/Shared/WebPreferences.yaml:833
> +PaintTimingEnabled:
> +  type: bool
> +  defaultValue: false
> +  humanReadableName: "Paint Timing"
> +  humanReadableDescription: "Enable PaintTiming API"
> +  webcoreBinding: RuntimeEnabledFeatures

This should be "category: experimental" right?

> Tools/WebKitTestRunner/TestController.cpp:939
> +    WKPreferencesSetPaintTimingEnabled(preferences, true);

WTR turns on all experimental features by default. Don't think you need this.

> LayoutTests/platform/mac-wk1/TestExpectations:567
> +imported/w3c/web-platform-tests/paint-timing  [ Skip ]
> +performance-api/paint-timing-apis.html [ Skip ]
> +performance-api/paint-timing-frames.html [ Skip ]
> +performance-api/paint-timing-with-worker.html [ Skip ]
> +http/tests/performance/performance-paint-timing-fcp-after-visually-non-empty-for-num-chars.html [ Skip ]
> +http/tests/performance/performance-paint-timing-fcp-after-visually-non-empty-for-style.html [ Skip ]
> +performance-api/performance-observer-first-contentful-paint.html [ Skip ]

Please sort.

Can we make performance-api/paint-timing/ and just skip the whole dir?
Comment 53 Noam Rosenthal 2020-04-28 12:37:41 PDT
Comment on attachment 397808 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=397808&action=review

>> Source/WebCore/dom/Document.cpp:3155
>> +void Document::reportPaintTimingIfNeeded()
> 
> I'd prefer this not be called "report". It doesn't synchronously call out to JS (which is good); it enqueues a perf entry, which is dispatched via the task queue (a zero-delay timer).

enqueuePerformanceTimingEntryIfNeeded()?

>> Source/WebCore/page/Page.cpp:1399
>> +        document.reportPaintTimingIfNeeded();
> 
> This is where "report" terminology is bad. It should be clear that this function does not trigger script synchronously.

Got it. changing to enqueuePaintTimingEntryIfNeeded()

>> Source/WebCore/platform/graphics/GraphicsContext.h:519
>> +    bool contenfulPaintDetected() const { return m_contenfulPaintDetected; }
> 
> We should probably change PaintInvalidationReasons to be less about invalidation, and more general. Then this could just be a value of PaintInvalidationReasons. Same for event region painting.
> 
> Not necessary for this patch though.

Yea, I found the "invalidation" terminology a bit misleading. will see if I can do a followup about it

>> Source/WebCore/rendering/TextPainter.cpp:111
>> +        if (!textRun.text().toStringWithoutCopying().isAllSpecialCharacters<isHTMLSpace>())
> 
> Not sure you need the toStringWithoutCopying() there.

Regular toString() is ok here then?

>> Source/WebKit/Shared/WebPreferences.yaml:833
>> +  webcoreBinding: RuntimeEnabledFeatures
> 
> This should be "category: experimental" right?

right.

>> Tools/WebKitTestRunner/TestController.cpp:939
>> +    WKPreferencesSetPaintTimingEnabled(preferences, true);
> 
> WTR turns on all experimental features by default. Don't think you need this.

ok

>> LayoutTests/platform/mac-wk1/TestExpectations:567
>> +performance-api/performance-observer-first-contentful-paint.html [ Skip ]
> 
> Please sort.
> 
> Can we make performance-api/paint-timing/ and just skip the whole dir?

ok
Comment 54 Noam Rosenthal 2020-04-28 13:48:01 PDT
Created attachment 397880 [details]
Patch for landing
Comment 55 Simon Fraser (smfr) 2020-04-28 14:17:48 PDT
> 
> >> Source/WebCore/rendering/TextPainter.cpp:111
> >> +        if (!textRun.text().toStringWithoutCopying().isAllSpecialCharacters<isHTMLSpace>())
> > 
> > Not sure you need the toStringWithoutCopying() there.
> 
> Regular toString() is ok here then?

In other places we just do textRun.text().isAllSpecialCharacters()
Comment 56 Noam Rosenthal 2020-04-28 14:29:58 PDT
Created attachment 397886 [details]
Patch for landing
Comment 57 EWS 2020-04-28 15:34:57 PDT
Committed r260851: <https://trac.webkit.org/changeset/260851>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 397886 [details].
Comment 58 Radar WebKit Bug Importer 2020-04-28 15:35:36 PDT
<rdar://problem/62551526>
Comment 59 Truitt Savell 2020-04-29 12:54:06 PDT
The new test http/tests/performance/paint-timing/performance-paint-timing-fcp-after-visually-non-empty-for-num-chars.html

Added in https://trac.webkit.org/changeset/260851/webkit

is a flaky failure on Mac

History:
https://results.webkit.org/?suite=layout-tests&test=http%2Ftests%2Fperformance%2Fpaint-timing%2Fperformance-paint-timing-fcp-after-visually-non-empty-for-num-chars.html

Diff:
--- /Volumes/Data/slave/catalina-release-tests-wk2/build/layout-test-results/http/tests/performance/paint-timing/performance-paint-timing-fcp-after-visually-non-empty-for-num-chars-expected.txt
+++ /Volumes/Data/slave/catalina-release-tests-wk2/build/layout-test-results/http/tests/performance/paint-timing/performance-paint-timing-fcp-after-visually-non-empty-for-num-chars-actual.txt
@@ -1,3 +1,3 @@
-PASS paintEntry.startTime > timestamps[expectedIterations] is true
+FAIL paintEntry.startTime > timestamps[expectedIterations] should be true. Was false.
 PASS paintEntry.startTime < timestamps[expectedIterations + 2] is true
Comment 60 Noam Rosenthal 2020-04-29 13:16:53 PDT
(In reply to Truitt Savell from comment #59)
> The new test
> http/tests/performance/paint-timing/performance-paint-timing-fcp-after-
> visually-non-empty-for-num-chars.html
> 
> Added in https://trac.webkit.org/changeset/260851/webkit
> 
> is a flaky failure on Mac
> 
> History:
> https://results.webkit.org/?suite=layout-
> tests&test=http%2Ftests%2Fperformance%2Fpaint-timing%2Fperformance-paint-
> timing-fcp-after-visually-non-empty-for-num-chars.html
> 
> Diff:
> ---
> /Volumes/Data/slave/catalina-release-tests-wk2/build/layout-test-results/
> http/tests/performance/paint-timing/performance-paint-timing-fcp-after-
> visually-non-empty-for-num-chars-expected.txt
> +++
> /Volumes/Data/slave/catalina-release-tests-wk2/build/layout-test-results/
> http/tests/performance/paint-timing/performance-paint-timing-fcp-after-
> visually-non-empty-for-num-chars-actual.txt
> @@ -1,3 +1,3 @@
> -PASS paintEntry.startTime > timestamps[expectedIterations] is true
> +FAIL paintEntry.startTime > timestamps[expectedIterations] should be true.
> Was false.
>  PASS paintEntry.startTime < timestamps[expectedIterations + 2] is true

It's OK to mark it as flaky for now, and I'll work on finding a solution for it. VNE tests are a bit difficult... I can prepare a TextExpectations patch tomorrow if someone doesn't beat me to it.