stylo: Have check_allowed_in read the Gecko bool table directly

RESOLVED FIXED in Firefox 56

Status

()

RESOLVED FIXED
a year ago
a year ago

People

(Reporter: xidorn, Assigned: xidorn)

Tracking

(Blocks: 1 bug)

Trunk
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

a year ago
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 hidden (mozreview-request)
(Assignee)

Comment 2

a year ago
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.)
(Assignee)

Updated

a year ago
See Also: → bug 1381693

Comment 3

a year ago
mozreview-review
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).
(Assignee)

Comment 4

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

Comment 5

a year ago
mozreview-review
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+
(Assignee)

Comment 6

a year ago
Ok, so this is blocked on servo/rust-bindgen#822.
(Assignee)

Updated

a year ago
Depends on: 1385147
(Assignee)

Comment 7

a year ago
Servo side: servo/servo#17913
Comment hidden (mozreview-request)

Comment 9

a year ago
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a0735c6f283a
Have check_allowed_in read gPropertyEabled table directly. r=manishearth

Comment 10

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a0735c6f283a
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.