Skip to content
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

Closed
wants to merge 13 commits into from

Conversation

gtanzer
Copy link
Collaborator

@gtanzer gtanzer commented Apr 3, 2024

@gtanzer gtanzer requested a review from domfarolino April 9, 2024 16:49
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
@gtanzer
Copy link
Collaborator Author

gtanzer commented Apr 11, 2024

Addressed comments % a question about "default" vs "initial" values.

@domfarolino
Copy link
Collaborator

Can you just "resolve" all comment threads that have been resolved at this point?

@domfarolino
Copy link
Collaborator

And resolve the merge conflict that this PR now has with master?

@gtanzer
Copy link
Collaborator Author

gtanzer commented May 4, 2024

@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)
#146

@domfarolino
Copy link
Collaborator

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.

spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
Copy link
Collaborator

@domfarolino domfarolino left a 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|.
Copy link
Collaborator

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.

Copy link
Collaborator

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.

@domfarolino
Copy link
Collaborator

@blu25 I think this can be closed now that #169 has landed, which subsumed this, right?

@blu25
Copy link
Collaborator

blu25 commented Nov 5, 2024

@blu25 I think this can be closed now that #169 has landed, which subsumed this, right?

Yes, this can be closed.

@blu25 blu25 closed this Nov 5, 2024
@blu25 blu25 deleted the disable-untrusted-network branch November 5, 2024 17:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants