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

script: Move the layout_wrapper outside of script. #17744

Merged
merged 1 commit into from Jul 15, 2017

Conversation

emilio
Copy link
Member

@emilio emilio commented Jul 15, 2017

This allows us to have ensure_data() and clear_data() functions on the TElement
trait, instead of hacking around it adding methods in random traits.

This also allows us to do some further cleanup, which I'd rather do in a
followup.


This change is Reviewable

@highfive
Copy link

Heads up! This PR modifies the following files:

  • @bholley: components/style/dom.rs, components/style/gecko/traversal.rs, components/style/gecko/wrapper.rs, components/style/traversal.rs
  • @canaltinova: components/style/dom.rs, components/style/gecko/traversal.rs, components/style/gecko/wrapper.rs, components/style/traversal.rs
  • @fitzgen: components/script/lib.rs, components/script/Cargo.toml, components/script/test.rs, components/script_layout_interface/wrapper_traits.rs, components/script/layout_wrapper.rs
  • @KiChjang: components/script/lib.rs, components/script/Cargo.toml, components/script/test.rs, components/script_layout_interface/wrapper_traits.rs, components/script/layout_wrapper.rs

@highfive
Copy link

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!
  • These commits modify style, layout, and script code, but no tests are modified. Please consider adding a test!

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Jul 15, 2017
@emilio
Copy link
Member Author

emilio commented Jul 15, 2017

r? @bholley for the layout data bits, @nox for the script bits.

@highfive highfive assigned bholley and unassigned cbrewster Jul 15, 2017
@emilio
Copy link
Member Author

emilio commented Jul 15, 2017

@bors-servo try

@bors-servo
Copy link
Contributor

⌛ Trying commit 9a933ef with merge 48ddb09...

bors-servo pushed a commit that referenced this pull request Jul 15, 2017
script: Move the layout_wrapper outside of script.

This allows us to have ensure_data() and clear_data() functions on the TElement
trait, instead of hacking around it adding methods in random traits.

This also allows us to do some further cleanup, which I'd rather do in a
followup.

<!-- 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/17744)
<!-- Reviewable:end -->
@emilio
Copy link
Member Author

emilio commented Jul 15, 2017

@bors-servo r=nox

  • Nox said this looked good to him, and the layout data side of things are just moving code around and the removal of the DomTraversal::ensure_element_data/clear_element_data, which were a hack.

@bors-servo
Copy link
Contributor

📌 Commit 9a933ef has been approved by nox

@highfive highfive assigned nox and unassigned bholley Jul 15, 2017
@highfive highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Jul 15, 2017
@bors-servo
Copy link
Contributor

⌛ Testing commit 9a933ef with merge 9b10aa89f5460919d8386077ffd55ad8381b9d44...

@bors-servo
Copy link
Contributor

💔 Test failed - mac-dev-unit

@highfive highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Jul 15, 2017
This allows us to have ensure_data() and clear_data() functions on the TElement
trait, instead of hacking around it adding methods in random traits.

This also allows us to do some further cleanup, which I'd rather do in a
followup.
@highfive highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-tests-failed The changes caused existing tests to fail. labels Jul 15, 2017
@emilio
Copy link
Member Author

emilio commented Jul 15, 2017

@bors-servo r=nox

  • Unit tests

@bors-servo
Copy link
Contributor

📌 Commit bf9369b has been approved by nox

@highfive highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Jul 15, 2017
@bors-servo
Copy link
Contributor

⌛ Testing commit bf9369b with merge 3497bbb...

bors-servo pushed a commit that referenced this pull request Jul 15, 2017
script: Move the layout_wrapper outside of script.

This allows us to have ensure_data() and clear_data() functions on the TElement
trait, instead of hacking around it adding methods in random traits.

This also allows us to do some further cleanup, which I'd rather do in a
followup.

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

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
Approved by: nox
Pushing 3497bbb to master...

@bors-servo bors-servo merged commit bf9369b into servo:master Jul 15, 2017
@highfive highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Jul 15, 2017
@bholley
Copy link
Contributor

bholley commented Jul 15, 2017

This is great. It also means that the servo-side fixup for style crate changes is more likely to happen in layout_thread, which has a much faster iteration time than script. Thanks emilio!

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.

None yet

6 participants