-
Notifications
You must be signed in to change notification settings - Fork 13
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
First pass: policies member #46
base: main
Are you sure you want to change the base?
Conversation
Note to @marcoscaceres: I know it says "must" (no caps) and this doc doesn’t have a conformance section, but it sounds like there is some interest in this moving forward toward becoming a spec (as opposed to being an amorphous pseudo-spec) so maybe that’s ok for now? |
index.html
Outdated
object are all optional, but are based on common policy types. The | ||
value given for each policy type must be a [=path-absolute-URL | ||
string=] that is resolvable within the application's | ||
[=manifest/navigation scope=] or a [=URL-scheme string=]. |
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.
A URL-scheme string
is defined as:
A URL-scheme string must be one ASCII alpha, followed by zero or more of ASCII alphanumeric, U+002B (+), U+002D (-), and U+002E (.).
This would make https
a valid value for any field in policies
.
Did you maybe mean absolute-URL-with-fragment string
?
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.
that is resolvable within the application's [=manifest/navigation scope=]
Based on this comment I think restrictions based on scope
should be dropped.
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 was writing this to say scope
only applies for path-absolute URLs (e.g., starting with /) but fully-qualified URLs are totally cool (and good catch with absolute-URL-with-fragment string
). Originally I’d written it with full URLs first but I thought it might be confusing that the URLS would always need to be in scope, which is not accurate. I could change it to state that it would be resolved in relation to the URL of the manifest.
I want to capture a thread with @tomayac:
Me:
Me:
I also want to note that I have reached out to our in-house counsel to get their perspective on this. I expect it will take a little bit to get their feedback as I know they’re usually pretty busy, but I will report back with their take on how international regulatory bodies might view this once I receive it. |
@tomayac Initial meeting with our legal counsel went well. Like me, they were also concerned with carrying policy text in 2 places and the potential
They are going to dig a little more and get back to me if they come up with any concerns with this approach. |
@aarongustafson Thank you for taking my Twitter feedback and relaying it here. I was on vacation and ignoring GitHub; else, I would have responded here in the first place. |
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.
Looking good. I agree that not requiring these to be in scope is a good idea.
I just realized that in the other spec, we should add a "process a URL member" algorithm... we do it enough times that having a generic algorithm for this might make sense. We could even have a "don't enforce scope" flag specifically for these kinds of members.
@marcoscaceres Do you want me to move the algorithm to Manifest? I can update that spec and reference it here. |
That would be great if you could, yes! |
Ignore 1017… I had not pulled from the manifest repo in a while and got out of sync. Once 1018 merges, I can update this PR to reference it and (if you’re good with it) merge this PR. |
@marcoscaceres Circling back to this, should I remove the URL parsing algorithm or keep it? We nixed the changes in the Manifest itself. |
16ed26b
to
924839e
Compare
@marcoscaceres Would love to circle back on this one and close it out with a merge. How should I proceed? |
@aarongustafson Could you rebase on the base branch and fix the CI problem? FWIW I'm happy with the current draft. |
CI check is passing now. I brought back the URL parsing as well since we abandoned that in the main spec. Requested approval from @marcoscaceres and 🤞🏻 we can merge. |
@marcoscaceres what do you think of the changes now? |
Based on the discussion in #40.