[webkit-reviews] review granted: [Bug 192224] [WPE] Add API to notify about frame displayed view backend callback : [Attachment 356172] Patch

[bugzilla-daemon at webkit.org]

Michael Catanzaro <mcatanzaro at igalia.com> has granted Carlos Garcia Campos
<cgarcia at igalia.com>'s request for review:
Bug 192224: [WPE] Add API to notify about frame displayed view backend callback
https://bugs.webkit.org/show_bug.cgi?id=192224

Attachment 356172: Patch

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




--- Comment #4 from Michael Catanzaro <mcatanzaro at igalia.com> ---
Comment on attachment 356172
  --> https://bugs.webkit.org/attachment.cgi?id=356172
Patch

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

> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:219
> +    FrameDisplayedCallback(const FrameDisplayedCallback&) = delete;
> +    FrameDisplayedCallback& operator=(const FrameDisplayedCallback&) =
delete;

Good thinking to make it uncopyable.

> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:410
> +	      
SetForScope<bool>inFrameDisplayedGuard(m_webView->priv->inFrameDisplayed,
true);

Space after >

> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:4147
> + * Add a callback to be called when the backend notifies that a frame has
been displayed in @web_view.

I don't think you need the detail about the backend:

"Add a callback to be called when the backend notifies that a frame has been
displayed in @web_view."

> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:4169
> + * Removes a #WebKitFrameDisplayedCallback previously added to @web_view
with
> + * webkit_web_view_add_frame_displayed_callback().

Maybe we should document that the callback may be called once more after this
function runs if the callbacks are currently being executed. Otherwise,
applications that remove one callback during another callback and don't expect
the original callback to be called again could crash. Admittedly, it would be
pretty weird for an application to try this, so maybe not that important, but
it seems unsafe and you do have a bunch of webView->priv->inFrameDisplayed
logic trying to ensure safety here, after all.

> Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitWebView.cpp:1240
> +	   : m_id(webkit_web_view_add_frame_displayed_callback(m_webView,
[](WebKitWebView*, gpointer userData) {
> +	       auto* test = static_cast<FrameDisplayedTest*>(userData);
> +	       if (!test->m_maxFrames)
> +		   return;
> +
> +	       if (++test->m_frameCounter == test->m_maxFrames)
> +		   RunLoop::main().dispatch([test] { test->quitMainLoop(); });
> +	   }, this, nullptr))

Don't you think this is a bit much for a constructor initializer? I would do
this in the body of the constructor. Up to you....