Bug 219947

Summary: Make FontCascade::CodePath an enum class
Product: WebKit Reporter: Rob Buis <rbuis>
Component: New BugsAssignee: Rob Buis <rbuis>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, changseok, darin, dino, esprehn+autocc, ews-watchlist, fmalita, glenn, gyuyoung.kim, kondapallykalyan, mmaxfield, pdr, sabouhallawa, schenney, sergio, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
ews-feeder: commit-queue-
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Rob Buis 2020-12-16 06:46:51 PST
Make FontCascade::CodePath an enum class.
Comment 1 Rob Buis 2020-12-16 06:51:49 PST
Created attachment 416336 [details]
Patch
Comment 2 Rob Buis 2020-12-16 07:22:31 PST
Created attachment 416338 [details]
Patch
Comment 3 Alex Christensen 2020-12-16 12:21:56 PST
Comment on attachment 416338 [details]
Patch

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

> Source/WebKit/ChangeLog:11
> +2020-12-16  Rob Buis  <rbuis@igalia.com>

Double change log entry.

> Source/WebCore/platform/graphics/FontCascade.h:-192
> -    enum CodePath { Auto, Simple, Complex, SimpleWithGlyphOverflow };

This should probably remain a nested enum.
FontCascade::CodePath::Auto
Comment 4 Rob Buis 2020-12-16 14:37:02 PST
Created attachment 416363 [details]
Patch
Comment 5 Rob Buis 2020-12-16 14:38:17 PST
Comment on attachment 416338 [details]
Patch

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

>> Source/WebKit/ChangeLog:11
>> +2020-12-16  Rob Buis  <rbuis@igalia.com>
> 
> Double change log entry.

Oops! Fixed.

>> Source/WebCore/platform/graphics/FontCascade.h:-192
>> -    enum CodePath { Auto, Simple, Complex, SimpleWithGlyphOverflow };
> 
> This should probably remain a nested enum.
> FontCascade::CodePath::Auto

A with verbose, but ok...
Comment 6 Alex Christensen 2020-12-16 16:41:42 PST
Looks like you missed one in Source\WebCore\platform\win\WebCoreTextRenderer.cpp
Comment 7 Rob Buis 2020-12-16 23:28:14 PST
Created attachment 416398 [details]
Patch
Comment 8 Rob Buis 2020-12-17 00:25:45 PST
Created attachment 416400 [details]
Patch
Comment 9 EWS 2020-12-17 03:19:10 PST
Committed r270932: <https://trac.webkit.org/changeset/270932>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 416400 [details].
Comment 10 Radar WebKit Bug Importer 2020-12-17 03:20:20 PST
<rdar://problem/72420593>
Comment 11 Darin Adler 2020-12-17 09:17:17 PST
Comment on attachment 416400 [details]
Patch

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

> Source/WebCore/platform/graphics/FontCascade.h:-41
> -// "X11/X.h" defines Complex to 0 and conflicts
> -// with Complex value in CodePath enum.
> -#ifdef Complex
> -#undef Complex
> -#endif

This change seems unrelated to the rest. If there is a macro named Complex, then it’s just as much a problem with the enum class as it as with the enum.
Comment 12 Rob Buis 2020-12-17 09:35:49 PST
Reopening to attach new patch.
Comment 13 Rob Buis 2020-12-17 09:35:53 PST
Created attachment 416430 [details]
Patch
Comment 14 Rob Buis 2020-12-17 09:40:03 PST
Comment on attachment 416400 [details]
Patch

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

>> Source/WebCore/platform/graphics/FontCascade.h:-41
>> -#endif
> 
> This change seems unrelated to the rest. If there is a macro named Complex, then it’s just as much a problem with the enum class as it as with the enum.

The change was intentional on my part but wrong :( I prepared the partial revert patch, I am wondering if there is a rename possible for Complex, but I bet others have considered that in the past already, and I guess we need to keep it that way to match ComplexText in some method names.
Comment 15 Alex Christensen 2020-12-17 09:41:51 PST
If GTK builds successfully without undefining Complex, let's just keep it removed.
Comment 16 Darin Adler 2020-12-17 09:44:54 PST
(In reply to Alex Christensen from comment #15)
> If GTK builds successfully without undefining Complex, let's just keep it
> removed.

Exactly, but that can be done even if we don’t do anything else.
Comment 17 Rob Buis 2020-12-17 10:03:12 PST
FWIW CodePath::Complex prints as 2 on my Linux Ubuntu setup. I wonder if we include X11 headers in any configuration...
Comment 18 Rob Buis 2021-01-24 07:32:52 PST
Comment on attachment 416430 [details]
Patch

I guess we can do this, if we want to remove such code it should probably be a separate bug with a clear title anyway.
Comment 19 EWS 2021-01-24 13:58:58 PST
Committed r271784: <https://trac.webkit.org/changeset/271784>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 416430 [details].