Bug 138918 – [GTK] Add WebKitWebView:is-playing-audio property

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Philippe Normand]

Could be implemented as a boolean property and a webkit_web_view_is_playing_audio() method.

[Adrian Perez]

In WebKitGTK+ we are using API::UIClient, so we can override API::UIClient::isPlayingAudioDidChange() in our subclass instead
of using the C-based API.

[Adrian Perez]

JFTR, I am working on this. Patch to come soon.

[Adrian Perez]

Created attachment 243273 [details]
Patch

[Adrian Perez]

(In reply to comment #3)
> Created attachment 243273 [details]
> Patch

WIP patch, tests and proper ChangeLog entries are missing. I will update it soon.

[Carlos Garcia Campos]

Comment on attachment 243273 [details]
Patch

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

Do not ask for review in wip patches. API looks good to me. Thanks!

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:179
> +    bool isPlayingAudio;

I don't think we need to duplicate this, since this is always mapped to WebPageProxy::isPlayingAudio(). In the case of strings, we duplicate the info because we need to convert the string to utf8 and we want to return a const char*, but for booleanas we can just return the WebPageProxy value.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:235
> +void webkitWebViewSetIsPlayingAudio(WebKitWebView* webView, bool isPlayingAudio)

So, instead of SetIsPlayingAudio, this could be webkitWebViewIsPlayingAudioChanged(), and we only need to emit the signal, since WebPageProxy::isPlayingAudioDidChange already ensures that UIClient::isPlayingAudioDidChange() is only called when the member has actually changed. We could also emit the signal directly from WebKitUIClient.cpp, but I think the code is easier to follow if the signal is emitted here.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2496
> + * connecting to the notify::is-playing-audio signal @web_view. This is

of @web_view.

[Adrian Perez]

Created attachment 243429 [details]
Patch

[Adrian Perez]

Created attachment 243430 [details]
Patch

[Adrian Perez]

(In reply to comment #7)
> Created attachment 243430 [details]
> Patch

This version includes a test case and the nits are fixed. But somehow
the test case does not get the notify::is-playing-audio signal emitted,
but if I add a signal handler in MiniBrowser and browse a website with
video or audio (e.g. YouTube) the signal is emitted properly. Probably
the issue has something to do with the even loop, but I could not find
the issue. Any pointers?

[Carlos Garcia Campos]

Comment on attachment 243430 [details]
Patch

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

The API looks perfect to me, r- only because of the tests

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitWebView.cpp:710
> +    const char* value = g_getenv("TEST_WEBKIT_API_WEBKIT2_RESOURCES_PATH");
> +    if (!value) {
> +        g_printerr("TEST_WEBKIT_API_WEBKIT2_RESOURCES_PATH environment variable not found\n");
> +        exit(1);

This is actually for C API tests, we avoid using env variables if possible.

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitWebView.cpp:715
> +    gsize bytesWritten;
> +    GUniquePtr<char> testResourcesPath(g_filename_to_utf8(value, -1, 0, &bytesWritten, 0));
> +    GUniquePtr<char> fullPath(g_build_filename(testResourcesPath.get(), name, nullptr));
> +    return g_strdup_printf("file://%s", fullPath.get());

This should be added to Test class, see getResourcesDir() in TestMain.h. We could either add a new method to get the resources in the C API tests dir, or add a parameter to  getResourcesDir() with an enum with values for WebKit2GTK or WebKit2 tests

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitWebView.cpp:721
> +    g_assert_cmpint(webkit_web_view_is_playing_audio(test->m_webView), ==, FALSE);

Don't use cmpint with booleans, use simply assert g_assert(!webkit_web_view_is_playing_audio(test->m_webView));

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitWebView.cpp:726
> +    g_assert_cmpint(webkit_web_view_is_playing_audio(test->m_webView), ==, FALSE);

Ditto.

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitWebView.cpp:729
> +    test->waitUntilIsPlayingAudioChanged();

So, the test gets stalled here and then times out? One possible reason could be that the view is not realized and the video is not actually played. You could try to add the view to a window, see WebViewTest::showInWindowAndWaitUntilMapped

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitWebView.cpp:731
> +    g_assert_cmpint(webkit_web_view_is_playing_audio(test->m_webView), ==, TRUE);

Don't use int for booleans

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitWebView.cpp:732
> +}

It would be interesting to test also that if the video is muted (or paused/stoped) the property is set to FALSE.

> Tools/TestWebKitAPI/gtk/WebKit2Gtk/WebViewTest.cpp:206
> +static void isPlayingAudioChanged(WebKitWebView* webView, GParamSpec*, WebViewTest* test)
> +{
> +    g_signal_handlers_disconnect_by_func(webView, reinterpret_cast<void*>(isPlayingAudioChanged), test);
> +    g_main_loop_quit(test->m_mainLoop);
> +}
> +
> +void WebViewTest::waitUntilIsPlayingAudioChanged()
> +{
> +    g_signal_connect(m_webView, "notify::is-playing-audio", G_CALLBACK(isPlayingAudioChanged), this);
> +    g_main_loop_run(m_mainLoop);
> +}

This is very specific to the is-playing-audio test. I would add a custom class to TestWebKitWebView.cpp, derived from WebViewTest.

[Adrian Perez]

Created attachment 243440 [details]
Patch

[Adrian Perez]

(In reply to comment #9)
> Comment on attachment 243430 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=243430&action=review
> 
> The API looks perfect to me, r- only because of the tests
> 
> > Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitWebView.cpp:710
> > +    const char* value = g_getenv("TEST_WEBKIT_API_WEBKIT2_RESOURCES_PATH");
> > +    if (!value) {
> > +        g_printerr("TEST_WEBKIT_API_WEBKIT2_RESOURCES_PATH environment variable not found\n");
> > +        exit(1);
> 
> This is actually for C API tests, we avoid using env variables if possible.

Fixed in the last revision of the patch.

> > Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitWebView.cpp:715
> > +    gsize bytesWritten;
> > +    GUniquePtr<char> testResourcesPath(g_filename_to_utf8(value, -1, 0, &bytesWritten, 0));
> > +    GUniquePtr<char> fullPath(g_build_filename(testResourcesPath.get(), name, nullptr));
> > +    return g_strdup_printf("file://%s", fullPath.get());
> 
> This should be added to Test class, see getResourcesDir() in TestMain.h. We
> could either add a new method to get the resources in the C API tests dir,
> or add a parameter to  getResourcesDir() with an enum with values for
> WebKit2GTK or WebKit2 tests

I have added a flag to getResourcesDir() as suggested.

> > Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitWebView.cpp:721
> > +    g_assert_cmpint(webkit_web_view_is_playing_audio(test->m_webView), ==, FALSE);
> 
> Don't use cmpint with booleans, use simply assert
> g_assert(!webkit_web_view_is_playing_audio(test->m_webView));

All of those are now plain g_assert()s.

> > Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitWebView.cpp:729
> > +    test->waitUntilIsPlayingAudioChanged();
> 
> So, the test gets stalled here and then times out? One possible reason could
> be that the view is not realized and the video is not actually played. You
> could try to add the view to a window, see
> WebViewTest::showInWindowAndWaitUntilMapped

Thanks a lot for the tip, this made the test case work perfectly.
 
> It would be interesting to test also that if the video is muted (or
> paused/stoped) the property is set to FALSE.

Added another check after pausing the video. And it works as expected ;)

> > +void WebViewTest::waitUntilIsPlayingAudioChanged()
> > +{
> > +    g_signal_connect(m_webView, "notify::is-playing-audio", G_CALLBACK(isPlayingAudioChanged), this);
> > +    g_main_loop_run(m_mainLoop);
> > +}
> 
> This is very specific to the is-playing-audio test. I would add a custom
> class to TestWebKitWebView.cpp, derived from WebViewTest.

Moved into a local class in TestWebKitWebView.cpp.

If a second reviewer agrees that the new API is good to go, it seems
to me that the the patch should be ready to land.

[Carlos Garcia Campos]

Comment on attachment 243440 [details]
Patch

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

The API is simple enough so that we probably don't need a double approval.

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitWebView.cpp:29
> +class WebViewIsPlayingAudioTest : public WebViewTest {

Following the pattern used in other tests, this should probably be IsPlayingAudioWebViewTest, but it doesn't really matter.

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitWebView.cpp:732
> +    GUniquePtr<char> resourceUrl(g_strdup_printf("file://%s", resourcePath.get()));

resourceUrl -> resourceURL
g_filename_to_uri is probably more appropriate than "file://%s"

> Tools/TestWebKitAPI/gtk/WebKit2Gtk/WebViewTest.h:50
> +    void waitUntilIsPlayingAudioChanged();

You forgot to remove this.

[Adrian Perez]

Created attachment 243495 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 243495 [details]
Patch

Clearing flags on attachment: 243495

Committed r177496: <http://trac.webkit.org/changeset/177496>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.