Summary: | remove expandAroundIdeographs static variables | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Cameron McCormack (:heycam) <heycam> | ||||||||||
Component: | Layout and Rendering | Assignee: | Cameron McCormack (:heycam) <heycam> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | bfulgham, cdumez, esprehn+autocc, ews-watchlist, glenn, gyuyoung.kim, hi, joepeck, kangil.han, macpherson, menard, mifenton, mmaxfield, pangle, simon.fraser, webkit-bug-importer, zalan | ||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||
Version: | WebKit Local Build | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Bug Depends on: | |||||||||||||
Bug Blocks: | 233488, 233747 | ||||||||||||
Attachments: |
|
Description
Cameron McCormack (:heycam)
2021-12-01 23:19:42 PST
Created attachment 445677 [details]
Patch with dependencies for EWS
Created attachment 445678 [details]
Patch
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. (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. Offline discussion with Myles led us to conclude that all ports are now using ComplexTextController, and so we can remove canExpandAroundIdeographsInComplexText entirely. Created attachment 446238 [details]
Patch to remove canExpandAroundIdeographsInComplexText which had test failures
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. (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. 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. Created attachment 446260 [details]
Patch to remove static variables
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]. |