Merged
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)
|
|
|
Loirooriol
force-pushed
the
stylo-2023-09-01
branch
from
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)
|
|
|
Loirooriol
force-pushed
the
stylo-2023-09-01
branch
from
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. |
mrobinson
reviewed
Mar 13, 2024
Loirooriol
force-pushed
the
stylo-2023-09-01
branch
2 times, most recently
from
553e901 to
9ad8a3f
Compare
|
🔨 Triggering try run (#8298575359) for Linux WPT, MacOS, Windows, Android |
Loirooriol
commented
Mar 15, 2024
Comment on lines
+3401
to
+3402
| // We can't use style::bloom::each_relevant_element_hash(*self, f) | ||
| // since DomRoot<Element> doesn't have the TElement trait. |
Contributor
Author
There was a problem hiding this comment.
BTW, not worth it to do it in this PR, but should DomRoot<Element> implement TElement?
Member
There was a problem hiding this comment.
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. |
mrobinson
approved these changes
Mar 15, 2024
Member
mrobinson
left a comment
mrobinson
left a comment
There was a problem hiding this comment.
Looks good! Just a question about the new function in element.rs.
Comment on lines
+3403
to
+3424
| 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 | ||
| } |
Member
There was a problem hiding this comment.
Out of curiosity, what made you use this set of attributes to add to the hash?
Contributor
Author
There was a problem hiding this comment.
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.
Member
There was a problem hiding this comment.
Thanks for the explanation. BTW, you can land this whenever you are ready.
Contributor
Author
There was a problem hiding this comment.
So how was I supposed to land? Should I force-push Stylo's main branch to the upgrade branch?
Comment on lines
+3401
to
+3402
| // We can't use style::bloom::each_relevant_element_hash(*self, f) | ||
| // since DomRoot<Element> doesn't have the TElement trait. |
Member
There was a problem hiding this comment.
Perhaps...Aren't we usually processing a LayoutDom node though?
Loirooriol
force-pushed
the
stylo-2023-09-01
branch
from
9ad8a3f to
78538dd
Compare
Loirooriol
force-pushed
the
stylo-2023-09-01
branch
from
78538dd to
90f6a99
Compare
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This continues #31437
Changelog:
./mach build -ddoes not report any errors./mach test-tidydoes not report any errors