Summary: | GamepadButton.prototype is missing 'touched' attribute | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Frank Olivier <frankolivier> | ||||||||||||||||||||
Component: | WebCore Misc. | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||
Status: | NEW --- | ||||||||||||||||||||||
Severity: | Normal | CC: | ahmad.saleem792, beidson, benjamin, cdumez, cmarcelo, darin, dino, esprehn+autocc, ews-watchlist, frankolivier, kondapallykalyan, smoley, webkit-bug-importer | ||||||||||||||||||||
Priority: | P2 | Keywords: | BrowserCompat, InRadar, WPTImpact | ||||||||||||||||||||
Version: | Safari 14 | ||||||||||||||||||||||
Hardware: | All | ||||||||||||||||||||||
OS: | All | ||||||||||||||||||||||
Attachments: |
|
Description
Frank Olivier
2020-12-15 17:17:06 PST
Created attachment 416344 [details]
Patch
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. 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) (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) Created attachment 416841 [details]
Patch
Created attachment 416906 [details]
Patch
Created attachment 416907 [details]
Patch
Created attachment 416917 [details]
Patch
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
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. 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
|