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
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.