Bug 162671

Summary: Use HarfBuzz ot-math API to parse the OpenType MATH table
Product: WebKit Reporter: Frédéric Wang (:fredw) <fred.wang>
Component: TextAssignee: Frédéric Wang (:fredw) <fred.wang>
Status: RESOLVED FIXED    
Severity: Normal CC: alex, annulen, behdad, benjamin, cdumez, cmarcelo, commit-queue, dbates, dr.khaled.hosny, mcatanzaro, mmaxfield, mrobinson, rego
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
URL: https://github.com/behdad/harfbuzz/
Bug Depends on: 162719    
Bug Blocks:    
Attachments:
Description Flags
Make jhbuild use the harfbuzz development version
none
WIP Patch (does not build with the current version of harfbuzz)
none
WIP Patch (does not build without fixing bug 162719)
none
Patch
none
Patch
none
Patch none

Frédéric Wang (:fredw)
Reported 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
Attachments
Make jhbuild use the harfbuzz development version (782 bytes, patch)
2016-09-28 05:29 PDT, Frédéric Wang (:fredw)
no flags
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
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
Patch (5.70 KB, patch)
2016-10-10 15:16 PDT, Frédéric Wang (:fredw)
no flags
Patch (6.29 KB, patch)
2016-10-29 07:09 PDT, Frédéric Wang (:fredw)
no flags
Patch (16.48 KB, patch)
2016-10-30 04:57 PDT, Frédéric Wang (:fredw)
no flags
Frédéric Wang (:fredw)
Comment 1 2016-09-28 05:29:44 PDT
Created attachment 290076 [details] Make jhbuild use the harfbuzz development version
Frédéric Wang (:fredw)
Comment 2 2016-09-28 09:29:07 PDT
Created attachment 290092 [details] WIP Patch (does not build with the current version of harfbuzz)
Khaled Hosny
Comment 3 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).
Frédéric Wang (:fredw)
Comment 4 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.
Frédéric Wang (:fredw)
Comment 5 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.
Frédéric Wang (:fredw)
Comment 6 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.
Frédéric Wang (:fredw)
Comment 7 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.
Frédéric Wang (:fredw)
Comment 8 2016-10-10 15:16:49 PDT
Myles C. Maxfield
Comment 9 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?
Frédéric Wang (:fredw)
Comment 10 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.
WebKit Commit Bot
Comment 11 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.
Frédéric Wang (:fredw)
Comment 12 2016-10-29 07:09:40 PDT
Frédéric Wang (:fredw)
Comment 13 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.
Michael Catanzaro
Comment 14 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.
Frédéric Wang (:fredw)
Comment 15 2016-10-30 04:57:16 PDT
Michael Catanzaro
Comment 16 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!
WebKit Commit Bot
Comment 17 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>
WebKit Commit Bot
Comment 18 2016-10-30 09:21:59 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.