[webkit-reviews] review granted: [Bug 184845] [GTK] Disable video autoplay : [Attachment 401891] Patch

[bugzilla-daemon at webkit.org]

Carlos Garcia Campos <cgarcia at igalia.com> has granted Charlie Turner
<cturner at igalia.com>'s request for review:
Bug 184845: [GTK] Disable video autoplay
https://bugs.webkit.org/show_bug.cgi?id=184845

Attachment 401891: Patch

https://bugs.webkit.org/attachment.cgi?id=401891&action=review




--- Comment #62 from Carlos Garcia Campos <cgarcia at igalia.com> ---
Comment on attachment 401891
  --> https://bugs.webkit.org/attachment.cgi?id=401891
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=401891&action=review

> Source/WebKit/UIProcess/API/glib/WebKitWebsitePolicies.cpp:71
> +    case WebKitAutoplayPolicy::WEBKIT_AUTOPLAY_ALLOW:

Didn't notice this before but you don't need the WebKitAutoplayPolicy:: since
it's not a C++ enum class.

> Source/WebKit/UIProcess/API/glib/WebKitWebsitePolicies.cpp:74
> +    case WebKitAutoplayPolicy::WEBKIT_AUTOPLAY_ALLOW_WITHOUT_SOUND:

Ditto.

> Source/WebKit/UIProcess/API/glib/WebKitWebsitePolicies.cpp:77
> +    case WebKitAutoplayPolicy::WEBKIT_AUTOPLAY_DENY:

Ditto.

> Source/WebKit/UIProcess/API/glib/WebKitWebsitePolicies.cpp:152
> +	       WEBKIT_AUTOPLAY_ALLOW_WITHOUT_SOUND,

Let's keep this as the default, yes.

> Source/WebKit/UIProcess/API/glib/WebKitWebsitePolicies.cpp:228
> +    case WebsiteAutoplayPolicy::Default:

I still don't see how Default can be set at this point.

> Source/WebKit/UIProcess/API/glib/WebKitWebsitePoliciesPrivate.h:26
> +API::WebsitePolicies*
webkitWebsitePoliciesGetWebsitePolicies(WebKitWebsitePolicies *);

WebKitWebsitePolicies * -> WebKitWebsitePolicies*

> Source/WebKit/UIProcess/API/glib/WebKitWebsitePoliciesPrivate.h:27
> +WebKit::WebsitePoliciesData
webkitWebsitePoliciesGetPoliciesData(WebKitWebsitePolicies *);

Ditto.

> Source/WebKit/UIProcess/API/gtk/WebKitWebsitePolicies.h:85
> +webkit_website_policies_get_autoplay_policy			
(WebKitWebsitePolicies* policies);

WebKitWebsitePolicies* -> WebKitWebsitePolicies *

> Source/WebKit/UIProcess/API/wpe/WebKitWebsitePolicies.h:85
> +webkit_website_policies_get_autoplay_policy			
(WebKitWebsitePolicies* policies);

Ditto.

> Tools/MiniBrowser/gtk/main.c:132
> +    { "autoplay-policy", 0, 0, G_OPTION_ARG_CALLBACK, parseAutoplayPolicy,
"Autoplay policy", NULL },

Add the possible options to the help string "Autoplay policy (allow,
allow-without-sound or deny)" for example

> Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitPolicyClient.cpp:118
> +	   m_websitePolicies =
adoptGRef(webkit_website_policies_new_with_policies("autoplay-policy", policy,
nullptr));

hmm, now that I see this used, I think autoplay-policy as a property of class
named WebKitWebsitePolicies is a bit redundant, what do you think about using
just autoplay?