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.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Rewrite installation process and install prompting logic #790
Rewrite installation process and install prompting logic #790
Changes from 3 commits
f9f49e4
7732d3a
d5a01d7
3a9683c
89cde75
f31386c
7fc9389
37cda87
eba0b45
39bf682
5aa3e1e
f420481
1c4ff86
7b0b5d7
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whether the document is installable ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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: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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed with @dominickng. Chrome does:
"accepted"
as soon as the user chooses to install, regardless of whether the installation actually succeeds (matching the proposed "in parallel" text).So I would prefer to keep this as-is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, alternative:
That way it's clear that in parallel goes off to do other things independently of |result|.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK that works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.