1559814 - Use cbindgen for grid line properties.

Status: RESOLVED FIXED

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

Description

[Emilio Cobos Álvarez (:emilio)]

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

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1559814 - Use a more similar representation in Rust and C++ for grid lines. r=heycam

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.

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1559814 - Remove GetGridLine since it's dead code now that Stylo serializes all grid lines. r=mats

We serialize these using Servo now.

Depends on D35195

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1559814 - Generate top-level function and constant declarations for the style crate. r=heycam

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

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1559814 - Use the cbindgen representation for grid line properties. r=mats

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

Comment 5

[Emilio Cobos Álvarez (:emilio)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9b92f64ce8b1de94881e648032b3a0f50c16e5bb

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1559814 - Update cbindgen. r=boris

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fd8b8f6dad49
Update cbindgen. r=boris

Comment 8

[Pulsebot]

Pushed by [e-mail]:
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

Comment 9

[Alexandru Michis [:malexandru]]

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

Comment 10

[Emilio Cobos Álvarez (:emilio)]

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

Comment 11

[Pulsebot]

Pushed by [e-mail]:
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

Comment 12

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/43c03fe50773
https://hg.mozilla.org/mozilla-central/rev/9e01b4a3ac61
https://hg.mozilla.org/mozilla-central/rev/e6e5d5bfc6ed
https://hg.mozilla.org/mozilla-central/rev/06b5dca91073