Bug 208482 – Canvas content should factor into visually-non-empty heuristic

• Status: RESOLVED
• Resolution: WORKSFORME
• Component: Layout and Rendering

[Ben Nham]

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.

[Radar WebKit Bug Importer]

<rdar://problem/59971943>

[Ben Nham]

Assigning to Alan since he said he could take a look at this.

[zalan]

Created attachment 392248 [details]
Patch

[Simon Fraser (smfr)]

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?

[zalan]

SVG content is tracked over here bug 208663

[Noam Rosenthal]

Created attachment 395137 [details]
Patch

[Noam Rosenthal]

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.

[Simon Fraser (smfr)]

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?

[Noam Rosenthal]

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.

[Noam Rosenthal]

(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.

[Noam Rosenthal]

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.

[Darin Adler]

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();

[Noam Rosenthal]

Created attachment 395245 [details]
Patch

[Noam Rosenthal]

Seems like this was fixed in https://trac.webkit.org/changeset/257899/webkit

[zalan]

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.

[Noam Rosenthal]

(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?

[Simon Fraser (smfr)]

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...?

[Noam Rosenthal]

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

[Noam Rosenthal]

(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...