Right now epiphany shows an audio icon on tabs that are playing audio. It would be nice if webkit provided a way to mute tabs that are playing audio. The tabs would then show a 'muted audio' icon instead.
We already have webkit_web_view_is_playing_audio(), what we need is something like webkit_web_view_set_can_play_audio(). It needs to be a property of WebKitWebView in order to implement the browser UI we want.
The infrastructure is already in place. You only need a new API in the WebView, that would call WebPage::setMuted().
OK cool. Sounds low-hanging. It's likely a little more complicated than that though, because the page will swap on navigation and we don't want the muted state to be reset. So when a new page becomes associated to the view, sync the muted state from the old page to the new page.
Created attachment 387812 [details] Patch
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 on attachment 387812 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=387812&action=review > Source/WebKit/ChangeLog:10 > + No new tests (OOPS!). A basic API test would check the default value, change it, then get it again and verify that it's actually changed. Tools/TestWebKitAPI/glib/WebKitGLib/WebViewTest.cpp would be a good place. > Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:3141 > + * Returns: %TRUE% if @web_view audio is muted or %FALSE% is audio is not muted. % goes only before, not after, TRUE and FALSE > Source/WebKit/UIProcess/WebPageProxy.h:1263 > + bool getMuted(); Should be const, and code style is to avoid "get": bool isMuted() const;
Comment on attachment 387812 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=387812&action=review > Source/WebKit/UIProcess/API/gtk/WebKitWebView.h:358 > +webkit_web_view_get_audio_muted (WebKitWebView *webView); Probably we also want a “WebKitWebView.audio-muted” GObject property, the same way we already have a “WebKitWebView.is-playing-audio” one. Using a property can be useful e.g. to easily bind a toggle button to it in the UI without needing to write additional code. Also it would allow to watch the property by connecting to the “notify::audio-muted” signal.
Created attachment 399004 [details] Patch
I've updated the patch, added the requested property and added an example implementation for MiniBrowser.
Comment on attachment 399004 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=399004&action=review Let's say I have muted the audio for the web view using webkit_web_view_set_audio_muted(). That changes a property of the view, so it should stay muted if the view swaps to a new page. Now the user loads a new webpage from a different origin. The view's WebPageProxy stays the same, but the WebCore::Page backing it in the web process is swapped. The new page is probably not muted until we mute it by calling getPage(webView).setMuted. So we probably need to do that manually when the process swaps... right? Can you test this please? I bet the muted state gets lost if we don't have any code to persist it. I see that *somehow* webkitWebViewIsPlayingAudioChanged is being called during process swaps, because the volume indicator on my Epiphany tabs resets properly. But this case is different because here we have to persist the muted state across multiple underlying pages, rather than just indicate what the state is. Other than that concern, the code LGTM. This also needs approved by a second GTK reviewer since it adds new API, and also an Apple owner since it touches cross-platform WebKit layer. > Source/WebCore/ChangeLog:3 > + Add isPageMuteDidChange to inform about audio mute changes. Just keep the title of the bug report here. > Source/WebCore/page/Page.cpp:1852 > forEachDocument([] (Document& document) { > document.pageMutedStateDidChange(); > }); > + > + chrome().client().isPageMutedDidChange(); > } I wonder what other code might call Page::setMuted? Is it something that will only be used by the web view level? If so, then this is fine. (If not, the API won't work be "sticky" as expected... we don't want anything besides webkit_web_view_set_audio_muted() to override the muted state.)
> Let's say I have muted the audio for the web view using > webkit_web_view_set_audio_muted(). That changes a property of the view, so > it should stay muted if the view swaps to a new page. Now the user loads a > new webpage from a different origin. The view's WebPageProxy stays the same, > but the WebCore::Page backing it in the web process is swapped. The new page > is probably not muted until we mute it by calling getPage(webView).setMuted. > So we probably need to do that manually when the process swaps... right? Can > you test this please? I bet the muted state gets lost if we don't have any > code to persist it. Tested youtube video and muted the page then switched over to a complete new origin and played a blog entry. Mute state is preserved. > > Source/WebCore/ChangeLog:3 > > + Add isPageMuteDidChange to inform about audio mute changes. > > Just keep the title of the bug report here. OK, will change it. > > Source/WebCore/page/Page.cpp:1852 > > forEachDocument([] (Document& document) { > > document.pageMutedStateDidChange(); > > }); > > + > > + chrome().client().isPageMutedDidChange(); > > } > > I wonder what other code might call Page::setMuted? Is it something that > will only be used by the web view level? If so, then this is fine. (If not, > the API won't work be "sticky" as expected... we don't want anything besides > webkit_web_view_set_audio_muted() to override the muted state.) Haven't had a closer look at the internal calling as i took is-playing-audio as a base and added audio mute analog.
Created attachment 399110 [details] Patch
I've updated the patch: - Simplified code (no longer touches WebCore components) - Fixed new symbol API version - Integrated your findings
Comment on attachment 399110 [details] Patch Can you add tests as well please? In Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitWebView.cpp for instance.
I tried it, but the tests are not compiling for me (without my test) -.-
What is the compilation error?
It's during linking: *snip* Tools/TestWebKitAPI/CMakeFiles/TestWebKitAPIInjectedBundle.dir/InjectedBundleController.cpp.o:InjectedBundleController.cpp:function TestWebKitAPI::InjectedBundleController::initialize(OpaqueWKBundle const*, void const*): error: undefined reference to 'WKBundleSetClient' Tools/TestWebKitAPI/CMakeFiles/TestWebKitAPIInjectedBundle.dir/InjectedBundleController.cpp.o:InjectedBundleController.cpp:function TestWebKitAPI::InjectedBundleController::initialize(OpaqueWKBundle const*, void const*): error: undefined reference to 'WKStringCreateWithUTF8CString' Tools/TestWebKitAPI/CMakeFiles/TestWebKitAPIInjectedBundle.dir/InjectedBundleController.cpp.o:InjectedBundleController.cpp:function TestWebKitAPI::InjectedBundleController::initialize(OpaqueWKBundle const*, void const*): error: undefined reference to 'WKDictionaryGetItemForKey' *snip*
Comment on attachment 399110 [details] Patch The major change here is that the GObject property was removed (probably a good idea, if Epiphany doesn't need it). I think the API tests are probably not intended to build unless using the update-webkitgtk-libs/build-webkit scripts. I'll add that to my TODO list.
(In reply to Michael Catanzaro from comment #18) > I think the API tests are probably not intended to build unless using the > update-webkitgtk-libs/build-webkit scripts. I'll add that to my TODO list. (To my long-term TODO. In the meantime, try using update-webkitgtk-libs and then build-webkit --gtk.)
So i've rebuild everything with the script, everything compiles and links. But i still cannot execute my test: neither executing it directly due to libraries not found nor with Tools/Scripts/run-gtk-tests (python2 vs python3). File "Tools/Scripts/run-gtk-tests", line 112 print "Could not start accessibility bus, so disabling TestWebKitAccessibility" ^ SyntaxError: Missing parentheses in call to 'print'. Did you mean print("Could not start accessibility bus, so disabling TestWebKitAccessibility")?
OK, obviously nobody has attempted to use the run-gtk-tests script in a long time, at least not since /usr/bin/python became python3 in Fedora. It deserves a separate bug report. Probably it's not reasonable to expect you to add a test when the test script is clearly not being maintained. At the same time, we do require a test to add the new API. So we are stuck. :) Maybe try running 2to3 on the test script and see if that's enough to make it work?
(In reply to Michael Catanzaro from comment #21) > OK, obviously nobody has attempted to use the run-gtk-tests script in a long > time, at least not since /usr/bin/python became python3 in Fedora. It > deserves a separate bug report. > > Probably it's not reasonable to expect you to add a test when the test > script is clearly not being maintained. At the same time, we do require a > test to add the new API. So we are stuck. :) Maybe try running 2to3 on the > test script and see if that's enough to make it work? Michael, please try to remember not everybody here is a Fedora user. Much less the bots.
Tried to update the script to python3, but the dependency needs to be fixed as well. Then i tried to set python2 as python in my local path, but due to missing dependencies which i cannot install at the moment it does not work. Am i supposed to fix webkit toolchain just to add a small contribution?
WebKit's migration to Python3 is not complete yet... Meanwhile perhaps you can setup a python2 pipenv or virtualenv.
Thanks for your help, i'll give it a try.
At this point, I would sooner spend effort trying to fix the linker error, but your choice....
Python2 attempt on Fedora 32 results in: Tools/Scripts/run-gtk-tests Traceback (most recent call last): File "/app/webkit/Tools/Scripts/run-gtk-tests", line 25, in <module> from gi.repository import Gio, GLib File "/home/jbrummer/.local/lib/python2.7/site-packages/gi/__init__.py", line 42, in <module> from . import _gi ImportError: /home/jbrummer/.local/lib/python2.7/site-packages/gi/_gi.so: undefined symbol: PyUnicodeUCS4_AsUTF8String So it will get more complicated. I could give it another try with standard build flow that i'm used to as Michael suggested. But in case this will not success: could someone take over and add the small test (i could provide the test sources)...
I can take a look tomorrow (West Europe daytime).
(In reply to Michael Catanzaro from comment #26) > At this point, I would sooner spend effort trying to fix the linker error, > but your choice.... We determined this was caused by using -DENABLE_API_TESTS=ON without -DENABLE_DEVELOPER_MODE=ON. This should probably be fixed, but that's probably too much adventure for this bug.
Created attachment 399162 [details] Patch
Comment on attachment 399162 [details] Patch Test LGTM.
Comment on attachment 399162 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=399162&action=review > Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:3198 > +void webkit_web_view_set_audio_muted(WebKitWebView* webView, gboolean muted) This is weird, because is-audio-muted is a read-only property. I know this is not set_is_audio_muted, but it's still confusing. > Source/WebKit/UIProcess/WebPageProxy.cpp:5982 > +bool WebPageProxy::isAudioMuted() const > +{ > + return (m_mutedState & MediaProducer::AudioIsMuted); > +} This could be in the header > Source/WebKit/WebProcess/WebPage/WebPage.cpp:5082 > + send(Messages::WebPageProxy::IsPageMutedDidChange()); Do we really need to get notified back by the web process, I thought this was because setting muted might fail, but since this is called unconditionally, and the UI process is getting the value already set there, I don't think we need this.
Thanks for your review, so what's your recommendation?
> > Source/WebKit/WebProcess/WebPage/WebPage.cpp:5082 > > + send(Messages::WebPageProxy::IsPageMutedDidChange()); > > Do we really need to get notified back by the web process, I thought this > was because setting muted might fail, but since this is called > unconditionally, and the UI process is getting the value already set there, > I don't think we need this. At least this way we know the initial value for a page or in case the page mute is changed somewhere else.
(In reply to Jan-Michael Brummer from comment #33) > Thanks for your review, so what's your recommendation? I guess I would make the property readwrite
(In reply to Jan-Michael Brummer from comment #34) > > > Source/WebKit/WebProcess/WebPage/WebPage.cpp:5082 > > > + send(Messages::WebPageProxy::IsPageMutedDidChange()); > > > > Do we really need to get notified back by the web process, I thought this > > was because setting muted might fail, but since this is called > > unconditionally, and the UI process is getting the value already set there, > > I don't think we need this. > At least this way we know the initial value for a page or in case the page > mute is changed somewhere else. I don't understand. The mute value is always set by the UI process, either calling setMuted or as initial parameter, so the initial value is the one we have in the WebPageProxy. Or did I miss anything? I don't know what you mean by changed somewhere else. WebPage::setMuted() is only called by the message handler when the IPC message is received and with the initial value. In both cases it's the value just set by the Ui process.
(In reply to Carlos Garcia Campos from comment #36) > (In reply to Jan-Michael Brummer from comment #34) > > > > Source/WebKit/WebProcess/WebPage/WebPage.cpp:5082 > > > > + send(Messages::WebPageProxy::IsPageMutedDidChange()); > > > > > > Do we really need to get notified back by the web process, I thought this > > > was because setting muted might fail, but since this is called > > > unconditionally, and the UI process is getting the value already set there, > > > I don't think we need this. > > At least this way we know the initial value for a page or in case the page > > mute is changed somewhere else. > > I don't understand. The mute value is always set by the UI process, either > calling setMuted or as initial parameter, so the initial value is the one we > have in the WebPageProxy. Or did I miss anything? I don't know what you mean > by changed somewhere else. WebPage::setMuted() is only called by the message > handler when the IPC message is received and with the initial value. In both > cases it's the value just set by the Ui process. You are right, I've mixed it up: It is only set by setMuted and the initial value. So what exactly are you proposing here? It's my second contribution here and i'm not yet 100% familiar with the whole code.
Do you want me to remove the changes in WebPage.cpp and just set the property in the webkit_web_view_set_audio_muted (assuming that the initial value is FALSE)?
(In reply to Jan-Michael Brummer from comment #37) > (In reply to Carlos Garcia Campos from comment #36) > > (In reply to Jan-Michael Brummer from comment #34) > > > > > Source/WebKit/WebProcess/WebPage/WebPage.cpp:5082 > > > > > + send(Messages::WebPageProxy::IsPageMutedDidChange()); > > > > > > > > Do we really need to get notified back by the web process, I thought this > > > > was because setting muted might fail, but since this is called > > > > unconditionally, and the UI process is getting the value already set there, > > > > I don't think we need this. > > > At least this way we know the initial value for a page or in case the page > > > mute is changed somewhere else. > > > > I don't understand. The mute value is always set by the UI process, either > > calling setMuted or as initial parameter, so the initial value is the one we > > have in the WebPageProxy. Or did I miss anything? I don't know what you mean > > by changed somewhere else. WebPage::setMuted() is only called by the message > > handler when the IPC message is received and with the initial value. In both > > cases it's the value just set by the Ui process. > You are right, I've mixed it up: It is only set by setMuted and the initial > value. So what exactly are you proposing here? We can just notify the property when we send the message to the web process. > It's my second contribution > here and i'm not yet 100% familiar with the whole code. Sure, feel free to ask any question here or in IRC. It's normal for first contributions to iterate several times on reviews.
(In reply to Jan-Michael Brummer from comment #38) > Do you want me to remove the changes in WebPage.cpp and just set the > property in the webkit_web_view_set_audio_muted (assuming that the initial > value is FALSE)? The initial value is FALSE. MediaProducer::MutedStateFlags m_mutedState { WebCore::MediaProducer::NoneMuted } The initial value sent to the web process will be that one unless the app calls setMuted() before the web process is spawned.
Created attachment 399262 [details] Patch
I've updated the patch according to your review comments. Time for another review?
Comment on attachment 399262 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=399262&action=review > Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:1289 > + WEBKIT_PARAM_READABLE)); I would make this read write since we have a wayt to get and set the muted value. > Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:3187 > +static void webkitWebViewIsAudioMutedChanged(WebKitWebView* webView) > +{ > + g_object_notify(G_OBJECT(webView), "is-audio-muted"); > +} I don't think we need a function for one line that is only called in one place. > Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:3190 > + * webkit_web_view_set_audio_muted: If we make the property read write this should be set_is_audio_muted > Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:3203 > + getPage(webView).setMuted(muted ? WebCore::MediaProducer::AudioIsMuted : WebCore::MediaProducer::NoneMuted); > + webkitWebViewIsAudioMutedChanged(webView); You should check the value has actually changed before emitting notify. > Source/WebKit/UIProcess/WebPageProxy.cpp:5982 > +bool WebPageProxy::isAudioMuted() const > +{ > + return (m_mutedState & MediaProducer::AudioIsMuted); > +} This could be in the header. The parenthesis aren't needed. > Tools/MiniBrowser/gtk/BrowserTab.c:365 > +static void audioClicked(GtkButton *button, gpointer user_data) user_data -> userData > Tools/MiniBrowser/gtk/BrowserTab.c:373 > +static void audioMutedChanged(WebKitWebView *webView, GParamSpec *pspec, gpointer user_data) Ditto. > Tools/MiniBrowser/gtk/BrowserTab.c:379 > + gboolean muted; > + > + muted = webkit_web_view_is_audio_muted(tab->webView); gboolean muted = webkit_web_view_is_audio_muted(tab->webView); > Tools/MiniBrowser/gtk/BrowserTab.c:383 > + image = gtk_image_new_from_icon_name("audio-volume-high-symbolic", GTK_ICON_SIZE_MENU); > + else > + image = gtk_image_new_from_icon_name("audio-volume-muted-symbolic", GTK_ICON_SIZE_MENU); Instead of repeating this I would GtkWidget *image = gtk_image_new_from_icon_name(muted ? "audio-volume-muted-symbolic" : "audio-volume-high-symbolic", GTK_ICON_SIZE_MENU);
Created attachment 399458 [details] Patch
Thanks for the quick review, I've integrated changes for all your findings/recommendations.
Comment on attachment 399458 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=399458&action=review > Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:1279 > + * This property becomes %TRUE as soon as web content is muted. Otherwise What do you mean by web content here? This doesn't change when you mute the audio using the media settings, for example, or any js api. In that case, the is-playing-audio property changes. And the other way around, setting this property to TRUE doesn't change the is-playing-audio property nor the media controls. I find this a it confusing.
Please propose a better description so that there is no need not be changed again (due to project or language barrier)
(In reply to Jan-Michael Brummer from comment #47) > Please propose a better description so that there is no need not be changed > again (due to project or language barrier) I'm not sure to be honest. Maybe instead of a property that you think you might monitor for changes in the page mute we could just add webkit_web_view_mute() and webkit_web_view_is_muted(), documenting that that we are making the view muted, independently of the mute state of the media elements. That way it's clear it's a user API only action, not something that can be changed by the web content. And that it doesn't affect the actual web contents either (media elements are not actually muted). What do you think?
(In reply to Carlos Garcia Campos from comment #48) > I'm not sure to be honest. Maybe instead of a property that you think you > might monitor for changes in the page mute we could just add > webkit_web_view_mute() and webkit_web_view_is_muted() Well the property is probably important because it allows using GBinding to sync UI to the state of the property. > documenting that that > we are making the view muted, independently of the mute state of the media > elements. That way it's clear it's a user API only action, not something > that can be changed by the web content. And that it doesn't affect the > actual web contents either (media elements are not actually muted). What do > you think? Of course, we should document this. I guess we could also rename from is-audio-muted to is-muted if you think that's more clear.
(In reply to Michael Catanzaro from comment #49) > (In reply to Carlos Garcia Campos from comment #48) > > I'm not sure to be honest. Maybe instead of a property that you think you > > might monitor for changes in the page mute we could just add > > webkit_web_view_mute() and webkit_web_view_is_muted() > > Well the property is probably important because it allows using GBinding to > sync UI to the state of the property. > > > documenting that that > > we are making the view muted, independently of the mute state of the media > > elements. That way it's clear it's a user API only action, not something > > that can be changed by the web content. And that it doesn't affect the > > actual web contents either (media elements are not actually muted). What do > > you think? > > Of course, we should document this. I guess we could also rename from > is-audio-muted to is-muted if you think that's more clear. Yes, I think so. We should document that when media elements in a page are muted, is-playing-audio becomes FALSE and that changing is-muted doesn't affect the media element states, and is-playing-audio would still be TRUE even after setting is-muted to TRUE.
Just a quick note: The reason for using "is-audio-muted" is that you can also mute a microphone. So adding a "is-microphone-muted" is easier and consistent.
(In reply to Jan-Michael Brummer from comment #51) > Just a quick note: The reason for using "is-audio-muted" is that you can > also mute a microphone. So adding a "is-microphone-muted" is easier and > consistent. …but not all audio inputs are microphones. You could be recording from something else, let's say a guitar amp, and capture audio from it to use in a web application, let's say a WebAudio-based guitar effects pedal. So “is-microphone-muted“ is a bad name, maybe “audio-capture-muted“ (or similar) would be better.
Thats the reason for my quotation marks, but you've got the point. So is a renaming necessary or is the current one ok?
Comment on attachment 399458 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=399458&action=review > Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:1276 > + * WebKitWebView:is-audio-muted: So we agreed on renaming from WebKitWebView:is-audio-muted to WebKitWebView:is-muted > Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:1280 > + * Whether the #WebKitWebView audio is muted from a page. > + * This property becomes %TRUE as soon as web content is muted. Otherwise > + * the property is set back to %FALSE. Well Carlos doesn't like the current description because it is just not true. This say the property changes when web content is muted, but it is unrelated to the web content state, it's a view-level mute. That is, the property will not actually become %TRUE when the web content is muted. How about: "Whether the #WebKitWebView audio is muted. When TRUE, web content will not be able to play audio." What is its interaction with is-playing-audio? Is it possible for both is-audio-muted and is-playing-audio to be TRUE at the same time?
Created attachment 400258 [details] Patch
After checkback with Michael I've renamed everything from is-audio-muted to is-muted and updated the property description to: * Whether the #WebKitWebView audio is muted. When TRUE, audio is silenced. * It may still be playing, i.e. WebKitWebView:is-playing-audio may be TRUE.
Comment on attachment 400258 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=400258&action=review New version looks good, r=me for the first of two GTK reviewer approvals. > Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:1279 > + * It may still be playing, i.e. WebKitWebView:is-playing-audio may be TRUE. %TRUE > Tools/ChangeLog:26 > +2020-05-26 Jan-Michael Brummer <jan.brummer@tabos.org> > + > + [GTK][WPE] API for WebView audio mute support > + https://bugs.webkit.org/show_bug.cgi?id=176119 > + > + Reviewed by NOBODY (OOPS!). > + > + * MiniBrowser/gtk/BrowserTab.c: > + (audioClicked): > + (audioMutedChanged): > + (browserTabConstructed): > + * TestWebKitAPI/Tests/WebKitGLib/TestWebKitWebView.cpp: > + (testWebViewIsAudioMuted): > + (beforeAll): > + > +2020-05-12 Jan-Michael Brummer <jan.brummer@tabos.org> > + > + Add mute button to notebook title. > + https://bugs.webkit.org/show_bug.cgi?id=176119 > + > + Reviewed by NOBODY (OOPS!). > + > + * MiniBrowser/gtk/BrowserTab.c: > + (audioClicked): > + (audioMutedChanged): > + (browserTabConstructed): Careful not to duplicate the ChangeLog.
Created attachment 400268 [details] Patch
Thanks Michael. Fixed TRUEs and ChangeLog.
Comment on attachment 400268 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=400268&action=review > Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:1279 > + * It may still be playing, i.e. WebKitWebView:is-playing-audio may be %TRUE. #WebKitWebView:is-playing-audio > Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:3207 > + * webkit_web_view_is_muted: Since this is a getter property. it should be webkit_web_view_get_is_muted
Created attachment 400597 [details] Patch
Carlos: Fixed your last two findings.
Committed r262321: <https://trac.webkit.org/changeset/262321> All reviewed patches have been landed. Closing bug and clearing flags on attachment 400597 [details].