[webkit-reviews] review denied: [Bug 176119] [GTK][WPE] API for WebView audio mute support : [Attachment 399262] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu May 14 22:37:21 PDT 2020
Carlos Garcia Campos <cgarcia at igalia.com> has denied Jan-Michael Brummer
<jan.brummer at tabos.org>'s request for review:
Bug 176119: [GTK][WPE] API for WebView audio mute support
https://bugs.webkit.org/show_bug.cgi?id=176119
Attachment 399262: Patch
https://bugs.webkit.org/attachment.cgi?id=399262&action=review
--- Comment #43 from Carlos Garcia Campos <cgarcia at igalia.com> ---
Comment on attachment 399262
--> https://bugs.webkit.org/attachment.cgi?id=399262
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);
More information about the webkit-reviews
mailing list