Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign uplayout: Re-enable parallel layout and refactor boxes significantly #2174
+1,504
−1,640
Conversation
highfive
commented
Apr 18, 2014
|
|
hoppipolla-critic-bot
commented
Apr 18, 2014
|
Critic review: https://critic.hoppipolla.co.uk/r/1318 This is an external review system which you may optionally use for the code review of your pull request. In order to help critic track your changes, please do not make in-place history rewrites (e.g. via |
pcwalton
changed the title
|
Added a small commit to fix up a safety issue that I realized we have. |
|
Left a few comments on critic |
|
@SimonSapin It's meant to be "based on top of" #2141, but I'll just close it. |
|
I’ve edited the PR message to add "Closes #2018". Hopefully GitHub will do its magic. |
src/components/main/layout/box_.rs
Outdated
| /// New-line chracter(\n)'s positions(relative, not absolute) | ||
| /// | ||
| /// FIXME(pcwalton): This is very inefficient; remove. |
This comment has been minimized.
This comment has been minimized.
src/components/main/layout/box_.rs
Outdated
| @@ -67,7 +66,7 @@ use url::Url; | |||
| /// content such as images are resized differently from tables, text, or other content. Different | |||
| /// types of boxes may also contain custom data; for example, text boxes contain text. | |||
| /// | |||
| /// FIXME(pcwalton): This can be slimmed down quite a bit. | |||
| /// FIXME(pcwalton): This can be slimmed down some. | |||
This comment has been minimized.
This comment has been minimized.
src/components/main/layout/box_.rs
Outdated
| } else { | ||
| (Au(0), Au(0)) | ||
| }; | ||
|
|
||
| // FIXME(pcwalton): This won't work well for inlines: is this OK? |
This comment has been minimized.
This comment has been minimized.
src/components/main/layout/box_.rs
Outdated
| @@ -825,35 +649,14 @@ impl Box { | |||
| } | |||
|
|
|||
| /// Returns the left offset from margin edge to content edge. | |||
| /// | |||
| /// FIXME(pcwalton): I think this method is pretty bogus, because it won't work for inlines. | |||
This comment has been minimized.
This comment has been minimized.
| // FIXME(pcwalton): This is a bit of an abuse of the logging infrastructure. We | ||
| // should have a real `SERVO_DEBUG` system. | ||
|
|
||
| // FIXME(pcwalton): This is a bit of an abuse of the logging |
This comment has been minimized.
This comment has been minimized.
| map: mut map | ||
| } = mem::replace(boxes, InlineBoxes::new()); | ||
|
|
||
| // FIXME(pcwalton): This is slow because vector shift is broken. :( |
This comment has been minimized.
This comment has been minimized.
src/components/main/layout/flow.rs
Outdated
|
|
||
| /// Propagates text alignment flags from an appropriate parent flow per CSS 2.1. | ||
| /// | ||
| /// FIXME(pcwalton): It would be cleaner and faster to make this a derived CSS property |
This comment has been minimized.
This comment has been minimized.
src/components/main/layout/inline.rs
Outdated
|
|
||
| /// Returns the dimensions of the padding in this fragment range. | ||
| pub fn padding(&self) -> SideOffsets2D<Au> { | ||
| // FIXME(pcwalton): Is Au(0) right here for the containing block? |
This comment has been minimized.
This comment has been minimized.
src/components/main/layout/inline.rs
Outdated
| /// DOM has changed, then the flow constructor will need to do more complicated surgery than | ||
| /// this function can provide. | ||
| /// | ||
| /// FIXME(pcwalton): It would be more efficient to not have to clone boxes all the time; i.e. |
This comment has been minimized.
This comment has been minimized.
src/components/main/layout/inline.rs
Outdated
| let mut old_list_iter = old_list.move_iter().peekable(); | ||
| let mut new_fragments_iter = new_fragments.iter().enumerate().peekable(); | ||
|
|
||
| // FIXME(pcwalton): I don't think this will work if multiple old fragments correspond to |
This comment has been minimized.
This comment has been minimized.
|
re-r? @SimonSapin. I had to fix an issue in border styles. |
|
re-r? @SimonSapin Filed #2288. |
from `Flow`s; in the process, remove `InlineInfo` in favor of the range-based design that was originally planned and halfway implemented. Now, the DOM tree structure for inline flows is reflected not by a series of arrays but instead by a flat list of ranges into the list of boxes. As part of this, the `border` and `padding` fields, which were incorrect in the case of inlines and necessitated separate `noncontent_inline_foo` methods, have been merged into a single `border_padding` field that is always guaranteed to be correct after width assignment, even for inlines.
This comment has been minimized.
This comment has been minimized.
|
r=SimonSapin |
This comment has been minimized.
This comment has been minimized.
|
saw approval from SimonSapin |
This comment has been minimized.
This comment has been minimized.
|
merging pcwalton/servo/reparallelize = 27276c0 into auto |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
all tests pass: |
This comment has been minimized.
This comment has been minimized.
|
fast-forwarding master to auto = 1ab22d9 |
bors-servo
pushed a commit
that referenced
this pull request
May 2, 2014
layout: Re-enable parallel layout by removing all `RefCell` instances from `Flow`s; in the process, remove `InlineInfo` in favor of the range-based design that was originally planned and halfway implemented. Now, the DOM tree structure for inline flows is reflected not by a series of arrays but instead by a flat list of ranges into the list of boxes. As part of this, the `border` and `padding` fields, which were incorrect in the case of inlines and necessitated separate `noncontent_inline_foo` methods, have been merged into a single `border_padding` field that is always guaranteed to be correct after width assignment, even for inlines. r? @SimonSapin and/or @larsbergstrom Closes #1280 Closes #1926 Closes #1999 Closes #2013 Closes #2018
bors-servo
merged commit 27276c0
into
servo:master
1 check passed
1 check passed
default
all tests passed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
pcwalton commentedApr 18, 2014
layout: Re-enable parallel layout by removing all
RefCellinstances fromFlows; in the process, removeInlineInfoin favor of the range-based design that was originally planned and halfway implemented.Now, the DOM tree structure for inline flows is reflected not by a
series of arrays but instead by a flat list of ranges into the list of
boxes. As part of this, the
borderandpaddingfields, which wereincorrect in the case of inlines and necessitated separate
noncontent_inline_foomethods, have been merged into a singleborder_paddingfield that is always guaranteed to be correct afterwidth assignment, even for inlines.
r? @SimonSapin and/or @larsbergstrom
Closes #1280
Closes #1926
Closes #1999
Closes #2013
Closes #2018