1381690 - stylo: Have check_allowed_in read the Gecko bool table directly

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Description

[Xidorn Quan [:xidorn] UTC+10]

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.

Comment 1

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1381690 - Have check_allowed_in read gPropertyEabled table directly.

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

Comment 2

[Xidorn Quan [:xidorn] UTC+10]

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.)

Comment 3

[Emilio Cobos Álvarez [:emilio]]

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).

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

Seems to work fine locally with "mach build-geckolib".

Comment 5

[Manish Goregaokar [:manishearth]]

Comment on attachment 8887272 [details]
Bug 1381690 - Have check_allowed_in read gPropertyEabled table directly.

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

Comment 6

[Xidorn Quan [:xidorn] UTC+10]

Ok, so this is blocked on servo/rust-bindgen#822.

Comment 7

[Xidorn Quan [:xidorn] UTC+10]

Servo side: servo/servo#17913

Comment 8

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8887272 [details]
Bug 1381690 - Have check_allowed_in read gPropertyEabled table directly.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/158068/diff/1-2/

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a0735c6f283a
Have check_allowed_in read gPropertyEabled table directly. r=manishearth

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a0735c6f283a