From 7375d09887b75c518dec00528d1d5a5f38d330c4 Mon Sep 17 00:00:00 2001 From: Martin Robinson Date: Tue, 1 Jul 2025 20:21:33 +0200 Subject: [PATCH] layout: Remove `TryFrom for NonReplacedContents` (#37815) This implementation is quite confusing as it makes it harder to tell that we are just looking for the case that `Contents` contains `NonReplacedContents`. Testing: This shouldn't have any functional change, so is covered by existing tests. Signed-off-by: Martin Robinson --- components/layout/dom_traversal.rs | 13 ----- components/layout/flow/construct.rs | 43 ++++++++-------- components/layout/table/construct.rs | 76 ++++++++++++---------------- 3 files changed, 55 insertions(+), 77 deletions(-) diff --git a/components/layout/dom_traversal.rs b/components/layout/dom_traversal.rs index 66d6204209c..0c74315b059 100644 --- a/components/layout/dom_traversal.rs +++ b/components/layout/dom_traversal.rs @@ -376,19 +376,6 @@ impl From for Contents { } } -impl std::convert::TryFrom for NonReplacedContents { - type Error = &'static str; - - fn try_from(contents: Contents) -> Result { - match contents { - Contents::NonReplaced(non_replaced_contents) => Ok(non_replaced_contents), - Contents::Replaced(_) => { - Err("Tried to covnert a `Contents::Replaced` into `NonReplacedContent`") - }, - } - } -} - impl NonReplacedContents { pub(crate) fn traverse<'dom>( self, diff --git a/components/layout/flow/construct.rs b/components/layout/flow/construct.rs index 81511bf6e55..830ab080bf1 100644 --- a/components/layout/flow/construct.rs +++ b/components/layout/flow/construct.rs @@ -3,7 +3,6 @@ * file, You can obtain one at https://mozilla.org/MPL/2.0/. */ use std::borrow::Cow; -use std::convert::TryFrom; use rayon::iter::{IntoParallelIterator, ParallelIterator}; use servo_arc::Arc; @@ -464,23 +463,27 @@ impl<'dom> BlockContainerBuilder<'dom, '_> { contents: Contents, box_slot: BoxSlot<'dom>, ) { - let (DisplayInside::Flow { is_list_item }, false) = - (display_inside, contents.is_replaced()) - else { - // If this inline element is an atomic, handle it and return. - let context = self.context; - let propagated_data = self.propagated_data; - let atomic = self.ensure_inline_formatting_context_builder().push_atomic( - IndependentFormattingContext::construct( - context, - info, - display_inside, - contents, - propagated_data, - ), - ); - box_slot.set(LayoutBox::InlineLevel(vec![atomic])); - return; + let (is_list_item, non_replaced_contents) = match (display_inside, contents) { + ( + DisplayInside::Flow { is_list_item }, + Contents::NonReplaced(non_replaced_contents), + ) => (is_list_item, non_replaced_contents), + (_, contents) => { + // If this inline element is an atomic, handle it and return. + let context = self.context; + let propagated_data = self.propagated_data; + let atomic = self.ensure_inline_formatting_context_builder().push_atomic( + IndependentFormattingContext::construct( + context, + info, + display_inside, + contents, + propagated_data, + ), + ); + box_slot.set(LayoutBox::InlineLevel(vec![atomic])); + return; + }, }; // Otherwise, this is just a normal inline box. Whatever happened before, all we need to do @@ -500,9 +503,7 @@ impl<'dom> BlockContainerBuilder<'dom, '_> { } // `unwrap` doesn’t panic here because `is_replaced` returned `false`. - NonReplacedContents::try_from(contents) - .unwrap() - .traverse(self.context, info, self); + non_replaced_contents.traverse(self.context, info, self); self.finish_anonymous_table_if_needed(); diff --git a/components/layout/table/construct.rs b/components/layout/table/construct.rs index 191faaa77c5..0ef39537e35 100644 --- a/components/layout/table/construct.rs +++ b/components/layout/table/construct.rs @@ -3,7 +3,6 @@ * file, You can obtain one at https://mozilla.org/MPL/2.0/. */ use std::borrow::Cow; -use std::convert::{TryFrom, TryInto}; use std::iter::repeat; use atomic_refcell::AtomicRef; @@ -769,11 +768,10 @@ impl<'dom> TraversalHandler<'dom> for TableBuilderTraversal<'_, 'dom> { let new_row_group_index = self.builder.table.row_groups.len() - 1; self.current_row_group_index = Some(new_row_group_index); - NonReplacedContents::try_from(contents).unwrap().traverse( - self.context, - info, - self, - ); + let Contents::NonReplaced(non_replaced_contents) = contents else { + unreachable!("Replaced should not have a LayoutInternal display type."); + }; + non_replaced_contents.traverse(self.context, info, self); self.finish_anonymous_row_if_needed(); self.current_row_group_index = None; @@ -787,14 +785,13 @@ impl<'dom> TraversalHandler<'dom> for TableBuilderTraversal<'_, 'dom> { self.finish_anonymous_row_if_needed(); let context = self.context; - let mut row_builder = TableRowBuilder::new(self, info, self.current_propagated_data); - NonReplacedContents::try_from(contents).unwrap().traverse( - context, - info, - &mut row_builder, - ); + + let Contents::NonReplaced(non_replaced_contents) = contents else { + unreachable!("Replaced should not have a LayoutInternal display type."); + }; + non_replaced_contents.traverse(context, info, &mut row_builder); row_builder.finish(); let row = ArcRefCell::new(TableTrack { @@ -822,11 +819,10 @@ impl<'dom> TraversalHandler<'dom> for TableBuilderTraversal<'_, 'dom> { columns: Vec::new(), }; - NonReplacedContents::try_from(contents).unwrap().traverse( - self.context, - info, - &mut column_group_builder, - ); + let Contents::NonReplaced(non_replaced_contents) = contents else { + unreachable!("Replaced should not have a LayoutInternal display type."); + }; + non_replaced_contents.traverse(self.context, info, &mut column_group_builder); let first_column = self.builder.table.columns.len(); if column_group_builder.columns.is_empty() { @@ -855,20 +851,17 @@ impl<'dom> TraversalHandler<'dom> for TableBuilderTraversal<'_, 'dom> { ))); }, DisplayLayoutInternal::TableCaption => { - let contents = match contents.try_into() { - Ok(non_replaced_contents) => { - IndependentNonReplacedContents::Flow(BlockFormattingContext::construct( - self.context, - info, - non_replaced_contents, - self.current_propagated_data, - false, /* is_list_item */ - )) - }, - Err(_replaced) => { - unreachable!("Replaced should not have a LayoutInternal display type."); - }, + let Contents::NonReplaced(non_replaced_contents) = contents else { + unreachable!("Replaced should not have a LayoutInternal display type."); }; + let contents = + IndependentNonReplacedContents::Flow(BlockFormattingContext::construct( + self.context, + info, + non_replaced_contents, + self.current_propagated_data, + false, /* is_list_item */ + )); let caption = ArcRefCell::new(TableCaption { context: IndependentFormattingContext { @@ -1016,21 +1009,18 @@ impl<'dom> TraversalHandler<'dom> for TableRowBuilder<'_, '_, 'dom, '_> { let propagated_data = self.propagated_data.disallowing_percentage_table_columns(); - let contents = match contents.try_into() { - Ok(non_replaced_contents) => { - BlockFormattingContext::construct( - self.table_traversal.context, - info, - non_replaced_contents, - propagated_data, - false, /* is_list_item */ - ) - }, - Err(_replaced) => { - unreachable!("Replaced should not have a LayoutInternal display type."); - }, + let Contents::NonReplaced(non_replaced_contents) = contents else { + unreachable!("Replaced should not have a LayoutInternal display type."); }; + let contents = BlockFormattingContext::construct( + self.table_traversal.context, + info, + non_replaced_contents, + propagated_data, + false, /* is_list_item */ + ); + self.finish_current_anonymous_cell_if_needed(); let cell = ArcRefCell::new(TableSlotCell {