[webkit-reviews] review granted: [Bug 219930] GamepadButton.prototype is missing 'touched' attribute : [Attachment 417136] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted frankolivier at apple.com's request for
review:
Bug 219930: GamepadButton.prototype is missing 'touched' attribute
https://bugs.webkit.org/show_bug.cgi?id=219930

Attachment 417136: Patch

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




--- 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?