Gamepad defines has a "gamepad" permission, with an allow list of 'self'.
<rdar://problem/83219098>
We are changing it to "all" instead, as "self" turned out not to be web compatible. https://github.com/w3c/gamepad/pull/156
Created attachment 444320 [details] Patch
Created attachment 444468 [details] Patch
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
Created attachment 444478 [details] Patch
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".
> 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.
Created attachment 444603 [details] Patch
> 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).
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.
> template<typename> class ExceptionOr; Ah, good idea! will add.
Created attachment 444622 [details] Patch
@Darin, just kindly checking if you could finish the review or if I should ping someone else? Really appreciate all the feedback thus far.
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.
My single comment didn’t imply that I was going to review this.
Well, no that’s not right. I could review it. But let’s deal with Youenn’s comments first.
Thanks for the additional feedback and comments. I'm traveling this week, but hope to address the feedback next week.
The permission policy was switched to "*".
Pull request: https://github.com/WebKit/WebKit/pull/21851
Committed 272199@main (82e7a7f64764): <https://commits.webkit.org/272199@main> Reviewed commits have been landed. Closing PR #21851 and removing active labels.