Summary: | Use HarfBuzz ot-math API to parse the OpenType MATH table | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Frédéric Wang (:fredw) <fred.wang> | ||||||||||||||
Component: | Text | Assignee: | 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
Frédéric Wang (:fredw)
2016-09-28 05:28:24 PDT
Created attachment 290076 [details]
Make jhbuild use the harfbuzz development version
Created attachment 290092 [details]
WIP Patch (does not build with the current version of harfbuzz)
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 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 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. Created attachment 290200 [details] WIP Patch (does not build without fixing bug 162719) New patch with the comment above addressed. 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. Created attachment 291172 [details]
Patch
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? (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. 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.
Created attachment 293290 [details]
Patch
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 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. Created attachment 293346 [details]
Patch
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 on attachment 293346 [details] Patch Clearing flags on attachment: 293346 Committed r208128: <http://trac.webkit.org/changeset/208128> All reviewed patches have been landed. Closing bug. |