Implement 'gamepad' Permissions Policy

[Marcos Caceres]

Gamepad defines has a "gamepad" permission, with an allow list of 'self'.

[Radar WebKit Bug Importer]

<rdar://problem/83219098>

[Marcos Caceres]

We are changing it to "all" instead, as "self" turned out not to be web compatible. 

https://github.com/w3c/gamepad/pull/156

[Marcos Caceres]

Created attachment 444320 [details]
Patch

[Marcos Caceres]

Created attachment 444468 [details]
Patch

[EWS Watchlist]

This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess

[Marcos Caceres]

Created attachment 444478 [details]
Patch

[Darin Adler]

Comment on attachment 444478 [details]
Patch

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

Feels like the test coverage is a bit light.

> Source/WebCore/Modules/gamepad/Navigator+Gamepad.idl:34
> +    [
> +      EnabledBySetting=GamepadsEnabled,
> +      CallWith=Document
> +    ] sequence<Gamepad> getGamepads();

I don’t think we have to break this out vertically just to fit one more attribute in. Unless this is copying and pasting directly from a specification or something.

    [EnabledBySetting=GamepadsEnabled, CallWith=Document] sequence<Gamepad> getGamepads();

> Source/WebCore/html/FeaturePolicy.cpp:53
> +        return "Gampad";

Typo: "Gamepad".

> Source/WebCore/html/FeaturePolicy.cpp:215
> +        if (item.startsWith("gamepad")) {

Not new, but why all the startsWith in this function? Is it valid to write "gamepadnotreallyimeantsomethingelse"?

> LayoutTests/http/tests/gamepad/gamepad-allow-attribute.https-expected.txt:4
> +CONSOLE MESSAGE: Feature policy 'Gampad' check failed for iframe with origin 'https://localhost:8443' and allow attribute 'gamepad 'none''.
> +CONSOLE MESSAGE: Feature policy 'Gampad' check failed for iframe with origin 'https://127.0.0.1:8443' and allow attribute 'gamepad 'none''.
> +CONSOLE MESSAGE: Feature policy 'Gampad' check failed for iframe with origin 'https://localhost:8443' and allow attribute 'gamepad 'self''.
> +CONSOLE MESSAGE: Feature policy 'Gampad' check failed for iframe with origin 'https://127.0.0.1:8443' and allow attribute 'gamepad https://localhost:8443'.

Here we have "Gampad".

[Marcos Caceres]

> Typo: "Gamepad".

Programming for 20+ years... never gets better 🤪

> Not new, but why all the startsWith in this function? Is it valid to write "gamepadnotreallyimeantsomethingelse"?

Yeah, the Permissions Policy parser there is fairly simple... it just does a split on ";", trims, and then checks the "startWith". I agree that it should probably be a little bit more robust. I think the updated Permission Policy spec has further parsing requirements, so eventually the parser might need to be rewritten.

[Marcos Caceres]

Created attachment 444603 [details]
Patch

[Marcos Caceres]

> Feels like the test coverage is a bit light.

I'm open to suggestion to what else to test. 

We might want to eventually consolidate+improve all the Permission Policy tests, as they all basically test the same way as I did here (I just copy-pasted-modified an existing test in the tree).

[Darin Adler]

Comment on attachment 444603 [details]
Patch

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

> Source/WebCore/Modules/gamepad/NavigatorGamepad.h:30
> +#include "ExceptionOr.h"

It’s fine to include ExceptionOr.h and we have been doing that in other headers. But I’ve been wondering lately if we could get away with forward declarations instead in a case like this one.

    template<typename> class ExceptionOr;

That typically works for other return values in other similar cases, but for some reason we haven’t done that for ExecptionOr.

[Marcos Caceres]

>     template<typename> class ExceptionOr;

Ah, good idea! will add.

[Marcos Caceres]

Created attachment 444622 [details]
Patch

[Marcos Caceres]

@Darin, just kindly checking if you could finish the review or if I should ping someone else? Really appreciate all the feedback thus far.

[youenn fablet]

Comment on attachment 444622 [details]
Patch

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

> Source/WebCore/Modules/gamepad/NavigatorGamepad.cpp:84
> +    if (!isFeaturePolicyAllowedByDocumentAndAllOwners(FeaturePolicy::Type::Gamepad, document, LogFeaturePolicyFailure::Yes))

I am not sure this is correct to get the document from the execution state.
It seems to me getting it from the navigator is better.
This could be tested with the edge case of a document having a same origin iframe, the iframe being denied gamepad permission, then calling iframe.contentWindow...
Can you check this case?

> LayoutTests/imported/w3c/ChangeLog:9
> +        * web-platform-tests/not-fully-active.https.html: Added.

This does not seem to be the right folder.
This should probably be in web-platform-tests/gamepad folder.
Looking at wpt, I can see tests that would seem useful, in particular gamepad-supported-by-feature-policy.html, which might be a bit redundant with http/tests/gamepad/gamepad-allow-attribute.https.html.
Can we import wpt/gampepad test, either in this patch or as a preliminary patch?
You can use Tools/Scripts/import-w3c-tests -t web-platform-tests/gamepad to do the import.

[Darin Adler]

My single comment didn’t imply that I was going to review this.

[Darin Adler]

Well, no that’s not right. I could review it. But let’s deal with Youenn’s comments first.

[Marcos Caceres]

Thanks for the additional feedback and comments. I'm traveling this week, but hope to address the feedback next week.