-
Notifications
You must be signed in to change notification settings - Fork 131
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
Entry settings object usage is probably wrong #98
Comments
Actually, I don't think there's any need to write tests here, as it's impossible for entry/incumbent/relevant/current to mismatch in whether or not they are a Window object. We can just change these all to relevant-of-context-object, or to current, at our leisure. I guess relevant-of-context-object is the way to go. |
Current seems nicer? But entry somehow seems purer. It would be a little annoying to change this before the Bikeshed conversion though. Do not want to rebase that across. |
Yeah happy to wait until after. |
Since you don't want to export entry from HTML, I'm just going to change this to current. I'm not going to use relevant since then I have to change more text and it doesn't matter. |
We used relevant for fetch and service worker in this situation, since that's what HTML recommends for consistency. |
Fwiw, in terms of implementation Gecko stores the "is an XHR for a window" state during the XHR constructor, by looking at the constructor's current global. But yes, I agree that all of these should be equivalent. |
See whatwg/fullscreen@f9df3ea for a rough outline of the process followed here. This fixes #98 in the process since referencing "entry settings object" was not really feasible. Unfortunately this change also changes some of the IDL formatting due to limitations of Bikeshed. Those are clearly called out through notes.
At least, when I scanned Chrome for usages of the entry settings object, I didn't see anything about XHR in there, and if I recall @bzbarsky did a similar thing for Gecko.
I can try to write up tests for this and submit a patch if my suspicions are confirmed.
The text was updated successfully, but these errors were encountered: