-
Notifications
You must be signed in to change notification settings - Fork 72
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
Specify multiple configURLs #667
base: main
Are you sure you want to change the base?
Conversation
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.
All to increase clarity.
Thanks @TallTed, done. (Some of the lines you commented on aren't new, I just changed the indentation) |
spec/index.bs
Outdated
set to the following steps, given a <a spec=fetch for=/>response</a> |response| and |responseBody|: | ||
1. Let |json| be the result of [=extract the JSON fetch response=] from |response| and | ||
|responseBody|. | ||
1. Set |discovery| to the result of [=converted to an IDL value|converting=] |json| |
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.
Why are we calling this discovery
?
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's what the code used to call it. I'm actually not sure why that name was picked.
spec/index.bs
Outdated
1. If |login_url| or |accounts_url| is failure, return failure. | ||
1. Wait for both |config| and |discovery| to be set. | ||
1. If |discovery| is null, return failure. | ||
1. If |rpOrigin| is not an [=opaque origin=], and |rootUrl|'s [=url/host=] is equal |
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 seems to mean that the well known file needs to be present now in this case (otherwise it will fail in the previous line), whereas before it could pass? Is this change intentional? Why can't we keep the existing behavior where we just do not fetch the well-known in this case?
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 was easier to specify that way and Chrome's implementation does fetch the well known file in this circumstance. However, Chrome ignores fetch failures, so you're right that I need to change this. Done.
spec/index.bs
Outdated
|config|.{{IdentityProviderAPIConfig/login_url}}, and |globalObject|. | ||
1. Set |accounts_url| to the result of [=computing the manifest URL=] with |provider|, | ||
|config|.{{IdentityProviderAPIConfig/accounts_endpoint}}, and |globalObject|. | ||
1. If |login_url| or |accounts_url| is failure, return failure. |
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.
Ah maybe an existing issue but I don't know if this is allowed to return on behalf of the algorithm since these steps are done in processResponseConsumeBody which is an algorithm of its own. So maybe modify variables as opposed to return failure?
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.
Done.
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.
Please take another look!
spec/index.bs
Outdated
set to the following steps, given a <a spec=fetch for=/>response</a> |response| and |responseBody|: | ||
1. Let |json| be the result of [=extract the JSON fetch response=] from |response| and | ||
|responseBody|. | ||
1. Set |discovery| to the result of [=converted to an IDL value|converting=] |json| |
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's what the code used to call it. I'm actually not sure why that name was picked.
spec/index.bs
Outdated
|config|.{{IdentityProviderAPIConfig/login_url}}, and |globalObject|. | ||
1. Set |accounts_url| to the result of [=computing the manifest URL=] with |provider|, | ||
|config|.{{IdentityProviderAPIConfig/accounts_endpoint}}, and |globalObject|. | ||
1. If |login_url| or |accounts_url| is failure, return failure. |
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.
Done.
spec/index.bs
Outdated
1. If |login_url| or |accounts_url| is failure, return failure. | ||
1. Wait for both |config| and |discovery| to be set. | ||
1. If |discovery| is null, return failure. | ||
1. If |rpOrigin| is not an [=opaque origin=], and |rootUrl|'s [=url/host=] is equal |
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 was easier to specify that way and Chrome's implementation does fetch the well known file in this circumstance. However, Chrome ignores fetch failures, so you're right that I need to change this. Done.
Bug: #552
Preview | Diff