Bug 234052

Summary: Use thread safe initialization in rotateLeftTransform
Product: WebKit Reporter: Cameron McCormack (:heycam) <heycam>
Component: Layout and RenderingAssignee: Cameron McCormack (:heycam) <heycam>
Status: NEW ---    
Severity: Normal CC: bfulgham, darin, mmaxfield, simon.fraser, webkit-bug-importer, zalan
Priority: P2 Keywords: InRadar
Version: WebKit Local Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 233488    
Attachments:
Description Flags
Patch
none
Patch
none
Patch mmaxfield: review+

Description Cameron McCormack (:heycam) 2021-12-08 18:22:49 PST
Use thread safe initialization in rotateLeftTransform
Comment 1 Cameron McCormack (:heycam) 2021-12-08 18:24:00 PST
Created attachment 446471 [details]
Patch
Comment 2 Cameron McCormack (:heycam) 2021-12-08 18:24:11 PST
I'd be open to the argument we shouldn't bother caching this object at all.
Comment 3 Darin Adler 2021-12-08 19:05:16 PST
Comment on attachment 446471 [details]
Patch

Another solution here would be to make the constructor for AffineTransform constexpr, then we could use a constexpr static and there is no thread issue there.
Comment 4 Cameron McCormack (:heycam) 2021-12-08 19:12:29 PST
(In reply to Darin Adler from comment #3)
> Another solution here would be to make the constructor for AffineTransform
> constexpr, then we could use a constexpr static and there is no thread issue
> there.

Good idea!
Comment 5 Cameron McCormack (:heycam) 2021-12-08 20:11:29 PST
Created attachment 446493 [details]
Patch
Comment 6 Cameron McCormack (:heycam) 2021-12-08 20:18:35 PST
Created attachment 446494 [details]
Patch
Comment 7 Darin Adler 2021-12-09 12:33:55 PST
Comment on attachment 446494 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=446494&action=review

> Source/WebCore/platform/graphics/transforms/AffineTransform.h:64
> +    WEBCORE_EXPORT constexpr AffineTransform();
> +    WEBCORE_EXPORT constexpr AffineTransform(double a, double b, double c, double d, double e, double f);

Should not need WEBCORE_EXPORT on constexpr functions, but if it builds, then I guess that proves me wrong.
Comment 8 Myles C. Maxfield 2021-12-10 15:16:49 PST
Comment on attachment 446494 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=446494&action=review

Feels a bit weird that the patch title only is relevant to like 1/2 of this patch.

> Source/WebCore/platform/graphics/coretext/FontCascadeCoreText.cpp:57
> +static constexpr AffineTransform rotateLeftTransform { 0, -1, 1, 0, 0, 0 };

🆒

> Tools/ChangeLog:3
> +        Remove unused AffineTransform variable from a test

Are you sure you meant to include this in this patch?
Comment 9 Cameron McCormack (:heycam) 2021-12-10 15:29:23 PST
(In reply to Myles C. Maxfield from comment #8)
> Feels a bit weird that the patch title only is relevant to like 1/2 of this
> patch.

I'll reword.

> > Tools/ChangeLog:3
> > +        Remove unused AffineTransform variable from a test
> 
> Are you sure you meant to include this in this patch?

Yes.  After changing the constructor to be constexpr, the compiler complains that the variable is unused.  (Before it didn't because I guess the constructor could be doing some useful work.)
Comment 10 Radar WebKit Bug Importer 2021-12-15 18:23:16 PST
<rdar://problem/86553047>