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 incumbent global. #14936

Merged
merged 2 commits into from Jan 17, 2017
Merged

Implement the incumbent global. #14936

merged 2 commits into from Jan 17, 2017

Conversation

@Ms2ger
Copy link
Contributor

Ms2ger commented Jan 10, 2017 •
edited by larsbergstrom

Fixes #10963.

This change is Reviewable
@highfive
Copy link

highfive commented Jan 10, 2017

Heads up! This PR modifies the following files:

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

highfive commented Jan 10, 2017

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!
@nox
nox reviewed Jan 10, 2017
View changes
components/script/dom/bindings/settings_stack.rs
.map(|entry| Root::from_ref(&*entry.global))
}).unwrap()
}

/// RAII struct that pushes and pops entries from the script settings stack.
pub struct AutoIncumbentScript {

This comment has been minimized.

Sign in to view
Copy link
@nox

nox Jan 10, 2017

Member

Shouldn't that be #[must_root]?

This comment has been minimized.

Sign in to view
Copy link
@Ms2ger

Ms2ger Jan 10, 2017

Author Contributor

There might be a better way to write it; the important thing is that we never dereference the field, we just compare / print the address.

This comment has been minimized.

Sign in to view
Copy link
@jdm

jdm Jan 10, 2017

Member

Store it as a usize, in that case?
@Ms2ger Ms2ger force-pushed the incumbent-global branch from 728004e to 4247a42 Jan 10, 2017
@jdm
jdm reviewed Jan 12, 2017
View changes
Copy link
Member

jdm left a comment

This implementation makes sense to me. A bit more documentation wouldn't go amiss, however!
components/script/dom/bindings/settings_stack.rs
}

impl Drop for AutoIncumbentScript {
/// https://html.spec.whatwg.org/multipage/#clean-up-after-running-a-callback

This comment has been minimized.

Sign in to view
Copy link
@jdm

jdm Jan 12, 2017

Member

It would be nice to have a comment that explains the relationship of the code to the spec text.
components/script/dom/bindings/settings_stack.rs
}

impl AutoIncumbentScript {
/// https://html.spec.whatwg.org/multipage/#prepare-to-run-a-callback

This comment has been minimized.

Sign in to view
Copy link
@jdm

jdm Jan 12, 2017

Member

It would be nice to have a comment that explains the relationship of the code to the spec text.
@Ms2ger Ms2ger force-pushed the incumbent-global branch from 4247a42 to 107ebf9 Jan 13, 2017
@Ms2ger
Copy link
Contributor Author

Ms2ger commented Jan 13, 2017

I've added a few step comments; let me know if that's sufficient.
@jdm
Copy link
Member

jdm commented Jan 13, 2017

@bors-servo: r+
@bors-servo
Copy link
Contributor

bors-servo commented Jan 13, 2017

📌 Commit 107ebf9 has been approved by jdm
@highfive highfive assigned jdm and unassigned wafflespeanut Jan 13, 2017
@bors-servo
Copy link
Contributor

bors-servo commented Jan 13, 2017

⌛ Testing commit 107ebf9 with merge a09a21c...
bors-servo added a commit that referenced this pull request Jan 13, 2017
@bors-servo
Auto merge of #14936 - servo:incumbent-global, r=jdm
Loading status checks…
a09a21c 
Implement the incumbent global.

Fixes #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/14936)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Jan 16, 2017

☔ The latest upstream changes (presumably #14994) made this pull request unmergeable. Please resolve the merge conflicts.
@jdm
Copy link
Member

jdm commented Jan 16, 2017

Grumble. Rebase and we'll try to merge it again!
@Ms2ger
Use usize for AutoEntryScript::global.
933b747
@Ms2ger Ms2ger force-pushed the incumbent-global branch from 107ebf9 to 0a30dc7 Jan 17, 2017
@Ms2ger
Copy link
Contributor Author

Ms2ger commented Jan 17, 2017

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

bors-servo commented Jan 17, 2017

📌 Commit 0a30dc7 has been approved by jdm
@Ms2ger Ms2ger dismissed jdm’s stale review Jan 17, 2017

Addressed

@bors-servo
Copy link
Contributor

bors-servo commented Jan 17, 2017

⌛ Testing commit 0a30dc7 with merge 8ab93e9...
bors-servo added a commit that referenced this pull request Jan 17, 2017
@bors-servo
Auto merge of #14936 - servo:incumbent-global, r=jdm
Loading status checks…
8ab93e9 
Implement the incumbent global.

Fixes #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/14936)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Jan 17, 2017

💔 Test failed - linux-dev
@Ms2ger
Implement the incumbent global.
Loading status checks…
51df04d 
Fixes #10963.
@Ms2ger Ms2ger force-pushed the incumbent-global branch from 0a30dc7 to 51df04d Jan 17, 2017
@Ms2ger
Copy link
Contributor Author

Ms2ger commented Jan 17, 2017

@bors-servo r=jdm

Tidy failed.
@bors-servo
Copy link
Contributor

bors-servo commented Jan 17, 2017

📌 Commit 51df04d has been approved by jdm
@bors-servo
Copy link
Contributor

bors-servo commented Jan 17, 2017

⌛ Testing commit 51df04d with merge 463a8bb...
bors-servo added a commit that referenced this pull request Jan 17, 2017
@bors-servo
Auto merge of #14936 - servo:incumbent-global, r=jdm
Loading status checks…
463a8bb 
Implement the incumbent global.

Fixes #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/14936)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Jan 17, 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 51df04d into master Jan 17, 2017
2 of 4 checks passed
2 of 4 checks passed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
dependency-ci Dependencies checked
Details
homu Test successful
Details
@Ms2ger Ms2ger deleted the incumbent-global branch Jan 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nox nox

@jdm jdm

Assignees

@jdm jdm

Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
@Ms2ger @highfive @jdm @bors-servo @nox @wafflespeanut
You can’t perform that action at this time.