Closed Bug 1381690 Opened 5 years ago Closed 5 years ago

stylo: Have check_allowed_in read the Gecko bool table directly

Categories

(Core :: CSS Parsing and Computation, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: xidorn, Assigned: xidorn)

References

Details

Attachments

(1 file)

It doesn't make sense to go through a FFI function to check the enable state of a property given that the table is accessible from Servo side directly.
I thought about also removing the EXPERIMENTAL table at the same time, but I didn't, because I suppose using a bit-set to do a pre-check may be more efficient that is more compact in memory, thus need less memory bandwidth for majority of properties.

If one day we can have pref cache work on a bit-set directly, we should remove that table for Gecko.

(Actually, it is possible. We can just use a callback rather than using pref bool cache.)
See Also: → 1381693
Comment on attachment 8887272 [details]
Bug 1381690 - Have check_allowed_in read gPropertyEabled table directly.

https://reviewboard.mozilla.org/r/158068/#review163250

::: servo/components/style/properties/properties.mako.rs:1179
(Diff revision 1)
>                  }
>              % endif
>              % if product == "gecko":
> +                use gecko_bindings::structs;
>                  let id = self.to_nscsspropertyid().unwrap();
> -                unsafe { bindings::Gecko_PropertyId_IsPrefEnabled(id) }
> +                unsafe { structs::nsCSSProps_gPropertyEnabled[id as usize] }

I'm not sure this will make the linker happy in Servo's CI, but it's probably ok.

While you're here, you can also change `Gecko_GetMediaFeatures`, and use `nsMediaFeatures::features` if you want and it works instead, with r=me (or file a bug and I can look into it).
Seems to work fine locally with "mach build-geckolib".
Comment on attachment 8887272 [details]
Bug 1381690 - Have check_allowed_in read gPropertyEabled table directly.

https://reviewboard.mozilla.org/r/158068/#review163294
Attachment #8887272 - Flags: review?(manishearth) → review+
Ok, so this is blocked on servo/rust-bindgen#822.
Depends on: 1385147
Servo side: servo/servo#17913
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a0735c6f283a
Have check_allowed_in read gPropertyEabled table directly. r=manishearth
https://hg.mozilla.org/mozilla-central/rev/a0735c6f283a
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.