From 4c16729a01999978326e21f9b2e69de764938792 Mon Sep 17 00:00:00 2001 From: Anthony Ramine Date: Tue, 31 Mar 2020 13:54:52 +0200 Subject: Make AttrHelpersForLayout methods be safe The unsafety isn't there, it's in the creation of the LayoutDom values. --- components/script/dom/attr.rs | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) (limited to 'components/script/dom') diff --git a/components/script/dom/attr.rs b/components/script/dom/attr.rs index cea5df2abf5..b8633619bbb 100644 --- a/components/script/dom/attr.rs +++ b/components/script/dom/attr.rs @@ -234,26 +234,26 @@ impl Attr { #[allow(unsafe_code)] pub trait AttrHelpersForLayout<'dom> { - unsafe fn value(self) -> &'dom AttrValue; - unsafe fn value_ref_forever(self) -> &'dom str; - unsafe fn value_tokens(self) -> Option<&'dom [Atom]>; - unsafe fn local_name_atom(self) -> LocalName; + fn value(self) -> &'dom AttrValue; + fn value_ref_forever(self) -> &'dom str; + fn value_tokens(self) -> Option<&'dom [Atom]>; + fn local_name_atom(self) -> LocalName; } #[allow(unsafe_code)] impl<'dom> AttrHelpersForLayout<'dom> for LayoutDom<'dom, Attr> { #[inline] - unsafe fn value(self) -> &'dom AttrValue { - (*self.unsafe_get()).value.borrow_for_layout() + fn value(self) -> &'dom AttrValue { + unsafe { self.unsafe_get().value.borrow_for_layout() } } #[inline] - unsafe fn value_ref_forever(self) -> &'dom str { + fn value_ref_forever(self) -> &'dom str { &**self.value() } #[inline] - unsafe fn value_tokens(self) -> Option<&'dom [Atom]> { + fn value_tokens(self) -> Option<&'dom [Atom]> { // This transmute is used to cheat the lifetime restriction. match *self.value() { AttrValue::TokenList(_, ref tokens) => Some(tokens), @@ -262,7 +262,7 @@ impl<'dom> AttrHelpersForLayout<'dom> for LayoutDom<'dom, Attr> { } #[inline] - unsafe fn local_name_atom(self) -> LocalName { - (*self.unsafe_get()).identifier.local_name.clone() + fn local_name_atom(self) -> LocalName { + unsafe { self.unsafe_get().identifier.local_name.clone() } } } -- cgit v1.2.3 From 4db84bede8ec9330568c01fcf243a0ace0cef18b Mon Sep 17 00:00:00 2001 From: Anthony Ramine Date: Tue, 31 Mar 2020 13:56:19 +0200 Subject: Rename AttrHelpersForLayout methods --- components/script/dom/attr.rs | 12 ++++++------ components/script/dom/element.rs | 10 +++++----- 2 files changed, 11 insertions(+), 11 deletions(-) (limited to 'components/script/dom') diff --git a/components/script/dom/attr.rs b/components/script/dom/attr.rs index b8633619bbb..b641fa5d615 100644 --- a/components/script/dom/attr.rs +++ b/components/script/dom/attr.rs @@ -235,9 +235,9 @@ impl Attr { #[allow(unsafe_code)] pub trait AttrHelpersForLayout<'dom> { fn value(self) -> &'dom AttrValue; - fn value_ref_forever(self) -> &'dom str; - fn value_tokens(self) -> Option<&'dom [Atom]>; - fn local_name_atom(self) -> LocalName; + fn as_str(self) -> &'dom str; + fn as_tokens(self) -> Option<&'dom [Atom]>; + fn local_name(self) -> LocalName; } #[allow(unsafe_code)] @@ -248,12 +248,12 @@ impl<'dom> AttrHelpersForLayout<'dom> for LayoutDom<'dom, Attr> { } #[inline] - fn value_ref_forever(self) -> &'dom str { + fn as_str(self) -> &'dom str { &**self.value() } #[inline] - 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), @@ -262,7 +262,7 @@ impl<'dom> AttrHelpersForLayout<'dom> for LayoutDom<'dom, Attr> { } #[inline] - fn local_name_atom(self) -> LocalName { + fn local_name(self) -> LocalName { unsafe { self.unsafe_get().identifier.local_name.clone() } } } diff --git a/components/script/dom/element.rs b/components/script/dom/element.rs index 602cb148d7a..35f952a676c 100644 --- a/components/script/dom/element.rs +++ b/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() && (*attr.unsafe_get()).namespace() == 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)] -- cgit v1.2.3 From 47c9f1912150b41695810f6c987f0ca208547bd4 Mon Sep 17 00:00:00 2001 From: Anthony Ramine Date: Tue, 31 Mar 2020 14:02:14 +0200 Subject: Don't clone the result of AttrHelpersForLayout::local_name There is no need to do that. Embrace returning borrows from the DOM, it good. --- components/script/dom/attr.rs | 6 +++--- components/script/dom/element.rs | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) (limited to 'components/script/dom') diff --git a/components/script/dom/attr.rs b/components/script/dom/attr.rs index b641fa5d615..a9f832b97bb 100644 --- a/components/script/dom/attr.rs +++ b/components/script/dom/attr.rs @@ -237,7 +237,7 @@ pub trait AttrHelpersForLayout<'dom> { fn value(self) -> &'dom AttrValue; fn as_str(self) -> &'dom str; fn as_tokens(self) -> Option<&'dom [Atom]>; - fn local_name(self) -> LocalName; + fn local_name(self) -> &'dom LocalName; } #[allow(unsafe_code)] @@ -262,7 +262,7 @@ impl<'dom> AttrHelpersForLayout<'dom> for LayoutDom<'dom, Attr> { } #[inline] - fn local_name(self) -> LocalName { - unsafe { self.unsafe_get().identifier.local_name.clone() } + fn local_name(self) -> &'dom LocalName { + unsafe { &self.unsafe_get().identifier.local_name } } } diff --git a/components/script/dom/element.rs b/components/script/dom/element.rs index 35f952a676c..6875d302e67 100644 --- a/components/script/dom/element.rs +++ b/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() && (*attr.unsafe_get()).namespace() == namespace + name == attr.local_name() && (*attr.unsafe_get()).namespace() == namespace }) .map(|attr| attr.to_layout()) } @@ -610,7 +610,7 @@ impl RawLayoutElementHelpers for Element { .iter() .filter_map(|attr| { let attr = attr.to_layout(); - if *name == attr.local_name() { + if name == attr.local_name() { Some(attr.value()) } else { None -- cgit v1.2.3 From 3e875ce3eb5270d31a13fbe0bd9796687465fcff Mon Sep 17 00:00:00 2001 From: Anthony Ramine Date: Tue, 31 Mar 2020 14:06:22 +0200 Subject: Introduce AttrHelpersForLayout::namespace --- components/script/dom/attr.rs | 6 ++++++ components/script/dom/element.rs | 2 +- 2 files changed, 7 insertions(+), 1 deletion(-) (limited to 'components/script/dom') diff --git a/components/script/dom/attr.rs b/components/script/dom/attr.rs index a9f832b97bb..cd1a98ad799 100644 --- a/components/script/dom/attr.rs +++ b/components/script/dom/attr.rs @@ -238,6 +238,7 @@ pub trait AttrHelpersForLayout<'dom> { fn as_str(self) -> &'dom str; fn as_tokens(self) -> Option<&'dom [Atom]>; fn local_name(self) -> &'dom LocalName; + fn namespace(self) -> &'dom Namespace; } #[allow(unsafe_code)] @@ -265,4 +266,9 @@ impl<'dom> AttrHelpersForLayout<'dom> for LayoutDom<'dom, Attr> { fn local_name(self) -> &'dom LocalName { unsafe { &self.unsafe_get().identifier.local_name } } + + #[inline] + fn namespace(self) -> &'dom Namespace { + unsafe { &self.unsafe_get().identifier.namespace } + } } diff --git a/components/script/dom/element.rs b/components/script/dom/element.rs index 6875d302e67..7d167bf2574 100644 --- a/components/script/dom/element.rs +++ b/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() && (*attr.unsafe_get()).namespace() == namespace + name == attr.local_name() && namespace == attr.namespace() }) .map(|attr| attr.to_layout()) } -- cgit v1.2.3 From e1e913d33c13a50ceed5a7ea19439b1b3d4d8ced Mon Sep 17 00:00:00 2001 From: Anthony Ramine Date: Tue, 31 Mar 2020 14:30:18 +0200 Subject: Make LayoutHTMLInputElementHelpers::value_for_layout return a cow --- components/script/dom/htmlinputelement.rs | 55 +++++++++++++++++-------------- components/script/dom/node.rs | 2 +- 2 files changed, 31 insertions(+), 26 deletions(-) (limited to 'components/script/dom') diff --git a/components/script/dom/htmlinputelement.rs b/components/script/dom/htmlinputelement.rs index 05eca7b35eb..5d7438ecc4a 100755 --- a/components/script/dom/htmlinputelement.rs +++ b/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 { - #[allow(unsafe_code)] - unsafe fn value_for_layout(self) -> String; +pub trait LayoutHTMLInputElementHelpers<'dom> { + fn value_for_layout(self) -> Cow<'dom, str>; #[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) -> 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 { - #[allow(unsafe_code)] - unsafe fn get_raw_attr_value( - input: LayoutDom<'_, HTMLInputElement>, - default: &str, - ) -> String { - let elem = input.upcast::(); - let value = (*elem.unsafe_get()) - .get_attr_val_for_layout(&ns!(), &local_name!("value")) - .unwrap_or(default); - String::from(value) + fn value_for_layout(self) -> Cow<'dom, str> { + fn get_raw_attr_value<'dom>( + input: LayoutDom<'dom, HTMLInputElement>, + default: &'static str, + ) -> Cow<'dom, str> { + unsafe { + input + .upcast::() + .unsafe_get() + .get_attr_val_for_layout(&ns!(), &local_name!("value")) + .unwrap_or(default) + .into() + } } - match (*self.unsafe_get()).input_type() { - InputType::Checkbox | InputType::Radio => String::new(), - InputType::File | InputType::Image => String::new(), + let placeholder = unsafe { &**self.unsafe_get().placeholder.borrow_for_layout() }; + match unsafe { self.unsafe_get().input_type() } { + 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::() + .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() } }, } diff --git a/components/script/dom/node.rs b/components/script/dom/node.rs index d92e3b6a364..b72dc6b695e 100644 --- a/components/script/dom/node.rs +++ b/components/script/dom/node.rs @@ -1463,7 +1463,7 @@ impl<'dom> LayoutNodeHelpers<'dom> for LayoutDom<'dom, Node> { } if let Some(input) = self.downcast::() { - return unsafe { input.value_for_layout() }; + return input.value_for_layout().into_owned(); } if let Some(area) = self.downcast::() { -- cgit v1.2.3 From 00c5ec202ccde20eace5c867578262efa5b69844 Mon Sep 17 00:00:00 2001 From: Anthony Ramine Date: Tue, 31 Mar 2020 14:35:37 +0200 Subject: Make LayoutHTMLTextAreaElementHelpers::value_for_layout safe --- components/script/dom/htmltextareaelement.rs | 25 ++++++++++++------------- components/script/dom/node.rs | 2 +- 2 files changed, 13 insertions(+), 14 deletions(-) (limited to 'components/script/dom') diff --git a/components/script/dom/htmltextareaelement.rs b/components/script/dom/htmltextareaelement.rs index 8ee42bc2c86..e3721a78213 100755 --- a/components/script/dom/htmltextareaelement.rs +++ b/components/script/dom/htmltextareaelement.rs @@ -56,8 +56,7 @@ pub struct HTMLTextAreaElement { } pub trait LayoutHTMLTextAreaElementHelpers { - #[allow(unsafe_code)] - unsafe fn value_for_layout(self) -> String; + fn value_for_layout(self) -> String; #[allow(unsafe_code)] unsafe fn selection_for_layout(self) -> Option>; #[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 { - let text = (*self.unsafe_get()) - .textinput - .borrow_for_layout() - .get_content(); - if text.is_empty() { - (*self.unsafe_get()) - .placeholder + fn value_for_layout(self) -> String { + let text = unsafe { + self.unsafe_get() + .textinput .borrow_for_layout() - .replace("\r\n", "\n") - .replace("\r", "\n") + .get_content() + }; + 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() } diff --git a/components/script/dom/node.rs b/components/script/dom/node.rs index b72dc6b695e..425e29d45f3 100644 --- a/components/script/dom/node.rs +++ b/components/script/dom/node.rs @@ -1467,7 +1467,7 @@ impl<'dom> LayoutNodeHelpers<'dom> for LayoutDom<'dom, Node> { } if let Some(area) = self.downcast::() { - return unsafe { area.value_for_layout() }; + return area.value_for_layout(); } panic!("not text!") -- cgit v1.2.3 From 409bd3d989c1877f8e0a5da92bd758dd20ac724b Mon Sep 17 00:00:00 2001 From: Anthony Ramine Date: Tue, 31 Mar 2020 14:40:47 +0200 Subject: Make LayoutCharacterDataHelpers::data_for_layout be safe --- components/script/dom/characterdata.rs | 9 ++++----- components/script/dom/node.rs | 3 +-- 2 files changed, 5 insertions(+), 7 deletions(-) (limited to 'components/script/dom') diff --git a/components/script/dom/characterdata.rs b/components/script/dom/characterdata.rs index 4e0d32d4456..f8c5acb5a8f 100644 --- a/components/script/dom/characterdata.rs +++ b/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 { - &(*self.unsafe_get()).data.borrow_for_layout() + fn data_for_layout(self) -> &'dom str { + unsafe { self.unsafe_get().data.borrow_for_layout() } } } diff --git a/components/script/dom/node.rs b/components/script/dom/node.rs index 425e29d45f3..07b7d60cbe6 100644 --- a/components/script/dom/node.rs +++ b/components/script/dom/node.rs @@ -1456,10 +1456,9 @@ impl<'dom> LayoutNodeHelpers<'dom> for LayoutDom<'dom, Node> { val } - #[allow(unsafe_code)] fn text_content(self) -> String { if let Some(text) = self.downcast::() { - return unsafe { text.upcast().data_for_layout().to_owned() }; + return text.upcast().data_for_layout().to_owned(); } if let Some(input) = self.downcast::() { -- cgit v1.2.3 From 6fe294fa5bab1b0ccd80d6dfb53be85adfaec4ad Mon Sep 17 00:00:00 2001 From: Anthony Ramine Date: Tue, 31 Mar 2020 14:58:56 +0200 Subject: Make LayoutNodeHelpers::text_content return a cow --- components/script/dom/node.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) (limited to 'components/script/dom') diff --git a/components/script/dom/node.rs b/components/script/dom/node.rs index 07b7d60cbe6..251f6fcab18 100644 --- a/components/script/dom/node.rs +++ b/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>; fn image_url(self) -> Option; fn image_density(self) -> Option; @@ -1456,17 +1456,17 @@ impl<'dom> LayoutNodeHelpers<'dom> for LayoutDom<'dom, Node> { val } - fn text_content(self) -> String { + fn text_content(self) -> Cow<'dom, str> { if let Some(text) = self.downcast::() { - return text.upcast().data_for_layout().to_owned(); + return text.upcast().data_for_layout().into(); } if let Some(input) = self.downcast::() { - return input.value_for_layout().into_owned(); + return input.value_for_layout(); } if let Some(area) = self.downcast::() { - return area.value_for_layout(); + return area.value_for_layout().into(); } panic!("not text!") -- cgit v1.2.3 From fb1ff3f097c0c8a994c9e7c4bb72c3ba5b64492d Mon Sep 17 00:00:00 2001 From: Anthony Ramine Date: Tue, 31 Mar 2020 15:23:06 +0200 Subject: Remove an obsolete comment --- components/script/dom/attr.rs | 1 - 1 file changed, 1 deletion(-) (limited to 'components/script/dom') diff --git a/components/script/dom/attr.rs b/components/script/dom/attr.rs index cd1a98ad799..e2e8b79f9af 100644 --- a/components/script/dom/attr.rs +++ b/components/script/dom/attr.rs @@ -255,7 +255,6 @@ impl<'dom> AttrHelpersForLayout<'dom> for LayoutDom<'dom, Attr> { #[inline] 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, -- cgit v1.2.3