[Webkit-unassigned] [Bug 176119] [GTK][WPE] API for WebView audio mute support

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=176119

Carlos Garcia Campos <cgarcia at igalia.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #399262|review?                     |review-
              Flags|                            |

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

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20200515/312e275a/attachment.htm>