1552708 - Use cbindgen for URIs

Status: RESOLVED FIXED

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

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1552708 - Use cbindgen for URIs. r=heycam

This doesn't clean up as much as a whole, but it's a step in the right
direction. In particular, it allows us to start using simple bindings for:

• Filters
• Shapes and images, almost. Need to:
  • Get rid of the complex -moz- gradient parsing (let
    layout.css.simple-moz-gradient.enabled get to release).
• Counters, almost. Need to:
  • Share the Attr representation with Gecko, by not using Option<>.
    • Just another variant should be enough (ContentItem::{Attr,Prefixedattr},
      maybe).

Which in turn allows us to remove a whole lot of bindings in followups to this.

The setup changes a bit. This also removes the double pointer I complained about
while reviewing the shared UA sheet patches. The old setup is:

SpecifiedUrl
 * CssUrl
   * Arc<CssUrlData>
     * String
     * UrlExtraData
 * UrlValueSource
   * Arc<CssUrlData>
   * load id
   * resolved uri
   * CORS mode.
   * ...

The new one removes the double reference to the url data via URLValue, and looks
like:

SpecifiedUrl
 * CssUrl
   * Arc<CssUrlData>
     * String
     * UrlExtraData
     * CorsMode
     * LoadData
       * load id
       * resolved URI

The LoadData is the only mutable bit that C++ can change, and is not used from
Rust. Ideally, in the future, we could just use rust-url to resolve the URL
after parsing or something, and make it all immutable. Maybe.

I've verified that this approach still works with the UA sheet patches (via the
LoadDataSource::Lazy).

The reordering of mWillChange is to avoid nsStyleDisplay from going over the
size limit. We want to split it up anyway in bug 1552587, but mBinding gains a
tag member, which means that we were having a bit of extra padding.

One thing I want to explore is to see if we can abuse rustc's non-zero
optimizations to predict the layout from C++, but that's something to explore at
some other point in time and with a lot of care and help from Michael (who sits
next to me and works on rustc ;)).

Comment 2

[Emilio Cobos Álvarez (:emilio)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=79d258a5265a38b6937f9e93225e3b96d12c1339 (that fails a couple moz-image-rect reftests that are fixed already)

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1bdbfbb5b342
Use cbindgen for URIs. r=heycam

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/80db9f845237
Tweak rust tests to account for more compact URL representation. r=bustage

Comment 5

[Cristian Brindusan [:cbrindusan]]

Backed out 3 changesets (Bug 1552708, Bug 1552878) for build bustages and compiler issues.

Backout: https://hg.mozilla.org/integration/autoland/rev/354e127d754fae84199afd2758e41e6175d40eb4

Push that started the failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=pending%2Crunning%2Csuccess%2Ctestfailed%2Cbusted%2Cexception&revision=1bdbfbb5b3424672257f3c850b69eced2e91f757&selectedJob=248531935

Failure log:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=248531935&repo=autoland&lineNumber=7212
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=248538653&repo=autoland&lineNumber=21809

Comment 6

[Emilio Cobos Álvarez (:emilio)]

12:57:26 INFO - LLVM ERROR: SEH unwind data splitting not yet implemented

Huh, wasn't that bug fixed? Nathan, do you know about how to deal with it?

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Oh well, I just pushed on top of the try runs for bug 1554006 and it's gone, so I hope that can land soon.

Comment 8

[Nathan Froyd [:froydnj]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #6)

12:57:26 INFO - LLVM ERROR: SEH unwind data splitting not yet implemented

Huh, wasn't that bug fixed? Nathan, do you know about how to deal with it?

It's very possible that the LLVM fix hasn't made it into the rustc we use...

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/388ab8151b4d
Use cbindgen for URIs. r=heycam

Comment 10

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/388ab8151b4d