Bug 162671 – Use HarfBuzz ot-math API to parse the OpenType MATH table

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

[Frédéric Wang (:fredw)]

The next version of HarfBuzz will provide a new API to parse the OpenType MATH table:

https://github.com/behdad/harfbuzz/blob/master/src/hb-ot-math.h 

When USE_HARFBUZZ is enabled, we should use that API instead of our own parsing code in Source/WebCore/platform/graphics/opentype/OpenTypeMathData.cpp

[Frédéric Wang (:fredw)]

Created attachment 290076 [details]
Make jhbuild use the harfbuzz development version

[Frédéric Wang (:fredw)]

Created attachment 290092 [details]
WIP Patch (does not build with the current version of harfbuzz)

[Khaled Hosny]

Comment on attachment 290092 [details]
WIP Patch (does not build with the current version of harfbuzz)

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

> Source/WebCore/platform/graphics/opentype/OpenTypeMathData.cpp:237
> +#if USE(HARFBUZZ)

I think the USE(HARFBUZZ) check is misplaced, it looks like the code won’t compile with ENABLE(OPENTYPE_MATH) and !USE(HARFBUZZ).

> Source/WebCore/platform/graphics/opentype/OpenTypeMathData.cpp:319
> +    }

This seems to be a mere stylistic change (no idea what is the policy here about such changes, though).

[Frédéric Wang (:fredw)]

Comment on attachment 290092 [details]
WIP Patch (does not build with the current version of harfbuzz)

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

>> Source/WebCore/platform/graphics/opentype/OpenTypeMathData.cpp:237
>> +#if USE(HARFBUZZ)
> 
> I think the USE(HARFBUZZ) check is misplaced, it looks like the code won’t compile with ENABLE(OPENTYPE_MATH) and !USE(HARFBUZZ).

Yes, that's right.

>> Source/WebCore/platform/graphics/opentype/OpenTypeMathData.cpp:319
>> +    }
> 
> This seems to be a mere stylistic change (no idea what is the policy here about such changes, though).

It's a mistake and should not have been done. (webkit-check-style will complain). Thanks for catching this.

[Frédéric Wang (:fredw)]

Comment on attachment 290092 [details]
WIP Patch (does not build with the current version of harfbuzz)

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

> Source/WebCore/platform/graphics/opentype/OpenTypeMathData.cpp:287
> +    return (font.platformData().size() * value) / scale;

This is actually not needed. We only need to divide by 65536 to convert to float.

[Frédéric Wang (:fredw)]

Created attachment 290200 [details]
WIP Patch (does not build without fixing bug 162719)

New patch with the comment above addressed.

[Frédéric Wang (:fredw)]

Comment on attachment 290200 [details]
WIP Patch (does not build without fixing bug 162719)

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

> Source/WebCore/platform/graphics/opentype/OpenTypeMathData.cpp:354
> +    unsigned variantsSize = sizeof (variants) / sizeof (variants[0]);

WTF_ARRAY_LENGTH should be used here.

> Source/WebCore/platform/graphics/opentype/OpenTypeMathData.cpp:367
> +    unsigned partsSize = sizeof (parts) / sizeof (parts[0]);

Ditto.

[Frédéric Wang (:fredw)]

Created attachment 291172 [details]
Patch

[Myles C. Maxfield]

Comment on attachment 290076 [details]
Make jhbuild use the harfbuzz development version

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

> Tools/gtk/jhbuild.modules:137
> +            checkoutdir="harfbuzz-0.9.35"/>

Is it really wise to rely on a development version?

[Frédéric Wang (:fredw)]

(In reply to comment #9)
> Is it really wise to rely on a development version?

This patch is for testing purpose only. See bug 162719, on which this bug depends.

[WebKit Commit Bot]

Attachment 291172 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/opentype/OpenTypeMathData.cpp:373:  Omit int when using unsigned  [runtime/unsigned] [1]
ERROR: Source/WebCore/platform/graphics/opentype/OpenTypeMathData.cpp:374:  Omit int when using unsigned  [runtime/unsigned] [1]
ERROR: Source/WebCore/platform/graphics/opentype/OpenTypeMathData.cpp:379:  Omit int when using unsigned  [runtime/unsigned] [1]
ERROR: Source/WebCore/platform/graphics/opentype/OpenTypeMathData.cpp:389:  Use nullptr instead of NULL.  [readability/null] [5]
ERROR: Source/WebCore/platform/graphics/opentype/OpenTypeMathData.cpp:391:  Omit int when using unsigned  [runtime/unsigned] [1]
Total errors found: 5 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Frédéric Wang (:fredw)]

Created attachment 293290 [details]
Patch

[Frédéric Wang (:fredw)]

Comment on attachment 293290 [details]
Patch

The new code is only going to be used on WebKitGTK+ (I opened bug 164177 for EFL). I am currently not able to build it, but last time I tried I also got one MathML test that required a small rebaseline.

[Michael Catanzaro]

Comment on attachment 293290 [details]
Patch

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

You'll need to add compile-time version checks to support building with Harfbuzz 0.9.35, per our new dependencies policy.

> Source/WebCore/platform/graphics/opentype/OpenTypeMathData.h:137
> +    hb_font_t* m_mathFont;

You should add a smart pointer. We should avoid holding or transferring ownership with raw pointers.

[Frédéric Wang (:fredw)]

Created attachment 293346 [details]
Patch

[Michael Catanzaro]

Comment on attachment 293346 [details]
Patch

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

> Source/cmake/OptionsGTK.cmake:64
> +# For old versions of HarfBuzz that do not expose an API for the OpenType MATH
> +# table, we enable our own code to parse that table.
> +if ("${PC_HARFBUZZ_VERSION}" VERSION_LESS "1.3.3")
> +    add_definitions(-DENABLE_OPENTYPE_MATH=1)
> +endif ()

Good solution!

[WebKit Commit Bot]

Comment on attachment 293346 [details]
Patch

Clearing flags on attachment: 293346

Committed r208128: <http://trac.webkit.org/changeset/208128>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.