Bug 193036 – [FreeType] Restore conditional compilation logic for recent HarfBuzz refactoring

• Status: RESOLVED
• Resolution: FIXED
• Component: Text

[Jim Mason]

Recent changes were made (bug# 192589) to refactor HarfBuzz font creation from OpenTypeMathData to FontPlatformData.  In the process, conditional compilation directives were lost.

The attached patch restores the conditional compilation logic.

[Jim Mason]

Created attachment 358077 [details]
Patch

[Michael Catanzaro]

Comment on attachment 358077 [details]
Patch

r=me to get back to the previous status quo, but it looks problematic. Jim, what port are you building, and with what build flags?

Hey Don, here's another one. ENABLE(OPENTYPE_MATH) is not a valid feature flag, but it's being used throughout several files under platform/graphics/opentype. It's defined only in OptionsGTK.cmake using cpp flags:

add_definitions(-DENABLE_OPENTYPE_MATH=1)

Then looking at USE(HARFBUZZ), my initial reaction is confusion. Surely USE(FREETYPE) must imply USE(HARFBUZZ), but it looks like USE(HARFBUZZ) is only set in OptionsPlayStation.cmake, which is nuts because GTK and WPE both use harfbuzz too, so it must be set somewhere else, but I can't find it. It's used in FontCascade.cpp, HbUniquePtr.h, and OpenTypeMathData.[cpp,h] so that's surely not enough places to actually build if harfbuzz were actually not present.

[Jim Mason]

(In reply to Michael Catanzaro from comment #2)
> Jim,
> what port are you building, and with what build flags?
> 
> Hey Don, here's another one. ENABLE(OPENTYPE_MATH) is not a valid feature
> flag, but it's being used throughout several files under
> platform/graphics/opentype. It's defined only in OptionsGTK.cmake using cpp
> flags

Indeed, OPENTYPE_MATH is being set for me automatically, as I am building using an older version of HarfBuzz.  OptionsGTK.cmake sets the flag if HarfBuzz is older than version 1.3.3, and I am using 1.0.6.

When the code was in OpenTypeMathData, it was compiled according to the construct:

    #if ENABLE(OPENTYPE_MATH)
    #elif USE(HARFBUZZ)
    ..code that was refactored lived here..

Thus, the code was compiled only if USE(HARFBUZZ) && !ENABLE(OPENTYPE_MATH).  Conditioning compilation on !ENABLE(OPENTYPE_MATH) is necessary, as the code references an API that is not exposed by the earlier versions of HarfBuzz.

[WebKit Commit Bot]

Comment on attachment 358077 [details]
Patch

Clearing flags on attachment: 358077

Committed r239554: <https://trac.webkit.org/changeset/239554>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/46951923>

[Jim Mason]

Thanks for the quick review, Michael.

To summarize, !ENABLE(OPENTYPE_MATH) is the active part of this fix.  Without it, users of older versions of HarfBuzz encounter effectively dead code that does not compile.

USE(HARFBUZZ) was included in the fix, as it was part of the original logic.  Whether it is necessary or redundant in other situations, I cannot say.  However, I can say in this specific case, it is unnecessary.