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

stylo: Rearrange some data structures in preparation for the new incremental restyle algorithm #13863

Merged
merged 1 commit into from Oct 21, 2016
Merged

stylo: Rearrange some data structures in preparation for the new incremental restyle algorithm #13863

merged 1 commit into from Oct 21, 2016
+330 −216

Conversation

@bholley
Copy link
Contributor

bholley commented Oct 21, 2016 •
edited by larsbergstrom

This change is Reviewable
@highfive
Copy link

highfive commented Oct 21, 2016

Heads up! This PR modifies the following files:

  • @KiChjang: components/script_layout_interface/wrapper_traits.rs, components/script/layout_wrapper.rs, components/script_layout_interface/lib.rs, components/script/script_thread.rs
  • @fitzgen: components/script_layout_interface/wrapper_traits.rs, components/script/layout_wrapper.rs, components/script_layout_interface/lib.rs, components/script/script_thread.rs
  • @emilio: components/style/gecko/wrapper.rs, components/layout/traversal.rs, ports/geckolib/glue.rs, components/style/gecko_bindings/structs_release.rs, components/style/gecko/traversal.rs, components/style/data.rs, components/style/gecko_bindings/structs_debug.rs, components/style/dom.rs, components/style/binding_tools/regen.py, components/layout/wrapper.rs, components/style/traversal.rs, components/style/matching.rs
@highfive
Copy link

highfive commented Oct 21, 2016

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!
@bholley
Copy link
Contributor Author

bholley commented Oct 21, 2016

@bors-servo try
@bors-servo
Copy link
Contributor

bors-servo commented Oct 21, 2016

🔒 Merge conflict
@bholley bholley force-pushed the bholley:shuffle_data_structures branch from 8fe5efb to 3001bda Oct 21, 2016
@bholley
Copy link
Contributor Author

bholley commented Oct 21, 2016

@bors-servo try
@bors-servo
Copy link
Contributor

bors-servo commented Oct 21, 2016

⌛ Trying commit 3001bda with merge 54fec86...
bors-servo added a commit that referenced this pull request Oct 21, 2016
@bors-servo
Auto merge of #13863 - bholley:shuffle_data_structures, r=<try>
Loading status checks…
54fec86 
stylo: Rearrange some data structures in preparation for the new incremental restyle algorithm

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

bors-servo commented Oct 21, 2016

💔 Test failed - linux-rel-css
@bholley bholley force-pushed the bholley:shuffle_data_structures branch from 3001bda to c3d5214 Oct 21, 2016
@bholley
Copy link
Contributor Author

bholley commented Oct 21, 2016

@bors-servo try
bors-servo added a commit that referenced this pull request Oct 21, 2016
@bors-servo
Auto merge of #13863 - bholley:shuffle_data_structures, r=<try>
Loading status checks…
e8d1726 
stylo: Rearrange some data structures in preparation for the new incremental restyle algorithm

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

bors-servo commented Oct 21, 2016

⌛ Trying commit c3d5214 with merge e8d1726...
@bors-servo
Copy link
Contributor

bors-servo commented Oct 21, 2016

☀️ Test successful - arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-dev
@bholley bholley force-pushed the bholley:shuffle_data_structures branch from c3d5214 to fa54cd7 Oct 21, 2016
@bholley
Copy link
Contributor Author

bholley commented Oct 21, 2016

r? @emilio
@bors-servo
Copy link
Contributor

bors-servo commented Oct 21, 2016

☔ The latest upstream changes (presumably #13848) made this pull request unmergeable. Please resolve the merge conflicts.
@emilio
emilio reviewed Oct 21, 2016
View changes
Copy link
Member

emilio left a comment •
edited

A few comments, the rest looks ok, but I'd like to review it again once the comments are addressed and this is rebased.
components/style/data.rs Outdated
};
}

pub fn ensure_restyle_data(&mut self) {

This comment has been minimized.

Sign in to view
@emilio

emilio Oct 21, 2016

Member

This doesn't seem to be called. It's fine if it's intentional, but if not should be removed.
components/style/data.rs Outdated
/// immutable, but for now we need to mutate it a bit before styling to
/// handle animations.
EmptyPrevious,
Previous(NodeStyles),

This comment has been minimized.

Sign in to view
@emilio

emilio Oct 21, 2016

Member

I think size_of::<NodeStyles>() should be equal to size_of::<Option<NodeStyles>>, since it contains an arc, and the discriminant of the option becomes the nullness of the Arc: https://play.rust-lang.org/?gist=bc261480736b63367ed39cc9c4feee82&version=stable&backtrace=0

So I think if it's clearer we should just use Option<NodeStyles>. Otherwise remove the comment about packing.
components/style/data.rs
}
}

pub fn gather_previous_styles<F>(&mut self, f: F)

This comment has been minimized.

Sign in to view
@emilio

emilio Oct 21, 2016

Member

I think to stick to the convention that Option has, we should call this take_previous_styles. I think the fact that the closure is only executed if it's Uninitialized is not documented nowhere.

Probably the more rusty name is something like take_previous_styles_or_else.

Also, if we change to Option<NodeStyles> instead of EmptyPrevious, this function would be way more idiomatic if you ask me.

This comment has been minimized.

Sign in to view
@bholley

bholley Oct 21, 2016

Author Contributor

Fixed
components/style/data.rs
impl RestyleData {
fn new() -> Self {
RestyleData {
_dummy: 42,

This comment has been minimized.

Sign in to view
@emilio

emilio Oct 21, 2016

Member

heh :)
@bholley bholley force-pushed the bholley:shuffle_data_structures branch from fa54cd7 to 273bd4e Oct 21, 2016
@bholley
Copy link
Contributor Author

bholley commented Oct 21, 2016

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

bors-servo commented Oct 21, 2016

📌 Commit 273bd4e has been approved by emilio
@bors-servo
Copy link
Contributor

bors-servo commented Oct 21, 2016

🔒 Merge conflict
@jdm jdm removed the S-awaiting-review label Oct 21, 2016
@bholley bholley force-pushed the bholley:shuffle_data_structures branch from 273bd4e to ae7efd4 Oct 21, 2016
@bholley
Copy link
Contributor Author

bholley commented Oct 21, 2016

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

bors-servo commented Oct 21, 2016

📌 Commit ae7efd4 has been approved by emilio
@bors-servo
Copy link
Contributor

bors-servo commented Oct 21, 2016

⌛ Testing commit ae7efd4 with merge 1fdf643...
bors-servo added a commit that referenced this pull request Oct 21, 2016
@bors-servo
Auto merge of #13863 - bholley:shuffle_data_structures, r=emilio
Loading status checks…
1fdf643 
stylo: Rearrange some data structures in preparation for the new incremental restyle algorithm

<!-- 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/13863)
<!-- Reviewable:end -->
@bholley bholley removed the S-needs-rebase label Oct 21, 2016
@bors-servo
Copy link
Contributor

bors-servo commented Oct 21, 2016

💔 Test failed - windows-dev
@bholley bholley force-pushed the bholley:shuffle_data_structures branch from ae7efd4 to 6e11e35 Oct 21, 2016
@bholley
Rearrange some data structures in preparation for the new incremental…
Loading status checks…
adf0fe9 
… restyle algorithm.

MozReview-Commit-ID: 8iOALQylOuK
@bholley bholley force-pushed the bholley:shuffle_data_structures branch from 6e11e35 to adf0fe9 Oct 21, 2016
@bholley
Copy link
Contributor Author

bholley commented Oct 21, 2016

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

bors-servo commented Oct 21, 2016

📌 Commit adf0fe9 has been approved by emilio
@bors-servo
Copy link
Contributor

bors-servo commented Oct 21, 2016

⌛ Testing commit adf0fe9 with merge 9cbac40...
bors-servo added a commit that referenced this pull request Oct 21, 2016
@bors-servo
Auto merge of #13863 - bholley:shuffle_data_structures, r=emilio
Loading status checks…
9cbac40 
stylo: Rearrange some data structures in preparation for the new incremental restyle algorithm

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

bors-servo commented Oct 21, 2016

💔 Test failed - mac-rel-css
@bholley bholley mentioned this pull request Oct 21, 2016
Closed
@bholley
Copy link
Contributor Author

bholley commented Oct 21, 2016

@bors-servo retry #13887
@bors-servo
Copy link
Contributor

bors-servo commented Oct 21, 2016

⚡ Previous build results for arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-wpt1, mac-rel-wpt2, windows-dev are reusable. Rebuilding only mac-rel-css...
@bholley bholley dismissed emilio’s stale review Oct 21, 2016

addressed.

@bors-servo
Copy link
Contributor

bors-servo commented Oct 21, 2016

☀️ Test successful - arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-dev
@bors-servo bors-servo merged commit adf0fe9 into servo:master Oct 21, 2016
2 of 3 checks passed
2 of 3 checks passed
continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details
@bors-servo bors-servo mentioned this pull request Oct 21, 2016
Merged
3 of 3 tasks complete
@bholley bholley deleted the bholley:shuffle_data_structures branch Oct 30, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emilio emilio

Assignees

@emilio emilio

Labels
S-tests-failed
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
@bholley @highfive @bors-servo @emilio @jdm @glennw
You can’t perform that action at this time.