layout: Add incremental box tree construction for inline boxes #38084
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
🔨 Triggering try run (#16294456385) for Linux (WPT) |
|
Test results for linux-wpt from try job (#16294456385): Flaky unexpected result (16)
Stable unexpected results that are known to be intermittent (18)
Stable unexpected results (1)
|
|
|
|
🔨 Triggering try run (#16413162149) for Linux (WPT) |
Taym95
reviewed
Jul 21, 2025
| InlineItem::StartInlineBox(inline_box) => Some(inline_box.clone()), | ||
| _ => None, | ||
| }) | ||
| .collect(); |
There was a problem hiding this comment.
.iter().rev().filter_map(...).collect() builds a new vector even if there are no matches
There was a problem hiding this comment.
Yes, it's true. But it should be unlikely.
|
Test results for linux-wpt from try job (#16413162149): Flaky unexpected result (20)
Stable unexpected results that are known to be intermittent (17)
|
|
✨ Try run (#16413162149) succeeded. |
Loirooriol
reviewed
Jul 21, 2025
| block_in_inline_splits: Vec<Vec<ArcRefCell<InlineItem>>>, | ||
|
|
||
| /// If the [`InlineBox`] of an inline-level element is not damaged, it can be reused | ||
| /// to support incremental layout. Since an [`InlineBox`] can be split by block |
There was a problem hiding this comment.
"Since" introduces a subordinated clause, but here there wouldn't be an independent clause owning it.
Suggested change
| /// to support incremental layout. Since an [`InlineBox`] can be split by block | |
| /// to support incremental layout. An [`InlineBox`] can be split by block |
| if let Some(LayoutBox::InlineLevel(inline_level_box)) = old_layout_box { | ||
| let old_block_in_inline_splits: Vec<ArcRefCell<InlineBox>> = inline_level_box | ||
| .iter() | ||
| .rev() // reverse to facilate the `Vec::pop` operation |
There was a problem hiding this comment.
Is this better than using a VecDeque?
There was a problem hiding this comment.
I think it is unnecessary to introduce the new data structure. No push operation is needed for old_block_in_inline_splits.
Comment on lines
285
to
295
| self.push_control_character_string(inline_box.borrow().base.style.bidi_control_chars().0); | ||
|
|
||
| // Don't push a `SharedInlineStyles` if we are pushing this box when splitting | ||
| // an IFC for a block-in-inline split. Shared styles are pushed as part of setting | ||
| // up the second split of the IFC. | ||
| if inline_box.is_first_split { | ||
| if inline_box.borrow().is_first_split { | ||
| self.shared_inline_styles_stack | ||
| .push(inline_box.shared_inline_styles.clone()); | ||
| .push(inline_box.borrow().shared_inline_styles.clone()); | ||
| } |
There was a problem hiding this comment.
Better just borrow once?
Suggested change
| self.push_control_character_string(inline_box.borrow().base.style.bidi_control_chars().0); | |
| // Don't push a `SharedInlineStyles` if we are pushing this box when splitting | |
| // an IFC for a block-in-inline split. Shared styles are pushed as part of setting | |
| // up the second split of the IFC. | |
| if inline_box.is_first_split { | |
| if inline_box.borrow().is_first_split { | |
| self.shared_inline_styles_stack | |
| .push(inline_box.shared_inline_styles.clone()); | |
| .push(inline_box.borrow().shared_inline_styles.clone()); | |
| } | |
| let borrowed_inline_box = inline_box.borrow(); | |
| self.push_control_character_string(borrowed_inline_box.base.style.bidi_control_chars().0); | |
| // Don't push a `SharedInlineStyles` if we are pushing this box when splitting | |
| // an IFC for a block-in-inline split. Shared styles are pushed as part of setting | |
| // up the second split of the IFC. | |
| if borrowed_inline_box.is_first_split { | |
| self.shared_inline_styles_stack | |
| .push(borrowed_inline_box.shared_inline_styles.clone()); | |
| } | |
| std::mem::drop(borrowed_inline_box); |
| izip!(self.inline_box_stack.iter(), block_in_inline_splits) | ||
| { | ||
| let old_block_in_inline_splits = std::mem::take(&mut self.old_block_in_inline_splits); | ||
| for (identifier, historical_inline_boxes, old_inline_boxes) in izip!( |
There was a problem hiding this comment.
I think "historical" and "old" aren't clearly telling these apart.
Signed-off-by: sharpshooter_pt <ibluegalaxy_taoj@163.com>
4bdbdfc to
8e000e4
Compare
Loirooriol
approved these changes
Jul 25, 2025
minghuaw
pushed a commit
to minghuaw/servo
that referenced
this pull request
Aug 1, 2025
…#38084) This changes extend the incremental box tree construction for inline boxes. Since an `InlineItem` can be split into multiple `InlineItem`s by a block element, the reason such an inline item is marked as damaged may simply be the removal of the block element or the need to reconstruct its box tree. Therefore, under the current LayoutDamage design, theoretically, even damaged inline items might still have some of their splits reusable. However, based on the principle of simplicity and effectiveness, this PR only considers reusing undamaged inline boxes. Testing: This should not change observable behavior and is thus covered by existing WPT tests. Signed-off-by: sharpshooter_pt <ibluegalaxy_taoj@163.com>
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.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This changes extend the incremental box tree construction for inline boxes. Since an
InlineItemcan be split into multipleInlineItems by a block element, the reason such an inline item is marked as damaged may simply be the removal of the block element or the need to reconstruct its box tree. Therefore, under the current LayoutDamage design, theoretically, even damaged inline items might still have some of their splits reusable. However, based on the principle of simplicity and effectiveness, this PR only considers reusing undamaged inline boxes.Testing: This should not change observable behavior and is thus covered by existing WPT tests.