Bug 214910 – Refactor HID gamepad code to be much less fragile and much easier to hack on

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Brady Eidson]

Refactor HID gamepad code to be much less fragile and much easier to hack on

This will pave the way for further improvements, such as trivially mapping specific controllers

[Brady Eidson]

(In reply to Brady Eidson from comment #0)
> Refactor HID gamepad code to be much less fragile and much easier to hack on
> 
> This will pave the way for further improvements, such as trivially mapping
> specific controllers

Such as over in https://bugs.webkit.org/show_bug.cgi?id=214911

[Brady Eidson]

Created attachment 405521 [details]
Patch

[Darin Adler]

Comment on attachment 405521 [details]
Patch

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

> Source/WebCore/platform/gamepad/SharedGamepadValue.h:36
> +class SharedGamepadValue {

The design of this class is a bit tricky. If you assign one to another then it redirects which shared value is being used. But if you assign a double to it, the shared value is overwritten. Both are done with the same "=" operator. I think there’s a reason to consider pointer-style boxing instead to distinguish those types of assuming.

> Source/WebCore/platform/gamepad/SharedGamepadValue.h:37
> +    WTF_MAKE_FAST_ALLOCATED;

This is needed on SharedGamepadValue::Data, which is heap allocated, and probably is not needed on this class, which won’t be.

> Source/WebCore/platform/gamepad/SharedGamepadValue.h:63
> +        Data()
> +            : value(0.0)
> +        {
> +        }
> +

No need for this. Could just put a 0 up above in the default constructor.

> Source/WebCore/platform/gamepad/mac/GenericHIDGamepad.cpp:41
> +    for (auto element : inputElements) {

Should use auto& so we don’t copy all the elements while iterating.

> Source/WebCore/platform/gamepad/mac/GenericHIDGamepad.cpp:71
> +        m_axisValues.append(SharedGamepadValue { });

I think just { } will do it without a class name. Or maybe you’d prefer 0.0?

> Source/WebCore/platform/gamepad/mac/GenericHIDGamepad.h:33
> +#include <wtf/FastMalloc.h>
> +#include <wtf/HexNumber.h>
> +#include <wtf/NeverDestroyed.h>

These should not be needed in the header.

> Source/WebCore/platform/gamepad/mac/GenericHIDGamepad.h:37
> +class GenericHIDGamepad : public HIDGamepad {

Can this class be final?

> Source/WebCore/platform/gamepad/mac/GenericHIDGamepad.h:41
> +protected:

Why protected instead of private? Is this a base class?

> Source/WebCore/platform/gamepad/mac/HIDGamepadElement.h:54
> +    HIDGamepadElement(const HIDElement&);

What is this used for?

> Source/WebCore/platform/gamepad/mac/HIDGamepadElement.h:60
> +class HIDGamepadButton : public HIDGamepadElement {

final?

> Source/WebCore/platform/gamepad/mac/HIDGamepadElement.h:62
> +    HIDGamepadButton(const HIDElement& element, SharedGamepadValue value)

Taking a SharedGamepadValue is OK, but intrinsically churn-y, like taking a Ref<>, as opposed to either a const Ref<>& or a Ref<>&&.

> Source/WebCore/platform/gamepad/mac/HIDGamepadElement.h:64
> +        , m_value(value)

WTFMove here would save some reference count churn.

> Source/WebCore/platform/gamepad/mac/HIDGamepadElement.h:70
> +protected:

Why protected? Is this a base class?

> Source/WebCore/platform/gamepad/mac/HIDGamepadElement.h:71
> +    HIDInputType gamepadValueChanged(IOHIDValueRef) override;

final?

> Source/WebCore/platform/gamepad/mac/HIDGamepadElement.h:74
> +private:
> +    SharedGamepadValue m_value;

Could move this to be an HIDGamepadElement data member, either protected or private, since both derived classes have it.

> Source/WebCore/platform/gamepad/mac/HIDGamepadElement.h:77
> +class HIDGamepadAxis : public HIDGamepadElement {

final?

> Source/WebCore/platform/gamepad/mac/HIDGamepadElement.h:79
> +    HIDGamepadAxis(const HIDElement& element, SharedGamepadValue value)

Taking a SharedGamepadValue is OK, but intrinsically churn-y, like taking a Ref<>, as opposed to either a const Ref<>& or a Ref<>&&.

> Source/WebCore/platform/gamepad/mac/HIDGamepadElement.h:81
> +        , m_value(value)

WTFMove here would save some reference count churn.

> Source/WebCore/platform/gamepad/mac/HIDGamepadElement.h:88
> +protected:

Why protected? Is this a base class?

> Source/WebCore/platform/gamepad/mac/HIDGamepadElement.h:89
> +    HIDInputType gamepadValueChanged(IOHIDValueRef) override;

final?

> Source/WebCore/platform/gamepad/mac/HIDGamepadElement.h:92
> +private:
> +    SharedGamepadValue m_value;

Could move this to be an HIDGamepadElement data member, either protected or private, since both derived classes have it.

> Source/WebCore/platform/mac/HIDDevice.cpp:83
> +        if (encounteredCookies.contains(cookie))
> +            continue;

Can we do the add operation here along with the contains as a single operation to avoid double hashing, or would that be wasteful or incorrect for the same element types?

> Source/WebCore/platform/mac/HIDDevice.cpp:99
> +        if (type == kIOHIDElementTypeCollection) {
> +            auto children = IOHIDElementGetChildren(element);
> +            for (CFIndex i = 0; i < CFArrayGetCount(children); ++i)
> +                elementQueue.append(checked_cf_cast<IOHIDElementRef>(CFArrayGetValueAtIndex(children, i)));
> +            continue;
> +        }
> +
> +        switch (type) {
> +        case kIOHIDElementTypeCollection: {
> +            auto children = IOHIDElementGetChildren(element);
> +            for (CFIndex i = 0; i < CFArrayGetCount(children); ++i)
> +                elementQueue.append(checked_cf_cast<IOHIDElementRef>(CFArrayGetValueAtIndex(children, i)));
> +            continue;
> +        }

This code is repeated twice. I presume you want to delete the if statement version.

> Source/WebCore/platform/mac/HIDDevice.h:30
> +#include <IOKit/hid/IOHIDDevice.h>

Can we forward declare IOHIDDeviceRef instead of pulling in the header?

> Source/WebCore/platform/mac/HIDDevice.h:32
> +#include <wtf/Vector.h>

Forward declaration should suffice. No actual vectors in this header.

> Source/WebCore/platform/mac/HIDDevice.h:39
> +    WTF_MAKE_FAST_ALLOCATED;

This is OK regardless, but do we ever allocate these on the heap?

> Source/WebCore/platform/mac/HIDDevice.h:41
> +    HIDDevice(IOHIDDeviceRef);

I suggest using explicit here.

> Source/WebCore/platform/mac/HIDElement.h:38
> +    HIDElement(IOHIDElementRef);

explicit

> Source/WebCore/platform/mac/HIDElement.h:39
> +    HIDElement(const HIDElement&) = default;

Are you doing this instead of Noncopyable, to usher in a new age?

> Source/WebCore/platform/mac/HIDElement.h:40
> +    virtual ~HIDElement() { }

No other virtual functions except the destructor? This is an unusual design pattern. Maybe this base class is only to help build derived classes and not for polymorphism. If so, maybe the constructor should be protected?

[Brady Eidson]

In typical "Darin mega-review" fashion, I took most advice.

Some direct replies inline.

(In reply to Darin Adler from comment #3)
> Comment on attachment 405521 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=405521&action=review
> 
> > Source/WebCore/platform/gamepad/SharedGamepadValue.h:36
> > +class SharedGamepadValue {
> 
> The design of this class is a bit tricky. If you assign one to another then
> it redirects which shared value is being used. But if you assign a double to
> it, the shared value is overwritten. Both are done with the same "="
> operator. I think there’s a reason to consider pointer-style boxing instead
> to distinguish those types of assuming.

Removed the explicit double()ness and just added a setter and getter.

> 
> > Source/WebCore/platform/gamepad/mac/HIDGamepadElement.h:54
> > +    HIDGamepadElement(const HIDElement&);
> 
> What is this used for?

First a HIDDevice is made.

Then it creates an array of base-class HIDElements.

Then a GenericHIDGamepad is created with that HIDDevice.

It walks that array, then creates specific subclasses of them (HIDGamepadButton, HIDGamepadAxis)
Those subclasses pass the original HIDElement data back down through the constructor chain so it doesn't need to be re-fetched.

> 
> > Source/WebCore/platform/gamepad/mac/HIDGamepadElement.h:60
> > +class HIDGamepadButton : public HIDGamepadElement {
> 
> final?

In a future patch no! But for this patch, sure.

> 
> > Source/WebCore/platform/gamepad/mac/HIDGamepadElement.h:70
> > +protected:
> 
> Why protected? Is this a base class?

And

> > Source/WebCore/platform/gamepad/mac/HIDGamepadElement.h:88
> > +protected:
> 
> Why protected? Is this a base class?

And
 
> > Source/WebCore/platform/gamepad/mac/HIDGamepadElement.h:74
> > +private:
> > +    SharedGamepadValue m_value;
> 
> Could move this to be an HIDGamepadElement data member, either protected or
> private, since both derived classes have it.

This my (undocumented) future knowledge of where this is going in the next pass.
I can make some of these for now.

> > Source/WebCore/platform/gamepad/mac/HIDGamepadElement.h:92
> > +private:
> > +    SharedGamepadValue m_value;
> 
> Could move this to be an HIDGamepadElement data member, either protected or
> private, since both derived classes have it.

I'll do that for now (but it will change quickly thereafter)

> > Source/WebCore/platform/mac/HIDDevice.cpp:83
> > +        if (encounteredCookies.contains(cookie))
> > +            continue;
> 
> Can we do the add operation here along with the contains as a single
> operation to avoid double hashing, or would that be wasteful or incorrect
> for the same element types?

It wouldn't be correct for some types.

> > Source/WebCore/platform/mac/HIDElement.h:39
> > +    HIDElement(const HIDElement&) = default;
> 
> Are you doing this instead of Noncopyable, to usher in a new age?

You might've misread `default` as `delete`

In an earlier version I needed the explicit default here, but no longer needed!

> 
> > Source/WebCore/platform/mac/HIDElement.h:40
> > +    virtual ~HIDElement() { }
> 
> No other virtual functions except the destructor? This is an unusual design
> pattern. Maybe this base class is only to help build derived classes and not
> for polymorphism. If so, maybe the constructor should be protected?

Earlier version needed it here, but now the virtual d'tor up in HIDGamepadElement is good enough, so I removed this.

[Brady Eidson]

Created attachment 405552 [details]
Patch

[EWS]

Committed r265079: <https://trac.webkit.org/changeset/265079>

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

[Radar WebKit Bug Importer]

<rdar://problem/66314709>