Gamepad.vibrationActuator: Add support for "trigger-rumble" effect type #9075
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cdumez
added
the
WebCore Misc.
|
EWS run on previous version of this PR (hash b35dd5c) |
cdumez
force-pushed
the
250352_trigger_rumble_second_attempt
branch
from
b35dd5c
to
24bea12
Compare
|
EWS run on current version of this PR (hash 24bea12) |
cdumez
requested review from
beidson,
achristensen07,
geoffreygaren,
brentfulgham and
youennf
|
Ping review? |
brentfulgham
approved these changes
Jan 27, 2023
| m_playingEffectPromise = WTFMove(promise); | ||
| GamepadProvider::singleton().playEffect(m_gamepad->index(), m_gamepad->id(), effectType, effectParameters, [this, protectedThis = makePendingActivity(*this), playingEffectPromise = m_playingEffectPromise](bool success) mutable { | ||
| if (m_playingEffectPromise != playingEffectPromise) | ||
| currentEffectPromise = WTFMove(promise); |
There was a problem hiding this comment.
Since we move the promise argument to the local currentEffectPromise variable, but don't move that into the playEffect closure, could this go out of scope and cause problems? Previously it was moved to the m_playingEffectPromise, which seems like it would keep it alive.
There was a problem hiding this comment.
currentEffectPromise is declared like so:
auto& currentEffectPromise = promiseForEffectType(effectType);
and it returns a reference to the right promise data member. I didn't change any lifetime here. The promise still gets stored as RefPtr in a data member. It is merely abstracted away by the currentEffectPromise C++ reference since we have more than one such data member now.
| currentEffectPromise = WTFMove(promise); | ||
| GamepadProvider::singleton().playEffect(m_gamepad->index(), m_gamepad->id(), effectType, effectParameters, [this, protectedThis = makePendingActivity(*this), playingEffectPromise = currentEffectPromise, effectType](bool success) mutable { | ||
| auto& currentEffectPromise = promiseForEffectType(effectType); | ||
| if (playingEffectPromise != currentEffectPromise) |
There was a problem hiding this comment.
What is keeping currentEffectPromise alive?
There was a problem hiding this comment.
promiseForEffectType() returns a reference to a data member that is a RefPtr.
https://bugs.webkit.org/show_bug.cgi?id=250352 rdar://104315486 Reviewed by Brent Fulgham and Geoffrey Garen. Gamepad.vibrationActuator: Add support for "trigger-rumble" effect type: - https://github.com/MicrosoftEdge/MSEdgeExplainers/blob/main/GamepadHapticsActuatorTriggerRumble/explainer.md It allows vibrating the triggers (which the XBox controller supports), while the existing "dual-rumble" only makes the handles vibrate. This isn't yet part of the specification at: - https://w3c.github.io/gamepad/extensions.html#dom-gamepadhapticeffecttype However, it is supported by Blink and used by XBox cloud games. I am adding support behind an experimental feature flag, off by default. * Source/WTF/Scripts/Preferences/UnifiedWebPreferences.yaml: * Source/WebCore/Modules/gamepad/GamepadEffectParameters.h: * Source/WebCore/Modules/gamepad/GamepadEffectParameters.idl: * Source/WebCore/Modules/gamepad/GamepadHapticActuator.cpp: (WebCore::GamepadHapticActuator::canPlayEffectType const): (WebCore::GamepadHapticActuator::playEffect): (WebCore::GamepadHapticActuator::stopEffects): (WebCore::GamepadHapticActuator::document const): (WebCore::GamepadHapticActuator::promiseForEffectType): * Source/WebCore/Modules/gamepad/GamepadHapticActuator.h: * Source/WebCore/Modules/gamepad/GamepadHapticEffectType.idl: * Source/WebCore/platform/gamepad/cocoa/GameControllerGamepad.mm: (WebCore::GameControllerGamepad::setupElements): * Source/WebCore/platform/gamepad/cocoa/GameControllerHapticEffect.h: * Source/WebCore/platform/gamepad/cocoa/GameControllerHapticEffect.mm: (WebCore::GameControllerHapticEffect::create): (WebCore::GameControllerHapticEffect::GameControllerHapticEffect): (WebCore::GameControllerHapticEffect::start): (WebCore::GameControllerHapticEffect::stop): (WebCore::GameControllerHapticEffect::leftEffectFinishedPlaying): (WebCore::GameControllerHapticEffect::rightEffectFinishedPlaying): (WebCore::GameControllerHapticEffect::strongEffectFinishedPlaying): Deleted. (WebCore::GameControllerHapticEffect::weakEffectFinishedPlaying): Deleted. * Source/WebCore/platform/gamepad/cocoa/GameControllerHapticEngines.h: (WebCore::GameControllerHapticEngines::leftHandleEngine): (WebCore::GameControllerHapticEngines::rightHandleEngine): (WebCore::GameControllerHapticEngines::leftTriggerEngine): (WebCore::GameControllerHapticEngines::rightTriggerEngine): (WebCore::GameControllerHapticEngines::strongEngine): Deleted. (WebCore::GameControllerHapticEngines::weakEngine): Deleted. * Source/WebCore/platform/gamepad/cocoa/GameControllerHapticEngines.mm: (WebCore::GameControllerHapticEngines::GameControllerHapticEngines): (WebCore::GameControllerHapticEngines::currentEffectForType): (WebCore::GameControllerHapticEngines::playEffect): (WebCore::GameControllerHapticEngines::stopEffects): (WebCore::GameControllerHapticEngines::ensureStarted): (WebCore::GameControllerHapticEngines::stop): * Source/WebCore/platform/gamepad/cocoa/GameControllerSoftLink.h: * Source/WebCore/platform/gamepad/cocoa/GameControllerSoftLink.mm: * Source/WebKit/Shared/WebCoreArgumentCoders.serialization.in: Canonical link: https://commits.webkit.org/259507@main
webkit-early-warning-system
force-pushed
the
250352_trigger_rumble_second_attempt
branch
from
24bea12
to
2dca512
Compare
|
Committed 259507@main (2dca512): https://commits.webkit.org/259507@main Reviewed commits have been landed. Closing PR #9075 and removing active labels. |
webkit-commit-queue
removed
the
merge-queue
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
2dca512
24bea12