Re: [w3c/gamepad] Add sequenceId slot and fix effect promises (PR #201)

@gterzian approved this pull request.

LGTM, with a few nits to consider. 

>                    <ol>
-                    <li>If
-                    [=this=].{{GamepadHapticActuator/[[playingEffectPromise]]}}
-                    is `null`, abort these steps.
-                    </li>
-                    <li>[=Resolve=]
-                    [=this=].{{GamepadHapticActuator/[[playingEffectPromise]]}}
-                    with {{GamepadHapticsResult/"complete"}}.
-                    </li>
-                    <li>Set
-                    [=this=].{{GamepadHapticActuator/[[playingEffectPromise]]}}
-                    to `null`.
+                    <li>If [=this=].{{GamepadHapticActuator/[[sequenceId]]}} is

You could consider replacing the conditional nesting with an early return(example [usage](https://html.spec.whatwg.org/#dom-trees:parser-inserted-flag)).

>                <ol>
                 <li>Let |effectPromise| be
                 [=this=].{{GamepadHapticActuator/[[playingEffectPromise]]}}.
                 </li>
+                <li>Set
+                [=this=].{{GamepadHapticActuator/[[playingEffectPromise]]}} to
+                `null`.
+                </li>
+                <li>[=Queue a global task=] on the [=relevant global object=]
+                of [=this=] using the [=gamepad task source=] to [=resolve=]
+                |effectPromise| with {{GamepadHapticsResult/"preempted"}}.
+                </li>
+              </ol>
+            </li>
+            <li>Let [=this=].{{GamepadHapticActuator/[[playingEffectPromise]]}}

consider adding a step asserting nullness of the promise at this point(for clarity). 

>                    <ol>
-                    <li>If |effectPromise| and
-                    [=this=].{{GamepadHapticActuator/[[playingEffectPromise]]}}
-                    are still the same, set
-                    [=this=].{{GamepadHapticActuator/[[playingEffectPromise]]}}
-                    to `null`.
-                    </li>
-                    <li>[=Queue a global task=] on the [=relevant global
-                    object=] of [=this=] using the [=gamepad task source=] to
-                    [=resolve=] |effectPromise| with
-                    {{GamepadHapticsResult/"preempted"}}.
+                    <li>If [=this=].{{GamepadHapticActuator/[[sequenceId]]}} is
+                    |resetSequenceId|:

same re early return.

> @@ -1185,8 +1200,11 @@ <h2>
             type=] |type|, return [=a promise rejected with=] reason
             {{NotSupportedError}}.
             </li>
-            <li>Let {{GamepadHapticActuator/[[playingEffectPromise]]}} be [=a
-            new promise=].
+            <li>Let [=this=].{{GamepadHapticActuator/[[playingEffectPromise]]}}

same here on asserting nullness of promise. 

-- 
Reply to this email directly or view it on GitHub:
https://github.com/w3c/gamepad/pull/201#pullrequestreview-2066834880
You are receiving this because you are subscribed to this thread.

Message ID: <w3c/gamepad/pull/201/review/2066834880@github.com>