Do not use borrowed types in the selectors::Element trait. #23463

RazrFalcon · 2019-05-26T16:18:23Z

This change allows implementing the selectors::Element trait for types that doesn't support borrowed types, like rc-tree.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #22972

This change is

highfive · 2019-05-26T16:18:30Z

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @asajeffrey (or someone else) soon.

highfive · 2019-05-26T16:18:31Z

Heads up! This PR modifies the following files:

@emilio: components/style/dom_apis.rs, components/style/rule_collector.rs, components/style/selector_map.rs, components/style/bloom.rs, components/style/sharing/mod.rs and 2 more

highfive · 2019-05-26T16:18:33Z

Warning

These commits modify style code, but no tests are modified. Please consider adding a test!

RazrFalcon · 2019-05-26T16:19:07Z

components/selectors/tree.rs

@@ -58,10 +58,21 @@ pub trait Element: Sized + Clone + Debug {

    fn is_html_element_in_html_document(&self) -> bool;

-    fn local_name(&self) -> &<Self::Impl as SelectorImpl>::BorrowedLocalName;
+    fn local_name_hash(&self) -> u32;


This is ugly. I know. We need three methods instead of one now...

I think we don't need get_local_name or get_namespace, instead fn local_name and fn namespace should move to TElement (in components/style/dom.rs).

RazrFalcon · 2019-05-26T16:19:59Z

components/style/selector_map.rs

@@ -226,7 +226,7 @@ impl SelectorMap<Rule> {
            }
        });

-        if let Some(rules) = self.local_name_hash.get(rule_hash_target.local_name()) {
+        if let Some(rules) = self.local_name_hash.get(&rule_hash_target.get_local_name()) {


This method invokes clone() now. Which is bad. By I don't know how to fix it.

This doesn't need to use selectors::Element. You can move the fn local_name signature to TElement. So selectors doesn't need get_local_name, and this shouldn't need to call clone().

emilio

Thanks for the patch! Needs some changes, I think it can be simpler.

emilio · 2019-05-26T16:35:57Z

components/style/selector_map.rs

@@ -226,7 +226,7 @@ impl SelectorMap<Rule> {
            }
        });

-        if let Some(rules) = self.local_name_hash.get(rule_hash_target.local_name()) {
+        if let Some(rules) = self.local_name_hash.get(&rule_hash_target.get_local_name()) {


This doesn't need to use selectors::Element. You can move the fn local_name signature to TElement. So selectors doesn't need get_local_name, and this shouldn't need to call clone().

emilio · 2019-05-26T16:35:57Z

components/style/bloom.rs

-    f(element.local_name().get_hash());
-    f(element.namespace().get_hash());
+    f(element.local_name_hash());
+    f(element.namespace_hash());


Same, this doesn't need to use selectors::Element, and selectors doesn't need Element::namespace_hash.

emilio · 2019-05-26T16:35:57Z

components/selectors/tree.rs

@@ -58,10 +58,21 @@ pub trait Element: Sized + Clone + Debug {

    fn is_html_element_in_html_document(&self) -> bool;

-    fn local_name(&self) -> &<Self::Impl as SelectorImpl>::BorrowedLocalName;
+    fn local_name_hash(&self) -> u32;


I think we don't need get_local_name or get_namespace, instead fn local_name and fn namespace should move to TElement (in components/style/dom.rs).

emilio · 2019-05-26T16:39:08Z

components/style/invalidation/stylesheets.rs

@@ -90,8 +90,7 @@ impl Invalidation {
                // This could look at the quirks mode of the document, instead
                // of testing against both names, but it's probably not worth
                // it.
-                let local_name = element.local_name();
-                return *local_name == **name || *local_name == **lower_name;
+                return element.has_local_name(name) || element.has_local_name(lower_name);


Similarly this shouldn't need to change.

emilio · 2019-05-26T16:39:08Z

components/style/rule_collector.rs

@@ -42,7 +42,7 @@ pub fn containing_shadow_ignoring_svg_use<E: TElement>(
    loop {
        let host = shadow.host();
        let host_is_svg_use_element =
-            host.is_svg_element() && host.local_name() == &*local_name!("use");
+            host.is_svg_element() && host.has_local_name(&local_name!("use"));


these don't need to change, but may be nicer this way, your call.

emilio · 2019-05-26T16:39:08Z

components/style/sharing/mod.rs

-
-        if target.namespace() != candidate.element.namespace() {
-            trace!("Miss: Namespace");
+        if !target.is_same_type(&candidate.element) {


This doesn't need to change either, and I'd prefer to keep the two checks to remain separate to see why it fails.

RazrFalcon · 2019-05-26T16:59:58Z

components/style/invalidation/element/element_wrapper.rs

+        &self,
+        local_name: &<Self::Impl as ::selectors::SelectorImpl>::BorrowedLocalName,
+    ) -> bool {
+        self.element.has_local_name(local_name)


Not sure it this is correct.

Yes, this is right, local name / namespace don't change.

emilio · 2019-05-26T17:11:09Z

components/style/dom.rs

@@ -877,6 +877,12 @@ pub trait TElement:
        hints: &mut V,
    ) where
        V: Push<ApplicableDeclarationBlock>;
+
+    /// Returns element's local name.
+    fn local_name(&self) -> &LocalName;


This still needs to use the Borrowed* gunk, so that you don't break Firefox.

Also this actually needs to be implemented, right?

Not sure what exact type should be used?

Yes. It's used in the code.


        Do not use borrowed types in the selectors::Element trait.

Closes #22972

RazrFalcon · 2019-05-27T19:37:13Z

Finally, I was able to build it locally without any errors.

RazrFalcon · 2019-05-29T10:35:21Z

@emilio ?

emilio

Yeah, sorry for the lag.

This looks great, thanks! But I need to fix the Firefox implementation as well in servo/components/style/gecko/wrapper.rs.

So I'll land this after #23456 lands, if you don't mind.

Thanks again!

RazrFalcon · 2019-05-29T19:49:38Z

Thanks. No problem.

emilio · 2019-06-03T12:47:45Z

I'm cherry-picking this in the next sync

emilio · 2019-06-03T13:47:28Z

#23503 includes this commit along with the Firefox changes needed. Thanks a lot for the patch @RazrFalcon!


        Bug 1556388 - Do not use borrowed types in the selectors::Element tra…

…it. r=emilio This cherry-picks servo/servo#23463 with a few fixes for the Gecko build.


        style: Do not use borrowed types in the selectors::Element trait.

Closes servo#22972 Closes servo#23463


        Bug 1556388 - Do not use borrowed types in the selectors::Element tra…

…it. r=emilio This cherry-picks servo/servo#23463 with a few fixes for the Gecko build.


        Auto merge of #23503 - emilio:gecko-sync, r=emilio

style: sync changes from mozilla-central See each individual commit for details. This also cherry-picks #23463 with a few fixes that were needed in Gecko-only code.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/23503)


        Auto merge of #23503 - emilio:gecko-sync, r=emilio

style: sync changes from mozilla-central See each individual commit for details. This also cherry-picks #23463 with a few fixes that were needed in Gecko-only code.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/23503)


        Bug 1556388 - Do not use borrowed types in the selectors::Element tra…

…it. r=emilio This cherry-picks servo/servo#23463 with a few fixes for the Gecko build. UltraBlame original commit: 53a5f4a6e1de21b62be288cac96763aeece78277


        Bug 1556388 - Do not use borrowed types in the selectors::Element tra…

…it. r=emilio This cherry-picks servo/servo#23463 with a few fixes for the Gecko build. UltraBlame original commit: 53a5f4a6e1de21b62be288cac96763aeece78277


        Bug 1556388 - Do not use borrowed types in the selectors::Element tra…

…it. r=emilio This cherry-picks servo/servo#23463 with a few fixes for the Gecko build. UltraBlame original commit: 53a5f4a6e1de21b62be288cac96763aeece78277

highfive assigned asajeffrey May 26, 2019

highfive added the S-awaiting-review label May 26, 2019

RazrFalcon reviewed May 26, 2019

View changes

emilio requested changes May 26, 2019

View changes

emilio reviewed May 26, 2019

View changes

RazrFalcon reviewed May 26, 2019

View changes

emilio reviewed May 26, 2019

View changes

RazrFalcon mentioned this pull request May 27, 2019

selectors::Element without borrowed types #22972

Closed

Do not use borrowed types in the selectors::Element trait.

Loading status checks…

99b8fac

Closes #22972

emilio approved these changes May 29, 2019

View changes

emilio mentioned this pull request Jun 3, 2019

style: sync changes from mozilla-central #23503

Merged

emilio closed this Jun 3, 2019

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Jun 4, 2019

Bug 1556388 - Do not use borrowed types in the selectors::Element tra…

8e33a62

…it. r=emilio This cherry-picks servo/servo#23463 with a few fixes for the Gecko build.

emilio added a commit to emilio/servo that referenced this pull request Jun 4, 2019

style: Do not use borrowed types in the selectors::Element trait.

6f1df51

Closes servo#22972 Closes servo#23463

lissyx pushed a commit to lissyx/mozilla-central that referenced this pull request Jun 4, 2019

Bug 1556388 - Do not use borrowed types in the selectors::Element tra…

edacec0

…it. r=emilio This cherry-picks servo/servo#23463 with a few fixes for the Gecko build.

servo / servo

Do not use borrowed types in the selectors::Element trait. #23463

Do not use borrowed types in the selectors::Element trait. #23463

RazrFalcon commented May 26, 2019 •

edited

highfive commented May 26, 2019

highfive commented May 26, 2019

highfive commented May 26, 2019

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

emilio left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

RazrFalcon commented May 27, 2019

RazrFalcon commented May 29, 2019

emilio left a comment

RazrFalcon commented May 29, 2019

emilio commented Jun 3, 2019

emilio commented Jun 3, 2019

servo / servo

Join GitHub today

Do not use borrowed types in the selectors::Element trait. #23463

Do not use borrowed types in the selectors::Element trait. #23463

Conversation

RazrFalcon commented May 26, 2019 • edited

highfive commented May 26, 2019

highfive commented May 26, 2019

highfive commented May 26, 2019

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

emilio left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

RazrFalcon commented May 27, 2019

RazrFalcon commented May 29, 2019

emilio left a comment

RazrFalcon commented May 29, 2019

emilio commented Jun 3, 2019

emilio commented Jun 3, 2019

RazrFalcon commented May 26, 2019 •

edited