Bug 234052 - Use thread safe initialization in rotateLeftTransform
Summary: Use thread safe initialization in rotateLeftTransform
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Cameron McCormack (:heycam)
URL:
Keywords: InRadar
Depends on:
Blocks: 233488
  Show dependency treegraph
 
Reported: 2021-12-08 18:22 PST by Cameron McCormack (:heycam)
Modified: 2021-12-15 18:23 PST (History)
6 users (show)

See Also:


Attachments
Patch (2.00 KB, patch)
2021-12-08 18:24 PST, Cameron McCormack (:heycam)
no flags Details | Formatted Diff | Diff
Patch (5.93 KB, patch)
2021-12-08 20:11 PST, Cameron McCormack (:heycam)
no flags Details | Formatted Diff | Diff
Patch (7.18 KB, patch)
2021-12-08 20:18 PST, Cameron McCormack (:heycam)
mmaxfield: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>