1348165 - stylo: Support getting authored property value

Status: RESOLVED WONTFIX

(Core :: CSS Parsing and Computation, enhancement, P2)

Description

[Xidorn Quan [:xidorn] UTC+11]

layout/style/test/chrome/test_author_specified_style.html is testing that we get the color value in a way similar to how author specifies it.

This basically means we need to have a real implementation of ServoDeclarationBlock::GetAuthoredPropertyValue rather than simply redirecting it to GetPropertyValue.

Servo stores an authored string if the value is an identifier, but it doesn't do much otherwise. Also Servo doesn't have a mechanism to distinguish between authored serialization and normalized serialization like Gecko.

We may need to add the mechanism to Servo and store some additional information for color there.

Comment 1

[Xidorn Quan [:xidorn] UTC+11]

There could be some non-trivial work for this in the Servo side (to implement the mechanism distinguishing authored serialization and normalized serialization). This is mostly for devtools support because they need to show the authored one in the panel.

Comment 2

[Bobby Holley (:bholley)]

Actually, bug 1349651 is higher priority, so swapping that one out for this one.

Comment 3

[Bobby Holley (:bholley)]

I guess this is the same set of code as bug 1349651 though, so probably makes sense for matt to work on this after that one.

Comment 4

[Xidorn Quan [:xidorn] UTC+11]

This doesn't seem to be something related to bug 1349651, so probably it makes less sense to assign to matt.

Stealing since I have some idea about how should we do this.

Comment 5

[Xidorn Quan [:xidorn] UTC+11]

Some note to myself:

In Gecko, we only do authored serialization for color values. (Searching reference of nsCSSValue::Serialization::eNormalized and nsCSSValue::Serialization::eAuthorSpecified, the only place which checks the value is [1]).

I think we need to do the following things to fix this:
1. change cssparser to make it return more detailed info for color values like what we have in nsCSSValue [2]
2. create a new method in ToCss trait, probably called to_authored_css, which by default just calls to_css.
3. impl to_authored_css for on all possible routes to color values

It is not completely clear how extensive the third step would be. Alternatively we can add a parameter to to_css just like what Gecko does, but I suspect that would need touching more code than this approach.

[1] https://dxr.mozilla.org/mozilla-central/rev/5801aa478de12a62b2b2982659e787fcc4268d67/layout/style/nsCSSValue.cpp#1679
[2] https://dxr.mozilla.org/mozilla-central/rev/5801aa478de12a62b2b2982659e787fcc4268d67/layout/style/nsCSSValue.h#494-510

Comment 6

[Xidorn Quan [:xidorn] UTC+11]

Another alternative is to create a new struct in cssparser to replace the W parameter. It can be something like
> struct CssFormatter<W> {
>     dest: W,
>     authored: bool,
> }

A side note that, it seems Rust's std::fmt::Formatter uses trait object for the dest field [1]. If we use that as well instead of the generic way above, the impl of to_css can be simplified (since no generic parameter is needed anymore), and it would also potentially save code size if we are passing several different types to to_css.


[1] https://github.com/rust-lang/rust/blob/e40ef964fe491b19c22dfb8dd36d1eab14223c36/src/libcore/fmt/mod.rs#L238

Comment 7

[Xidorn Quan [:xidorn] UTC+11]

Simon, which way do you think would be the best?

The candidates would be:
(1) add to_authored_css to ToCss and implement it separately for all needed paths
(2) add an additional parameter to to_css
(3) replace the generic Write parameter with a CssFormatter which holds a generic Write and the flag
(4) same as (3) but hold a trait object &Write instead in CssFormatter

Comment 8

[Xidorn Quan [:xidorn] UTC+11]

I assume that serialization isn't a very performance-critical area, and dynamic dispatch wouldn't lead to much slowness for this case since most of successive virtual calls would be from the same vtable, so it shouldn't have much cache miss. I would personally prefer (4).

Comment 9

[Simon Sapin (:SimonSapin)]

> I assume that serialization isn't a very performance-critical area

It is, for Element.style’s getters: bug 1355599. I don’t think we should add virtual calls. In fact I’d like to try https://crates.io/crates/fast_fmt instead of std::fmt. One of the key differences is that (the ambitiously-named, we’ll have to see how that goes in practice) fast_fmt does not use dynamic dispatch where std::fmt does.

I think (1) is error prone: if we forget to override to_authored_css in the impl for, say, linear-gradient(), then the color stops will silently have the "non-author" serialization. Worse, when we do override it we’d have to duplicate the serialization logic for these types, all the way from "roots" (property values) to all cases that involve colors.

This leaves (2) and (3). I don’t have a strong opinion between those. They both require touching every ToCss impls, but in both cases most of it might be doable with sed.

Comment 10

[Xidorn Quan [:xidorn] UTC+11]

I suspect how much burden the virtual call would add...

Anyway, if you think we shouldn't risk adding a virtual call here, I'd probably go (3). I feel that way would be more extendable when we want to add more options in the future. (We probably wouldn't be adding anything else, but it isn't worse than (2) in any aspect as far as I can see.)

Comment 11

[Bobby Holley (:bholley)]

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #10)
> I suspect how much burden the virtual call would add...

Virtual calls are certainly not cheap - on the order of an RMU atomic operation even in the case where everything is in-cache.

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Note that devtools doesn't seem to call this according to bug 1302513... It'd be unfortunate to implement this just for nothing.

Comment 13

[Emilio Cobos Álvarez (:emilio)]

jryans, do you know if devtools actually call this API? Seems like we should just get rid of it if not.

Comment 14

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #13)
> jryans, do you know if devtools actually call this API? Seems like we should
> just get rid of it if not.

Hmm, I agree, it does appear unused, and Tom's comments in bug 1302513 seem to confirm this.  I think I just assumed this was needed because I was aware of the DevTools as authored feature, and I saw this existing bug on file, so I didn't investigate further than that.

Sounds like we can't WONTFIX this and work on removal in bug 1302513.

Thanks to everyone who thought about the design here, and I'm sorry if time was wasted on this. :(