Repository navigation
Add id member to manifest - #988
Conversation
|
Nice - I can also see this fleshing out the section "Updating the manifest" as well with the algorithm |
Will do, thanks for reminding! |
|
@mgiuca can you take a look at this? |
mgiuca
left a comment
There was a problem hiding this comment.
Very good, overall. Everything's in the right place and largely conveys the meaning I think you intended. So don't be alarmed by the high volume of comments. These are details :)
Now I've made a lot of specific phrasing suggestions. Feel free to reword them. They are by no means perfect or canonical, just the way that I would personally phrase things. Ask if you have questions.
Thanks, it's exciting to see this finally coming together!
marcoscaceres
left a comment
There was a problem hiding this comment.
Looking really good @philloooo! Excited to implement this in Gecko. I'll make a start once you make the next round of updates.
aarongustafson
left a comment
There was a problem hiding this comment.
Just some minor suggestions.
|
thanks for the review! @aarongustafson @marcoscaceres @mgiuca |
Use ParseURL for manifest id parsing logic. This allows id to be both specified as a full url or a string as path to app's origin. This allows a more consistent spec processing definition, see in spec pull request w3c/manifest#988 (comment). The processed manifest id is still passed as a normalized relative url path to WebAppProvider system to keep the manifest_id saved in sync system the same and backward-compatible since changing the manifest_id format in sync system will make new apps not syncable to previous versions of Chromium. Bug: 1182363 Change-Id: I65a7ca615bae3ee504b605abba954f60dc5d2c31 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3042714 Commit-Queue: Phillis Tang <phillis@chromium.org> Reviewed-by: Daniel Murphy <dmurph@chromium.org> Cr-Commit-Position: refs/heads/master@{#904128}
|
Apologies, I haven't got around to re-reviewing this. Looks like Aaron and Marcos are happy. I'd like to take another look at it, but if I don't get to it by ~Monday, then you can probably go ahead and merge it. |
|
@philloooo, I hope to implement this next week. I may have more feedback then! Thanks again for the great work. |
marcoscaceres
left a comment
There was a problem hiding this comment.
Started to implement but hit some issues.
@philloooo, made some suggested changes to the algorithm.
I didn't get a chance to feed the table values into it, but they could serve as unit tests for it.
marcoscaceres
left a comment
There was a problem hiding this comment.
@philloooo I was writing some additional tests for Gecko and realized we didn't deal with URL fragments. I think we should probably remove them? Wdyt?
marcoscaceres
left a comment
There was a problem hiding this comment.
Request to send update as a different PR.
Moved to #1011 |
|
Some of the w3c systems are currently down, which is causing the CI errors. |
spec change w3c/manifest#988 Differential Revision: https://phabricator.services.mozilla.com/D126331
spec change w3c/manifest#988 Differential Revision: https://phabricator.services.mozilla.com/D126331
spec change w3c/manifest#988 Differential Revision: https://phabricator.services.mozilla.com/D126331
Closes #586
Explainer https://github.com/philloooo/pwa-unique-id/blob/main/explainer.md
This change:
Implementation commitment (delete if not making normative changes):
If change is normative, and it adds or changes a member:
Commit message:
Explicitly defines what uniquely identifies a web application and allow developers to specify the identifier in the manifest.
💥 Error: connect ETIMEDOUT 128.30.52.89:443 💥
PR Preview failed to build. (Last tried on Oct 5, 2021, 11:30 PM UTC).
More
PR Preview relies on a number of web services to run. There seems to be an issue with the following one:
🚨 Spec Generator - Spec Generator is the web service used to build specs that rely on ReSpec.
🔗 Related URL
If you don't have enough information above to solve the error by yourself (or to understand to which web service the error is related to, if any), please file an issue.