[webkit-reviews] review denied: [Bug 184845] [GTK] Disable video autoplay : [Attachment 401730] Patch

[bugzilla-daemon at webkit.org]

Carlos Garcia Campos <cgarcia at igalia.com> has denied 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 401730: Patch

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




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

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

Let's start with this API, then we will see if we really need the function to
update policies for existing web views. I think we should try to implement the
feature in ephy to ensure the API is enough (I can help with that if needed).

> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:4678
> + * associated with the view.

Indent this line with 4 spaces.

> Source/WebKit/UIProcess/API/glib/WebKitWebsitePolicies.cpp:106
> +	  
policies->priv->websitePolicies->setAutoplayPolicy(WebKit::WebsiteAutoplayPolic
y::Allow);

No need for WebKit:: here

> Source/WebKit/UIProcess/API/glib/WebKitWebsitePolicies.cpp:109
> +	  
policies->priv->websitePolicies->setAutoplayPolicy(WebKit::WebsiteAutoplayPolic
y::AllowWithoutSound);

Ditto.

> Source/WebKit/UIProcess/API/glib/WebKitWebsitePolicies.cpp:112
> +	  
policies->priv->websitePolicies->setAutoplayPolicy(WebKit::WebsiteAutoplayPolic
y::Deny);

Ditto.

> Source/WebKit/UIProcess/API/glib/WebKitWebsitePolicies.cpp:115
> +    default:
> +	   ASSERT_NOT_REACHED();

Do not add default, since we are handling all the possible cases.

> Source/WebKit/UIProcess/API/glib/WebKitWebsitePolicies.cpp:127
> +    case PROP_AUTOPLAY_POLICY: {
> +	   webkitWebsitePoliciesSetAutoplayPolicy(policies,
static_cast<WebKitAutoplayPolicy>(g_value_get_enum(value)));
> +	   break;
> +    }

Brackets aren't needed here.

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

I'm not sure about changing the default behavior here. I guess it's ok, since
it doesn't require apps to handle any new api.

> Source/WebKit/UIProcess/API/glib/WebKitWebsitePolicies.cpp:161
> + *

Add some body here even if it's just Create a new #WebKitWebsitePolicies

> Source/WebKit/UIProcess/API/glib/WebKitWebsitePolicies.cpp:175
> + *

Ditto.

> Source/WebKit/UIProcess/API/glib/WebKitWebsitePolicies.cpp:220
> +    case WebKit::WebsiteAutoplayPolicy::Allow:

No need for WebKit::

> Source/WebKit/UIProcess/API/glib/WebKitWebsitePolicies.cpp:227
> +    case WebKit::WebsiteAutoplayPolicy::Default:
> +	   return WEBKIT_AUTOPLAY_ALLOW_WITHOUT_SOUND;

Is it really possible? This is a construct-only parameter so the setter is
always called on construction with the default value
WEBKIT_AUTOPLAY_ALLOW_WITHOUT_SOUND

> Source/WebKit/UIProcess/API/glib/WebKitWebsitePolicies.cpp:229
> +    default:
> +	   ASSERT_NOT_REACHED();

Do not add default when handling all possible values, we want the compiler to
complain if new values are added to the enum.

> Source/WebKit/UIProcess/API/gtk/WebKitWebsitePolicies.h:61
> + * @WEBKIT_AUTOPLAY_ALLOW_WITHOUT_SOUND: Allow videos to autoplay if
> + * they have no audio track, or if their audio track is muted.

I thought this allowed to autoplay all videos, but the all started muted, like
chrome does in android. 

Indent the second line with 4 spaces.

> Tools/MiniBrowser/gtk/main.c:574
> +    WebKitWebsitePolicies *defaultWebsitePolicies =
webkit_website_policies_new_with_policies(
> +	   "autoplay-policy", WEBKIT_AUTOPLAY_ALLOW_WITHOUT_SOUND,
> +	   NULL);

We don't need to do this as long as it's the default option already. I would a
command line option to set any autoplay-policy so that we can manually test
with MiniBrowser.

> Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitPolicyClient.cpp:117
> +	   return m_autoplayed && *m_autoplayed;

m_autoplayed.valueOr(False);

> Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitPolicyClient.cpp:320
> +    test->m_autoplayed = WTF::nullopt;
> +    test->m_policyDecisionResponse = PolicyClientTest::UseWithPolicy;
> +    test->m_respondToPolicyDecisionAsynchronously = true;
> +    test->loadURI(resourceURL.get());
> +    // Run until the user content messages come back from
autoplay-check.html
> +    g_main_loop_run(test->m_mainLoop);

What we normally do it adding a method to the test class, in this case
something like 

bool loadURIAndWaitForAutoPlayed(const char* uri)
{
    m_autoplayed = WTF::nullopt;
    m_policyDecisionResponse = PolicyClientTest::UseWithPolicy;
    loadURI(uri);
    g_main_loop_run(m_mainLoop);
    return m_autoplayed.valueOr(False);
}

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

adoptGRef()

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

adoptGRef()

> Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitPolicyClient.cpp:343
> +    // Now check dynamically updating the loader policies

This comment should be Test with WEBKIT_AUTOPLAY_DENY I guess.

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

adoptGRef()

> Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitPolicyClient.cpp:407
> +    PolicyClientTest::add("WebKitPolicyClient", "autoplay-policy",
testAutoplayPolicy);
> +    // WARNING: This test must come last, it uses racey constructs that
> +    // interfere nondeterminisically with any test running after it.

What other tests do fail if run after this one? We should try to avoid tests to
depend on each other

> Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitPolicyClient.cpp:409
>  }

We should also tests the default website policies in WebKitWebView API.