WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
208499
Implement FCP (first contentful paint)
https://bugs.webkit.org/show_bug.cgi?id=208499
Summary
Implement FCP (first contentful paint)
Noam Rosenthal
Reported
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
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
Show Obsolete
(32)
View All
Add attachment
proposed patch, testcase, etc.
Ryosuke Niwa
Comment 1
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.
Noam Rosenthal
Comment 2
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
Noam Rosenthal
Comment 3
2020-03-20 06:17:01 PDT
Created
attachment 394078
[details]
Patch
Noam Rosenthal
Comment 4
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).
Ryosuke Niwa
Comment 5
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.
Ryosuke Niwa
Comment 6
2020-03-20 20:01:12 PDT
Comment on
attachment 394078
[details]
Patch r- on that basis
Noam Rosenthal
Comment 7
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 :)
Noam Rosenthal
Comment 8
2020-03-23 11:33:06 PDT
Created
attachment 394280
[details]
Patch
Noam Rosenthal
Comment 9
2020-03-23 11:35:55 PDT
A new patch. explicity referring to VNE.
Noam Rosenthal
Comment 10
2020-03-23 11:51:28 PDT
Created
attachment 394284
[details]
Patch
Noam Rosenthal
Comment 11
2020-03-24 09:23:32 PDT
Created
attachment 394370
[details]
Patch
Noam Rosenthal
Comment 12
2020-03-30 04:11:16 PDT
Created
attachment 394899
[details]
Patch
Noam Rosenthal
Comment 13
2020-03-30 04:51:06 PDT
Created
attachment 394904
[details]
Patch
Noam Rosenthal
Comment 14
2020-03-30 05:29:12 PDT
Created
attachment 394907
[details]
Patch
Noam Rosenthal
Comment 15
2020-03-30 05:49:06 PDT
Created
attachment 394909
[details]
Patch
Noam Rosenthal
Comment 16
2020-03-30 06:25:53 PDT
Created
attachment 394911
[details]
Patch
Noam Rosenthal
Comment 17
2020-03-30 07:12:57 PDT
Created
attachment 394913
[details]
Patch
Noam Rosenthal
Comment 18
2020-03-30 07:54:30 PDT
Created
attachment 394917
[details]
Patch
Noam Rosenthal
Comment 19
2020-03-30 08:25:40 PDT
Created
attachment 394919
[details]
Patch
Noam Rosenthal
Comment 20
2020-03-30 11:51:10 PDT
Created
attachment 394942
[details]
Patch
Noam Rosenthal
Comment 21
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.
Simon Fraser (smfr)
Comment 22
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.
alan
Comment 23
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.
Noam Rosenthal
Comment 24
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.
Noam Rosenthal
Comment 25
2020-04-03 00:19:28 PDT
Created
attachment 395353
[details]
Patch
Noam Rosenthal
Comment 26
2020-04-03 00:44:57 PDT
Created
attachment 395354
[details]
Patch
Noam Rosenthal
Comment 27
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.
Noam Rosenthal
Comment 28
2020-04-05 23:08:52 PDT
Created
attachment 395549
[details]
Patch
Noam Rosenthal
Comment 29
2020-04-10 11:24:03 PDT
Created
attachment 396106
[details]
Patch
Noam Rosenthal
Comment 30
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.
Noam Rosenthal
Comment 31
2020-04-13 10:47:24 PDT
Created
attachment 396308
[details]
Patch
alan
Comment 32
2020-04-13 13:41:34 PDT
Comment on
attachment 396308
[details]
Patch The rendering bits look good. Great test coverage!
Simon Fraser (smfr)
Comment 33
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.
Noam Rosenthal
Comment 34
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
Noam Rosenthal
Comment 35
2020-04-15 02:03:27 PDT
Created
attachment 396515
[details]
Patch
Noam Rosenthal
Comment 36
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.
Noam Rosenthal
Comment 37
2020-04-16 04:48:45 PDT
Created
attachment 396634
[details]
Patch
Noam Rosenthal
Comment 38
2020-04-16 06:56:07 PDT
Created
attachment 396645
[details]
Patch
Noam Rosenthal
Comment 39
2020-04-16 08:15:33 PDT
Created
attachment 396651
[details]
Patch
Simon Fraser (smfr)
Comment 40
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....
Noam Rosenthal
Comment 41
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).
Noam Rosenthal
Comment 42
2020-04-21 04:21:43 PDT
Created
attachment 397073
[details]
Patch
Noam Rosenthal
Comment 43
2020-04-21 04:22:32 PDT
New patch uses fake-paint mechanism instead of custom tree walking.
Noam Rosenthal
Comment 44
2020-04-21 06:52:25 PDT
Created
attachment 397077
[details]
Patch
Noam Rosenthal
Comment 45
2020-04-21 09:20:45 PDT
Created
attachment 397087
[details]
Patch: using fake paint
Noam Rosenthal
Comment 46
2020-04-21 09:21:41 PDT
Created
attachment 397088
[details]
Patch: using fake paint
Noam Rosenthal
Comment 47
2020-04-23 01:48:03 PDT
Created
attachment 397330
[details]
Patch
Noam Rosenthal
Comment 48
2020-04-27 12:26:45 PDT
Created
attachment 397716
[details]
Patch
Noam Rosenthal
Comment 49
2020-04-27 13:47:33 PDT
Created
attachment 397730
[details]
Patch
Noam Rosenthal
Comment 50
2020-04-27 21:45:28 PDT
Created
attachment 397802
[details]
Patch
Noam Rosenthal
Comment 51
2020-04-27 22:27:02 PDT
Created
attachment 397808
[details]
Patch
Simon Fraser (smfr)
Comment 52
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?
Noam Rosenthal
Comment 53
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
Noam Rosenthal
Comment 54
2020-04-28 13:48:01 PDT
Created
attachment 397880
[details]
Patch for landing
Simon Fraser (smfr)
Comment 55
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()
Noam Rosenthal
Comment 56
2020-04-28 14:29:58 PDT
Created
attachment 397886
[details]
Patch for landing
EWS
Comment 57
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]
.
Radar WebKit Bug Importer
Comment 58
2020-04-28 15:35:36 PDT
<
rdar://problem/62551526
>
Truitt Savell
Comment 59
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
Noam Rosenthal
Comment 60
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.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug