[webkit-reviews] review granted: [Bug 207966] [First paint] Introduce FrameView::m_firstVisuallyNonEmptyLayoutCallbackPending : [Attachment 391203] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted zalan
<zalan at apple.com>'s request for review:
Bug 207966: [First paint] Introduce
FrameView::m_firstVisuallyNonEmptyLayoutCallbackPending
https://bugs.webkit.org/show_bug.cgi?id=207966

Attachment 391203: Patch

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




--- Comment #3 from Simon Fraser (smfr) <simon.fraser at apple.com> ---
Comment on attachment 391203
  --> https://bugs.webkit.org/attachment.cgi?id=391203
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=391203&action=review

> Source/WebCore/page/FrameView.cpp:5144
> +	       addPaintPendingMilestones(DidFirstMeaningfulPaint);

I"m sad that we're giving up on the distinction between "first contentful
paint" and "first meaningful paint". How does "relevant repainted objects"
threshold fit into this?

> Source/WebCore/page/FrameView.h:924
> +    bool m_contentIsQualifiedAsVisuallyNonEmpty { false };
> +    bool m_firstVisuallyNonEmptyLayoutCallbackPending { true };

Maybe collapse these two into a "state" enum.

If not, please be consistent with naming. Maybe "contentIsQualified" ->
qualifiesAs, and m_firstVisuallyNonEmptyLayoutCallbackPending ->
m_didFirstMeaningfulPaintMilestone pending