[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