Bug 162671 - Use HarfBuzz ot-math API to parse the OpenType MATH table
Summary: Use HarfBuzz ot-math API to parse the OpenType MATH table
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Text (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Frédéric Wang (:fredw)
URL: https://github.com/behdad/harfbuzz/
Keywords:
Depends on: 162719
Blocks:
  Show dependency treegraph
 
Reported: 2016-09-28 05:28 PDT by Frédéric Wang (:fredw)
Modified: 2016-10-30 09:21 PDT (History)
13 users (show)

See Also:


Attachments
Make jhbuild use the harfbuzz development version (782 bytes, patch)
2016-09-28 05:29 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
WIP Patch (does not build with the current version of harfbuzz) (6.53 KB, patch)
2016-09-28 09:29 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
WIP Patch (does not build without fixing bug 162719) (5.61 KB, patch)
2016-09-29 04:58 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (5.70 KB, patch)
2016-10-10 15:16 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (6.29 KB, patch)
2016-10-29 07:09 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (16.48 KB, patch)
2016-10-30 04:57 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Frédéric Wang (:fredw) 2016-09-28 05:28:24 PDT
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
Comment 1 Frédéric Wang (:fredw) 2016-09-28 05:29:44 PDT
Created attachment 290076 [details]
Make jhbuild use the harfbuzz development version
Comment 2 Frédéric Wang (:fredw) 2016-09-28 09:29:07 PDT
Created attachment 290092 [details]
WIP Patch (does not build with the current version of harfbuzz)
Comment 3 Khaled Hosny 2016-09-29 01:38:44 PDT
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).
Comment 4 Frédéric Wang (:fredw) 2016-09-29 01:46:20 PDT
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.
Comment 5 Frédéric Wang (:fredw) 2016-09-29 04:56:19 PDT
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.
Comment 6 Frédéric Wang (:fredw) 2016-09-29 04:58:23 PDT
Created attachment 290200 [details]
WIP Patch (does not build without fixing bug 162719)

New patch with the comment above addressed.
Comment 7 Frédéric Wang (:fredw) 2016-09-30 02:22:47 PDT
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.
Comment 8 Frédéric Wang (:fredw) 2016-10-10 15:16:49 PDT
Created attachment 291172 [details]
Patch
Comment 9 Myles C. Maxfield 2016-10-10 15:55:17 PDT
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?
Comment 10 Frédéric Wang (:fredw) 2016-10-10 23:41:22 PDT
(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.
Comment 11 WebKit Commit Bot 2016-10-29 06:03:46 PDT
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.
Comment 12 Frédéric Wang (:fredw) 2016-10-29 07:09:40 PDT
Created attachment 293290 [details]
Patch
Comment 13 Frédéric Wang (:fredw) 2016-10-29 07:53:21 PDT
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.
Comment 14 Michael Catanzaro 2016-10-29 08:18:47 PDT
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.
Comment 15 Frédéric Wang (:fredw) 2016-10-30 04:57:16 PDT
Created attachment 293346 [details]
Patch
Comment 16 Michael Catanzaro 2016-10-30 08:27:06 PDT
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!
Comment 17 WebKit Commit Bot 2016-10-30 09:21:52 PDT
Comment on attachment 293346 [details]
Patch

Clearing flags on attachment: 293346

Committed r208128: <http://trac.webkit.org/changeset/208128>
Comment 18 WebKit Commit Bot 2016-10-30 09:21:59 PDT
All reviewed patches have been landed.  Closing bug.