-
Notifications
You must be signed in to change notification settings - Fork 32
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
Spec disableUntrustedNetwork API surface #146
Conversation
Addressed comments % a question about "default" vs "initial" values. |
Can you just "resolve" all comment threads that have been resolved at this point? |
And resolve the merge conflict that this PR now has with |
@domfarolino Fixed merge conflicts. The open threads aren't resolved per se. They depend on your response here: (whether I should commit the suggestions or do nothing) |
I don't care which word we use "default" vs "initial". I just want it to be consistent, and this PR makes it inconsistent with https://github.com/WICG/fenced-frame/pull/146/files#diff-6f5a1d8263b0b0c42e2716ba5750e3652e359532647ac934c1c70086ae3ceddaR1213. So I'd either make this PR consistent with https://github.com/WICG/fenced-frame/pull/146/files#diff-6f5a1d8263b0b0c42e2716ba5750e3652e359532647ac934c1c70086ae3ceddaR1213 or make that line consistent with this PR. Whichever word you prefer. |
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.
LGTM % small comment about null fenced frame config instance
1. Let |instance| be [=this=]'s [=relevant global object=]'s [=Window/browsing context=]'s | ||
[=browsing context/fenced frame config instance=]. | ||
|
||
1. If |instance| is null, then [=resolve=] |p| with {{undefined}} and return |p|. |
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 I am so sorry but I feel like I've asked when this can be null before, and I cannot for the life of me find the previous comments with the answer to this. This method is only accessible when instance is non-null, right? https://wicg.github.io/fenced-frame/#dom-window-fence. So is this condition needed? /cc @blu25 who might also know.
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.
Here's the discussion from my PR about the same line: #169 (comment)
I think all of these should be fine to remove (or assert if you feel strongly about that) since a condition for getting access to window.fence
is that the browsing context must have a fenced frame config instance.
Preview | Diff