-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Add a new section detailing the various potential realms #1091
Conversation
@@ -87502,7 +87467,8 @@ interface <dfn>NavigatorOnLine</dfn> { | |||
|
|||
<ol> | |||
|
|||
<li><p>Label <var>settings</var> as a <span>candidate entry settings object</span>.</p></li> | |||
<li><p>Set <var>settings</var>'s <span>realm execution context</span>'s <span>is candidate entry | |||
execution context</span> flag to true.</p></li> |
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.
Either set/unset an "x flag", or set "x" to true/false.
Really glad you are sorting this out, even with examples \o/ @bholley might want to have a look too I think. |
new specifications, and we are considering whether it can be removed from the platform. See <a | ||
href="https://www.w3.org/Bugs/Public/show_bug.cgi?id=26603">Bugzilla bug 26603</a>. Currently, the | ||
<span data-x="concept-incumbent-everything">incumbent</span> concept is used in some security | ||
checks.</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.
Per https://www.w3.org/Bugs/Public/show_bug.cgi?id=26603#c36 I think we're stuck with the incumbent, so it isn't really accurate to suggest that it can be removed. But we should continue to discourage people from using it.
I kinda skimmed, but it looks great in general - thanks for clarifying all this stuff! |
Great! One thing I wasn't entirely clear on is whether entry is a legacy concept or not. Some bugs seem to indicate that we shouldn't use it for origin checks, but that's not necessarily the same as saying we shouldn't use it at all. But in https://www.w3.org/Bugs/Public/show_bug.cgi?id=26603#c41, you say
So I might want to tweak the wording. Our recommendation is to use "current" in most cases, except to use "relevant" in the particular case explained here (i.e. creating an object which is cached)? |
ff5effc
to
8e7cf7e
Compare
Addressed review feedback, and also expanded on the relevant example (with getBattery()). I've assumed that we want to recommend against both entry and incumbent going forward, and tell people to use either current or relevant. I've updated the rendered version so the links in the OP will reflect these latest changes. |
Yeah, we should discourage entry unless there's a very good specific reason. Current and Relevant should suffice in almost all cases. |
<p>Each page has its own <span>browsing context</span>, and thus its own <span>JavaScript | ||
realm</span>, <span>global object</span>, and <span>settings object</span>.</p> | ||
|
||
<p>When the button is pressed in <code data-x="">a.html</code>, then:</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.
I think "When the print
function is called in response to pressing the button in …" would be more accurate.
Thanks, this is great! |
8e7cf7e
to
b907520
Compare
Fixed @Ms2ger's helpful finds. |
<dl> | ||
<dt><dfn data-x="concept-entry-everything">Entry</dfn></dt> | ||
<dd>This corresponds to the script that initiated the currently running script action: i.e., | ||
the script that was on the stack when the user agent called in to author code.</dd> |
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.
Probably better to phrase it as "the function or script that the user agent called into when it called into author code"?
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 and the other descriptions are fairly informal, so your suggestions for making them more precise are appreciated. I'll incorporate them now.
Thank you for writing this down! |
Pushed a commit and updated the live version with @bzbarsky's fixes. Still some discussion going on in #473 (comment), but it might be time to merge this and work on further follow-ups in another PR? What do people think? |
Per offthread discussions with @bzbarsky I need to make it clearer that "relevant" is a predicate that applies to objects, in contrast to current/incumbent/entry which are ambient values. That is, you can have "relevant settings object of X" and "relevant settings object of Y". In most algorithms it's "the relevant settings object of this" that matters. |
ea1d188
to
b24ce6d
Compare
Updated; the last two commits should address @bzbarsky's concerns. |
This adds a new section under scripts detailing the entry, incumbent, current, and relevant concepts with regard to realms/global objects/ environment settings objects. This centralizes information that was previously somewhat spread out, and adds lots of details, as well as an example. This takes care of some of the pain points noted in #473, in particular fixing the definition of entry settings object to be more clearly defined in terms of the JavaScript execution context stack. There is still some work to do in that issue: namely, updating Web IDL with regard to tracking the entry settings object, and also making sure that incumbent settings object is correctly defined. This closes #167 by finally implementing the plan in #167 (comment) for making the correspondences clear.
b24ce6d
to
b06ad64
Compare
@bzbarsky any remaining concerns or can I go ahead and merge this? |
No remaining concerns, I think. |
As discussed in #473, starting especially from around #473 (comment), the definition of incumbent introduced in #401 falls down in certain important cases. In order to fix this, we introduce several new concepts, which takes care of these trickier examples. These examples are now included in the spec, and spell out exactly how exactly incumbent settings object calculation works in increasingly-complex scenarios. The new algorithms "prepare to run a callback" and "clean up after running a callback" will be used by Web IDL, similarly to how it already uses "prepare to run script" and "clean up after running script." Another notable change is that EnqueueJob now correctly tracks the necessary goings-on in order to make the incumbent settings object work correctly when promises are used to schedule callbacks. However, we noticed that it does not correctly track the entry settings object; the previous text, introduced in #1091, incorrectly referred to job.[[Realm]], which does not exist. The correct fix is unfortunately not obvious. So we add a warning there for now, with #1426 tracking further work.
This adds a new section under scripts detailing the entry, incumbent,
current, and relevant concepts with regard to realms/global objects/
environment settings objects. This centralizes information that was
previously somewhat spread out, and adds lots of details, as well as
an example.
This takes care of some of the pain points noted in #473, in particular
fixing the definition of entry settings object to be more clearly
defined in terms of the JavaScript execution context stack. There is
still some work to do in that issue: namely, updating Web IDL with
regard to tracking the entry settings object, and also making sure that
incumbent settings object is correctly defined, which I haven't yet tackled.
This closes #167 by finally implementing the plan in
#167 (comment) for
making the correspondences clear.
/cc @bzbarsky; your review would be much appreciated.
https://dl.dropboxusercontent.com/u/20140634/realms-globals-settings/index.html contains a rendered version. In particular: