[webkit-reviews] review denied: [Bug 208499] Implement FCP (first contentful paint) : [Attachment 396308] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied Noam Rosenthal
<noam at webkit.org>'s request for review:
Bug 208499: Implement FCP (first contentful paint)
https://bugs.webkit.org/show_bug.cgi?id=208499

Attachment 396308: Patch

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




--- Comment #33 from Simon Fraser (smfr) <simon.fraser at apple.com> ---
Comment on attachment 396308
  --> https://bugs.webkit.org/attachment.cgi?id=396308
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().intersec
ts(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.