Rewrite installation process and install prompting logic #790
Conversation
|
@dominickng to also have a look. This PR will likely be a prerequisite to fix #784 and #789 (I started work on #784 and found the algorithm to be non-sensical, hence this PR). |
There was a problem hiding this comment.
Looking pretty good @mgiuca. Thanks for cleaning this up. Just one small issue I found with "return result". Rest are mostly just nits.
index.html
Outdated
| "AppBannerPromptOutcome">accepted</a>, then in parallel, run the <a> | ||
| steps to install the web application</a>. | ||
| </li> | ||
| <li>Return <var>result</var>. |
There was a problem hiding this comment.
This is ambiguous: is result returned before "in parallel" runs, or after? As it reads (to me), it's returning before, which I think is incorrect, right?
There was a problem hiding this comment.
It's intended to be before (i.e., your reading is correct). I don't think it's ambiguous: that's precisely the meaning of "in parallel" (otherwise I wouldn't have said "in parallel").
The result of "present an install prompt" should be the user's choice, not the success of the installation. The result of this promise reflects the user's choice, whereas the appinstalled event reflects whether it was actually installed.
At least, that was my intention. Now I'm not so sure. Looking at Chrome's app_banner_manager, the code that sends BannerAccepted (which results in the promise resolving with "accepted") has a comment:
// Catch only kSuccessNewInstall and kUserInstallDeclined. Report nothing on
// all other errors.implying that maybe we resolve with "accepted" if the user accepts AND installation is success, and perhaps we never resole if the user accepts and installation fails??? @dominickng Can you confirm what Chrome is doing here?
We should decide what the (currently unspecified) behaviour should be if the user accepts the banner but the installation fails: if we want to resolve with "accepted" then the proposed text is correct. If not, we should delete "in parallel" and figure out what to resolve with (perhaps we should introduce a new "accepted-but-failed" result)?
There was a problem hiding this comment.
Discussed with @dominickng. Chrome does:
- On Android, it resolves with
"accepted"as soon as the user chooses to install, regardless of whether the installation actually succeeds (matching the proposed "in parallel" text). - On Windows / Chrome OS / Linux / macOS, if the user accepts and the install "fails", the promise won't resolve. But the failures are all very much edge cases (like something has gone horribly wrong, or the web contents is destroyed, etc), not expected to happen in normal situations. We would consider these edge cases Chrome bugs if they happened. For all intents and purposes, the proposed "in parallel" text matches our behaviour.
So I would prefer to keep this as-is.
There was a problem hiding this comment.
Ok, alternative:
<li>Return |result| and in parallel: ....
That way it's clear that in parallel goes off to do other things independently of |result|.
| <li>If there is already an <a>installation process</a> being | ||
| <a data-lt="present an install prompt">presented</a>, terminate this | ||
| algorithm. | ||
| <li>If there is already an <a data-lt= |
There was a problem hiding this comment.
We probably need need some kind of internal slot to track this, which can be reset as part of task once installation completes (and can hook into manual installation)... we don't need to do it as part of this PR tho.
There was a problem hiding this comment.
Yep. You're tracking this at #761 so I thought I'd just leave it for now.
Similarly, we should probably add an internal slot for "is installable" since that's a document-global property that I'm adding, but I didn't want to do it in this PR.
Co-Authored-By: Marcos Cáceres <[email protected]>
Co-Authored-By: Marcos Cáceres <[email protected]>
Co-Authored-By: Marcos Cáceres <[email protected]>
|
Thanks for the quick review, @marcoscaceres. I've accepted or made most of the changes you proposed. I will follow up with @dominickng about the behavioural question. |
| attempt installation. | ||
| <li>Let <var>manifest</var> and <var>manifest URL</var> be the values | ||
| that were created during <a>steps to determine installability of the | ||
| document</a>. |
There was a problem hiding this comment.
whether the document is installable ?
There was a problem hiding this comment.
I think this phrasing is OK, given that we also define "installability signals" (keeping it this way implies that the two are connected, which they are).
There was a problem hiding this comment.
Pre-approving... I still think the the |result| + in parallel could be a bit clearer, but overall this is a great improvement. Nice work @mgiuca.
index.html
Outdated
| "AppBannerPromptOutcome">accepted</a>, then in parallel, run the <a> | ||
| steps to install the web application</a>. | ||
| </li> | ||
| <li>Return <var>result</var>. |
There was a problem hiding this comment.
Ok, alternative:
<li>Return |result| and in parallel: ....
That way it's clear that in parallel goes off to do other things independently of |result|.
Rewrite installation process and install prompting logic per w3c#788. - Introduces a new concept of the document being "installable", a stateful property of the document based on validity of the manifest. - Adds a new process, "steps to determine installability of the document", split out from "steps to install the web application". - Removed the concept of "installation process", "installation succeeded", "installation canceled" (which was unused; see w3c#787) and "installation failed". These concepts are now part of the "steps to install the web application". - "Steps to install the web application" now assumes that the document is installable, and that user has already accepted the install prompt, where previously the determination of installability and prompting were all part of that same process. - Processes that show an install prompt now explicitly call "steps to install the web application" after the user accepts the prompt (see w3c#786). Normative changes are to fix bugs in the spec which made it non-implementable; I believe these simply bring the spec in line with all known implementations, so no implementation change is required due to this spec change. Closes w3c#786, w3c#787, w3c#788.
This change (choose one):
changes normative sections without changing behavior)
Commit message:
Rewrite installation process and install prompting logic per #788.
property of the document based on validity of the manifest.
out from "steps to install the web application".
"installation canceled" (which was unused; see Steps to install the web application does not take into account whether the installation process was canceled #787) and "installation
failed". These concepts are now part of the "steps to install the web
application".
installable, and that user has already accepted the install prompt, where
previously the determination of installability and prompting were all part of
that same process.
the web application" after the user accepts the prompt (see BeforeInstallPromptEvent.prompt() does not actually perform installation #786).
Normative changes are to fix bugs in the spec which made it non-implementable; I
believe these simply bring the spec in line with all known implementations, so
no implementation change is required due to this spec change.
Closes #786, #787, #788.
Preview | Diff