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

Disable incremental reflow for multicol and their descendants. #8763

Closed
wants to merge 3 commits into from

Conversation

@SimonSapin
Copy link
Member

SimonSapin commented Dec 1, 2015

Fragmentation with dynamic updates is hard.

r? @pcwalton

Review on Reviewable

@highfive
Copy link

highfive commented Dec 1, 2015

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!
  • These commits modify layout code, but no reftests are modified. Please consider adding a reftest!
@eefriedman
Copy link
Contributor

eefriedman commented Dec 1, 2015

I'm not sure I understand why reconstructing the flows helps here; flow construction isn't sensitive to the width and height of each column, so you'll end up with exactly the same flow tree that you started with if, for example, the viewport is resized. What am I missing?

On a side-note, this PR doesn't build as-is.

@pcwalton
Copy link
Contributor

pcwalton commented Dec 1, 2015

Why not just set RECONSTRUCT_FLOW unconditionally if any MulticolFlow is present? It feels to me like the style system is the wrong place to do this, but maybe I'm missing something.

@nox
Copy link
Member

nox commented Dec 1, 2015

/home/travis/build/servo/servo/components/layout/display_list_builder.rs:1798:22: 1798:57 warning: unnecessary parentheses around `for` head expression, #[warn(unused_parens)] on by default
/home/travis/build/servo/servo/components/layout/display_list_builder.rs:1798         for index in (0..self.fragments.fragments.len()) {
                                                                                                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/travis/build/servo/servo/components/layout/construct.rs:1327:17: 1327:45 error: no method named `in_fragmentation_container` found for type `&ConcreteThreadSafeLayoutNode` in the current scope
/home/travis/build/servo/servo/components/layout/construct.rs:1327         if node.in_fragmentation_container() {
                                                                                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/travis/build/servo/servo/components/layout/css/matching.rs:769:30: 772:26 error: no method named `set_in_fragmentation_container` found for type `&ConcreteLayoutNode` in the current scope
/home/travis/build/servo/servo/components/layout/css/matching.rs:769                         self.set_in_fragmentation_container(
/home/travis/build/servo/servo/components/layout/css/matching.rs:770                             parent.as_ref().map_or(false, |p| p.in_fragmentation_container()) ||
/home/travis/build/servo/servo/components/layout/css/matching.rs:771                             layout_data.shared_data.style.as_ref().unwrap().is_multicol()
/home/travis/build/servo/servo/components/layout/css/matching.rs:772                         );
/home/travis/build/servo/servo/components/layout/css/matching.rs:770:65: 770:93 error: no method named `in_fragmentation_container` found for type `&ConcreteLayoutNode` in the current scope
/home/travis/build/servo/servo/components/layout/css/matching.rs:770                             parent.as_ref().map_or(false, |p| p.in_fragmentation_container()) ||
                                                                                                                                     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
error: aborting due to 3 previous errors
Could not compile `layout`.
@nox nox removed the S-tests-failed label Dec 1, 2015
@SimonSapin SimonSapin force-pushed the no-incremental-multicol branch from 387fe5f to 0c213f6 Dec 2, 2015
@SimonSapin
Copy link
Member Author

SimonSapin commented Dec 2, 2015

Oops, forgot to commit my latest changes. Done now.

@eefriedman the plan is to implement CSS fragmentation by having one flow for each fragment, keeping a tree structure. So one flow from flow construction can be broken up into multiple flows during layout. Without this change (or something like it), fragments would sometimes have to be pieced back together, or content moved from on fragment to another. Always going from unfragmented to fragmented is easier.

@pcwalton I didn’t know about RECONSTRUCT_FLOW, I’ll try that.

@eefriedman
Copy link
Contributor

eefriedman commented Dec 2, 2015

Hmm... okay, that makes some sense. That said, we probably don't want this to kick in if, for example, the color of a link changes.

@SimonSapin
Copy link
Member Author

SimonSapin commented Dec 2, 2015

Right, we can skip layout entirely for style changes that only affect painting. That’s not specific to fragmentation.

@SimonSapin
Copy link
Member Author

SimonSapin commented Dec 2, 2015

Is restyle damage propagated down the tree? By the time construct.rs sees RECONSTRUCT_FLOW on a multicol node, isn’t it too late to decide to reconstruct the descendant flows? (Since flow construction is bottom-up)

@SimonSapin SimonSapin force-pushed the no-incremental-multicol branch from 0c213f6 to 48670c6 Dec 3, 2015
@SimonSapin SimonSapin force-pushed the no-incremental-multicol branch from 48670c6 to 4dcb812 Dec 29, 2015
@SimonSapin
Copy link
Member Author

SimonSapin commented Dec 29, 2015

Pushed another commit to sequentialize assign_block_size, as fragmentation will be intertwined with block size computation.

@bors-servo
Copy link
Contributor

bors-servo commented Dec 30, 2015

The latest upstream changes (presumably #9051) made this pull request unmergeable. Please resolve the merge conflicts.

@SimonSapin SimonSapin force-pushed the no-incremental-multicol branch 2 times, most recently from 15dacf8 to 15167f6 Dec 30, 2015
@bors-servo
Copy link
Contributor

bors-servo commented Jan 4, 2016

The latest upstream changes (presumably #9110) made this pull request unmergeable. Please resolve the merge conflicts.

SimonSapin added 3 commits Sep 18, 2015
Fragmentation with dynamic updates is hard.
Fragmentation will be intertwined with block size calculation.
@SimonSapin
Copy link
Member Author

SimonSapin commented Jan 6, 2016

Closing in favor of #9170.

@SimonSapin SimonSapin closed this Jan 6, 2016
@nox nox deleted the no-incremental-multicol branch Jan 8, 2016
bors-servo added a commit that referenced this pull request Jan 12, 2016
Add CSS Multicolumn support with block fragmentation

![a](https://cloud.githubusercontent.com/assets/291359/12147538/bfb198ac-b499-11e5-9936-c54c93d0b1ed.png)

Includes/supersedes #8763.

r? @mbrubeck

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9170)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this pull request Jan 12, 2016
Add CSS Multicolumn support with block fragmentation

![a](https://cloud.githubusercontent.com/assets/291359/12147538/bfb198ac-b499-11e5-9936-c54c93d0b1ed.png)

Includes/supersedes #8763.

r? @mbrubeck

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9170)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this pull request Jan 13, 2016
Add CSS Multicolumn support with block fragmentation

![a](https://cloud.githubusercontent.com/assets/291359/12147538/bfb198ac-b499-11e5-9936-c54c93d0b1ed.png)

Includes/supersedes #8763.

r? @mbrubeck

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9170)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this pull request Jan 28, 2016
Add CSS Multicolumn support with block fragmentation

![a](https://cloud.githubusercontent.com/assets/291359/12147538/bfb198ac-b499-11e5-9936-c54c93d0b1ed.png)

Includes/supersedes #8763.

r? @mbrubeck

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9170)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this pull request Jan 28, 2016
Add CSS Multicolumn support with block fragmentation

![a](https://cloud.githubusercontent.com/assets/291359/12147538/bfb198ac-b499-11e5-9936-c54c93d0b1ed.png)

Includes/supersedes #8763.

r? @mbrubeck

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9170)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this pull request Jan 29, 2016
Add CSS Multicolumn support with block fragmentation

![a](https://cloud.githubusercontent.com/assets/291359/12147538/bfb198ac-b499-11e5-9936-c54c93d0b1ed.png)

Includes/supersedes #8763.

r? @mbrubeck

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9170)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.