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? |
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!
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.
|
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 <[email protected]> Reviewed-by: Daniel Murphy <[email protected]> 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. |
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.
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
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 <[email protected]> Reviewed-by: Daniel Murphy <[email protected]> Cr-Commit-Position: refs/heads/master@{#904128} NOKEYCHECK=True GitOrigin-RevId: 3ef03e903e21bab3db9ff191f0ce9fc7e90944f9
The change was requested from spec editor review w3c/manifest#988 (review). This makes manifest parsing to treat specifying empty string for id field same as it's not specified. Updated |ParseURL| to add |ignore_empty_string| param. This is needed because we currently parse url fields inconsistently. Bug: 1182363 Change-Id: Ie5f6d331da7075f7c919f6e2120ea3840fc650f6 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3177726 Commit-Queue: Phillis Tang <[email protected]> Reviewed-by: Daniel Murphy <[email protected]> Cr-Commit-Position: refs/heads/main@{#924541} NOKEYCHECK=True GitOrigin-RevId: 6fc55b5ee6ccec48d2e0ee47fb0e4bed0e1f8040
Requested from app manifest pull request w3c/manifest#988 (review). It makes sense to remove fragment from the id because different fragments still identify the same resource. Bug: 1182363 Change-Id: Ic8d258f380efec5b172990418fbe57c269f3ff66 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3188751 Reviewed-by: Daniel Murphy <[email protected]> Commit-Queue: Phillis Tang <[email protected]> Cr-Commit-Position: refs/heads/main@{#925484} NOKEYCHECK=True GitOrigin-RevId: 9814468fb1bf3cd8554c2ed5a1de092d95275688
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.