1404243 - Refactor for StyleShapeSource to support shape-outside: <image>

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement, P3)

Description

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

To support shape-outside: <image> in bug 1404222. I'll need to do some refactor to StyleShapeSource. The patch series might be large enough to land.

Comment 1

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: Bug 1404243 Part 1 - Move StyleShapeSource's large methods to nsStyleStruct.cpp.

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

Comment 2

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: Bug 1404243 Part 2 - Change StyleShapeSource::operator==() to use logic in DefinitelyEquals().

The original operator==() (implemented by EqualsInternal<true>) is not been
used. Therefore, I expand EqualsInternal<false> into it, and move it to
nsStyleStruct.cpp.

Also, use DefinitelyEqualURIs() in nsStyleStruct.cpp in operator==().

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

Comment 3

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: Bug 1404243 Part 3 - Remove refcount for StyleBasicShape, and use UniquePtr to hold it.

After StyleBasicShape is set to StyleShapeSource, it's life cycle never go
beyond StyleShapeSource, so I make StyleBasicShape hold by a UniquePtr in
StyleShapeSource.

Also, replace all raw pointers to StyleBasicShape by UniquePtr in all APIs.

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

Comment 4

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: style: Remove refcount for StyleBasicShape

This is due to Bug 1404243 Part 3.

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

Comment 5

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: Bug 1404243 Part 4 - Change StyleShapeSource's URL value storage by using nsStyleImage.

This is for implementing shape-outside: <image> later.

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

Comment 6

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: style: Change URL value storage in StyleShapeSource.

This is due to Bug 1404243 Part 4.

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

Comment 7

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=01ea28adbad00458a0e3bb09a67da4886f91e162

Comment 8

[Cameron McCormack (:heycam)]

Comment on attachment 8913928 [details]
Bug 1404243 Part 1 - Move StyleShapeSource's large methods to nsStyleStruct.cpp.

https://reviewboard.mozilla.org/r/184972/#review190352

Comment 9

[Cameron McCormack (:heycam)]

Comment on attachment 8913929 [details]
Bug 1404243 Part 2 - Change StyleShapeSource::operator==() to use logic in DefinitelyEquals().

https://reviewboard.mozilla.org/r/184974/#review190354

r=me with that.

::: layout/style/nsStyleStruct.cpp:1078
(Diff revision 1)
>    }
>    return *this;
>  }
>  
>  bool
> +StyleShapeSource::operator==(const StyleShapeSource& aOther) const

I think I would prefer to keep this method named DefinitelyEquals.  When I see operator== I think it is really checking for exact equality, but since this isn't, I feel like we should use a different name.

Comment 10

[Cameron McCormack (:heycam)]

Comment on attachment 8913930 [details]
Bug 1404243 Part 3 - Remove refcount for StyleBasicShape, and use UniquePtr to hold it.

https://reviewboard.mozilla.org/r/184976/#review190356

Comment 11

[Cameron McCormack (:heycam)]

Comment on attachment 8913931 [details]
style: Remove refcount for StyleBasicShape

https://reviewboard.mozilla.org/r/184978/#review190358

Comment 12

[Cameron McCormack (:heycam)]

Comment on attachment 8913932 [details]
Bug 1404243 Part 4 - Change StyleShapeSource's URL value storage by using nsStyleImage.

https://reviewboard.mozilla.org/r/184980/#review190360

::: layout/style/nsStyleStruct.cpp:1045
(Diff revision 1)
>  
>  // --------------------
>  // StyleShapeSource
>  
>  StyleShapeSource::StyleShapeSource(const StyleShapeSource& aSource)
>    : StyleShapeSource()

We don't need this delegated constructor call any more.

Comment 13

[Cameron McCormack (:heycam)]

Comment on attachment 8913933 [details]
style: Change URL value storage in StyleShapeSource.

https://reviewboard.mozilla.org/r/184982/#review190362

Comment 14

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Comment on attachment 8913929 [details]
Bug 1404243 Part 2 - Change StyleShapeSource::operator==() to use logic in DefinitelyEquals().

https://reviewboard.mozilla.org/r/184974/#review190354

> I think I would prefer to keep this method named DefinitelyEquals.  When I see operator== I think it is really checking for exact equality, but since this isn't, I feel like we should use a different name.

I agree with you. However, after supporting `shape-outside: <image>`, I'll remove the `DefinitelyEqualURIs` and deligate the comparison to  `nsStyleImage::operator==`. By then, `StyleShapeSource::operator==` won't have any implementation detail related to `DefinitelyEquals`.

Comment 15

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Comment on attachment 8913928 [details]
Bug 1404243 Part 1 - Move StyleShapeSource's large methods to nsStyleStruct.cpp.

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

Comment 16

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Comment on attachment 8913929 [details]
Bug 1404243 Part 2 - Change StyleShapeSource::operator==() to use logic in DefinitelyEquals().

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

Comment 17

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Comment on attachment 8913930 [details]
Bug 1404243 Part 3 - Remove refcount for StyleBasicShape, and use UniquePtr to hold it.

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

Comment 18

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Comment on attachment 8913932 [details]
Bug 1404243 Part 4 - Change StyleShapeSource's URL value storage by using nsStyleImage.

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

Comment 19

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: Servo PR #18700

Comment 20

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

https://hg.mozilla.org/integration/autoland/rev/7bd101e8bb6e

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3d8e44508ed0
Part 1 - Move StyleShapeSource's large methods to nsStyleStruct.cpp. r=heycam
https://hg.mozilla.org/integration/autoland/rev/84fba2528a6c
Part 2 - Change StyleShapeSource::operator==() to use logic in DefinitelyEquals(). r=heycam
https://hg.mozilla.org/integration/autoland/rev/64f6005d9a5c
Part 3 - Remove refcount for StyleBasicShape, and use UniquePtr to hold it. r=heycam
https://hg.mozilla.org/integration/autoland/rev/1a7862326a70
Part 4 - Change StyleShapeSource's URL value storage by using nsStyleImage. r=heycam

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/02c27216914e
Some more heap write analysis tweaks. r=me

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/3d8e44508ed0
https://hg.mozilla.org/mozilla-central/rev/84fba2528a6c
https://hg.mozilla.org/mozilla-central/rev/64f6005d9a5c
https://hg.mozilla.org/mozilla-central/rev/1a7862326a70
https://hg.mozilla.org/mozilla-central/rev/02c27216914e