Skip to content

/ servo

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

Implement the entry global. #14840

Merged
merged 1 commit into from Jan 6, 2017
Merged

Implement the entry global. #14840

merged 1 commit into from Jan 6, 2017

Conversation

@Ms2ger
Copy link
Contributor

Ms2ger commented Jan 4, 2017 •
edited by larsbergstrom

Partial fix for #10963.

This change is Reviewable
@highfive
Copy link

highfive commented Jan 4, 2017

Heads up! This PR modifies the following files:

  • @fitzgen: components/script/dom/testbinding.rs, components/script/dom/bindings/mod.rs, components/script/dom/globalscope.rs, components/script/dom/bindings/callback.rs, components/script/dom/bindings/settings_stack.rs, components/script/dom/dedicatedworkerglobalscope.rs, components/script/dom/webidls/TestBinding.webidl, components/script/script_runtime.rs, components/script/dom/serviceworkerglobalscope.rs, components/script/timers.rs
  • @KiChjang: components/script/dom/testbinding.rs, components/script/dom/bindings/mod.rs, components/script/dom/globalscope.rs, components/script/dom/bindings/callback.rs, components/script/dom/bindings/settings_stack.rs, components/script/dom/dedicatedworkerglobalscope.rs, components/script/dom/webidls/TestBinding.webidl, components/script/script_runtime.rs, components/script/dom/serviceworkerglobalscope.rs, components/script/timers.rs
@highfive
Copy link

highfive commented Jan 4, 2017

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!
@Ms2ger
Copy link
Contributor Author

Ms2ger commented Jan 5, 2017

r? @nox
@highfive highfive assigned nox and unassigned mbrubeck Jan 5, 2017
@nox
nox reviewed Jan 5, 2017
View changes
Copy link
Member

nox left a comment

The code looks ok but I have many questions.
components/script/dom/globalscope.rs Outdated
/// Returns the global scope from a JS global object.
#[allow(unsafe_code)]
pub unsafe fn from_global(global: *mut JSObject) -> Root<Self> {
global_scope_from_global(global)

This comment has been minimized.

Sign in to view
Copy link
@nox

nox Jan 5, 2017

Member

Could you inline global_scope_from_global here and remove it?
components/script/dom/bindings/settings_stack.rs Outdated
fn drop(&mut self) {
let entry = STACK.with(|stack| {
let mut stack = stack.borrow_mut();
stack.pop().unwrap()

This comment has been minimized.

Sign in to view
Copy link
@nox

nox Jan 5, 2017

Member

AutoEntryScript can move so these can be dropped out of order, I'm pretty sure that's not intended. Seems like we want something similar to rooted!, which would make the whole TLS stack useless AFAICT?
components/script/dom/bindings/settings_stack.rs
/// Returns the ["entry"] global object.
///
/// ["entry"]: https://html.spec.whatwg.org/multipage/#entry
pub fn entry_global() -> Root<GlobalScope> {

This comment has been minimized.

Sign in to view
Copy link
@nox

nox Jan 5, 2017

Member

Nit: could you make that a static method on AutoEntryScript? Less things to import that way.
components/script/dom/dedicatedworkerglobalscope.rs Outdated
@@ -231,6 +232,7 @@ impl DedicatedWorkerGlobalScope {

{
let _ar = AutoWorkerReset::new(&global, worker);
let _aes = AutoEntryScript::new(global.upcast());

This comment has been minimized.

Sign in to view
Copy link
@nox

nox Jan 5, 2017

Member

Instead of manual RAII, can't we just call that in execute_script? Seems like just before it is the only call site you changed, no?
@Ms2ger Ms2ger force-pushed the entry-global branch 7 times, most recently from 87a1c8e to 0890a6e Jan 5, 2017
@Ms2ger
Copy link
Contributor Author

Ms2ger commented Jan 5, 2017

This is good to go as far as I'm concerned.

CC @jdm
@Ms2ger
Copy link
Contributor Author

Ms2ger commented Jan 6, 2017

@bors-servo r=nox
@bors-servo
Copy link
Contributor

bors-servo commented Jan 6, 2017

📌 Commit 0890a6e has been approved by nox
@bors-servo
Copy link
Contributor

bors-servo commented Jan 6, 2017

⌛ Testing commit 0890a6e with merge 553735a...
bors-servo added a commit that referenced this pull request Jan 6, 2017
@bors-servo
Auto merge of #14840 - servo:entry-global, r=nox
Loading status checks…
553735a 
Implement the entry global.

Partial fix for #10963.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14840)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Jan 6, 2017

💔 Test failed - linux-dev
@Ms2ger
Implement the entry global.
Loading status checks…
cb47a7e 
Partial fix for #10963.
@Ms2ger Ms2ger force-pushed the entry-global branch from 0890a6e to cb47a7e Jan 6, 2017
@Ms2ger
Copy link
Contributor Author

Ms2ger commented Jan 6, 2017

@bors-servo r=nox
@bors-servo
Copy link
Contributor

bors-servo commented Jan 6, 2017

📌 Commit cb47a7e has been approved by nox
@bors-servo
Copy link
Contributor

bors-servo commented Jan 6, 2017

⌛ Testing commit cb47a7e with merge 85d3bbd...
bors-servo added a commit that referenced this pull request Jan 6, 2017
@bors-servo
Auto merge of #14840 - servo:entry-global, r=nox
Loading status checks…
85d3bbd 
Implement the entry global.

Partial fix for #10963.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14840)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Jan 6, 2017

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-gnu-dev, windows-msvc-dev
@bors-servo bors-servo merged commit cb47a7e into master Jan 6, 2017
3 of 4 checks passed
3 of 4 checks passed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
dependency-ci Dependencies checked
Details
homu Test successful
Details
@bors-servo bors-servo mentioned this pull request Jan 6, 2017
Closed
1 of 5 tasks complete
@Ms2ger Ms2ger deleted the entry-global branch Jan 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nox nox

Assignees

@nox nox

Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
@Ms2ger @highfive @bors-servo @nox @mbrubeck
You can’t perform that action at this time.