Closed Bug 1448225 Opened 6 years ago Closed 6 years ago

Convert StylePrefs to StaticPrefs

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: n.nethercote, Assigned: n.nethercote)

References

Details

Attachments

(1 file)

The new StaticPrefs mechanism can replace StylePrefs.
Mind that StylePrefs is there for sharing the prefs value with Rust code, so any new mechanism replacing it needs to be accessible there as well.
Looking at the patch in bug 1436655, I suspect your current approach doesn't quite fit into the requirement of StylePrefs, because using a function to get and init the stuff means we would need to use FFI for that, which is not scalable and not performant, and that's the reason why we created this standalone struct.
> I suspect your current approach doesn't quite fit into the requirement of StylePrefs

My patch is now attached. I recommend reading it instead of imagining what it looks like :)
OK, that looks fine. I guess I somehow missed some part of your patch in bug 1436655, although I'm still a bit concerned about the build time indication I mentioned there.
Comment on attachment 8961643 [details]
Bug 1448225 - Convert StylePrefs to StaticPrefs.

https://reviewboard.mozilla.org/r/230522/#review236036

Looks fine, modulo the nits. I think it's not great to need to worry about function calls when reading prefs, though I don't see any callsite here off-hand where it'd be a problem enough to be a show-stopper...

That being said, maybe those functions should be inline? They only return a static after all.

::: layout/base/nsCSSFrameConstructor.cpp:4545
(Diff revision 1)
>  
>    // If we're emulating -moz-box with flexbox, then treat it as non-XUL and
>    // return null (except for scrollcorners which have to be XUL becuase their
>    // parent reflows them with BoxReflow() which means they have to get
>    // actual-XUL frames).
> -  if (StylePrefs::sEmulateMozBoxWithFlex &&
> +  if (StaticPrefs::layout_css_emulate_moz_box_with_flex() &&

So these involve now a function call instead of just a static variable access, which is hopefully just a cache miss.

Some of them can be somewhat hot, is there something  on the plans to potentially improve it?

::: modules/libpref/init/StaticPrefList.h:111
(Diff revision 1)
> +//---------------------------------------------------------------------------
> +
> +VARCACHE_PREF(
> +  "gfx.font_rendering.opentype_svg.enabled",
> +   gfx_font_rendering_opentype_svg_enabled,
> +   bool, true

nit: Indentation looks inconsistent, you put the type indented more to the left in some cases, and to the right in others.

::: servo/components/style/gecko/generated/structs.rs:12266
(Diff revision 1)
> -            pub static mut StylePrefs_sEmulateMozBoxWithFlex: bool;
> +            pub static mut StaticPrefs_sVarCache_layout_css_emulate_moz_box_with_flex: bool;
>          }
>          #[test]
> -        fn bindgen_test_layout_StylePrefs() {
> +        fn bindgen_test_layout_StaticPrefs() {
>              assert_eq!(
> -                ::std::mem::size_of::<StylePrefs>(),
> +                ::std::mem::size_of::<StaticPrefs>(),

FWIW this file is autogenerated, you can generate the proper one from taskcluster (there's a `stylo-bindings` artifact there). You'd need to use one from non-debug builds, that's all.

::: servo/components/style/gecko/media_queries.rs:613
(Diff revision 1)
>                  }
>  
>                  let result = {
>                      let mut feature_name = &**ident;
>  
> -                    if unsafe { structs::StylePrefs_sWebkitPrefixedAliasesEnabled } &&
> +                    if unsafe { structs::StaticPrefs_sVarCache_layout_css_prefixes_webkit } &&

Would it be reasonable to use the same machinery we use to generate the `atom!` macro to generate a `pref!` macro?

That way we could do `pref!("layout.css.prefixes.webkit")` from here, which looks much nicer.

Maybe file a followup for that? I can look into it if you're busy with other stuff.

Maybe it'd be nice to do it in a standalone crate so it's available to the rest of Gecko code too instead of just to `style`.
Attachment #8961643 - Flags: review?(emilio) → review+
[Triage 2018/03/23 - P3]
Priority: -- → P3
> That being said, maybe those functions should be inline? They only return a
> static after all.

True. I'll put the definition in StaticPrefs.h so it can be inlined. In an earlier version of this patch that was difficult but some other things in libpref have changed, so it's now possible.
The Servo PR for this bug is at https://github.com/servo/servo/pull/20508.
> Would it be reasonable to use the same machinery we use to generate the
> `atom!` macro to generate a `pref!` macro?
> 
> That way we could do `pref!("layout.css.prefixes.webkit")` from here, which
> looks much nicer.
> 
> Maybe file a followup for that? I can look into it if you're busy with other
> stuff.

A follow-up sounds good.
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/2ee160335e15
Convert StylePrefs to StaticPrefs. r=emilio
We're sorry - something has gone wrong while rewriting or rebasing your commits. The commits being pushed no longer match what was requested. Please file a bug.
(In reply to Mozilla Autoland from comment #14)
> We're sorry - something has gone wrong while rewriting or rebasing your
> commits. The commits being pushed no longer match what was requested. Please
> file a bug.

I don't know what to make of this, given that comment 13 shows that the autoland succeeded...
(In reply to Nicholas Nethercote [:njn] from comment #15)
> (In reply to Mozilla Autoland from comment #14)
> > We're sorry - something has gone wrong while rewriting or rebasing your
> > commits. The commits being pushed no longer match what was requested. Please
> > file a bug.
> 
> I don't know what to make of this, given that comment 13 shows that the
> autoland succeeded...

I had to push manually because the tree was closed, then autoland couldn't land anything.
https://hg.mozilla.org/mozilla-central/rev/2ee160335e15
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Assignee: nobody → n.nethercote
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: