Closed Bug 1559814 Opened 1 year ago Closed 1 year ago

Use cbindgen for grid line properties.

Categories

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

task

Tracking

()

RESOLVED FIXED
mozilla69
Tracking Status
firefox69 --- fixed

People

(Reporter: emilio, Assigned: emilio)

References

Details

Attachments

(5 files)

This will also introduce the ability to generate style system constants via cbindgen.

Type: defect → task
Priority: -- → P3

Option<> is not FFI-safe, so if we want to use the same representation
everywhere we need to get rid of it. This also makes it take the same amount of
memory as the C++ representation, and it's not very complex, I'd think.

This needs https://github.com/eqrion/cbindgen/pull/362, but I expect it to be
uncontroversial. I'll add a patch to this bug when it's merged to update it.

cbindgen historically didn't include these, but it turns out to be pretty useful
to generate constants for the style crate (since the binding crate is
servo/ports/geckolib).

An alternative is to get a completely different cbindgen-generated header for
these, but that seems a bit wasteful. This generates the constants with the
Style prefix (so we'll get StyleMAX_GRID_LINE for example), which is very
ugly. But we probably want to eventually stop using the Style prefix and use a
namespace instead, plus it's trivial to do auto kMaxLine = StyleMAX_GRID_LINE,
for example, so it's probably not a huge deal.

Another alternative would be to use associated consts, which are generated by
cbindgen. Something like:

struct GridConstants([u8; 0]);
impl GridConstants {
    const MAX_GRID_LINE: i32 = 10000;
}

Which would yield something like:

static const int32 StyleGridConstants_MAX_GRID_LINE = 10000;

I'm not sure if you find it preferrable, but I'm also happy to change it in a
follow-up to use this.

We need to fix a few manual C++ function signature definitions to match the C++
declaration.

Depends on D35196

We clamp earlier (parse time rather than computed value time), but that's the
only behavior change, which I think doesn't really matter.

Depends on D35197

Attachment #9072540 - Attachment description: Bug 1559814 - Remove some dead code. r=mats → Bug 1559814 - Remove GetGridLine since it's dead code now that Stylo serializes all grid lines. r=mats
Attachment #9072541 - Attachment description: Bug 1559814 - Generate top-level function and constant declarations from parsed dependencies. r=mats,#style → Bug 1559814 - Generate top-level function and constant declarations for the style crate. r=mats,#style
Attachment #9074188 - Attachment description: Bug 1559814 - Update cbindgen. r=#style,boris → Bug 1559814 - Update cbindgen. r=boris
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/362d9435ca4e
Use a more similar representation in Rust and C++ for grid lines. r=heycam
https://hg.mozilla.org/integration/autoland/rev/62f9d89fb827
Remove GetGridLine since it's dead code now that Stylo serializes all grid lines. r=mats
https://hg.mozilla.org/integration/autoland/rev/fec03c5d3661
Generate top-level function and constant declarations for the style crate. r=heycam
https://hg.mozilla.org/integration/autoland/rev/11ec9de59076
Use the cbindgen representation for grid line properties. r=mats

Backed out 4 changesets for causing bustages.

Backout link: https://hg.mozilla.org/integration/autoland/rev/c95bc282ae96b48c3249b3d4942b7c15fbda5a58

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel%2Crunnable&revision=11ec9de590766855f98bdc3fbe839c0a1209c8e2&selectedJob=253581393

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=253581393&repo=autoland&lineNumber=7370

[task 2019-06-26T20:55:17.834Z] 20:55:17 ERROR - make[1]: *** [force-cargo-test-run] Error 101
[task 2019-06-26T20:55:17.834Z] 20:55:17 INFO - make[1]: Leaving directory '/builds/worker/workspace/build/src/obj-firefox/toolkit/library/rust'
[task 2019-06-26T20:55:17.834Z] 20:55:17 INFO - /builds/worker/workspace/build/src/config/recurse.mk:101: recipe for target 'toolkit/library/rust/rusttests' failed
[task 2019-06-26T20:55:17.834Z] 20:55:17 INFO - make: *** [toolkit/library/rust/rusttests] Error 2

Flags: needinfo?(emilio)
Attachment #9072539 - Attachment description: Bug 1559814 - Use a more similar representation in Rust and C++ for grid lines. r=mats,#style → Bug 1559814 - Use a more similar representation in Rust and C++ for grid lines. r=heycam
Attachment #9072541 - Attachment description: Bug 1559814 - Generate top-level function and constant declarations for the style crate. r=mats,#style → Bug 1559814 - Generate top-level function and constant declarations for the style crate. r=heycam
Attachment #9072542 - Attachment description: Bug 1559814 - Use the cbindgen representation for grid line properties. r=mats,#style → Bug 1559814 - Use the cbindgen representation for grid line properties. r=mats

Just a few tests that are now "passing" because the grid line representation is more compact. I updated it by removing the boxed=true.

Flags: needinfo?(emilio)
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/43c03fe50773
Use a more similar representation in Rust and C++ for grid lines. r=heycam
https://hg.mozilla.org/integration/autoland/rev/9e01b4a3ac61
Remove GetGridLine since it's dead code now that Stylo serializes all grid lines. r=mats
https://hg.mozilla.org/integration/autoland/rev/e6e5d5bfc6ed
Generate top-level function and constant declarations for the style crate. r=heycam
https://hg.mozilla.org/integration/autoland/rev/06b5dca91073
Use the cbindgen representation for grid line properties. r=mats
Regressions: 1562372
No longer regressions: 1562372
You need to log in before you can comment on or make changes to this bug.