Could be implemented as a boolean property and a webkit_web_view_is_playing_audio() method.
In WebKitGTK+ we are using API::UIClient, so we can override API::UIClient::isPlayingAudioDidChange() in our subclass instead of using the C-based API.
JFTR, I am working on this. Patch to come soon.
Created attachment 243273 [details] Patch
(In reply to comment #3) > Created attachment 243273 [details] > Patch WIP patch, tests and proper ChangeLog entries are missing. I will update it soon.
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.
Created attachment 243429 [details] Patch
Created attachment 243430 [details] Patch
(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?
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.
Created attachment 243440 [details] Patch
(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.
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.
Created attachment 243495 [details] Patch
Comment on attachment 243495 [details] Patch Clearing flags on attachment: 243495 Committed r177496: <http://trac.webkit.org/changeset/177496>
All reviewed patches have been landed. Closing bug.