Re: [w3c/manifest] Feat(events): add BeforeInstallPromptEvent (#520)

mgiuca commented on this pull request.

Some more thoughts on the algorithm definition. Sorry it took the whole day to reply. I was ... distracted.

> +          which are set when the event is <a data-lt=
+          "construct a BeforeInstallPromptEvent">constructed</a>:
+        </p>
+        <dl>
+          <dt>
+            <dfn>[[\didPrompt]]</dfn>
+          </dt>
+          <dd>
+            A boolean, initially <code>false</code>. Represents if this event
+            was used to <a>present an install prompt</a> to the end-user.
+          </dd>
+          <dt>
+            <dfn>[[\promptOutcome]]</dfn>
+          </dt>
+          <dd>
+            A <a>AppBannerPromptOutcome</a> enum value, initially set to

s/A/An

> +            A boolean, initially <code>false</code>. Represents if this event
+            was used to <a>present an install prompt</a> to the end-user.
+          </dd>
+          <dt>
+            <dfn>[[\promptOutcome]]</dfn>
+          </dt>
+          <dd>
+            A <a>AppBannerPromptOutcome</a> enum value, initially set to
+            <code>null</code>. Represents the outcome of <a>presenting an
+            install prompt</a>.
+          </dd>
+          <dt>
+            <dfn>[[\userResponsePromise]]</dfn>
+          </dt>
+          <dd>
+            A promise, which resolves with an <a>PromptResponseObject</a>,

s/an/a

> +        <section>
+          <h4>
+            Constructor
+          </h4>
+          <p>
+            To construct an instance of the <a>BeforeInstallPromptEvent</a>
+            interface, run the <dfn data-lt=
+            "construct a BeforeInstallPromptEvent">steps to construct a
+            <code>BeforeInstallPromptEvent</code> event</dfn>:
+          </p>
+          <ol>
+            <li>Let <a>[[\didPrompt]]</a> be <code>false</code>.
+            </li>
+            <li>Let <a>[[\userResponsePromise]]</a> be a newly created promise.
+            </li>
+            <li>Let <a>[[\promptOutcome]]</a> be <code>null</code>.

I think promptOutcome is unnecessary.

If you search the remaining text for promptOutcome, note that it is always assigned to, never read from, except:

1. "If [[promptOutcome]] is not null" -- that's just using it as a Boolean not an outcome value.
2. "whose userChoice member is set to event's [[promptOutcome]]." -- promptOutcome was just set to a value immediately above, so it doesn't have to be a member of BIPE, it can just be a local variable to that algorithm.

Therefore, promptOutcome can be replaced with a Boolean. But I suspect we can simplify it further.

> +        </section>
+        <section>
+          <h4>
+            <code>prompt()</code> method
+          </h4>
+          <p>
+            The <dfn>prompt</dfn> method, when called, runs the following
+            steps:
+          </p>
+          <ol>
+            <li>Let <var>p</var> be a newly created promise.
+            </li>
+            <li>Run the following steps <a>in parallel</a>:
+              <ol>
+                <li>If <a>[[\promptOutcome]]</a> is not <code>null</code>,
+                resolve <var>p</var> with <a>[[\userResponsePromise]]</a> and

Wait, "resolve p with userResponsePromise"? That means to resolve a promise with a promise. I think you mean "resolve p with promptOutcome"... but I'm trying to eliminate this (see above).

Why is p a new promise at all? Why can't we just have prompt() return userResponsePromise?

Then we can kill promptOutcome entirely.

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