Bug 214188 – Add Gamepad tests that exercise the native frameworks

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Brady Eidson]

Add Gamepad tests that exercise the native frameworks

We need to directly drive GCF (and probably IOHID) framework functionality to test gamepads.

[Radar WebKit Bug Importer]

<rdar://problem/65343674>

[Brady Eidson]

Created attachment 404972 [details]
Patch

[Brady Eidson]

Created attachment 404976 [details]
Patch

[Brady Eidson]

Created attachment 404979 [details]
Patch

[Brady Eidson]

Created attachment 404987 [details]
Patch

[Brady Eidson]

Created attachment 404991 [details]
Patch

[Tim Horton]

Comment on attachment 404991 [details]
Patch

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

> Source/WebKit/UIProcess/Gamepad/UIGamepadProvider.h:60
> +    size_t numberOfConnectedGamepads() { return m_gamepads.size(); }

`const`

> Tools/TestWebKitAPI/mac/VirtualGamepad.mm:29
> +#if PLATFORM(MAC) && USE(APPLE_INTERNAL_SDK)

internal-SDK-only tests are quite unusual. is there any reason a combination of SPI headers + stuff is insufficient?

[Brady Eidson]

Created attachment 405007 [details]
Patch

[Brady Eidson]

Created attachment 405008 [details]
Patch

[Brady Eidson]

Created attachment 405012 [details]
Patch

[Brady Eidson]

(In reply to Tim Horton from comment #7)
> Comment on attachment 404991 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=404991&action=review
> 
> > Source/WebKit/UIProcess/Gamepad/UIGamepadProvider.h:60
> > +    size_t numberOfConnectedGamepads() { return m_gamepads.size(); }
> 
> `const`
> 
> > Tools/TestWebKitAPI/mac/VirtualGamepad.mm:29
> > +#if PLATFORM(MAC) && USE(APPLE_INTERNAL_SDK)
> 
> internal-SDK-only tests are quite unusual. is there any reason a combination
> of SPI headers + stuff is insufficient?

ChangeLog covers it - Need an entitlement we can currently only sign in internal builds.

[Tim Horton]

Comment on attachment 405012 [details]
Patch

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

> Tools/TestWebKitAPI/Tests/mac/HIDGamepads.mm:98
> +    [[webView window] resignKeyWindow];
> +    [[webView window] makeKeyWindow];

This could use a comment (also I think it's technically working around a bug? but I'm not sure what component the bug is in?)

> Tools/TestWebKitAPI/mac/VirtualGamepad.h:35
> +@class NSString;
> +@class HIDDevice;
> +@class HIDUserDevice;

Sort it! Also, any chance of this header ending up in C++ code? Maybe OBJC_CLASS? (the rest of it seems C++-clean at a glance).

> Tools/TestWebKitAPI/mac/VirtualGamepad.mm:45
> +const uint8_t NimbusDescriptor[] = {

This all hurts a bit. Are there not constants for this? Also, I think everything inside the {} should be indented.

> Tools/TestWebKitAPI/mac/VirtualGamepad.mm:130
> +    m_dispatchQueue = adoptNS(dispatch_queue_create(0, DISPATCH_QUEUE_SERIAL));

I think this should be OSObjectPtr/adoptOSObject? But not 100%, and it may not matter.

> Tools/TestWebKitAPI/mac/VirtualGamepad.mm:133
> +    auto uuid = adoptNS([[NSUUID alloc] init]);
> +    m_uniqueID = [uuid UUIDString];

I think NSUUID.UUID.UUIDString instead of these two

[Brady Eidson]

(In reply to Tim Horton from comment #12)
> Comment on attachment 405012 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=405012&action=review
> 
> > Tools/TestWebKitAPI/Tests/mac/HIDGamepads.mm:98
> > +    [[webView window] resignKeyWindow];
> > +    [[webView window] makeKeyWindow];
> 
> This could use a comment (also I think it's technically working around a
> bug? but I'm not sure what component the bug is in?)

It's not a bug, but rather a really intentional quirky behavior... comment added.

> > Tools/TestWebKitAPI/mac/VirtualGamepad.h:35
> > +@class NSString;
> > +@class HIDDevice;
> > +@class HIDUserDevice;
> 
> Sort it! Also, any chance of this header ending up in C++ code? Maybe
> OBJC_CLASS? (the rest of it seems C++-clean at a glance).

It probably won't ever be in non-objc code, but I changed it anyways.


> > Tools/TestWebKitAPI/mac/VirtualGamepad.mm:45
> > +const uint8_t NimbusDescriptor[] = {
> 
> This all hurts a bit. Are there not constants for this? Also, I think
> everything inside the {} should be indented.

This (and future descriptors that I'll be adding for testing) are actual raw dumps from devices. Normally you literally just see an unformatted string of hex digits.

This formatting is how HID descriptors are generally denoted as a term of art - In the USB HID spec and whatever tooling you might find to help work with them.

I'll actually add a link to the web-based tool I used to turn the raw descriptor into this "human readable" formatting"

As for the bytes themselves, there certainly are not constants for them. Some are "common and constant" but many are actually bitfield options put together.

Think of the bytes as the machine code, and the "human readable" descriptions as the assembly code.


> > Tools/TestWebKitAPI/mac/VirtualGamepad.mm:130
> > +    m_dispatchQueue = adoptNS(dispatch_queue_create(0, DISPATCH_QUEUE_SERIAL));
> 
> I think this should be OSObjectPtr/adoptOSObject? But not 100%, and it may
> not matter.

Never (that I remember) used those, will take a look!

> > Tools/TestWebKitAPI/mac/VirtualGamepad.mm:133
> > +    auto uuid = adoptNS([[NSUUID alloc] init]);
> > +    m_uniqueID = [uuid UUIDString];
> 
> I think NSUUID.UUID.UUIDString instead of these two

Indeed.

[Brady Eidson]

Created attachment 405018 [details]
PFL

[Darin Adler]

Comment on attachment 405012 [details]
Patch

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

>>> Tools/TestWebKitAPI/mac/VirtualGamepad.mm:130
>>> +    m_dispatchQueue = adoptNS(dispatch_queue_create(0, DISPATCH_QUEUE_SERIAL));
>> 
>> I think this should be OSObjectPtr/adoptOSObject? But not 100%, and it may not matter.
> 
> Never (that I remember) used those, will take a look!

I wonder if we can make this subtle mistake a compile-time error.

[EWS]

Committed r264769: <https://trac.webkit.org/changeset/264769>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 405018 [details].