Bug 233750 – remove expandAroundIdeographs static variables

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Cameron McCormack (:heycam)]

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.

[Cameron McCormack (:heycam)]

Created attachment 445677 [details]
Patch with dependencies for EWS

[Cameron McCormack (:heycam)]

Created attachment 445678 [details]
Patch

[Myles C. Maxfield]

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.

[Cameron McCormack (:heycam)]

(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.

[Cameron McCormack (:heycam)]

Offline discussion with Myles led us to conclude that all ports are now using ComplexTextController, and so we can remove canExpandAroundIdeographsInComplexText entirely.

[Cameron McCormack (:heycam)]

Created attachment 446238 [details]
Patch to remove canExpandAroundIdeographsInComplexText which had test failures

[Myles C. Maxfield]

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.

[Cameron McCormack (:heycam)]

(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.

[Cameron McCormack (:heycam)]

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.

[Cameron McCormack (:heycam)]

Created attachment 446260 [details]
Patch to remove static variables

[EWS]

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].

[Radar WebKit Bug Importer]

<rdar://problem/86186510>