Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upID and class selectors are ASCII case-insensitive in quirks mode. #17213
Conversation
highfive
commented
Jun 7, 2017
|
Heads up! This PR modifies the following files:
|
highfive
commented
Jun 7, 2017
|
|
ID and class selectors are ASCII case-insensitive in quirks mode. https://bugzilla.mozilla.org/show_bug.cgi?id=1363778 <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/17213) <!-- Reviewable:end -->
|
|
|
@bors-servo try |
ID and class selectors are ASCII case-insensitive in quirks mode. https://bugzilla.mozilla.org/show_bug.cgi?id=1363778 <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/17213) <!-- Reviewable:end -->
|
Per https://bugzilla.mozilla.org/show_bug.cgi?id=1363778#c9 this is missing some changes in |
|
|
ID and class selectors are ASCII case-insensitive in quirks mode. https://bugzilla.mozilla.org/show_bug.cgi?id=1363778 <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/17213) <!-- Reviewable:end -->
ID and class selectors are ASCII case-insensitive in quirks mode. https://bugzilla.mozilla.org/show_bug.cgi?id=1363778 <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/17213) <!-- Reviewable:end -->
|
So that gets unexpected passes. I'm a little worried about performance: it's adding a bunch of branches to hot paths and in the quirks case a bunch of to_ascii_lowercase() stuff. The branches should really be able to be hoisted much higher: the quirks mode boolean is an invariant for a stylist as far as I know. We could conceivably even make things generic on the quirks mode if we wanted to; I just don't know what that would do to the codesize. For the to_ascii_lowercase bit, I'm not sure whether that's enough of a problem to try to do something where the actual key lookups are in-place ASCII-case-insensitive (which is what Gecko does). Might want to measure. |
|
|
|
I don’t think making this completely generic works, we still want a runtime branch or dispatch at some point, even if hoisted higher. As far as I can tell, Gecko does dynamic dispatch for this (a struct of function pointers): http://searchfox.org/mozilla-central/rev/1a054419976437d0778a2b89be1b00207a744e94/layout/style/nsCSSRuleProcessor.cpp#368-384 I don’t know which is preferable. I considered in-place ASCII case-insensitive hash map lookups but for hashing atoms we use a pre-computed |
|
Yeah, I'm extremely worried about the branches here. I agree that we should make the decision once higher-up and then call monomorphized code. |
|
|
|
Macros+generics stuff removed per IRC discussion http://logs.glob.uno/?c=mozilla%23servo&s=12+Jun+2017&e=12+Jun+2017#c695467 , added pre-computation of |
| Some(Atom::from(ptr)) | ||
| } | ||
| fn has_id(&self, id: &Atom, case_sensitivity: CaseSensitivity) -> bool { | ||
| self.get_id().map_or(false, |atom| case_sensitivity.eq_atom(&atom, id)) |
This comment has been minimized.
This comment has been minimized.
bholley
Jun 12, 2017
Contributor
We could tweak this to skip the refcount traffic from get_id(), which would probably be a significant win given that the atom refcounting is atomic. Can you either fix that here or file a followup?
This comment has been minimized.
This comment has been minimized.
|
r=me with that. @bors-servo delegate+ |
|
|
|
@bors-servo r=bholley |
|
|
ID and class selectors are ASCII case-insensitive in quirks mode. https://bugzilla.mozilla.org/show_bug.cgi?id=1363778 <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/17213) <!-- Reviewable:end -->
|
Copied from above: --- a/layout/reftests/bugs/reftest.list
+++ b/layout/reftests/bugs/reftest.list
@@ -1555,7 +1555,7 @@ needs-focus == 568441.html 568441-ref.html
== 571347-1b.html 571347-1-ref.html
== 571347-2a.html 571347-2-ref.html
== 571347-2b.html 571347-2-ref.html
-fails-if(styloVsGecko||stylo) == 571347-2c.html 571347-2-ref.html
+== 571347-2c.html 571347-2-ref.html
== 571347-2d.html 571347-2-ref.html
== 571347-3.html 571347-3-ref.html
== 572598-1.html 572598-ref.html
@@ -1667,7 +1667,7 @@ fuzzy-if(Android,8,500) fuzzy-if(skiaContent,2,1) fuzzy-if(webrender,3,19) == 63
fuzzy-if(Android,8,500) == 637852-3.html 637852-3-ref.html
== 641770-1.html 641770-1-ref.html
== 641856-1.html 641856-1-ref.html
-fails-if(styloVsGecko||stylo) == 645491-1.html 645491-1-ref.html
+== 645491-1.html 645491-1-ref.html
== 645647-1.html 645647-1-ref.html
== 645647-2.html 645647-2-ref.html
== 645768-1.html 645768-1-ref.html |
|
|
|
Pushed expectations update: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=ad90f0a5b56c2f2adaaa4e9b66f5da071f22b611 |
SimonSapin commentedJun 7, 2017
•
edited by metajack
https://bugzilla.mozilla.org/show_bug.cgi?id=1363778
This change is