[Webkit-unassigned] [Bug 206033] Standard gamepad mapping for GameControllerGamepads

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=206033

Dean Jackson <dino at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |dino at apple.com
 Attachment #387406|review?                     |review-
              Flags|                            |

--- 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.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20200111/b1d8b39b/attachment-0001.htm>