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

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted 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 397808: Patch

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




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

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

> Source/WebCore/dom/Document.cpp:3155
> +void Document::reportPaintTimingIfNeeded()

I'd prefer this not be called "report". It doesn't synchronously call out to JS
(which is good); it enqueues a perf entry, which is dispatched via the task
queue (a zero-delay timer).

> Source/WebCore/dom/Document.h:2050
> +    bool m_didReportFirstContentfulPaint { false };

didEnqueue?

> Source/WebCore/page/Page.cpp:1399
> +	   document.reportPaintTimingIfNeeded();

This is where "report" terminology is bad. It should be clear that this
function does not trigger script synchronously.

> Source/WebCore/platform/graphics/GraphicsContext.h:519
> +    void setContentfulPaintDetected() { m_contenfulPaintDetected = true; }
> +    bool contenfulPaintDetected() const { return m_contenfulPaintDetected; }

We should probably change PaintInvalidationReasons to be less about
invalidation, and more general. Then this could just be a value of
PaintInvalidationReasons. Same for event region painting.

Not necessary for this patch though.

> Source/WebCore/rendering/TextPainter.cpp:111
> +	   if
(!textRun.text().toStringWithoutCopying().isAllSpecialCharacters<isHTMLSpace>()
)

Not sure you need the toStringWithoutCopying() there.

> Source/WebKit/Shared/WebPreferences.yaml:833
> +PaintTimingEnabled:
> +  type: bool
> +  defaultValue: false
> +  humanReadableName: "Paint Timing"
> +  humanReadableDescription: "Enable PaintTiming API"
> +  webcoreBinding: RuntimeEnabledFeatures

This should be "category: experimental" right?

> Tools/WebKitTestRunner/TestController.cpp:939
> +    WKPreferencesSetPaintTimingEnabled(preferences, true);

WTR turns on all experimental features by default. Don't think you need this.

> LayoutTests/platform/mac-wk1/TestExpectations:567
> +imported/w3c/web-platform-tests/paint-timing  [ Skip ]
> +performance-api/paint-timing-apis.html [ Skip ]
> +performance-api/paint-timing-frames.html [ Skip ]
> +performance-api/paint-timing-with-worker.html [ Skip ]
>
+http/tests/performance/performance-paint-timing-fcp-after-visually-non-empty-f
or-num-chars.html [ Skip ]
>
+http/tests/performance/performance-paint-timing-fcp-after-visually-non-empty-f
or-style.html [ Skip ]
> +performance-api/performance-observer-first-contentful-paint.html [ Skip ]

Please sort.

Can we make performance-api/paint-timing/ and just skip the whole dir?