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

Editorial: Use settings object definition. #330

Merged
merged 3 commits into from
Apr 14, 2020
Merged

Conversation

inexorabletash
Copy link
Member

@inexorabletash inexorabletash commented Apr 14, 2020

Closes #329

Use the formal definition for environment settings object rather than the imprecise "global scope used to access this".

No normative behavior changes, just being more precise.


Preview | Diff

Copy link

@mkruisselbrink mkruisselbrink left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this correct? As far as I can tell chrome uses the relevant settings object of |this|, not the current settings object (i.e. the ScriptState passed in by our bindings code is the result of calling ScriptState::ForRelevantRealm(info), and the rest of the IDB code then uses that to figure out origin, mojo interface broker, etc).

@inexorabletash
Copy link
Member Author

Ah, you're right, relevant is probably correct.

(Aside: I need to understand why NFS is using a mix of current vs. relevant.)

@mkruisselbrink
Copy link

Relevant realm/settings object needs to be applied to a particular platform object (i.e. this).

(which coincidently is also part of the reason of the mix of current and relevant in NFS, static methods like getSystemDirectory don't have a |this|/platform object, so relevant realm doesn't make sense in that context, hence why that uses the current settings object instead (which also matches what our bindings do for CallWith=ScriptState on static methods). Most of the other cases of current realm in NFS should probably be relevant though, I'll need to fix that...)

@inexorabletash inexorabletash merged commit d1256fe into master Apr 14, 2020
@inexorabletash inexorabletash deleted the envsettings branch July 23, 2020 16:15
inexorabletash added a commit that referenced this pull request Feb 1, 2021
* Use settings object definition.
* Relevant settings object not, current
* relevant to what? this
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.

Make "global scope" references more concrete
2 participants