[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

--- Comment #4 from Brady Eidson <beidson at apple.com> ---
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.

-- 
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/20200730/7c8ef267/attachment.htm>