Closed
Bug 1348165
Opened 8 years ago
Closed 7 years ago
stylo: Support getting authored property value
Categories
(Core :: CSS Parsing and Computation, enhancement, P2)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
WONTFIX
People
(Reporter: xidorn, Assigned: bradwerth)
References
Details
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.
Reporter | ||
Comment 1•8 years ago
|
||
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.
Blocks: stylo-devtools
Updated•8 years ago
|
Priority: -- → P2
Updated•8 years ago
|
Assignee: nobody → mbrubeck
Priority: P2 → P1
Comment 2•8 years ago
|
||
Actually, bug 1349651 is higher priority, so swapping that one out for this one.
Assignee: mbrubeck → nobody
Priority: P1 → P2
Comment 3•8 years ago
|
||
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.
Assignee: nobody → mbrubeck
Reporter | ||
Comment 4•7 years ago
|
||
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.
Assignee: mbrubeck → xidorn+moz
Reporter | ||
Comment 5•7 years ago
|
||
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
Reporter | ||
Comment 6•7 years ago
|
||
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
Reporter | ||
Comment 7•7 years ago
|
||
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
Flags: needinfo?(simon.sapin)
Reporter | ||
Comment 8•7 years ago
|
||
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•7 years ago
|
||
> 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.
Flags: needinfo?(simon.sapin)
Reporter | ||
Comment 10•7 years ago
|
||
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•7 years ago
|
||
(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.
Assignee | ||
Updated•7 years ago
|
Assignee: xidorn+moz → bwerth
Comment 12•7 years ago
|
||
Note that devtools doesn't seem to call this according to bug 1302513... It'd be unfortunate to implement this just for nothing.
See Also: → 1302513
Comment 13•7 years ago
|
||
jryans, do you know if devtools actually call this API? Seems like we should just get rid of it if not.
Flags: needinfo?(jryans)
(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. :(
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: needinfo?(jryans)
Resolution: --- → WONTFIX
You need to log in
before you can comment on or make changes to this bug.
Description
•