Closed Bug 1412486 Opened 3 years ago Closed 3 years ago

stylo: Upgrade bindgen to 0.31

Categories

(Core :: CSS Parsing and Computation, enhancement)

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox57 --- wontfix
firefox58 --- fixed

People

(Reporter: xidorn, Assigned: emilio)

References

(Blocks 2 open bugs)

Details

Attachments

(2 files, 5 obsolete files)

Bug 1412486: Update bindgen.
59 bytes, text/x-review-board-request
xidorn
: review+
Details
Bug 1412486: Make the replacement for UniquePtr not have a deleter parameter.
59 bytes, text/x-review-board-request
xidorn
: review+
Details 
Per bug 1412077 comment 5, upgrade bindgen to the latest version should improve the build time of style crate by quite a bit.
Taking this since one contributor is trying to do the Servo change and I promised to help.
Assignee: nobody → emilio
But there is really nothing about Servo change... It has to be verified with a Gecko build.

Does the contributor has a working Stylo build?
It does not. For now, blockers:

 - https://github.com/rust-lang-nursery/rust-bindgen/pull/1111 (actually not a blocker I guess, but nice)
 - https://github.com/rust-lang-nursery/rust-bindgen/pull/1112
 - https://github.com/rust-lang-nursery/rust-bindgen/pull/1113 (fixed by https://github.com/rust-lang-nursery/rust-bindgen/pull/1114)

After those merge I'll try again, I don't want to remember how to do all the [replace] setup.
Blocks: 1412441
I'm pushing a WIP now because I want to pull this from the office tomorrow.
Comment on attachment 8923220 [details]
Bug 1412486: Update bindgen.

https://reviewboard.mozilla.org/r/194362/#review199768

Cancel review for wildcards in `rusty-enums`.

::: layout/style/ServoBindings.toml:134
(Diff revision 2)
> +    "nsIDocument_Type",
> +    "nsCSSUnit",
> +    "nsCSSPropertyID",
> +    "nsCSSCounterDesc",
> +    "nsMediaExpression_Range",
> +    "nsMediaFeature.*",

You mean `nsMediaFeature::RangeType` and `nsMediaFeature::ValueType`? I think we should list them out explicitly here. Also it doesn't seem to me `nsMediaFeature::RequirementFlags` should be a rust enum. It should probably be put in `bitfield-enums` section.

::: layout/style/ServoBindings.toml:142
(Diff revision 2)
> +    # FIXME: use proper namespaces and such when
> +    # https://github.com/rust-lang-nursery/rust-bindgen/issues/1125
> +    # is fixed.
> +    "nsStyleImageLayers_LayerType",
> +    "nsStyleImageOrientation_Angles",
> +    "nsTimingFunction.*",

Same here, I think we should have `nsTimingFunction::Type` and `nsTimingFunction::Keyword` listed explicitly.

::: layout/style/ServoBindings.toml:143
(Diff revision 2)
> +    # https://github.com/rust-lang-nursery/rust-bindgen/issues/1125
> +    # is fixed.
> +    "nsStyleImageLayers_LayerType",
> +    "nsStyleImageOrientation_Angles",
> +    "nsTimingFunction.*",
> +    ".*ServoElementSnapshot.*",

This is really unclear to me what is covered here... `ServoElementSnapshotFlags` should probably be in `bitfield-enums`, and I don't find anything else.

::: layout/style/ServoBindings.toml:144
(Diff revision 2)
> +    # is fixed.
> +    "nsStyleImageLayers_LayerType",
> +    "nsStyleImageOrientation_Angles",
> +    "nsTimingFunction.*",
> +    ".*ServoElementSnapshot.*",
> +    ".*Side",

It doesn't seem to me that we have that many `Side` enums... [1] Could you also just list what we need here?

[1] https://searchfox.org/mozilla-central/search?q=enum+.*Side%5Cb&case=true&regexp=true&path=.h%24

::: layout/style/ServoBindings.toml:145
(Diff revision 2)
> +    ".*PlaybackDirection",
> +    ".*FillMode",
> +    ".*HalfCorner",

Why arbitrary prefix? Because of namespace?

::: layout/style/ServoBindings.toml:148
(Diff revision 2)
> +    "Type",
> +    "Flags",

What `Type` and `Flags`?

::: layout/style/ServoBindings.toml:150
(Diff revision 2)
> +    ".*URLMatchingFunction",
> +    ".*Style.*",
> +    ".*ColorID",
> +    ".*BooleanFlag",
> +    ".*CSSPseudoElementType",
> +    ".*CSSPseudoClassType",
> +    ".*FontID",
> +    ".*MatrixTransformOperator",

Well... I'm really not a big fan of using wildcard like this... Unless there are really a large number of things with restrictive common pattern, please just list them out.

::: servo/components/style/build_gecko.rs:390
(Diff revision 2)
>              }
>          }
>  
>          let builder = Builder::get_initial_builder()
>              .enable_cxx_namespaces()
> +            .rustfmt_bindings(false)

If bindgen is messing code further... we probably want to integrate rustfmt at some point... That would require some bootstrap and tooltool change I guess. Probably worth filing a bug for.

::: servo/components/style/build_gecko.rs:426
(Diff revision 2)
> +                                .map(|s| format!("\\s*{}\\s*", s))
> +                                .collect::<Vec<_>>()
> +                                .join("::");
> +
>                  fixups.push(Fixup {
> -                    pat: format!("\\broot::{}\\b", gecko),
> +                    pat: format!("\\broot\\s*::\\s*{}\\b", gecko),

With these many backslashes, it would probably be better to just use raw string literal `r"\broot\s*::\s*::{}\b"?

::: servo/components/style/build_gecko.rs:514
(Diff revision 2)
>              // that has been fixed in clang 4.0+. When we switch people
>              // to libclang 4.0, we can remove this.
>              .handle_table_items("array-types", |builder, item| {
>                  let cpp_type = item["cpp-type"].as_str().unwrap();
>                  let rust_type = item["rust-type"].as_str().unwrap();
> -                builder.hide_type(format!("nsTArrayBorrowed_{}", cpp_type))
> +                builder

Is it because those types are not in whitelist so we don't need to hide them at all?
Attachment #8923220 - Flags: review?(xidorn+moz)
Comment on attachment 8923338 [details]
Bug 1412486: Don't force ComputedValues to derive Debug.

https://reviewboard.mozilla.org/r/194538/#review199774
Attachment #8923338 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8923339 [details]
Bug 1412486: Make the replacement for UniquePtr not have a deleter parameter.

https://reviewboard.mozilla.org/r/194540/#review199776
Attachment #8923339 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8923340 [details]
Bug 1412486: Unconditionally implement Debug for RefPtr.

https://reviewboard.mozilla.org/r/194542/#review199780

::: commit-message-d26df:3
(Diff revision 1)
> +Bindgen doesn't know how to derive debug for a Gecko font family list. Hopefully
> +it doesn't need to.

It doesn't seem to me there is anything particularly hard to derive `Debug` for font family list... Is this a bindgen bug?

::: servo/components/style/gecko_bindings/sugar/refptr.rs
(Diff revision 1)
>  /// Trait for types which can be shared across threads in RefPtr.
>  pub unsafe trait ThreadSafeRefCounted: RefCounted {}
>  
>  /// A custom RefPtr implementation to take into account Drop semantics and
>  /// a bit less-painful memory management.
> -#[derive(Debug)]

Also I think this is really a Rust bug to require `T: Debug` here... Pointers are unconditionally `Debug`. Maybe Rust always requires all type params to be `Debug` to derive `Debug` for a generic type? That restriction doesn't really make sense to me...
Attachment #8923340 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8923341 [details]
Bug 1412486: Update bindgen to 0.31.2.

https://reviewboard.mozilla.org/r/194544/#review199782
Attachment #8923341 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8923341 [details]
Bug 1412486: Update bindgen to 0.31.2.

https://reviewboard.mozilla.org/r/194544/#review199784

::: servo/components/style/Cargo.toml:82
(Diff revision 1)
>  kernel32-sys = "0.2"
>  
>  [build-dependencies]
>  lazy_static = "0.2"
>  log = "0.3"
> -bindgen = { version = "0.29.1", optional = true }
> +bindgen = { version = "0.31.2", optional = true }

Is it possible for us to update bindgen in `js/rust/Cargo.toml` at the same time so that we stop having duplicate bindgens in tree?

Maybe this can be in a separate bug if that would also be some non-trivial work...
Comment on attachment 8923343 [details]
Bug 1412486: Avoid using deprecated bindgen methods.

https://reviewboard.mozilla.org/r/194548/#review199788
Attachment #8923343 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8923220 [details]
Bug 1412486: Update bindgen.

https://reviewboard.mozilla.org/r/194362/#review199976
Attachment #8923220 - Flags: review?(xidorn+moz) → review+
Attachment #8923338 - Attachment is obsolete: true
Attachment #8923340 - Attachment is obsolete: true
Attachment #8923341 - Attachment is obsolete: true
Attachment #8923343 - Attachment is obsolete: true
Attachment #8923342 - Attachment is obsolete: true
status-firefox57=wontfix unless someone thinks this bug should block 57
status-firefox57: --- → wontfix
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/46a4ddbc0541
Make the replacement for UniquePtr not have a deleter parameter. r=xidorn
https://hg.mozilla.org/integration/autoland/rev/b21ef5bb00d3
Update bindgen. r=xidorn
You need to log in before you can comment on or make changes to this bug.