[webkit-reviews] review granted: [Bug 173897] Unify post-painting callbacks : [Attachment 313964] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Tim Horton <thorton at apple.com>'s
request for review:
Bug 173897: Unify post-painting callbacks
https://bugs.webkit.org/show_bug.cgi?id=173897

Attachment 313964: Patch

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




--- Comment #2 from Darin Adler <darin at apple.com> ---
Comment on attachment 313964
  --> https://bugs.webkit.org/attachment.cgi?id=313964
Patch

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

r=me assuming everything works as well as I expect it will.

> Source/WebCore/page/FrameView.cpp:1238
> +    frame().page()->addOneShotPostPaintCallback([protectedThis =
makeRef(*this)] {
> +	   InspectorInstrumentation::didComposite(protectedThis.get().frame());
> +    });

I probably would have just protected the frame, not the frame view.

> Source/WebCore/page/Page.cpp:2425
> +    while (!m_oneShotPostPaintCallbacks.isEmpty()) {
> +	   auto callback = m_oneShotPostPaintCallbacks.takeLast();
> +	   callback();
> +    }

I would have written this as a one-liner, I think.

> Source/WebCore/page/Page.h:604
> +    WEBCORE_EXPORT void
addOneShotPostPaintCallback(WTF::Function<void()>&&);

Can this be on MainFrame instead of Page? Would avoid having to do null checks.
Or maybe if there was a MainFrameView.

> Source/WebKit/mac/WebCoreSupport/WebChromeClient.mm:1128
> +	   if (auto corePage = [webView page])
> +	       corePage->dispatchAndClearPostPaintCallbacks();

Annoying to capture the web view just so we can get its page. Makes me wish the
function was on MainFrame so we could just capture that.

> Source/WebKit2/WebProcess/WebPage/mac/TiledCoreAnimationDrawingArea.h:91
> +    void scheduleOneShotPostPaintCallback() override;

final?