New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update Stylo to 2023-09-01 #31609
Update Stylo to 2023-09-01 #31609
Conversation
🔨 Triggering try run (#8224509076) for Linux WPT, MacOS, Windows, Android |
Test results for linux-wpt-layout-2013 from try job (#8224509076): Flaky unexpected result (24)
Stable unexpected results that are known to be intermittent (13)
Stable unexpected results (45)
|
Test results for linux-wpt-layout-2020 from try job (#8224509076): Flaky unexpected result (17)
Stable unexpected results that are known to be intermittent (43)
Stable unexpected results (283)
|
|
42ebe60
to
8b5dc67
Compare
🔨 Triggering try run (#8228482110) for Linux WPT |
Test results for linux-wpt-layout-2013 from try job (#8228482110): Flaky unexpected result (20)
Stable unexpected results that are known to be intermittent (7)
Stable unexpected results (21)
|
Test results for linux-wpt-layout-2020 from try job (#8228482110): Flaky unexpected result (19)
Stable unexpected results that are known to be intermittent (14)
Stable unexpected results (21)
|
|
8b5dc67
to
58dfa4b
Compare
🔨 Triggering try run (#8254753058) for Linux WPT, MacOS, Windows, Android |
Test results for linux-wpt-layout-2013 from try job (#8254753058): Flaky unexpected result (14)
Stable unexpected results that are known to be intermittent (13)
|
Test results for linux-wpt-layout-2020 from try job (#8254753058): Flaky unexpected result (18)
Stable unexpected results that are known to be intermittent (10)
|
✨ Try run (#8254753058) succeeded. |
553e901
to
9ad8a3f
Compare
🔨 Triggering try run (#8298575359) for Linux WPT, MacOS, Windows, Android |
// We can't use style::bloom::each_relevant_element_hash(*self, f) | ||
// since DomRoot<Element> doesn't have the TElement trait. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW, not worth it to do it in this PR, but should DomRoot<Element>
implement TElement?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps...Aren't we usually processing a LayoutDom node though?
Test results for linux-wpt-layout-2013 from try job (#8298575359): Flaky unexpected result (14)
Stable unexpected results that are known to be intermittent (18)
|
Test results for linux-wpt-layout-2020 from try job (#8298575359): Flaky unexpected result (18)
Stable unexpected results that are known to be intermittent (19)
|
✨ Try run (#8298575359) succeeded. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Just a question about the new function in element.rs
.
f(Element::local_name(self).get_hash()); | ||
f(Element::namespace(self).get_hash()); | ||
|
||
if let Some(ref id) = *self.id_attribute.borrow() { | ||
f(id.get_hash()); | ||
} | ||
|
||
if let Some(attr) = self.get_attribute(&ns!(), &local_name!("class")) { | ||
for class in attr.value().as_tokens() { | ||
f(AtomIdent::cast(class).get_hash()); | ||
} | ||
} | ||
|
||
for attr in self.attrs.borrow().iter() { | ||
let name = style::values::GenericAtomIdent::cast(attr.local_name()); | ||
if !style::bloom::is_attr_name_excluded_from_filter(name) { | ||
f(name.get_hash()); | ||
} | ||
} | ||
|
||
true | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of curiosity, what made you use this set of attributes to add to the hash?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's just an adaptation of each_relevant_element_hash
:
pub fn each_relevant_element_hash<E, F>(element: E, mut f: F)
where
E: TElement,
F: FnMut(u32),
{
f(element.local_name().get_hash());
f(element.namespace().get_hash());
if let Some(id) = element.id() {
f(id.get_hash());
}
element.each_class(|class| f(class.get_hash()));
element.each_attr_name(|name| {
if !is_attr_name_excluded_from_filter(name) {
f(name.get_hash())
}
});
}
In practice I believe it doesn't really matter, since this was added for :has()
which is disabled on Servo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the explanation. BTW, you can land this whenever you are ready.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So how was I supposed to land? Should I force-push Stylo's main branch to the upgrade branch?
// We can't use style::bloom::each_relevant_element_hash(*self, f) | ||
// since DomRoot<Element> doesn't have the TElement trait. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps...Aren't we usually processing a LayoutDom node though?
9ad8a3f
to
78538dd
Compare
78538dd
to
90f6a99
Compare
This continues #31437
Changelog:
./mach build -d
does not report any errors./mach test-tidy
does not report any errors