Closed Bug 1295662 Opened 3 years ago Closed 3 years ago

stylo: Add bindings for StyleClipPath

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: manishearth, Assigned: manishearth)

References

Details

Attachments

(2 files)

Comment on attachment 8781632 [details]
Bug 1295662 - stylo: Add bindings for StyleClipPath;

https://reviewboard.mozilla.org/r/72018/#review70044

r=me with these changes.

::: layout/style/ServoBindings.cpp:748
(Diff revision 3)
> +
> +  *aDst = *aSrc;
> +}
> +
> +void
> +Gecko_ReleaseClipPath(mozilla::StyleClipPath* aClip) {

Nit: "{" on new line.

Can we call this Gecko_DestroyClipPath instead?  "Release" only makes sense (to me) for refcounted objects, and we use "Destroy" in the name of the style struct destructor-invoking functions.

::: layout/style/ServoBindings.cpp:753
(Diff revision 3)
> +Gecko_ReleaseClipPath(mozilla::StyleClipPath* aClip) {
> +  aClip->~StyleClipPath();
> +}
> +
> +mozilla::StyleBasicShape*
> +Gecko_NewBasicShape(mozilla::StyleBasicShapeType aType) {

Nit: "{" on new line.

::: layout/style/ServoBindings.cpp:754
(Diff revision 3)
> +  RefPtr<StyleBasicShape> ptr = new mozilla::StyleBasicShape(aType);
> +  ptr->AddRef();
> +  return ptr.forget().take();

This is refcounted too much -- storing it in the RefPtr will give it a refcount of 1, and the AddRef call will give it a refcount of 2.

So either:

  RefPtr<StyleBasicShape> ptr = ...;
  return ptr.forget().take();

or:

  StyleBasicShape* ptr = ...;
  ptr->AddRef();
  return ptr;
Attachment #8781632 - Flags: review?(cam) → review+
Comment on attachment 8781958 [details]
Bug 1295662 - Document layout of nsStyleCorners and inset StyleBasicShape;

https://reviewboard.mozilla.org/r/72264/#review70048

::: layout/style/nsStyleCoord.h:374
(Diff revision 1)
>  protected:
> +  // Stored as:
> +  // top-left.x, top-left.y,
> +  // top-right.x, top-right.y,
> +  // bottom-right.x, bottom-right.y,
> +  // bottom-left.x, bottom-left.y

Can you adjust the comment above nsStyleCorners to list the corners in order, too?  (Or perhaps only have one comment, either that one or you new one, that lists the order.)

::: layout/style/nsStyleStruct.h:2613
(Diff revision 1)
> +  nsTArray<nsStyleCoord> mCoordinates;  // mCoordinates has coordinates
> +                                        // for polygon or radii for
> -  // ellipse and circle.
> +                                        // ellipse and circle.
> -  nsTArray<nsStyleCoord> mCoordinates;
> -  Position mPosition;
> -  nsStyleCorners mRadius;
> +                                        //(top, right, bottom, left) for inset
> +
> +  Position mPosition;                   // position of center for ellipse
> +                                        // or circle
> +
> +  nsStyleCorners mRadius;               // radius for inset (0 if not set)

Nit: I'd probably prefer the comments going above the declarations, to avoid them taking up more lines than necessary here.  At least for the mCoordinates one.  Also " " between the "//" and "(top,".
Attachment #8781958 - Flags: review?(cam) → review+
Pushed by cmccormack@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/751ab2b45ddc
Document layout of nsStyleCorners and inset StyleBasicShape; r=heycam
https://hg.mozilla.org/integration/autoland/rev/8bb342dfaa0e
stylo: Add bindings for StyleClipPath; r=heycam
https://hg.mozilla.org/mozilla-central/rev/751ab2b45ddc
https://hg.mozilla.org/mozilla-central/rev/8bb342dfaa0e
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.