1553252 - Use cbindgen for text-overflow.

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Emilio Cobos Álvarez (:emilio)]

The way to fix this would be:

• Mark TextOverflowSide with #[repr(C, u8)], and change the Box<str> for crate::OwnedStr: https://searchfox.org/mozilla-central/rev/6c9f60f8cc064a1005cd8141ecd526578ae9da7a/servo/components/style/values/specified/text.rs#138
• Mark computed::TextOverflow with #[repr(C)]: https://searchfox.org/mozilla-central/rev/6c9f60f8cc064a1005cd8141ecd526578ae9da7a/servo/components/style/values/computed/text.rs#116
• Add TextOverflow to the list here: https://searchfox.org/mozilla-central/rev/6c9f60f8cc064a1005cd8141ecd526578ae9da7a/servo/ports/geckolib/cbindgen.toml#49
• ... And a line like { gecko = "StyleTextOverflow", servo = "values::computed::TextOverflow" }, here: https://searchfox.org/mozilla-central/rev/6c9f60f8cc064a1005cd8141ecd526578ae9da7a/layout/style/ServoBindings.toml#396
• Then change the style struct to have a mozilla::StyleTextOverflow instead of nsStyleTextOverflow
• Rewrite the code that uses nsStyleTextOverflow to use mozilla::StyleTextOverflow. You could put the helpers that nsStyleTextOverflow has in a section like this: https://searchfox.org/mozilla-central/rev/6c9f60f8cc064a1005cd8141ecd526578ae9da7a/servo/ports/geckolib/cbindgen.toml#440, or inline them as static functions in the only file that uses them.
• Remove text-overflow and the text-overflow-specific code from: https://searchfox.org/mozilla-central/rev/6c9f60f8cc064a1005cd8141ecd526578ae9da7a/servo/components/style/properties/gecko.mako.rs#3414
• Remove nsStyleTextOverflow and co.
• Profit?

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Violet, would you be interested in taking this since you mentioned some interest in it?

This is the one I found that wasn't either too trivial or too complex, and you get to remove a good chunk of code :)

I outlined what has to be done above, but I may have goofed somewhere, definitely let me know if you get stuck or what not.

Thanks!

Comment 2

[violet.bugreport]

Thanks, that's very helpful!

Comment 3

[violet.bugreport]

I think we'll need to add default c'tor support for union in cbindgen first, because this seems to be the first time a non-POD member is inside a union (OwnedStr in TextOverflowSide). C++ will delete the default c'tor for union if it has a member with nontrivial default c'tor, the generated code won't compile:

static StyleTextOverflowSide String(const StyleOwnedStr &a0) {
  StyleTextOverflowSide result;
  ::new (&result.string._0) (StyleOwnedStr)(a0);
  result.tag = Tag::String;
  return result;
}

Comment 4

[violet.bugreport]

Attachment: default-ctor-support-for-cbindgen.patch

This patch to cbindgen should be ok.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

No need to, see: https://searchfox.org/mozilla-central/rev/6c9f60f8cc064a1005cd8141ecd526578ae9da7a/servo/ports/geckolib/cbindgen.toml#431

Your patch looks ok, but probably the destructor should be private to avoid footgunning somebody. That's why I've been reluctant to add that functionality to cbindgen.

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Err, the constructor, I mean.

Comment 7

[violet.bugreport]

Oh, I see. So the patch isn't necessary. Do you think I should submit a PR to cbindgen?

A probably trivial question: what's the recommended way to modify a third-party rust package and see how it works in gecko locally? My current approach looks quite awkward. I have to modify this line https://searchfox.org/mozilla-central/rev/6c9f60f8cc064a1005cd8141ecd526578ae9da7a/layout/style/RunCbindgen.py#27 to use the path of my local cbindgen repo in order to test whether it works in gecko.

Comment 8

[Emilio Cobos Álvarez (:emilio)]

You can put CBINDGEN=/path/to/cbindgen in your mozconfig and that should do the trick. If you need to push to try with your changes, you need to also change: https://searchfox.org/mozilla-central/source/taskcluster/scripts/misc/build-cbindgen.sh#6 and point it to your repo as well.

For other third-party rust packages it's a bit easier, since they're not expected to be built before the export phase. For example, if you wanted to modify cssparser, you could add cssparser = {path = "third_party/rust/cssparser"} to:

https://searchfox.org/mozilla-central/rev/6c9f60f8cc064a1005cd8141ecd526578ae9da7a/Cargo.toml#59

I think submitting a PR to cbindgen is not really necessary, though if this pattern becomes really frequent we could consider adding it.

Comment 9

[violet.bugreport]

Attachment: Bug 1553252 - Use cbindgen for text-overflow

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3304277f5bde
Use cbindgen for text-overflow r=emilio

Comment 11

[Oana Pop-Rus]

Backed out changeset 3304277f5bde (bug 1553252) for build bustages on Windows

Push that started the failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=superseded%2Ctestfailed%2Cbusted%2Cexception%2Crunnable&selectedJob=247974781&revision=3304277f5bde56d355b81d3caa62673f4545b0e3

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

Backout: https://hg.mozilla.org/integration/autoland/rev/7db5627ab9bd5c0267d23796e6b3e099091ace7d

Comment 12

[Emilio Cobos Álvarez (:emilio)]

https://searchfox.org/mozilla-central/rev/952521e6164ddffa3f34bc8cfa5a81afc5b859c4/editor/libeditor/EditorCommands.cpp#32 and similar are evil :(

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/378c7b4699e1
Use cbindgen for text-overflow r=emilio

Comment 14

[violet.bugreport]

That using namespace is definitely bad, but it's still strange why the bustage only happened on Windows. This sort of name clashing should've been diagnosed by clang, gcc as well, but they compiled successfully.

Actually, clang/gcc does report error on a minimal example that is essentially the same as the actual case: https://wandbox.org/permlink/jrWo9p2KNhgBlOen. I'm not sure if there is any flag or build system related stuff that silences the error diagnostic.

Comment 15

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/378c7b4699e1

Comment 16

[Emilio Cobos Álvarez (:emilio)]

(In reply to violet.bugreport from comment #14)

That using namespace is definitely bad, but it's still strange why the bustage only happened on Windows. This sort of name clashing should've been diagnosed by clang, gcc as well, but they compiled successfully.

Actually, clang/gcc does report error on a minimal example that is essentially the same as the actual case: https://wandbox.org/permlink/jrWo9p2KNhgBlOen. I'm not sure if there is any flag or build system related stuff that silences the error diagnostic.

We use unified builds, so if different cpp files are built, they may change how the final compilation unit looks like. That's probably what's happening.