1287435 - stylo: Add bindings for nsStyleCoord::Calc

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Manish Goregaokar [:manishearth]]

Attachment: stylo: Add bindings for nsStyleCoord::Calc. v1

Companion to https://github.com/servo/servo/pull/12465

Comment 1

[Emilio Cobos Álvarez (:emilio)]

You probably want to also change http://searchfox.org/mozilla-central/source/layout/style/nsStyleCoord.h#107 to use NS_INLINE_DECL_THREADSAFE_REFCOUNTING.

Comment 2

[Cameron McCormack (:heycam)]

Comment on attachment 8771972 [details] [diff] [review]
stylo: Add bindings for nsStyleCoord::Calc. v1

Review of attachment 8771972 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, though please re-request review if changes to the Servo PR cause significant changes here.

::: layout/style/ServoBindings.cpp
@@ +576,4 @@
>    aLayer->Initialize(aLayerType);
>  }
>  
> +void Gecko_ResetStyleCoord(nsStyleUnit* aUnit, nsStyleUnion* aValue) {

Nit: new line after "void" and before "{".

@@ +579,5 @@
> +void Gecko_ResetStyleCoord(nsStyleUnit* aUnit, nsStyleUnion* aValue) {
> +  nsStyleCoord::Reset(*aUnit, *aValue);
> +}
> +
> +void Gecko_SetStyleCoordCalcValue(nsStyleUnit* aUnit, nsStyleUnion* aValue, nsStyleCoord::CalcValue aCalc) {

Nit: here too.

@@ +584,5 @@
> +  nsStyleCoord::Calc* calcRef = new nsStyleCoord::Calc();
> +  calcRef->mLength = aCalc.mLength;
> +  calcRef->mPercent = aCalc.mPercent;
> +  calcRef->mHasPercent = aCalc.mHasPercent;
> +  *aUnit = nsStyleUnit::eStyleUnit_Calc;

Please MOZ_ASSERT what you expect *aUnit to contain, at the top of the function.  (Either that it's eStyleUnit_Null or just a non-eStyleUnit_Calc value.)

@@ +586,5 @@
> +  calcRef->mPercent = aCalc.mPercent;
> +  calcRef->mHasPercent = aCalc.mHasPercent;
> +  *aUnit = nsStyleUnit::eStyleUnit_Calc;
> +  aValue->mPointer = calcRef;
> +  calcRef->AddRef();

This is OK, but more Gecko-idiomatically written as:

  RefPtr<nsStyleCoord::Calc> calc = new nsStyleCoord::Calc();
  ...
  aValue->mPointer = calc.forget();

so that the object has a non-zero refcount when interacted with, before returning it.

@@ +589,5 @@
> +  aValue->mPointer = calcRef;
> +  calcRef->AddRef();
> +}
> +
> +void Gecko_AddRefCalc(nsStyleCoord::Calc* aCalc) {

Nit: here too.

Either use NS_DECL_THREADSAFE_FFI_REFCOUNTING in ServoBindings.h to declare the AddRef (and an unnecessary Release, though I think that's fine) function for nsStyleCoord::Calc, or name the function Gecko_AddRefCalcArbitraryThread to keep the naming consistent with the functions generated by that macro.

And as Emilio says, make Calc support threadsafe refcounting.

Comment 3

[Manish Goregaokar [:manishearth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9215ce516781

Comment 4

[Manish Goregaokar [:manishearth]]

Attachment: 8771972: stylo: Add bindings for nsStyleCoord::Calc. v2

Updated, comments addressed.

I didn't change it to RefPtr because that will require a reinterpret_cast (static_cast doesn't work). The same pattern used here is used in SetCoordCalc(), so I'll stick to it.

Comment 5

[Manish Goregaokar [:manishearth]]

Looks like manually adding jobs doesn't work for me, trying again in https://treeherder.mozilla.org/#/jobs?repo=try&revision=604c7455a17e

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d17d4140f859
stylo: Add bindings for nsStyleCoord::Calc; r=heycam

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d17d4140f859