1323147 - stylo: Use string and nsCSSPropertyID for stylo CSSOM FFI directly rather than using Atom

Status: RESOLVED FIXED

(Core :: DOM: CSS Object Model, defect)

Description

[Xidorn Quan [:xidorn] UTC+10]

In servo/servo#14535, Simon changes the Servo impl to use enum for properties, and thus it no longer makes much sense to use atom for passing property names across the FFI.

This bug is about changing the Gecko side code (as well as the glue code in Servo side) to remove the unnecessary additional conversion.

Comment 1

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1323147 part 1 - Pass string and nsCSSPropertyID for property names across FFI.

Review commit: https://reviewboard.mozilla.org/r/98340/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/98340/

Comment 2

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1323147 part 2 - Remove static atoms of CSS properties.

This is basically a backout of bug 1294299 part 7.

Review commit: https://reviewboard.mozilla.org/r/98342/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/98342/

Comment 3

[Cameron McCormack (:heycam)]

Comment on attachment 8818204 [details]
Bug 1323147 part 1 - Pass string and nsCSSPropertyID for property names across FFI.

https://reviewboard.mozilla.org/r/98340/#review98544

::: dom/animation/KeyframeEffectReadOnly.cpp:1027
(Diff revision 1)
>          Servo_DeclarationBlock_SerializeOneValue(
> -          propertyValue.mServoDeclarationBlock, atom, false, &stringValue);
> +          propertyValue.mServoDeclarationBlock,
> +          propertyValue.mProperty, &stringValue);

Can you add an assertion in here that the property value is < eCSSProperty_COUNT_no_shorthands?

::: servo/ports/geckolib/glue.rs:613
(Diff revision 1)
> +macro_rules! get_property_id_from_nscsspropertyid {
> +    ($property_id: ident, $ret: expr) => {{
> +        match PropertyId::from_nscsspropertyid($property_id) {
> +            Ok(property_id) => property_id,
> +            Err(()) => { return $ret; }
> +        }
> +    }}
> +}

Eventually it should be an error if we have an nsCSSPropertyID that we can't turn into a PropertyId.  Can you add a warn!() or something in here in the error case?  (Later, once we have full property coverage, we can make it a debug_assert!().)

Comment 4

[Cameron McCormack (:heycam)]

Comment on attachment 8818205 [details]
Bug 1323147 part 2 - Remove static atoms of CSS properties.

https://reviewboard.mozilla.org/r/98342/#review98546

Comment 5

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8818204 [details]
Bug 1323147 part 1 - Pass string and nsCSSPropertyID for property names across FFI.

https://reviewboard.mozilla.org/r/98340/#review98544

> Can you add an assertion in here that the property value is < eCSSProperty_COUNT_no_shorthands?

From the code below, it seems it really can have shorthands here?

> Eventually it should be an error if we have an nsCSSPropertyID that we can't turn into a PropertyId.  Can you add a warn!() or something in here in the error case?  (Later, once we have full property coverage, we can make it a debug_assert!().)

I think that should be something inside `PropertyId::from_nscsspropertyid`. We can do that there as a followup in Servo side.

Actually I think in the future it should simply be an infallible `impl From<nsCSSPropertyID> for PropertyId`, and then we don't really need this macro anymore.

Comment 6

[Cameron McCormack (:heycam)]

Comment on attachment 8818204 [details]
Bug 1323147 part 1 - Pass string and nsCSSPropertyID for property names across FFI.

https://reviewboard.mozilla.org/r/98340/#review98544

> I think that should be something inside `PropertyId::from_nscsspropertyid`. We can do that there as a followup in Servo side.
> 
> Actually I think in the future it should simply be an infallible `impl From<nsCSSPropertyID> for PropertyId`, and then we don't really need this macro anymore.

Sounds fine to do it as a followup, thanks.

Comment 7

[Cameron McCormack (:heycam)]

Comment on attachment 8818204 [details]
Bug 1323147 part 1 - Pass string and nsCSSPropertyID for property names across FFI.

https://reviewboard.mozilla.org/r/98340/#review98554

::: dom/animation/KeyframeEffectReadOnly.cpp:1027
(Diff revision 1)
>          Servo_DeclarationBlock_SerializeOneValue(
> -          propertyValue.mServoDeclarationBlock, atom, false, &stringValue);
> +          propertyValue.mServoDeclarationBlock,
> +          propertyValue.mProperty, &stringValue);

Oh, yes, sorry: eCSSProperty_COUNT.

Although, if we add the assertion on the Servo side, we won't need it here.  So let's do that instead.

Comment 8

[Simon Sapin (:SimonSapin)]

Comment on attachment 8818204 [details]
Bug 1323147 part 1 - Pass string and nsCSSPropertyID for property names across FFI.

https://reviewboard.mozilla.org/r/98340/#review99144

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a738fdc3ca07
part 1 - Pass string and nsCSSPropertyID for property names across FFI. r=heycam,SimonSapin
https://hg.mozilla.org/integration/mozilla-inbound/rev/bede335a0b59
part 2 - Remove static atoms of CSS properties. r=heycam

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a738fdc3ca07
https://hg.mozilla.org/mozilla-central/rev/bede335a0b59