Bug 233750 - remove expandAroundIdeographs static variables
Summary: remove expandAroundIdeographs static variables
Status: RESOLVED FIXED
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 233747
  Show dependency treegraph
 
Reported: 2021-12-01 23:19 PST by Cameron McCormack (:heycam)
Modified: 2021-12-07 21:12 PST (History)
17 users (show)

See Also:


Attachments
Patch with dependencies for EWS (66.71 KB, patch)
2021-12-01 23:20 PST, Cameron McCormack (:heycam)
no flags Details | Formatted Diff | Diff
Patch (5.93 KB, patch)
2021-12-01 23:21 PST, Cameron McCormack (:heycam)
heycam: commit-queue+
Details | Formatted Diff | Diff
Patch to remove canExpandAroundIdeographsInComplexText which had test failures (9.72 KB, patch)
2021-12-07 14:34 PST, Cameron McCormack (:heycam)
mmaxfield: review+
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch to remove static variables (5.93 KB, patch)
2021-12-07 17:15 PST, Cameron McCormack (:heycam)
no flags 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-01 23:19:42 PST
With OffscreenCanvas, we can call into ComplexTextController::adjustGlyphsAndAdvances, FontCascade::expansionOpportunityCountInternal, and WidthIterator::calculateAdditionalWidth from worker threads.  These all have a static variable variable with an initializer that calls FontCascade::canExpandAroundIdeographsInComplexText, and such initializers are not safe under -fno-threadsafe-statics.

canExpandAroundIdeographsInComplexText is a simple enough function in all ports (it just returns a constant bool) that it's not worth caching the result in a static.
Comment 1 Cameron McCormack (:heycam) 2021-12-01 23:20:59 PST
Created attachment 445677 [details]
Patch with dependencies for EWS
Comment 2 Cameron McCormack (:heycam) 2021-12-01 23:21:23 PST
Created attachment 445678 [details]
Patch
Comment 3 Myles C. Maxfield 2021-12-07 00:51:45 PST
Comment on attachment 445678 [details]
Patch

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

> Source/WebCore/ChangeLog:18
> +        canExpandAroundIdeographsInComplexText is a simple enough function in
> +        all ports (it just returns a constant bool) that it's not worth caching
> +        the result in a static.

Can we make the function constexpr?

Do you know why the other ports can't expand around ideographs? Expanding around ideographs is a property of written language, not a property of which OS you're running on.
Comment 4 Cameron McCormack (:heycam) 2021-12-07 13:04:15 PST
(In reply to Myles C. Maxfield from comment #3)
> Can we make the function constexpr?

Since constexpr functions are implicitly inline, we'd need to define new header files like FontCascadeInlines{Cocoa,Win,Cairo}.h and include the right one in the cpp files that call the function.  I don't think it's worth it.

> Do you know why the other ports can't expand around ideographs? Expanding
> around ideographs is a property of written language, not a property of which
> OS you're running on.

I don't know.  The function was added in bug 53184 and I suspect it's just that support wasn't added for non-Cocoa ports at the time.
Comment 5 Cameron McCormack (:heycam) 2021-12-07 14:04:52 PST
Offline discussion with Myles led us to conclude that all ports are now using ComplexTextController, and so we can remove canExpandAroundIdeographsInComplexText entirely.
Comment 6 Cameron McCormack (:heycam) 2021-12-07 14:34:56 PST
Created attachment 446238 [details]
Patch to remove canExpandAroundIdeographsInComplexText which had test failures
Comment 7 Myles C. Maxfield 2021-12-07 14:47:01 PST
Comment on attachment 446238 [details]
Patch to remove canExpandAroundIdeographsInComplexText which had test failures

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

Very excited!

> Source/WebCore/ChangeLog:14
> +        With OffscreenCanvas, we can call into
> +        ComplexTextController::adjustGlyphsAndAdvances,
> +        FontCascade::expansionOpportunityCountInternal, and
> +        WidthIterator::calculateAdditionalWidth from worker threads.
> +        These all have a static variable variable with an initializer
> +        that calls FontCascade::canExpandAroundIdeographsInComplexText,
> +        and such initializers are not safe under -fno-threadsafe-statics.

This should probably be updated.
Comment 8 Cameron McCormack (:heycam) 2021-12-07 14:54:04 PST
(In reply to Myles C. Maxfield from comment #7)
> This should probably be updated.

It's still accurate as a description of the code before the change.
Comment 9 Cameron McCormack (:heycam) 2021-12-07 16:05:12 PST
There's a failure in a ruby test on Windows, so perhaps it's not safe after all to remove those functions? At least without further digging. I'm going to move that removal patch to another bug, and land the original one here.
Comment 10 Cameron McCormack (:heycam) 2021-12-07 17:15:00 PST
Created attachment 446260 [details]
Patch to remove static variables
Comment 11 EWS 2021-12-07 18:12:21 PST
Committed r286637 (244950@main): <https://commits.webkit.org/244950@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 446260 [details].
Comment 12 Radar WebKit Bug Importer 2021-12-07 18:13:23 PST
<rdar://problem/86186510>