Skip to content

Commit 513b937

Browse files
author
bors-servo
authored
Auto merge of #25572 - pshaughn:atomnames, r=<try>
Make name content attributes consistently atoms and put them in rare_data for fast access <!-- Please describe your changes on the following line: --> All codepaths setting the name content attribute now use an atom, which is also stored in rare_data for direct lookup by a get_name method. Paralleling the get_name method, I added a get_id method, which makes some internal id-lookup cases nicer. A new test tests for a name setter on every HTML element type. In addition to its overt and upstreamable purpose of checking IDL property reflection semantics, for us this test also hits some Servo assertions that make sure the name is an atom in every case. If the test doesn't crash, even a failed test case still has the attribute as an atom rather than some other type. The failed cases are for elements that we have unimplemented or completely stubbed; I added a few missing name IDL properties to otherwise implemented elements. I also made a _mozilla copy of the working part of an existing but not-working test, as I explain in #25057 (comment) --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: --> - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #25570 and make progress on #25057 <!-- Either: --> - [X] There are tests for these changes <!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.--> <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
2 parents 35eb6dd + 8112cea commit 513b937

30 files changed

+351
-135
lines changed

‎components/atoms/static_atoms.txt‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -84,6 +84,7 @@ radio
8484
range
8585
ratechange
8686
readystatechange
87+
referrer
8788
reftest-wait
8889
rejectionhandled
8990
removetrack

‎components/script/dom/document.rs‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -823,6 +823,7 @@ impl Document {
823823
}
824824

825825
fn get_anchor_by_name(&self, name: &str) -> Option<DomRoot<Element>> {
826+
// TODO faster name lookups (see #25548)
826827
let check_anchor = |node: &HTMLAnchorElement| {
827828
let elem = node.upcast::<Element>();
828829
elem.get_attribute(&ns!(), &local_name!("name"))
@@ -4096,9 +4097,7 @@ impl DocumentMethods for Document {
40964097
if element.namespace() != &ns!(html) {
40974098
return false;
40984099
}
4099-
element
4100-
.get_attribute(&ns!(), &local_name!("name"))
4101-
.map_or(false, |attr| &**attr.value() == &*name)
4100+
element.get_name().map_or(false, |atom| *atom == *name)
41024101
})
41034102
}
41044103

@@ -4308,6 +4307,7 @@ impl DocumentMethods for Document {
43084307
}
43094308
// https://html.spec.whatwg.org/multipage/#dom-document-nameditem-filter
43104309
fn filter_by_name(name: &Atom, node: &Node) -> bool {
4310+
// TODO faster name lookups (see #25548)
43114311
let html_elem_type = match node.type_id() {
43124312
NodeTypeId::Element(ElementTypeId::HTMLElement(type_)) => type_,
43134313
_ => return false,

‎components/script/dom/element.rs‎

Lines changed: 43 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1388,11 +1388,27 @@ impl Element {
13881388
// https://dom.spec.whatwg.org/#concept-element-attributes-get-by-name
13891389
pub fn get_attribute_by_name(&self, name: DOMString) -> Option<DomRoot<Attr>> {
13901390
let name = &self.parsed_name(name);
1391-
self.attrs
1391+
let maybe_attribute = self
1392+
.attrs
13921393
.borrow()
13931394
.iter()
13941395
.find(|a| a.name() == name)
1395-
.map(|js| DomRoot::from_ref(&**js))
1396+
.map(|js| DomRoot::from_ref(&**js));
1397+
fn id_and_name_must_be_atoms(name: &LocalName, maybe_attr: &Option<DomRoot<Attr>>) -> bool {
1398+
if *name == local_name!("id") || *name == local_name!("name") {
1399+
match maybe_attr {
1400+
None => true,
1401+
Some(ref attr) => match *attr.value() {
1402+
AttrValue::Atom(_) => true,
1403+
_ => false,
1404+
},
1405+
}
1406+
} else {
1407+
true
1408+
}
1409+
}
1410+
debug_assert!(id_and_name_must_be_atoms(name, &maybe_attribute));
1411+
maybe_attribute
13961412
}
13971413

13981414
pub fn set_attribute_from_parser(
@@ -1787,6 +1803,14 @@ impl Element {
17871803
let other = other.upcast::<Element>();
17881804
self.root_element() == other.root_element()
17891805
}
1806+
1807+
pub fn get_id(&self) -> Option<Atom> {
1808+
self.id_attribute.borrow().clone()
1809+
}
1810+
1811+
pub fn get_name(&self) -> Option<Atom> {
1812+
self.rare_data().as_ref()?.name_attribute.clone()
1813+
}
17901814
}
17911815

17921816
impl ElementMethods for Element {
@@ -1823,6 +1847,8 @@ impl ElementMethods for Element {
18231847
}
18241848

18251849
// https://dom.spec.whatwg.org/#dom-element-id
1850+
// This always returns a string; if you'd rather see None
1851+
// on a null id, call get_id
18261852
fn Id(&self) -> DOMString {
18271853
self.get_string_attribute(&local_name!("id"))
18281854
}
@@ -2770,6 +2796,20 @@ impl VirtualMethods for Element {
27702796
}
27712797
}
27722798
},
2799+
&local_name!("name") => {
2800+
// Keep the name in rare data for fast access
2801+
self.ensure_rare_data().name_attribute =
2802+
mutation.new_value(attr).and_then(|value| {
2803+
let value = value.as_atom();
2804+
if value != &atom!("") {
2805+
Some(value.clone())
2806+
} else {
2807+
None
2808+
}
2809+
});
2810+
// TODO: notify the document about the name change
2811+
// once it has a name_map (#25548)
2812+
},
27732813
_ => {
27742814
// FIXME(emilio): This is pretty dubious, and should be done in
27752815
// the relevant super-classes.
@@ -2788,6 +2828,7 @@ impl VirtualMethods for Element {
27882828
fn parse_plain_attribute(&self, name: &LocalName, value: DOMString) -> AttrValue {
27892829
match name {
27902830
&local_name!("id") => AttrValue::from_atomic(value.into()),
2831+
&local_name!("name") => AttrValue::from_atomic(value.into()),
27912832
&local_name!("class") => AttrValue::from_serialized_tokenlist(value.into()),
27922833
_ => self
27932834
.super_type()

‎components/script/dom/htmlanchorelement.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -152,7 +152,7 @@ impl HTMLAnchorElementMethods for HTMLAnchorElement {
152152
make_getter!(Name, "name");
153153

154154
// https://html.spec.whatwg.org/multipage/#dom-a-name
155-
make_setter!(SetName, "name");
155+
make_atomic_setter!(SetName, "name");
156156

157157
// https://html.spec.whatwg.org/multipage/#dom-a-rev
158158
make_getter!(Rev, "rev");

‎components/script/dom/htmlbuttonelement.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -142,7 +142,7 @@ impl HTMLButtonElementMethods for HTMLButtonElement {
142142
make_getter!(Name, "name");
143143

144144
// https://html.spec.whatwg.org/multipage/#dom-fe-name
145-
make_setter!(SetName, "name");
145+
make_atomic_setter!(SetName, "name");
146146

147147
// https://html.spec.whatwg.org/multipage/#dom-button-value
148148
make_getter!(Value, "value");

‎components/script/dom/htmlcollection.rs‎

Lines changed: 16 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -366,11 +366,12 @@ impl HTMLCollectionMethods for HTMLCollection {
366366
return None;
367367
}
368368

369+
let key = Atom::from(key);
370+
369371
// Step 2.
370372
self.elements_iter().find(|elem| {
371-
elem.get_string_attribute(&local_name!("id")) == key ||
372-
(elem.namespace() == &ns!(html) &&
373-
elem.get_string_attribute(&local_name!("name")) == key)
373+
elem.get_id().map_or(false, |id| id == key) ||
374+
(elem.namespace() == &ns!(html) && elem.get_name().map_or(false, |id| id == key))
374375
})
375376
}
376377

@@ -392,17 +393,20 @@ impl HTMLCollectionMethods for HTMLCollection {
392393
// Step 2
393394
for elem in self.elements_iter() {
394395
// Step 2.1
395-
let id_attr = elem.get_string_attribute(&local_name!("id"));
396-
if !id_attr.is_empty() && !result.contains(&id_attr) {
397-
result.push(id_attr)
396+
if let Some(id_atom) = elem.get_id() {
397+
let id_str = DOMString::from(&*id_atom);
398+
if !result.contains(&id_str) {
399+
result.push(id_str);
400+
}
398401
}
399402
// Step 2.2
400-
let name_attr = elem.get_string_attribute(&local_name!("name"));
401-
if !name_attr.is_empty() &&
402-
!result.contains(&name_attr) &&
403-
*elem.namespace() == ns!(html)
404-
{
405-
result.push(name_attr)
403+
if *elem.namespace() == ns!(html) {
404+
if let Some(name_atom) = elem.get_name() {
405+
let name_str = DOMString::from(&*name_atom);
406+
if !result.contains(&name_str) {
407+
result.push(name_str)
408+
}
409+
}
406410
}
407411
}
408412

‎components/script/dom/htmlfieldsetelement.rs‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ use crate::dom::bindings::codegen::Bindings::HTMLFieldSetElementBinding;
77
use crate::dom::bindings::codegen::Bindings::HTMLFieldSetElementBinding::HTMLFieldSetElementMethods;
88
use crate::dom::bindings::inheritance::{Castable, ElementTypeId, HTMLElementTypeId, NodeTypeId};
99
use crate::dom::bindings::root::{DomRoot, MutNullableDom};
10+
use crate::dom::bindings::str::DOMString;
1011
use crate::dom::document::Document;
1112
use crate::dom::element::{AttributeMutation, Element};
1213
use crate::dom::htmlcollection::{CollectionFilter, HTMLCollection};
@@ -88,6 +89,12 @@ impl HTMLFieldSetElementMethods for HTMLFieldSetElement {
8889
// https://html.spec.whatwg.org/multipage/#dom-fieldset-disabled
8990
make_bool_setter!(SetDisabled, "disabled");
9091

92+
// https://html.spec.whatwg.org/multipage/#dom-fe-name
93+
make_atomic_setter!(SetName, "name");
94+
95+
// https://html.spec.whatwg.org/multipage/#dom-fe-name
96+
make_getter!(Name, "name");
97+
9198
// https://html.spec.whatwg.org/multipage/#dom-fae-form
9299
fn GetForm(&self) -> Option<DomRoot<HTMLFormElement>> {
93100
self.form_owner()

‎components/script/dom/htmlformcontrolscollection.rs‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ use crate::dom::node::Node;
1717
use crate::dom::radionodelist::RadioNodeList;
1818
use crate::dom::window::Window;
1919
use dom_struct::dom_struct;
20+
use servo_atoms::Atom;
2021

2122
#[dom_struct]
2223
pub struct HTMLFormControlsCollection {
@@ -61,9 +62,11 @@ impl HTMLFormControlsCollectionMethods for HTMLFormControlsCollection {
6162
return None;
6263
}
6364

65+
let name = Atom::from(name);
66+
6467
let mut filter_map = self.collection.elements_iter().filter_map(|elem| {
65-
if elem.get_string_attribute(&local_name!("name")) == name ||
66-
elem.get_string_attribute(&local_name!("id")) == name
68+
if elem.get_name().map_or(false, |n| n == name) ||
69+
elem.get_id().map_or(false, |i| i == name)
6770
{
6871
Some(elem)
6972
} else {
@@ -87,7 +90,7 @@ impl HTMLFormControlsCollectionMethods for HTMLFormControlsCollection {
8790
// specifically HTMLFormElement::Elements(),
8891
// and the collection filter excludes image inputs.
8992
Some(RadioNodeListOrElement::RadioNodeList(
90-
RadioNodeList::new_controls_except_image_inputs(window, form, name),
93+
RadioNodeList::new_controls_except_image_inputs(window, form, &name),
9194
))
9295
}
9396
// Step 3

0 commit comments

Comments
 (0)