Bug 192224 - [WPE] Add API to notify about frame displayed view backend callback
Summary: [WPE] Add API to notify about frame displayed view backend callback
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WPE WebKit (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 191906 192627
Blocks:
  Show dependency treegraph
 
Reported: 2018-11-30 06:42 PST by Carlos Garcia Campos
Modified: 2018-12-17 05:31 PST (History)
9 users (show)

See Also:


Attachments
Patch (11.53 KB, patch)
2018-11-30 06:47 PST, Carlos Garcia Campos
mcatanzaro: review+
Details | Formatted Diff | Diff
Patch for landing (12.28 KB, patch)
2018-12-17 03:26 PST, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Patch for landing (14.15 KB, patch)
2018-12-17 03:49 PST, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2018-11-30 06:42:56 PST
Add webkit_web_view_add_frame_displayed_callback() and webkit_web_view_remove_frame_displayed_callback().
Comment 1 Carlos Garcia Campos 2018-11-30 06:47:08 PST
Created attachment 356172 [details]
Patch

This patch doesn't work with the fdo backend, see https://github.com/Igalia/WPEBackend-fdo/pull/28
Comment 2 Build Bot 2018-11-30 06:48:29 PST
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
Comment 3 Carlos Garcia Campos 2018-12-10 01:32:44 PST
Ping reviewers
Comment 4 Michael Catanzaro 2018-12-10 11:42:44 PST
Comment on attachment 356172 [details]
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....
Comment 5 Carlos Garcia Campos 2018-12-10 23:37:17 PST
Comment on attachment 356172 [details]
Patch

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

>> 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."

I mentioned explicitly on purpose after realizing that fdo backend was not dispatching the frame displayed signal. So, I want to make it clear that this callback depends on the backend.

>> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:4169
>> + * 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.

Not really, that's actually bug, good catch :-) We should check callback is not in frameDisplayedCallbacksToRemove before emitting the signal. I'll try to make a test case.

>> Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitWebView.cpp:1240
>> +        }, 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....

This is just a test...
Comment 6 Carlos Garcia Campos 2018-12-12 02:59:11 PST
Committed r239103: <https://trac.webkit.org/changeset/239103>
Comment 7 WebKit Commit Bot 2018-12-12 08:25:16 PST
Re-opened since this is blocked by bug 192627
Comment 8 Carlos Garcia Campos 2018-12-17 03:26:44 PST
Created attachment 357433 [details]
Patch for landing
Comment 9 Carlos Garcia Campos 2018-12-17 03:49:18 PST
Created attachment 357434 [details]
Patch for landing
Comment 10 Carlos Garcia Campos 2018-12-17 05:31:35 PST
Committed r239264: <https://trac.webkit.org/changeset/239264>