[webkit-reviews] review denied: [Bug 208499] Implement FCP (first contentful paint) : [Attachment 394942] 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 394942: Patch

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




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