Currently, we block the initial render on our "visually non-empty" heuristic. This heuristic depends on (among other things) how much text is in the document, how many image pixels are in the document, and whether or not the main resource is fully parsed. However, SVG and canvas content is not considered by the heuristic. This causes a delayed first contentful paint for certain websites. For instance, YouTube's first contentful paint is often delayed until far after the main HTML resource is parsed: 1. They don't ship down any text initially, so we won't trip over the visually non-empty character threshold 2. They don't ship down any actual images initially, so we won't trip over the filled-image-pixel threshold (everything that looks like an image is actually an SVG) 3. On an uncached load, we won't paint after parsing the initial HTML either, since there's usually an outstanding web font loading, and our heuristic delays the first paint until the web fonts load if there isn't enough character or image content We could improve our FCP for YT and other similar sites by including an estimate of SVG and canvas content sizes in the visually-non-empty heuristic. This will also improve FCP as reported by https://w3c.github.io/paint-timing/#first-contentful-paint.
<rdar://problem/59971943>
Assigning to Alan since he said he could take a look at this.
Created attachment 392248 [details] Patch
Comment on attachment 392248 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=392248&action=review > Source/WebCore/rendering/svg/RenderSVGRoot.cpp:62 > + if (this->style().width().isFixed() && this->style().height().isFixed()) > + view().frameView().incrementVisuallyNonEmptyPixelCount({ this->style().width().intValue(), this->style().height().intValue() }); Don't you need to convert non-pixel fixed types to pixels?
SVG content is tracked over here bug 208663
Created attachment 395137 [details] Patch
Alan, I hope it's OK that I took a crack at this! Trying to match canvas VNE behavior to FCP, as requested in https://bugs.webkit.org/show_bug.cgi?id=78011#c7 I know that this still has the same problem as https://bugs.webkit.org/show_bug.cgi?id=208501#c14, thinking we can fix those in one go.
Comment on attachment 395137 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=395137&action=review Why no test? > Source/WebCore/ChangeLog:12 > + Reviewed by NOBODY (OOPS!). This goes above the description. > Source/WebCore/html/HTMLCanvasElement.cpp:465 > + didAttachContext(); > // Need to make sure a RenderLayer and compositing layer get created for the Canvas. > invalidateStyleAndLayerComposition(); Why is this in a different order to the previous one? > Source/WebCore/rendering/RenderHTMLCanvas.cpp:113 > + view().frameView().incrementVisuallyNonEmptyPixelCount(ceiledIntSize(intrinsicSize())); When called from the constructor, is intrinsicSize set yet?
Comment on attachment 395137 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=395137&action=review >> Source/WebCore/ChangeLog:12 >> + Reviewed by NOBODY (OOPS!). > > This goes above the description. Gotcha >> Source/WebCore/html/HTMLCanvasElement.cpp:465 >> invalidateStyleAndLayerComposition(); > > Why is this in a different order to the previous one? No reason, will make more consistent. >> Source/WebCore/rendering/RenderHTMLCanvas.cpp:113 >> + view().frameView().incrementVisuallyNonEmptyPixelCount(ceiledIntSize(intrinsicSize())); > > When called from the constructor, is intrinsicSize set yet? I believe so. Will make sure.
(In reply to Simon Fraser (smfr) from comment #8) > Comment on attachment 395137 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=395137&action=review > > Why no test? Can I get some pointers to how this kind of feature is tested? https://trac.webkit.org/changeset/257952/webkit, for example, adds similar functionality and did not add tests.
Comment on attachment 395137 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=395137&action=review >>> Source/WebCore/rendering/RenderHTMLCanvas.cpp:113 >>> + view().frameView().incrementVisuallyNonEmptyPixelCount(ceiledIntSize(intrinsicSize())); >> >> When called from the constructor, is intrinsicSize set yet? > > I believe so. Will make sure. Actually, it has to be available, since the rendering context is available.
Comment on attachment 395137 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=395137&action=review > Source/WebCore/html/HTMLCanvasElement.cpp:927 > + auto& canvasRenderer = downcast<RenderHTMLCanvas>(*renderer); > + canvasRenderer.didAttachContext(); Better as a one-liner: downcast<RenderHTMLCanvas>(*renderer).didAttachContext();
Created attachment 395245 [details] Patch
Seems like this was fixed in https://trac.webkit.org/changeset/257899/webkit
Comment on attachment 395245 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=395245&action=review > Source/WebCore/rendering/RenderHTMLCanvas.cpp:115 > +void RenderHTMLCanvas::incrementVisuallyNonEmptyPixelCountIfNeeded() > +{ > + if (m_didIncrementVisuallyNonEmptyPixelCount) > + return; > + > + view().frameView().incrementVisuallyNonEmptyPixelCount(ceiledIntSize(intrinsicSize())); > + m_didIncrementVisuallyNonEmptyPixelCount = true; > +} This is another example where this logic would be much better in a dedicated first paint class with all the tracking.
(In reply to zalan from comment #15) > Comment on attachment 395245 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=395245&action=review > > > Source/WebCore/rendering/RenderHTMLCanvas.cpp:115 > > +void RenderHTMLCanvas::incrementVisuallyNonEmptyPixelCountIfNeeded() > > +{ > > + if (m_didIncrementVisuallyNonEmptyPixelCount) > > + return; > > + > > + view().frameView().incrementVisuallyNonEmptyPixelCount(ceiledIntSize(intrinsicSize())); > > + m_didIncrementVisuallyNonEmptyPixelCount = true; > > +} > > This is another example where this logic would be much better in a dedicated > first paint class with all the tracking. I saw that you already made canvas increment the pixel count in https://trac.webkit.org/changeset/257899/webkit, isn't this patch superfluous now?
Comment on attachment 395245 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=395245&action=review > Source/WebCore/rendering/RenderHTMLCanvas.h:54 > + bool m_didIncrementVisuallyNonEmptyPixelCount { false }; Why doesn't this use RenderElement's didMarkFor...?
Comment on attachment 395245 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=395245&action=review >> Source/WebCore/rendering/RenderHTMLCanvas.h:54 >> + bool m_didIncrementVisuallyNonEmptyPixelCount { false }; > > Why doesn't this use RenderElement's didMarkFor...? Oh damn! It was added after this patch. Will fix. But I want to know first if this is even needed, given https://trac.webkit.org/changeset/257899/webkit
(In reply to Noam Rosenthal from comment #18) > Comment on attachment 395245 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=395245&action=review > > >> Source/WebCore/rendering/RenderHTMLCanvas.h:54 > >> + bool m_didIncrementVisuallyNonEmptyPixelCount { false }; > > > > Why doesn't this use RenderElement's didMarkFor...? > > Oh damn! It was added after this patch. Will fix. But I want to know first > if this is even needed, given https://trac.webkit.org/changeset/257899/webkit Ah it was in my other patch... sure. I will revise this after the bg-image patch goes in, once we figure out if this is needed...