Closed Bug 1297322 Opened 3 years ago Closed 3 years ago

expose Gecko prefs to Servo so that they can influence CSS parsing

Categories

(Core :: CSS Parsing and Computation, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: heycam, Assigned: manishearth)

References

Details

Attachments

(2 files)

We have a bunch of CSS features in Gecko that are behind prefs, but Servo doesn't know anything about those prefs.  We need to expose the prefs and what features they control to Servo so that it can drop disabled properties/values/rules/etc.
It would be nice to avoid the yuckiness of having Servo call into Gecko to read prefs directly (among other things, that would make it tricky to do off-main-thread parsing should we ever decide to do that).

Could we create a struct of booleans (and corresponding rust-bindgen bindings), use a bunch of BoolVarCaches to maintain it, and pass a const reference to that struct to Servo when parsing stylesheets?
(Const reference would require Cells in the generated bindings, probably.)
(In reply to :Ms2ger (⌚ UTC+1/+2) from comment #2)
> (Const reference would require Cells in the generated bindings, probably.)

How come? I just mean that we pass it a pointer to that struct and promise not to change it for the duration of the parse call.
I guess that works, since the BoolVarCache could only run on the main thread, which is blocked at that time.
Sounds good to me.  We can put the pref names in the mako template property definitions, then generate the array of bools and the call to Gecko to register the BoolVarCaches.
(In reply to Cameron McCormack (:heycam) from comment #5)
> Sounds good to me.  We can put the pref names in the mako template property
> definitions, then generate the array of bools and the call to Gecko to
> register the BoolVarCaches.

That seems a bit complicated - would it work to just explicitly enumerate all of the prefs in a C++ struct, register the caches on the C++ side, and then expose a bindgen-ed view of that to Rust? Or are there really a zillion of these prefs?
I don't know about a zillion.  There are 24 unique prefs mentioned in nsCSSPropList.h in my checkout, with 74 properties referencing a pref.  There will be a few more to deal with individual property values.  Looks like we have some kind of pref change in nsCSSPropList.h (a new property behind a pref, or a removal of a pref) around once every few weeks.  Whether this is too much for an additional manually managed list of prefs, I'm not sure.  It's probably fine.
Can we make some clever macros that allow us to generate this struct by #including nsCSSPropList.h?
I don't think we can generate a struct that has a single member per pref with an generated identifier, since we define the pref to use in nsCSSPropList.h using a string literal, and we can't make the pre-processor generate an identifier from that.

But if you don't mind having storage for eCSSProperty_COUNT (~350) booleans, you could generate one booleam member per property, like:

  struct PropertyPrefs
  {
    bool mAll;
    bool mAlignContent;
    bool mAlignItems;
    ...
  };

where the non-pref controlled properties are just initialized with true.
(In reply to Cameron McCormack (:heycam) from comment #9)
> I don't think we can generate a struct that has a single member per pref
> with an generated identifier, since we define the pref to use in
> nsCSSPropList.h using a string literal, and we can't make the pre-processor
> generate an identifier from that.

But there are other arguments to the macro, right? i.e.:

CSS_PROP_BACKGROUND(
    background-blend-mode,
    background_blend_mode,
    BackgroundBlendMode,
    CSS_PROPERTY_PARSE_VALUE_LIST |
        CSS_PROPERTY_APPLIES_TO_FIRST_LETTER_AND_FIRST_LINE |
        CSS_PROPERTY_APPLIES_TO_PLACEHOLDER |
        CSS_PROPERTY_VALUE_LIST_USES_COMMAS,
    "layout.css.background-blend-mode.enabled",
    VARIANT_KEYWORD, // used by list parsing
    kBlendModeKTable,
    CSS_PROP_NO_OFFSET,
    eStyleAnimType_None)

Seems like we could just take the third argument (the method name) and use that as an ident?

I guess we might still need to distinguish the with-pref and without-pref cases, but probably not that much work.

> But if you don't mind having storage for eCSSProperty_COUNT (~350) booleans,
> you could generate one booleam member per property, like:
> 
>   struct PropertyPrefs
>   {
>     bool mAll;
>     bool mAlignContent;
>     bool mAlignItems;
>     ...
>   };
> 
> where the non-pref controlled properties are just initialized with true.

Given that it's a singleton this is probably fine, though less elegant on the bindgen side. Would this mean that we'd always check the bool unconditionally in Servo? It seems better to have mako only generate pref checks for the prefable properties (like we do now), but that's another list to maintain...
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #10)
> But there are other arguments to the macro, right? i.e.:
> 
> CSS_PROP_BACKGROUND(
>     background-blend-mode,
>     background_blend_mode,
>     BackgroundBlendMode,
>     CSS_PROPERTY_PARSE_VALUE_LIST |
>         CSS_PROPERTY_APPLIES_TO_FIRST_LETTER_AND_FIRST_LINE |
>         CSS_PROPERTY_APPLIES_TO_PLACEHOLDER |
>         CSS_PROPERTY_VALUE_LIST_USES_COMMAS,
>     "layout.css.background-blend-mode.enabled",
>     VARIANT_KEYWORD, // used by list parsing
>     kBlendModeKTable,
>     CSS_PROP_NO_OFFSET,
>     eStyleAnimType_None)
> 
> Seems like we could just take the third argument (the method name) and use
> that as an ident?
> 
> I guess we might still need to distinguish the with-pref and without-pref
> cases, but probably not that much work.

Ah.  I was thinking we'd want to avoid having multiple bools for the same pref (as often we use the same pref to control multiple properties), but yes if we're happy to have the 74 booleans rather than 24 booleans then we can use the property DOM name as the basis for the identifier.

> > But if you don't mind having storage for eCSSProperty_COUNT (~350) booleans,
> > you could generate one booleam member per property, like:
> > 
> >   struct PropertyPrefs
> >   {
> >     bool mAll;
> >     bool mAlignContent;
> >     bool mAlignItems;
> >     ...
> >   };
> > 
> > where the non-pref controlled properties are just initialized with true.
> 
> Given that it's a singleton this is probably fine, though less elegant on
> the bindgen side. Would this mean that we'd always check the bool
> unconditionally in Servo? It seems better to have mako only generate pref
> checks for the prefable properties (like we do now), but that's another list
> to maintain...

I think we can have the C++ pre-processor generate something that's a compile-time constant that says whether it's a pref controlled property or not.
(In reply to Cameron McCormack (:heycam) from comment #11)
> (In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #10)
> > But there are other arguments to the macro, right? i.e.:
> > 
> > CSS_PROP_BACKGROUND(
> >     background-blend-mode,
> >     background_blend_mode,
> >     BackgroundBlendMode,
> >     CSS_PROPERTY_PARSE_VALUE_LIST |
> >         CSS_PROPERTY_APPLIES_TO_FIRST_LETTER_AND_FIRST_LINE |
> >         CSS_PROPERTY_APPLIES_TO_PLACEHOLDER |
> >         CSS_PROPERTY_VALUE_LIST_USES_COMMAS,
> >     "layout.css.background-blend-mode.enabled",
> >     VARIANT_KEYWORD, // used by list parsing
> >     kBlendModeKTable,
> >     CSS_PROP_NO_OFFSET,
> >     eStyleAnimType_None)
> > 
> > Seems like we could just take the third argument (the method name) and use
> > that as an ident?
> > 
> > I guess we might still need to distinguish the with-pref and without-pref
> > cases, but probably not that much work.
> 
> Ah.  I was thinking we'd want to avoid having multiple bools for the same
> pref (as often we use the same pref to control multiple properties), but yes
> if we're happy to have the 74 booleans rather than 24 booleans then we can
> use the property DOM name as the basis for the identifier.
> 
> > > But if you don't mind having storage for eCSSProperty_COUNT (~350) booleans,
> > > you could generate one booleam member per property, like:
> > > 
> > >   struct PropertyPrefs
> > >   {
> > >     bool mAll;
> > >     bool mAlignContent;
> > >     bool mAlignItems;
> > >     ...
> > >   };
> > > 
> > > where the non-pref controlled properties are just initialized with true.
> > 
> > Given that it's a singleton this is probably fine, though less elegant on
> > the bindgen side. Would this mean that we'd always check the bool
> > unconditionally in Servo? It seems better to have mako only generate pref
> > checks for the prefable properties (like we do now), but that's another list
> > to maintain...
> 
> I think we can have the C++ pre-processor generate something that's a
> compile-time constant that says whether it's a pref controlled property or
> not.

Yeah, but if we don't know it at compile time, then Mako needs to unconditionally generate pref-checking code, which could then be optimized out at compile time with your proposed scheme. I think we'd still need to big list of booleans in this case, though it might be worth it to have this problem taken care of automatically.

The other solution would be to to encode the pref control relationships with a python file. This could be imported by mako, and also used by gecko at build time to spit out the appropriate macros (similar to the generation of the style struct list we have today).

I think I probably prefer the big static list of booleans plus compile-time constants to optimize out the lookups for the non-prefable bits. Making it a bitfield would be even better, but that might get tricky with bindgen.
I just realized we already have nsCSSProps::gPropertyEnabled, which is a static list of booleans with BoolVarCaches already added.  We can bindgen nsCSSProps and then add some macro-generated enums indicating whether a given property is prefable.
Priority: -- → P3
Manish said he'd work on this.
Assignee: nobody → manishearth
Flags: needinfo?(manishearth)
Manish, I'm glad you made it work with bindgen that way, but I wonder why not using the `gPropertyEnabled` table instead of doing an FFI call just for that?
Also, why the need of exposing the length separately? Why not using the name and checking `.len()`? They're constants anyway.
Comment on attachment 8825249 [details]
Bug 1297322: stylo: Hook property parsing into Gecko prefs;

https://reviewboard.mozilla.org/r/103422/#review104038

::: servo/components/style/properties/properties.mako.rs:919
(Diff revision 1)
> +            # itself?
> +            pref_ident = property.ident
> +            if pref_ident == "float":
> +                pref_ident = "float_"
> +        %>
> +        if structs::root::mozilla::SERVO_PREF_NAME_${pref_ident}.len() > 1 {

I'm a bit afraid that rustc won't optimize this branch out. The array size is known statically (and is part of a `const`), so this *should* work, but llvm works in mysterious ways. I'm not sure if we can rely on this (will ask around)

The alternative is to have `const size_t SERVO_PREF_LEN_foo = sizeof(pref)` on the gecko side. That won't require `const char*` support, and is probably more guaranteed an optimization.
> Also, why the need of exposing the length separately? Why not using the name and checking `.len()`? They're constants anyway.

That's exactly what I was doing; I messed up with the git add so you saw a previous version of the proplist file.

> Manish, I'm glad you made it work with bindgen that way, but I wonder why not using the `gPropertyEnabled` table instead of doing an FFI call just for that?

The consts have to exist since it needs to get optimized away in the common case. The FFI call is called in the rare case you come across a disabled prop, so it's no big deal. I prefer doing this over statics (safer, and uses the public API from nsCSSProps) but am okay with changing it to use a static.
> I'm a bit afraid that rustc won't optimize this branch out.

It does not in debug mode. I suspect using a regular const will not fare better. Trying release.
Flags: needinfo?(manishearth)
While I wait for a stylo release build to complete, testing it out with a simple rust program shows that llvm is able to optimize out both direct access to a length constant and accessing a known-size array via `.len()`, in release mode. In no-opt mode neither are touched.
> I'm a bit afraid that rustc won't optimize this branch out.

It does in release mode. Debug mode has a few hundred occurrences of Gecko_PropertyId_IsPrefEnabled, whereas release has 23 (so the related branches must have been optimized out).

I feel like we can rely on this.
Comment on attachment 8825248 [details]
Bug 1297322: stylo: Expose property-pref mappings;

https://reviewboard.mozilla.org/r/103420/#review104826

::: layout/style/ServoPropPrefList.h:1
(Diff revision 2)
> +#ifndef mozilla_ServoPropPrefList_h

Nit: please include the license / modeline boilerplate at the top of the file, followed by a newline.  Also please put newlines around the #include, and don't bother indenting the macros by four spaces.

::: layout/style/ServoPropPrefList.h:3
(Diff revision 2)
> +#ifndef mozilla_ServoPropPrefList_h
> +#define mozilla_ServoPropPrefList_h
> +#include <cstddef>

What do we need this include for, btw?

::: layout/style/ServoPropPrefList.h:8
(Diff revision 2)
> +#include <cstddef>
> +namespace mozilla {
> +
> +    #define CSS_PROP(name_, id_, method_, flags_, pref_, parsevariant_, kwtable_, \
> +                     stylestruct_, stylestructoffset_, animtype_)                 \
> +        const char* SERVO_PREF_NAME_##id_ = pref_;

Per IRC comments, let's make these bools rather than char*s, with a value like |sizeof(pref_) == 1|, since we don't need the actual pref name on the Servo side.
Comment on attachment 8825249 [details]
Bug 1297322: stylo: Hook property parsing into Gecko prefs;

https://reviewboard.mozilla.org/r/103422/#review104828

r=me with the consequential changes after making the part 1 consts bools.  (I'd be happy too with reading directly out of nsCSSProps::gPropertyPrefs, but since there shouldn't be too many pref-enabled properties we encounter, it's fine to use the FFI call.)

::: layout/style/ServoBindings.cpp:1058
(Diff revision 2)
>    return &aCSSValue->GetArrayValue()->Item(aIndex);
>  }
>  
> +
> +bool
> +Gecko_PropertyId_IsPrefEnabled(nsCSSPropertyID id) {

Nit: { on new line, and since we're in the .cpp file here, call the argument aID.
Attachment #8825249 - Flags: review?(cam) → review+
Comment on attachment 8825248 [details]
Bug 1297322: stylo: Expose property-pref mappings;

https://reviewboard.mozilla.org/r/103420/#review105040

Looks good!
Attachment #8825248 - Flags: review?(cam) → review+
Pushed by manishearth@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/dc8fefd833ac
stylo: Expose property-pref mappings; r=heycam
https://hg.mozilla.org/integration/autoland/rev/d26f3e43c32d
stylo: Hook property parsing into Gecko prefs; r=heycam
https://hg.mozilla.org/mozilla-central/rev/dc8fefd833ac
https://hg.mozilla.org/mozilla-central/rev/d26f3e43c32d
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.