Closed
Bug 1448225
Opened 7 years ago
Closed 7 years ago
Convert StylePrefs to StaticPrefs
Categories
(Core :: CSS Parsing and Computation, enhancement, P3)
Core
CSS Parsing and Computation
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.
Comment 1•7 years ago
|
||
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.
Comment 2•7 years ago
|
||
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.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 4•7 years ago
|
||
> 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 :)
Comment 5•7 years ago
|
||
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 6•7 years ago
|
||
mozreview-review |
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+
Assignee | ||
Comment 8•7 years ago
|
||
> 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.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 10•7 years ago
|
||
The Servo PR for this bug is at https://github.com/servo/servo/pull/20508.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 12•7 years ago
|
||
> 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.
Comment 13•7 years ago
|
||
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/2ee160335e15
Convert StylePrefs to StaticPrefs. r=emilio
Comment 14•7 years ago
|
||
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.
Assignee | ||
Comment 15•7 years ago
|
||
(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...
Comment 16•7 years ago
|
||
(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.
Comment 17•7 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Updated•7 years ago
|
Assignee: nobody → n.nethercote
You need to log in
before you can comment on or make changes to this bug.
Description
•