Bug 219930 – GamepadButton.prototype is missing 'touched' attribute

• Component: WebCore Misc.
• Status: NEW

[Frank Olivier]

GamepadButton.prototype is missing 'touched' attribute in WebKit

Chrome, Firefox both have the 'touched' attribute

From https://w3c.github.io/gamepad/#dom-gamepadbutton-touched

touched attribute
The touched state of the button. If the button is capable of detecting touch, this property MUST be true if the button is currently being touched, and false otherwise. If the button is not capable of detecting touch and is capable of reporting an analog value, this property MUST be true if the value property is greater than 0, and false if the value is 0. If the button is not capable of detecting touch and can only report a digital value, this property MUST mirror the pressed attribute.

Created attachment 416344 [details]
Patch

[Dean Jackson]

Comment on attachment 416344 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        No new tests (OOPS!).

Please remove this line. You don't need to list a new test since you've modified an existing one.

> Source/WebCore/Modules/gamepad/GamepadButton.cpp:50
> +    If the button is capable of detecting touch, this property MUST be true if the button is currently being touched, and false otherwise. If the button is not capable of detecting touch and is capable of reporting an analog value, this property MUST be true if the value property is greater than 0, and false if the value is 0. If the button is not capable of detecting touch and can only report a digital value, this property MUST mirror the pressed attribute.

Please wrap this line.

Also, I think we need to add a comment on the implementation  e.g.

FIXME: Update if/when we can support touches on a controller.

[Brady Eidson]

Comment on attachment 416344 [details]
Patch

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

> Source/WebCore/Modules/gamepad/GamepadButton.cpp:52
> +    /*
> +    https://www.w3.org/TR/gamepad/#dom-gamepadbutton-touched
> +    If the button is capable of detecting touch, this property MUST be true if the button is currently being touched, and false otherwise. If the button is not capable of detecting touch and is capable of reporting an analog value, this property MUST be true if the value property is greater than 0, and false if the value is 0. If the button is not capable of detecting touch and can only report a digital value, this property MUST mirror the pressed attribute.
> +    */
> +    return pressed();

Is there something we can do better here in GCF-land? Does it support touch sensitivity yet? (That was the original motivation here, to allow distinguishing touched and pressed)

[Brady Eidson]

(In reply to Brady Eidson from comment #3)
> Comment on attachment 416344 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=416344&action=review
> 
> > Source/WebCore/Modules/gamepad/GamepadButton.cpp:52
> > +    /*
> > +    https://www.w3.org/TR/gamepad/#dom-gamepadbutton-touched
> > +    If the button is capable of detecting touch, this property MUST be true if the button is currently being touched, and false otherwise. If the button is not capable of detecting touch and is capable of reporting an analog value, this property MUST be true if the value property is greater than 0, and false if the value is 0. If the button is not capable of detecting touch and can only report a digital value, this property MUST mirror the pressed attribute.
> > +    */
> > +    return pressed();
> 
> Is there something we can do better here in GCF-land? Does it support touch
> sensitivity yet? (That was the original motivation here, to allow
> distinguishing touched and pressed)

(I know this is basically what your comment says, but what I'm suggesting is make sure GameController.framework doesn't already offer it somehow)

[Radar WebKit Bug Importer]

<rdar://problem/72596955>

Created attachment 416841 [details]
Patch

Created attachment 416906 [details]
Patch

Created attachment 416907 [details]
Patch

Created attachment 416917 [details]
Patch

[Dean Jackson]

Comment on attachment 416917 [details]
Patch

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

> Source/WebKit/Shared/Gamepad/GamepadData.cpp:46
> +, m_buttonValues(WTF::map(buttonValues, [](const auto& button) { return button.value(); }))
> +, m_buttonTouchedValues(WTF::map(buttonValues, [](const auto& button) { return button.touched(); }))
> +, m_buttonPressedValues(WTF::map(buttonValues, [](const auto& button) { return button.pressed(); }))

Nit: formatting error here.

> Source/WebKit/Shared/Gamepad/GamepadData.h:72
>      Vector<double> m_buttonValues;
> +    Vector<bool> m_buttonTouchedValues;
> +    Vector<bool> m_buttonPressedValues;

I wonder if we should have a struct with value, touchedValue and PressedValue, and then just a Vector of those?

> Source/WebKit/WebProcess/Gamepad/WebGamepad.cpp:73
> +        m_buttonValues[i] = SharedGamepadButtonValue(gamepadData.buttonValues()[i], gamepadData.buttonPressedValues()[i], gamepadData.buttonTouchedValues()[i]);

should we ASSERT that buttonValues + buttonPressedValues + buttonTouchedValues all have the same size?

Created attachment 417136 [details]
Patch

[Darin Adler]

Comment on attachment 417136 [details]
Patch

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

> Source/WebCore/platform/gamepad/SharedGamepadValue.h:133
> +    /*
> +    template<class Decoder>
> +    static Optional<MonotonicTime> decode(Decoder& decoder)
> +    {
> +        Optional<double> time;
> +        decoder >> time;
> +        if (!time)
> +            return WTF::nullopt;
> +        return MonotonicTime::fromRawSeconds(*time);
> +    }
> +    */

Normally we don’t include commented-out code.

[Darin Adler]

Comment on attachment 417136 [details]
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?

Created attachment 418331 [details]
Patch

Created attachment 418345 [details]
Patch

Created attachment 418354 [details]
Patch

[Ahmad Saleem]

Failing - https://wpt.fyi/results/gamepad/idlharness.window.html?label=master&label=experimental&aligned&q=idlharness%20safari%3Afail%20chrome%3Apass%20firefox%3Apass