Closed Bug 1287435 Opened 4 years ago Closed 4 years ago

stylo: Add bindings for nsStyleCoord::Calc

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: manishearth, Assigned: manishearth)

References

Details

Attachments

(1 file, 1 obsolete file)

Attachment #8771972 - Attachment is patch: true
Attachment #8771972 - Flags: review?(cam)
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 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.
Attachment #8771972 - Flags: review?(cam) → review+
Assignee: nobody → manishearth
Status: NEW → ASSIGNED
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.
Attachment #8771972 - Attachment is obsolete: true
Attachment #8772286 - Flags: review+
Keywords: checkin-needed
Keywords: checkin-needed
Looks like manually adding jobs doesn't work for me, trying again in https://treeherder.mozilla.org/#/jobs?repo=try&revision=604c7455a17e
Keywords: checkin-needed
Pushed by cmccormack@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d17d4140f859
stylo: Add bindings for nsStyleCoord::Calc; r=heycam
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/d17d4140f859
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.