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

RESOLVED FIXED in Firefox 58

Status

()

enhancement
P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: TYLin, Assigned: TYLin)

Tracking

(Blocks 1 bug)

unspecified
mozilla58
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox58 fixed)

Details

Attachments

(5 attachments, 2 obsolete attachments)

Assignee

Description

2 years ago
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 8

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

https://reviewboard.mozilla.org/r/184972/#review190352
Attachment #8913928 - Flags: review?(cam) → review+

Comment 9

2 years ago
mozreview-review
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.
Attachment #8913929 - Flags: review?(cam) → review+

Comment 10

2 years ago
mozreview-review
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
Attachment #8913930 - Flags: review?(cam) → review+

Comment 11

2 years ago
mozreview-review
Comment on attachment 8913931 [details]
style: Remove refcount for StyleBasicShape

https://reviewboard.mozilla.org/r/184978/#review190358
Attachment #8913931 - Flags: review?(cam) → review+

Comment 12

2 years ago
mozreview-review
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.
Attachment #8913932 - Flags: review?(cam) → review+

Comment 13

2 years ago
mozreview-review
Comment on attachment 8913933 [details]
style: Change URL value storage in StyleShapeSource.

https://reviewboard.mozilla.org/r/184982/#review190362
Attachment #8913933 - Flags: review?(cam) → review+
Assignee

Comment 14

2 years ago
mozreview-review-reply
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Updated

2 years ago
Attachment #8913931 - Attachment is obsolete: true
Assignee

Updated

2 years ago
Attachment #8913933 - Attachment is obsolete: true
Assignee

Comment 19

2 years ago
Posted file Servo PR #18700

Comment 21

2 years ago
Pushed by tlin@mozilla.com:
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

2 years ago
Pushed by cmccormack@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/02c27216914e
Some more heap write analysis tweaks. r=me
You need to log in before you can comment on or make changes to this bug.