Closed Bug 1553252 Opened 7 months ago Closed 7 months ago

Use cbindgen for text-overflow.

Categories

(Core :: CSS Parsing and Computation, defect)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect
Priority:
Not set

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla69
Tracking Flags:
Tracking Status
firefox69 --- fixed

People

(Reporter: emilio, Assigned: violet.bugreport)

References

Details

Attachments

(2 files)

default-ctor-support-for-cbindgen.patch
3.63 KB, patch
Details | Diff | Splinter Review
Bug 1553252 - Use cbindgen for text-overflow
47 bytes, text/x-phabricator-request
Details | Review

The way to fix this would be:

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!

Flags: needinfo?(violet.bugreport)

Thanks, that's very helpful!

Assignee: nobody → violet.bugreport
Status: NEW → ASSIGNED
Flags: needinfo?(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;
}

This patch to cbindgen should be ok.

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.

Err, the constructor, I mean.

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.

Flags: needinfo?(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.

Flags: needinfo?(emilio)
See Also: → 1553801
Pushed by violet.bugreport@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/3304277f5bde
Use cbindgen for text-overflow r=emilio
Pushed by violet.bugreport@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/378c7b4699e1
Use cbindgen for text-overflow r=emilio

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.

Flags: needinfo?(violet.bugreport)

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

Status: ASSIGNED → RESOLVED
Closed: 7 months ago
status-firefox69: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla69

(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.

You need to log in before you can comment on or make changes to this bug.