Bug 176119 – [GTK][WPE] API for WebView audio mute support

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

[Hussam Al-Tayeb]

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.

[Michael Catanzaro]

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.

[Philippe Normand]

The infrastructure is already in place. You only need a new API in the WebView, that would call WebPage::setMuted().

[Michael Catanzaro]

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.

[Jan-Michael Brummer]

Created attachment 387812 [details]
Patch

[EWS Watchlist]

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

[Michael Catanzaro]

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;

[Adrian Perez]

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.

[Jan-Michael Brummer]

Created attachment 399004 [details]
Patch

[Jan-Michael Brummer]

I've updated the patch, added the requested property and added an example implementation for MiniBrowser.

[Michael Catanzaro]

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

[Jan-Michael Brummer]

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

[Jan-Michael Brummer]

Created attachment 399110 [details]
Patch

[Jan-Michael Brummer]

I've updated the patch: 
 - Simplified code (no longer touches WebCore components)
 - Fixed new symbol API version
 - Integrated your findings

[Philippe Normand]

Comment on attachment 399110 [details]
Patch

Can you add tests as well please? In Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitWebView.cpp for instance.

[Jan-Michael Brummer]

I tried it, but the tests are not compiling for me (without my test) -.-

[Philippe Normand]

What is the compilation error?

[Jan-Michael Brummer]

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*

[Michael Catanzaro]

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.

[Michael Catanzaro]

(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.)

[Jan-Michael Brummer]

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")?

[Michael Catanzaro]

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?

[Philippe Normand]

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

[Jan-Michael Brummer]

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?

[Philippe Normand]

WebKit's migration to Python3 is not complete yet... Meanwhile perhaps you can setup a python2 pipenv or virtualenv.

[Jan-Michael Brummer]

Thanks for your help, i'll give it a try.

[Michael Catanzaro]

At this point, I would sooner spend effort trying to fix the linker error, but your choice....

[Jan-Michael Brummer]

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

[Philippe Normand]

I can take a look tomorrow (West Europe daytime).

[Michael Catanzaro]

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

[Jan-Michael Brummer]

Created attachment 399162 [details]
Patch

[Michael Catanzaro]

Comment on attachment 399162 [details]
Patch

Test LGTM.

[Carlos Garcia Campos]

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.

[Jan-Michael Brummer]

Thanks for your review, so what's your recommendation?

[Jan-Michael Brummer]

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

[Carlos Garcia Campos]

(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

[Carlos Garcia Campos]

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

[Jan-Michael Brummer]

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

[Jan-Michael Brummer]

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)?

[Carlos Garcia Campos]

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

[Carlos Garcia Campos]

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

[Jan-Michael Brummer]

Created attachment 399262 [details]
Patch

[Jan-Michael Brummer]

I've updated the patch according to your review comments. Time for another review?

[Carlos Garcia Campos]

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);

[Jan-Michael Brummer]

Created attachment 399458 [details]
Patch

[Jan-Michael Brummer]

Thanks for the quick review, I've integrated changes for all your findings/recommendations.

[Carlos Garcia Campos]

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.

[Jan-Michael Brummer]

Please propose a better description so that there is no need not be changed again (due to project or language barrier)

[Carlos Garcia Campos]

(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?

[Michael Catanzaro]

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

[Carlos Garcia Campos]

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

[Jan-Michael Brummer]

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.

[Adrian Perez]

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

[Jan-Michael Brummer]

Thats the reason for my quotation marks, but you've got the point. So is a renaming necessary or is the current one ok?

[Michael Catanzaro]

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?

[Jan-Michael Brummer]

Created attachment 400258 [details]
Patch

[Jan-Michael Brummer]

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.

[Michael Catanzaro]

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
> [phone]  Jan-Michael Brummer  <[e-mail]>
> +
> +        [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):
> +
> [phone]  Jan-Michael Brummer  <[e-mail]>
> +
> +        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.

[Jan-Michael Brummer]

Created attachment 400268 [details]
Patch

[Jan-Michael Brummer]

Thanks Michael. Fixed TRUEs and ChangeLog.

[Carlos Garcia Campos]

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

[Jan-Michael Brummer]

Created attachment 400597 [details]
Patch

[Jan-Michael Brummer]

Carlos: Fixed your last two findings.

[EWS]

Committed r262321: <https://trac.webkit.org/changeset/262321>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 400597 [details].