[webkit-reviews] review denied: [Bug 206033] Standard gamepad mapping for GameControllerGamepads : [Attachment 387406] Patch

[bugzilla-daemon at webkit.org]

Dean Jackson <dino at apple.com> has denied James Howard <jameshoward at mac.com>'s
request for review:
Bug 206033: Standard gamepad mapping for GameControllerGamepads
https://bugs.webkit.org/show_bug.cgi?id=206033

Attachment 387406: Patch

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




--- Comment #5 from Dean Jackson <dino at apple.com> ---
Comment on attachment 387406
  --> https://bugs.webkit.org/attachment.cgi?id=387406
Patch

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

Almost there. I'd like to see some comments in the change log, and rationale
for including the home button.

> Source/WebCore/ChangeLog:7
> +	   Standard gamepad mapping for GameControllerGamepads
> +	   https://bugs.webkit.org/show_bug.cgi?id=206033
> +
> +	   Reviewed by NOBODY (OOPS!).
> +

Please explain what you did in the patch here, and why.

For example, I see you swapped some axis mappings. Was that a bug?

Also, it seems you added the mapping field, but we only support the canonical
standard mapping. Do we plan to add labels for the types of controllers we do
support?

> Source/WebCore/platform/gamepad/cocoa/GameControllerGamepad.mm:36
> + at interface GCExtendedGamepad (ButtonHome)
> +
> + at property (readonly, nonatomic) GCControllerButtonInput *_buttonHome; //
Internal in macOS 10.15, iOS 13.0
> +
> + at end

I don't think we should expose this. It's currently reserved for system use. Is
there a good reason you want it?

> Source/WebCore/platform/gamepad/cocoa/GameControllerGamepad.mm:105
> +    if (@available(macOS 10.15.0, iOS 13.0, *)) {

We compile per-platform, so you'll have to use:
#if (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101500) ||
(PLATFORM(IOS_FAMILY) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 130000)

> Source/WebCore/platform/gamepad/cocoa/GameControllerGamepad.mm:111
> +    if (@available(macOS 10.14.1, iOS 12.1, *)) {

Ditto here? Did they really only get added in a .1 version? (checks notes....
they did but I think it will be ok to just use	__MAC_OS_X_VERSION_MIN_REQUIRED
>= 101400 since we compile with a newer SDK)

Note that you don't have to have the iOS clause because iOS 12 is the minimum
version we support at the moment.

> Source/WebCore/platform/gamepad/cocoa/GameControllerGamepad.mm:118
> +    // Home/Guide
> +    if ([m_extendedGamepad.get() respondsToSelector:@selector(_buttonHome)])
> +	   bindButton(m_extendedGamepad.get()._buttonHome, 16);

Noted above: I don't think we should expose this.