[Webkit-unassigned] [Bug 214910] Refactor HID gamepad code to be much less fragile and much easier to hack on

[bugzilla-daemon at webkit.org]

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

Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |darin at apple.com
 Attachment #405521|review?                     |review+
              Flags|                            |

--- Comment #3 from Darin Adler <darin at apple.com> ---
Comment on attachment 405521
  --> https://bugs.webkit.org/attachment.cgi?id=405521
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?

-- 
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/20200729/3abe7aeb/attachment.htm>