1295662 - stylo: Add bindings for StyleClipPath

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Manish Goregaokar [:manishearth]]

Servo side at https://github.com/servo/servo/pull/12878

Comment 1

[Manish Goregaokar [:manishearth]]

Attachment: Bug 1295662 - stylo: Add bindings for StyleClipPath;

Review commit: https://reviewboard.mozilla.org/r/72018/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/72018/

Comment 2

[Manish Goregaokar [:manishearth]]

Attachment: Bug 1295662 - Document layout of nsStyleCorners and inset StyleBasicShape;

Review commit: https://reviewboard.mozilla.org/r/72264/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/72264/

Comment 3

[Manish Goregaokar [:manishearth]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/72018/diff/1-2/

Comment 4

[Manish Goregaokar [:manishearth]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/72018/diff/2-3/

Comment 5

[Cameron McCormack (:heycam)]

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;

Comment 6

[Cameron McCormack (:heycam)]

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,".

Comment 7

[Manish Goregaokar [:manishearth]]

Comment on attachment 8781958 [details]
Bug 1295662 - Document layout of nsStyleCorners and inset StyleBasicShape;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/72264/diff/1-2/

Comment 8

[Manish Goregaokar [:manishearth]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/72018/diff/3-4/

Comment 9

[Pulsebot]

Pushed by [e-mail]:
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

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/751ab2b45ddc
https://hg.mozilla.org/mozilla-central/rev/8bb342dfaa0e