[Webkit-unassigned] [Bug 219930] GamepadButton.prototype is missing 'touched' attribute

[bugzilla-daemon at webkit.org]

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

Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #417136|review?                     |review+
              Flags|                            |

--- Comment #13 from Darin Adler <darin at apple.com> ---
Comment on attachment 417136
  --> https://bugs.webkit.org/attachment.cgi?id=417136
Patch

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

Generally looks good, but we can do something simpler.

> Source/WebCore/platform/gamepad/SharedGamepadValue.h:67
> +class SharedGamepadButtonValue : public SharedGamepadValue {

I wish this was more like a struct and same for SharedGamepadValue. All these getters and setters don’t seem helpful. These are simple data holders.

> Source/WebCore/platform/gamepad/SharedGamepadValue.h:70
> +        : SharedGamepadValue()

Should be able to omit this.

> Source/WebCore/platform/gamepad/SharedGamepadValue.h:71
> +        , m_simulate(true)

This leaves m_touched and m_pressed uninitialized. If someone then calls setTouched, then the uninitialized value of m_pressed is "exposed". Let’s just always initialize.

After studying this, I came to the conclusion we don’t need m_simulate at all.

> Source/WebCore/platform/gamepad/SharedGamepadValue.h:77
> +        , m_simulate(true)

Instead we should just initialize like this:

    , m_touched(value > 0.1)
    , m_pressed(m_touched)

That will work.

> Source/WebCore/platform/gamepad/SharedGamepadValue.h:81
> +    explicit SharedGamepadButtonValue(double value, bool touched, bool pressed)

No need for "explicit" here.

> Source/WebCore/platform/gamepad/SharedGamepadValue.h:120
> +        encoder << value();
> +        encoder << touched();
> +        encoder << pressed();

A little strange that encoding strips away the "simulated" state.

> Source/WebCore/platform/gamepad/SharedGamepadValue.h:154
> +    bool m_touched;
> +    bool m_pressed;
> +    bool m_simulate;

These should be initialized here. I suggest false/false/false.

    bool m_touched { false };

etc.

Also, the name should probably be m_simulated, not m_simulate.

> Source/WebKit/Shared/Gamepad/GamepadData.h:61
> -    const Vector<double>& buttonValues() const { return m_buttonValues; }
> +    const Vector<WebCore::SharedGamepadButtonValue>& buttonValues() const { return m_buttonValues; }

Seems like the sharing here is very strange. Why do these values need to be shared, and not just a struct?

-- 
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/20210107/eb60505b/attachment.htm>