Auto merge of #26070 - servo:layout-2020-more-cleanups, r=SimonSapin

Make LayoutNodeHelpers::text_content return a cow

What does it mean? It means that layout can process `Text` nodes' contents by just borrowing them, potentially saving us from a lot of unnecessary tempoorary allocations, which is nice.

components/layout/wrapper.rs

@@ -130,7 +130,7 @@ where
             });
         }
 
-        TextContent::Text(self.node_text_content().into_boxed_str())
+        TextContent::Text(self.node_text_content().into_owned().into_boxed_str())
     }
 
     fn restyle_damage(self) -> RestyleDamage {

components/layout_2020/dom_traversal.rs

@@ -17,6 +17,7 @@ use script_layout_interface::wrapper_traits::{
 };
 use script_layout_interface::HTMLCanvasDataSource;
 use servo_arc::Arc as ServoArc;
+use std::borrow::Cow;
 use std::marker::PhantomData as marker;
 use std::sync::{Arc, Mutex};
 use style::dom::{OpaqueNode, TNode};
@@ -59,7 +60,12 @@ pub(super) trait TraversalHandler<'dom, Node>
 where
     Node: 'dom,
 {
-    fn handle_text(&mut self, node: Node, text: String, parent_style: &ServoArc<ComputedValues>);
+    fn handle_text(
+        &mut self,
+        node: Node,
+        text: Cow<'dom, str>,
+        parent_style: &ServoArc<ComputedValues>,
+    );
 
     /// Or pseudo-element
     fn handle_element(
@@ -166,7 +172,7 @@ fn traverse_pseudo_element_contents<'dom, Node>(
     for item in items {
         match item {
             PseudoElementContentItem::Text(text) => {
-                handler.handle_text(node, text, pseudo_element_style)
+                handler.handle_text(node, text.into(), pseudo_element_style)
             },
             PseudoElementContentItem::Replaced(contents) => {
                 let item_style = anonymous_style.get_or_insert_with(|| {
@@ -351,7 +357,7 @@ impl Drop for BoxSlot<'_> {
 
 pub(crate) trait NodeExt<'dom>: 'dom + Copy + LayoutNode<'dom> + Send + Sync {
     fn is_element(self) -> bool;
-    fn as_text(self) -> Option<String>;
+    fn as_text(self) -> Option<Cow<'dom, str>>;
 
     /// Returns the image if it’s loaded, and its size in image pixels
     /// adjusted for `image_density`.
@@ -378,7 +384,7 @@ where
         self.to_threadsafe().as_element().is_some()
     }
 
-    fn as_text(self) -> Option<String> {
+    fn as_text(self) -> Option<Cow<'dom, str>> {
         if self.is_text_node() {
             Some(self.to_threadsafe().node_text_content())
         } else {

components/layout_2020/flow/construct.rs

@@ -16,6 +16,7 @@ use crate::style_ext::{ComputedValuesExt, DisplayGeneratingBox, DisplayInside, D
 use rayon::iter::{IntoParallelIterator, ParallelIterator};
 use rayon_croissant::ParallelIteratorExt;
 use servo_arc::Arc;
+use std::borrow::Cow;
 use std::convert::{TryFrom, TryInto};
 use style::properties::ComputedValues;
 use style::selector_parser::PseudoElement;
@@ -286,7 +287,12 @@ where
         }
     }
 
-    fn handle_text(&mut self, node: Node, input: String, parent_style: &Arc<ComputedValues>) {
+    fn handle_text(
+        &mut self,
+        node: Node,
+        input: Cow<'dom, str>,
+        parent_style: &Arc<ComputedValues>,
+    ) {
         let (leading_whitespace, mut input) = self.handle_leading_whitespace(&input);
         if leading_whitespace || !input.is_empty() {
             // This text node should be pushed either to the next ongoing

components/layout_thread/dom_wrapper.rs

@@ -67,6 +67,7 @@ use selectors::sink::Push;
 use servo_arc::{Arc, ArcBorrow};
 use servo_atoms::Atom;
 use servo_url::ServoUrl;
+use std::borrow::Cow;
 use std::fmt;
 use std::fmt::Debug;
 use std::hash::{Hash, Hasher};
@@ -828,7 +829,7 @@ impl<'le> ::selectors::Element for ServoLayoutElement<'le> {
             .dom_children()
             .all(|node| match node.script_type_id() {
                 NodeTypeId::Element(..) => false,
-                NodeTypeId::CharacterData(CharacterDataTypeId::Text(TextTypeId::Text)) => unsafe {
+                NodeTypeId::CharacterData(CharacterDataTypeId::Text(TextTypeId::Text)) => {
                     node.node.downcast().unwrap().data_for_layout().is_empty()
                 },
                 _ => true,
@@ -1110,9 +1111,8 @@ impl<'ln> ThreadSafeLayoutNode<'ln> for ServoThreadSafeLayoutNode<'ln> {
         self.node
     }
 
-    fn node_text_content(&self) -> String {
+    fn node_text_content(self) -> Cow<'ln, str> {
-        let this = unsafe { self.get_jsmanaged() };
+        unsafe { self.get_jsmanaged().text_content() }
-        return this.text_content();
     }
 
     fn selection(&self) -> Option<Range<ByteIndex>> {

components/layout_thread_2020/dom_wrapper.rs

@@ -67,6 +67,7 @@ use selectors::sink::Push;
 use servo_arc::{Arc, ArcBorrow};
 use servo_atoms::Atom;
 use servo_url::ServoUrl;
+use std::borrow::Cow;
 use std::fmt;
 use std::fmt::Debug;
 use std::hash::{Hash, Hasher};
@@ -835,7 +836,7 @@ impl<'le> ::selectors::Element for ServoLayoutElement<'le> {
             .dom_children()
             .all(|node| match node.script_type_id() {
                 NodeTypeId::Element(..) => false,
-                NodeTypeId::CharacterData(CharacterDataTypeId::Text(TextTypeId::Text)) => unsafe {
+                NodeTypeId::CharacterData(CharacterDataTypeId::Text(TextTypeId::Text)) => {
                     node.node.downcast().unwrap().data_for_layout().is_empty()
                 },
                 _ => true,
@@ -1117,9 +1118,8 @@ impl<'ln> ThreadSafeLayoutNode<'ln> for ServoThreadSafeLayoutNode<'ln> {
         self.node
     }
 
-    fn node_text_content(&self) -> String {
+    fn node_text_content(self) -> Cow<'ln, str> {
-        let this = unsafe { self.get_jsmanaged() };
+        unsafe { self.get_jsmanaged().text_content() }
-        return this.text_content();
     }
 
     fn selection(&self) -> Option<Range<ByteIndex>> {

components/script/dom/attr.rs

@@ -234,27 +234,27 @@ impl Attr {
 
 #[allow(unsafe_code)]
 pub trait AttrHelpersForLayout<'dom> {
-    unsafe fn value(self) -> &'dom AttrValue;
+    fn value(self) -> &'dom AttrValue;
-    unsafe fn value_ref_forever(self) -> &'dom str;
+    fn as_str(self) -> &'dom str;
-    unsafe fn value_tokens(self) -> Option<&'dom [Atom]>;
+    fn as_tokens(self) -> Option<&'dom [Atom]>;
-    unsafe fn local_name_atom(self) -> LocalName;
+    fn local_name(self) -> &'dom LocalName;
+    fn namespace(self) -> &'dom Namespace;
 }
 
 #[allow(unsafe_code)]
 impl<'dom> AttrHelpersForLayout<'dom> for LayoutDom<'dom, Attr> {
     #[inline]
-    unsafe fn value(self) -> &'dom AttrValue {
+    fn value(self) -> &'dom AttrValue {
-        (*self.unsafe_get()).value.borrow_for_layout()
+        unsafe { self.unsafe_get().value.borrow_for_layout() }
     }
 
     #[inline]
-    unsafe fn value_ref_forever(self) -> &'dom str {
+    fn as_str(self) -> &'dom str {
         &**self.value()
     }
 
     #[inline]
-    unsafe fn value_tokens(self) -> Option<&'dom [Atom]> {
+    fn as_tokens(self) -> Option<&'dom [Atom]> {
-        // This transmute is used to cheat the lifetime restriction.
         match *self.value() {
             AttrValue::TokenList(_, ref tokens) => Some(tokens),
             _ => None,
@@ -262,7 +262,12 @@ impl<'dom> AttrHelpersForLayout<'dom> for LayoutDom<'dom, Attr> {
     }
 
     #[inline]
-    unsafe fn local_name_atom(self) -> LocalName {
+    fn local_name(self) -> &'dom LocalName {
-        (*self.unsafe_get()).identifier.local_name.clone()
+        unsafe { &self.unsafe_get().identifier.local_name }
+    }
+
+    #[inline]
+    fn namespace(self) -> &'dom Namespace {
+        unsafe { &self.unsafe_get().identifier.namespace }
     }
 }

components/script/dom/characterdata.rs

@@ -280,16 +280,15 @@ impl CharacterDataMethods for CharacterData {
     }
 }
 
-#[allow(unsafe_code)]
 pub trait LayoutCharacterDataHelpers<'dom> {
-    unsafe fn data_for_layout(self) -> &'dom str;
+    fn data_for_layout(self) -> &'dom str;
 }
 
-#[allow(unsafe_code)]
 impl<'dom> LayoutCharacterDataHelpers<'dom> for LayoutDom<'dom, CharacterData> {
+    #[allow(unsafe_code)]
     #[inline]
-    unsafe fn data_for_layout(self) -> &'dom str {
+    fn data_for_layout(self) -> &'dom str {
-        &(*self.unsafe_get()).data.borrow_for_layout()
+        unsafe { self.unsafe_get().data.borrow_for_layout() }
     }
 }
 

components/script/dom/element.rs

@@ -578,7 +578,7 @@ pub unsafe fn get_attr_for_layout<'dom>(
         .iter()
         .find(|attr| {
             let attr = attr.to_layout();
-            *name == attr.local_name_atom() && (*attr.unsafe_get()).namespace() == namespace
+            name == attr.local_name() && namespace == attr.namespace()
         })
         .map(|attr| attr.to_layout())
 }
@@ -600,7 +600,7 @@ impl RawLayoutElementHelpers for Element {
         namespace: &Namespace,
         name: &LocalName,
     ) -> Option<&'a str> {
-        get_attr_for_layout(self, namespace, name).map(|attr| attr.value_ref_forever())
+        get_attr_for_layout(self, namespace, name).map(|attr| attr.as_str())
     }
 
     #[inline]
@@ -610,7 +610,7 @@ impl RawLayoutElementHelpers for Element {
             .iter()
             .filter_map(|attr| {
                 let attr = attr.to_layout();
-                if *name == attr.local_name_atom() {
+                if name == attr.local_name() {
                     Some(attr.value())
                 } else {
                     None
@@ -656,7 +656,7 @@ impl<'dom> LayoutElementHelpers<'dom> for LayoutDom<'dom, Element> {
         get_attr_for_layout(&*self.unsafe_get(), &ns!(), &local_name!("class")).map_or(
             false,
             |attr| {
-                attr.value_tokens()
+                attr.as_tokens()
                     .unwrap()
                     .iter()
                     .any(|atom| case_sensitivity.eq_atom(atom, name))
@@ -668,7 +668,7 @@ impl<'dom> LayoutElementHelpers<'dom> for LayoutDom<'dom, Element> {
     #[inline]
     unsafe fn get_classes_for_layout(self) -> Option<&'dom [Atom]> {
         get_attr_for_layout(&*self.unsafe_get(), &ns!(), &local_name!("class"))
-            .map(|attr| attr.value_tokens().unwrap())
+            .map(|attr| attr.as_tokens().unwrap())
     }
 
     #[allow(unsafe_code)]

components/script/dom/htmlinputelement.rs

@@ -67,7 +67,7 @@ use profile_traits::ipc;
 use script_layout_interface::rpc::TextIndexResponse;
 use script_traits::ScriptToConstellationChan;
 use servo_atoms::Atom;
-use std::borrow::ToOwned;
+use std::borrow::Cow;
 use std::cell::Cell;
 use std::ops::Range;
 use std::ptr::NonNull;
@@ -705,9 +705,8 @@ impl HTMLInputElement {
     }
 }
 
-pub trait LayoutHTMLInputElementHelpers {
+pub trait LayoutHTMLInputElementHelpers<'dom> {
-    #[allow(unsafe_code)]
+    fn value_for_layout(self) -> Cow<'dom, str>;
-    unsafe fn value_for_layout(self) -> String;
     #[allow(unsafe_code)]
     unsafe fn size_for_layout(self) -> u32;
     #[allow(unsafe_code)]
@@ -726,41 +725,47 @@ unsafe fn get_raw_textinput_value(input: LayoutDom<HTMLInputElement>) -> DOMStri
         .get_content()
 }
 
-impl LayoutHTMLInputElementHelpers for LayoutDom<'_, HTMLInputElement> {
+impl<'dom> LayoutHTMLInputElementHelpers<'dom> for LayoutDom<'dom, HTMLInputElement> {
     #[allow(unsafe_code)]
-    unsafe fn value_for_layout(self) -> String {
+    fn value_for_layout(self) -> Cow<'dom, str> {
-        #[allow(unsafe_code)]
+        fn get_raw_attr_value<'dom>(
-        unsafe fn get_raw_attr_value(
+            input: LayoutDom<'dom, HTMLInputElement>,
-            input: LayoutDom<'_, HTMLInputElement>,
+            default: &'static str,
-            default: &str,
+        ) -> Cow<'dom, str> {
-        ) -> String {
+            unsafe {
-            let elem = input.upcast::<Element>();
+                input
-            let value = (*elem.unsafe_get())
+                    .upcast::<Element>()
-                .get_attr_val_for_layout(&ns!(), &local_name!("value"))
+                    .unsafe_get()
-                .unwrap_or(default);
+                    .get_attr_val_for_layout(&ns!(), &local_name!("value"))
-            String::from(value)
+                    .unwrap_or(default)
+                    .into()
+            }
         }
 
-        match (*self.unsafe_get()).input_type() {
+        let placeholder = unsafe { &**self.unsafe_get().placeholder.borrow_for_layout() };
-            InputType::Checkbox | InputType::Radio => String::new(),
+        match unsafe { self.unsafe_get().input_type() } {
-            InputType::File | InputType::Image => String::new(),
+            InputType::Checkbox | InputType::Radio => "".into(),
+            InputType::File | InputType::Image => "".into(),
             InputType::Button => get_raw_attr_value(self, ""),
             InputType::Submit => get_raw_attr_value(self, DEFAULT_SUBMIT_VALUE),
             InputType::Reset => get_raw_attr_value(self, DEFAULT_RESET_VALUE),
             InputType::Password => {
-                let text = get_raw_textinput_value(self);
+                let text = unsafe { get_raw_textinput_value(self) };
                 if !text.is_empty() {
-                    text.chars().map(|_| PASSWORD_REPLACEMENT_CHAR).collect()
+                    text.chars()
+                        .map(|_| PASSWORD_REPLACEMENT_CHAR)
+                        .collect::<String>()
+                        .into()
                 } else {
-                    String::from((*self.unsafe_get()).placeholder.borrow_for_layout().clone())
+                    placeholder.into()
                 }
             },
             _ => {
-                let text = get_raw_textinput_value(self);
+                let text = unsafe { get_raw_textinput_value(self) };
                 if !text.is_empty() {
-                    String::from(text)
+                    text.into()
                 } else {
-                    String::from((*self.unsafe_get()).placeholder.borrow_for_layout().clone())
+                    placeholder.into()
                 }
             },
         }

components/script/dom/htmltextareaelement.rs

@@ -56,8 +56,7 @@ pub struct HTMLTextAreaElement {
 }
 
 pub trait LayoutHTMLTextAreaElementHelpers {
-    #[allow(unsafe_code)]
+    fn value_for_layout(self) -> String;
-    unsafe fn value_for_layout(self) -> String;
     #[allow(unsafe_code)]
     unsafe fn selection_for_layout(self) -> Option<Range<usize>>;
     #[allow(unsafe_code)]
@@ -67,19 +66,19 @@ pub trait LayoutHTMLTextAreaElementHelpers {
 }
 
 impl LayoutHTMLTextAreaElementHelpers for LayoutDom<'_, HTMLTextAreaElement> {
-    #[allow(unrooted_must_root)]
     #[allow(unsafe_code)]
-    unsafe fn value_for_layout(self) -> String {
+    fn value_for_layout(self) -> String {
-        let text = (*self.unsafe_get())
+        let text = unsafe {
-            .textinput
+            self.unsafe_get()
-            .borrow_for_layout()
+                .textinput
-            .get_content();
-        if text.is_empty() {
-            (*self.unsafe_get())
-                .placeholder
                 .borrow_for_layout()
-                .replace("\r\n", "\n")
+                .get_content()
-                .replace("\r", "\n")
+        };
+        if text.is_empty() {
+            let placeholder = unsafe { self.unsafe_get().placeholder.borrow_for_layout() };
+            // FIXME(nox): Would be cool to not allocate a new string if the
+            // placeholder is single line, but that's an unimportant detail.
+            placeholder.replace("\r\n", "\n").replace("\r", "\n").into()
         } else {
             text.into()
         }

components/script/dom/node.rs

@@ -87,7 +87,7 @@ use servo_arc::Arc;
 use servo_atoms::Atom;
 use servo_url::ServoUrl;
 use smallvec::SmallVec;
-use std::borrow::ToOwned;
+use std::borrow::Cow;
 use std::cell::{Cell, UnsafeCell};
 use std::cmp;
 use std::default::Default;
@@ -1326,7 +1326,7 @@ pub trait LayoutNodeHelpers<'dom> {
     unsafe fn init_style_and_layout_data(self, _: OpaqueStyleAndLayoutData);
     unsafe fn take_style_and_layout_data(self) -> OpaqueStyleAndLayoutData;
 
-    fn text_content(self) -> String;
+    fn text_content(self) -> Cow<'dom, str>;
     fn selection(self) -> Option<Range<usize>>;
     fn image_url(self) -> Option<ServoUrl>;
     fn image_density(self) -> Option<f64>;
@@ -1456,18 +1456,17 @@ impl<'dom> LayoutNodeHelpers<'dom> for LayoutDom<'dom, Node> {
         val
     }
 
-    #[allow(unsafe_code)]
+    fn text_content(self) -> Cow<'dom, str> {
-    fn text_content(self) -> String {
         if let Some(text) = self.downcast::<Text>() {
-            return unsafe { text.upcast().data_for_layout().to_owned() };
+            return text.upcast().data_for_layout().into();
         }
 
         if let Some(input) = self.downcast::<HTMLInputElement>() {
-            return unsafe { input.value_for_layout() };
+            return input.value_for_layout();
         }
 
         if let Some(area) = self.downcast::<HTMLTextAreaElement>() {
-            return unsafe { area.value_for_layout() };
+            return area.value_for_layout().into();
         }
 
         panic!("not text!")

components/script_layout_interface/wrapper_traits.rs

@@ -17,6 +17,7 @@ use net_traits::image::base::{Image, ImageMetadata};
 use range::Range;
 use servo_arc::Arc;
 use servo_url::ServoUrl;
+use std::borrow::Cow;
 use std::fmt::Debug;
 use std::sync::Arc as StdArc;
 use style::attr::AttrValue;
@@ -262,7 +263,7 @@ pub trait ThreadSafeLayoutNode<'dom>:
     /// data flags, and we have this annoying trait separation between script and layout :-(
     unsafe fn unsafe_get(self) -> Self::ConcreteNode;
 
-    fn node_text_content(&self) -> String;
+    fn node_text_content(self) -> Cow<'dom, str>;
 
     /// If the insertion point is within this node, returns it. Otherwise, returns `None`.
     fn selection(&self) -> Option<Range<ByteIndex>>;