selectors::Element without borrowed types

[RazrFalcon]

Currently, selectors::Element defines two methods that require borrowed data: local_name and namespace.

The problem is that a usual rc-tree unable to produce a reference that will live long enough. The kuchiki crate uses the NodeDataRef hack that involves unsafe, which is unacceptable in my case.

Can those methods be replaced with something like has_local_name and has_namespace?

@SimonSapin

[SimonSapin]

I think NodeDataRef uses the same technique as https://crates.io/crates/owning_ref. Would that be more acceptable?

@emilio, any thoughts on this? I think originally we had accessors rather than boolean testing in order to support hash maps indexed by local named, ID, class, etc. But those have since moved out of the selectors crate to Servo’s and Firefox style crate.

[RazrFalcon]

Should I use owning_ref for Node itself or for BorrowedLocalName? Because it doesn't work in both cases (kinda).

error[E0117]: only traits defined in the current crate can be implemented for arbitrary types
   --> src/parser/css2.rs:131:1
    |
131 | impl selectors::Element for OwningRef<Node, NodeData> {
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ impl doesn't use types inside crate
    |
    = note: the impl does not reference only types defined in this crate
    = note: define and implement a trait or new type instead

[SimonSapin]

Oh sorry I misremembered. Element::local_name returns &Something (because of lack of generic associated types), so OwningRef likely won’t help there.

Is the problem that you have entire nodes inside a bug RefCell, which needs to stay borrowed while the local name is accessed? In that case what Kuchiki does different that’s relevant is not NodeDataRef, but rather having some components of nodes in their individual smaller Cell or RefCell. This does not include the element’s name, with the trade-off that the only way to change an element’s name is to create a new one and move the other data and child nodes.

[RazrFalcon]

Yes, the usual Rc<RefCell<T>> mantra based on rctree. So for my use case has_* methods are preferred.

And since there are no other CSS parsers, I have to find at least some solution. So any suggestions are welcome.

[emilio]

has_local_name seems fine, but you need something else to deal with nth-of-type (has_same_local_name_as?). It seems a bit unfortunate though.

[SimonSapin]

As far as I can tell the only three possible resolutions here are:

• Replace local_name with has_local_name and same_local_name_as. I agree feels awkward, but it might the only way that doesn’t involve distruptive changes to other code or unknown time delays.

• Restructure rctree to remove the One Big RefCell. Instead, make it more like Kuchiki and have each link to other nodes in individual Cells (which does support !Copy types since 1.17, but some extension traits make that easier to use). Then, the user of rctree can choose to have only parts of their payload (the T in Node<T>) be in smaller Cells or RefCells. In particular, if the local name is not in a cell, the implementation of selectors::Element::local_name can safely return a &LocalName reference.

• Wait until Generic Associated Types are stable in the language, and have Element::local_name(&'a self) return Self::LocalName<'a> instead of &'a Self::LocalName. That way, your associated type can be for example a std::cell::Ref value obtained with Ref::map, or an owned values that was cloned out of the node. I think this is the “best” solution, but unfortunately GATs likely won’t be ready for months or more.

[RazrFalcon]

Hi. I've tried the second method and it made API a bit nicer, since I don't need Ref and RefMut this way, but in the end I have to wrap all my data in RefCells, which is horrible and no go for me. Am I missing something?

Here is the struct that I put in the rctree. And all fields must be mutable, so I have to wrap all of them in RefCell.

[SimonSapin]

I don’t think you’re missing anything. This particular case just hadn’t come up before, element name and namespace are immutable in the (web’s) DOM.

This leave GATs, which will likely take longer than when I assume you would like to use selectors in your project, or has_local_name/same_local_name_as. @emilio How would you feel about migrating Stylo to that API?

[RazrFalcon]

element name and namespace are immutable in the (web’s) DOM.

Oh, I see.

Yes, I want to use cssparser/selectors in svgdom->resvg.

[emilio]

Do you really need a mutable tag / namespace?

I'm ok changing the style system to such an API, though I find it still slightly unfortunate :)

[RazrFalcon]

The problem is that I'm using rctree, which doesn't support this at all. So I have to rewrite rctree and svgdom completely to be able to support this.

[RazrFalcon]

Any updates?

[emilio]

Patches welcome I guess? I'm happy to review such a change.

[RazrFalcon]

I hope that I will be able at least build servo from sources...

[RazrFalcon]

As expected:

error: failed to run custom build command for `gstreamer-webrtc-sys v0.7.0`
process didn't exit successfully: `/media/data/servo/target/debug/build/gstreamer-webrtc-sys-0ea001806f557833/build-script-build` (exit code: 1)
--- stderr
`"pkg-config" "--libs" "--cflags" "gstreamer-webrtc-1.0 >= 1.14"` did not exit successfully: exit code: 1
--- stderr
Package gstreamer-webrtc-1.0 was not found in the pkg-config search path.
Perhaps you should add the directory containing `gstreamer-webrtc-1.0.pc'
to the PKG_CONFIG_PATH environment variable
No package 'gstreamer-webrtc-1.0' found

Looks like gstreamer-webrtc isn't available on gentoo at all.

#23050

UPD: it was my mistake. I was able to build it on gentoo just fine.

[jdm]

You can probably use ./mach build -p style to work around the gstreamer build issue for the time being.

[RazrFalcon]

@jdm Thanks! I was able to build it now.

[RazrFalcon]

Here is my attempt: #23463

Mostly failed...

[SimonSapin]

@RazrFalcon Does this new API work for your use case? https://doc.servo.org/selectors/trait.Element.html#tymethod.has_local_name

I can publish to crates.io. Is there anything else to tweak before that?

[RazrFalcon]

Yep, I've already tested it and it works perfectly. Thanks.