Re: [w3c/manifest] WIP: beforeinstallprompt (#506)

marcoscaceres commented on this pull request.

@mgiuca, again many thanks for all the feedback. I'm going to close this PR and send a fresh, hopefully final version. 

> +};
+
+dictionary BeforeInstallPromptEventInit : EventInit {
+  AppBannerPromptOutcome userChoice;
+};
+      </pre>
+        <p>
+          The <dfn><code>BeforeInstallPromptEvent</code></dfn> is dispatched
+          prior to activating an <a>automated install prompt</a>, allowing a
+          developer to prevent the default action for an install prompt.
+        </p>
+        <p>
+          Thus, the default action of the <a>BeforeInstallPromptEvent</a> is to
+          present an automated install prompt to the end user. Cancelling the
+          default action (via <code>.preventDefault()</code>) prevents the user
+          agent from presenting the automated install prompt.

Added example

> @@ -0,0 +1,143 @@
+/*globals BeforeInstallPromptEvent, DOMException*/

Yeah, I left it open so could call the methods from the console. 

> @@ -0,0 +1,143 @@
+/*globals BeforeInstallPromptEvent, DOMException*/
+"use strict";
+const internalSlots = new WeakMap();

heh, I'm not a fan of the reverse :) However, once we put this into it's own scope, as suggested in your first comment, it won't matter. Hopefully JS will get real private members in the future. 

> @@ -0,0 +1,143 @@
+/*globals BeforeInstallPromptEvent, DOMException*/
+"use strict";
+const internalSlots = new WeakMap();
+const installProcesses = [];
+const AppBannerPromptOutcome = new Set([
+  "accepted",
+  "dismissed",
+]);
+
+class BeforeInstallPromptEvent extends Event {
+  constructor(typeArg, eventInit) {
+    // WebIDL Guard. Wont be in spec in spec as it's all handled by WebIDL.

fixed

> +    }
+    super(typeArg, Object.assign({ cancelable: true }, eventInit));
+
+    if (eventInit && typeof eventInit.userChoice !== "undefined" && !AppBannerPromptOutcome.has(String(eventInit.userChoice))) {
+      const msg = `The provided value '${eventInit.userChoice}' is not a valid` +
+        "enum value of type AppBannerPromptOutcome.";
+      throw new TypeError(msg);
+    }
+    // End WebIDL guard.
+    const internal = {
+      didPrompt: false,
+    };
+
+    internal.userChoice = new Promise((resolve) => {
+      internal.userChoiceHandlers = {
+        resolve,

So, before it held .resolve and .reject. But will change to your suggestion:
```
internal.userChoiceResolver = resolve;
```

> +      const promptOutcome = await showInstallPrompt();
+      internalSlots.get(this).userChoiceHandlers.resolve(promptOutcome);
+    }.bind(this)())
+  }
+
+  get userChoice() {
+    return internalSlots.get(this).userChoice;
+  }
+}
+
+async function notifyBeforeInstallPrompt(element) {
+  await trackReadyState();
+  if (installProcesses.length) {
+    return;
+  }
+  const event = new BeforeInstallPromptEvent("beforeinstallprompt");

Yeah, I was thinking of doing that (I've just been commenting out the checks while I play with this). Obviously, trusted gets set automatically when the real even is created by the browser through the binding layer. 

Within a few weeks, we will have both Chrome and Firefox implementing this for real in C++, so let's keep the current sample implementation as is.  

> +  }
+}
+
+async function notifyBeforeInstallPrompt(element) {
+  await trackReadyState();
+  if (installProcesses.length) {
+    return;
+  }
+  const event = new BeforeInstallPromptEvent("beforeinstallprompt");
+  window.dispatchEvent(event);
+  if (!event.defaultPrevented) {
+    await showInstallPrompt(element);
+  }
+}
+
+function trackReadyState() {

Technically you are correct, but as this doesn't use a "await" keyword anywhere, it's fine to leave looking like a sync function. 

> +    .then(value => console.assert(value === "dismissed"))
+    .catch(err => console.assert(false, assertion, err));
+} {
+  const assertion = "BeforeInstallPromptEvent has a prompt() method";
+  const e = new BeforeInstallPromptEvent("");
+  console.assert("prompt" in e, assertion);
+} {
+  const assertion = "prompt() is a function";
+  const e = new BeforeInstallPromptEvent("");
+  console.assert(e.prompt instanceof Function, assertion);
+} {
+  const assertion = "prompt() takes no arguments";
+  const e = new BeforeInstallPromptEvent("");
+  console.assert(e.prompt.length === 0, assertion);
+} {
+  const assertion = "calling prompt with unstrusted event results in NotAllowedError error";

fixed

> +    console.assert(false, assertion);
+  } catch (err) {
+    console.assert(err instanceof DOMException, assertion);
+    console.assert(err.type = "NotAllowedError", assertion);
+  }
+  try {
+    e.prompt();
+    console.assert(false, assertion);
+  } catch (err) {
+    console.assert(err instanceof DOMException, assertion);
+    console.assert(err.type = "InvalidStateError", assertion);
+  }
+}
+
+// install events
+window.addEventListener("install", (ev) => {

Heh. Got used to the old way 👍  fixed!

-- 
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/506#pullrequestreview-4999048