Bug 67933

Created attachment 107050 [details] WIP Patch V0

Created attachment 107051 [details] patch for chromium Applying this patch to chromium is needed to test the WIP patch. The WIP patch isn't ready for review. I'd like to ask general suggestions/comments. The patch contains the same problems of WIP patch on bug 64930. > - Indic/South East Asian(SEA) scripts are not rendered correctly because harfbuzz-ng doesn't support fully these script yet. > - I don't see performance regression on page cycler test intl1, but selectionRect() remarkably slow than the old harfbuzz. In addition, Arabic scripts doesn't shape correctly with harfbuzz ToT for now. I'll investigate this problem.

I tried reviewing using the review tool but it says I'm not authorized to edit the attachment. Here's my review in plain text. Let me know if I can reupload it for better readability. View in context: https://bugs.webkit.org/attachment.cgi?id=107050&action=review Thanks for the work! Looks great. I've commented on all the points that can be improved. > Source/WebCore/platform/graphics/chromium/FontLinux.cpp:218 > + positions[i].set(harfbuzzRun->positions()[i].width(),harfbuzzRun->positions()[i].height()); Why not get the pointer once outside the loop like you do for infos? > Source/WebCore/platform/graphics/chromium/FontLinux.cpp:223 > + canvas->drawPosText(&glyphs[0], numGlyphs << 1, &positions[0], fillPaint); Excute my ignorance. Do we need a debug assertion or something to make sure paint->setTextEncoding(SkPaint::kGlyphID_TextEncoding); has been done? > Source/WebCore/platform/graphics/harfbuzz-ng/HarfbuzzFaceChromiumLinux.cpp:51 > + // hb_position_t is a 26.6 fixed point format. To be clear, this is not really the case. hb_position_t is whatever you as the user decide it to be. Ie. how you set the font scale. And maybe 16.16 is more appropriate for example. > Source/WebCore/platform/graphics/harfbuzz-ng/HarfbuzzFaceChromiumLinux.cpp:61 > + uint16_t glyph = codepoint; Should handle non-BMP codepoints. > Source/WebCore/platform/graphics/harfbuzz-ng/HarfbuzzFaceChromiumLinux.cpp:68 > + extents->y_bearing = SkiaScalarToHarfbuzzPosition(skBounds.fTop); I *think* we need a negative sign here? > Source/WebCore/platform/graphics/harfbuzz-ng/HarfbuzzFaceChromiumLinux.cpp:87 > + paint.textToGlyphs(text, length, &glyph16); Is this the easiest way Skia provides? Looks tedious to me. > Source/WebCore/platform/graphics/harfbuzz-ng/HarfbuzzFaceChromiumLinux.cpp:89 > + return true; This is not good. Inhibits some tricks we do with unsupported glyphs. Any way we can detect whether *glyph is the unknown glyph? Even returning !!*glyph would be better than returning true. > Source/WebCore/platform/graphics/harfbuzz-ng/HarfbuzzFaceChromiumLinux.cpp:98 > + font->setupPaint(&paint); This seem to be a recurring thing. Maybe we can setup one beforehand and include it in the fontData closure? Then we just need to setupPaint once before calling hb_shape(). Currently we're doing setupPaint() at least two times per characters, and that's going to show up... > Source/WebCore/platform/graphics/harfbuzz-ng/HarfbuzzFaceChromiumLinux.cpp:106 > + return 0; You're better off not setting this function instead of setting it and returning 0. Because the fallback implementation at least uses em-height which is a very good approximation of this. > Source/WebCore/platform/graphics/harfbuzz-ng/HarfbuzzFaceChromiumLinux.cpp:112 > + // implementation does. Correct. Because glyph origin is left-baseline. > Source/WebCore/platform/graphics/harfbuzz-ng/HarfbuzzFaceChromiumLinux.cpp:118 > + // FIXME: Can't support vertical origin for now because Skia doesn't provide such features. Or just not set it. Same lack of functionality either way :(. > Source/WebCore/platform/graphics/harfbuzz-ng/HarfbuzzFaceChromiumLinux.cpp:125 > + return 0; Again, better not set it. In the future HarfBuzz will be able to read the kern table directly if you don't set this callback. > Source/WebCore/platform/graphics/harfbuzz-ng/HarfbuzzFaceChromiumLinux.cpp:130 > + // Won't support vertical kerning because FreeType doesn't support it. Same here. Although vertical 'kern' table probably wouldn't be implemented as I'm yet to see a font that uses it. > Source/WebCore/platform/graphics/harfbuzz-ng/HarfbuzzFaceChromiumLinux.cpp:146 > + // We can't support this. (Skia doesn't provide the feature) Same thing. Don't set. In the future HarfBuzz may read the 'glyf' table itself if you don't set this callback. > Source/WebCore/platform/graphics/harfbuzz-ng/HarfbuzzFaceChromiumLinux.cpp:165 > + } For good style, call hb_font_funcs_make_immutable() here. > Source/WebCore/platform/graphics/harfbuzz-ng/HarfbuzzFaceChromiumLinux.cpp:182 > + return 0; Free buffer here? > Source/WebCore/platform/graphics/harfbuzz-ng/HarfbuzzFaceChromiumLinux.cpp:208 > + int scale = size * (1 << 16) / 1000; Humm. This looks wrong. I assume size is pixel-size. In which case you just need (size << 6) here, since you chose to work in 26.6 earlier. I suggested working in 16.16, but whichever you choose, just be consistent. > Source/WebCore/platform/graphics/harfbuzz-ng/HarfbuzzFaceChromiumLinux.cpp:210 > +} Calling hb_font_make_immutable would be good style. > Source/WebCore/platform/graphics/harfbuzz-ng/HarfbuzzShaper.cpp:46 > + return (float)value / 64.0; Maybe round at least? Humm. The function name and comment are inconsistent with the code. It *is* returning a subpixel position as a float. I don't see the ToInteger part. > Source/WebCore/platform/graphics/harfbuzz-ng/HarfbuzzShaper.cpp:72 > + m_features[i].tag = HB_TAG(tag[0], tag[1], tag[2], tag[3]); I assume we know that we definitely have four bytes here? If not, you may want to use hb_tag_from_string(). > Source/WebCore/platform/graphics/harfbuzz-ng/HarfbuzzShaper.cpp:82 > +} Use some ICU thing here to handle all space characters? Handle NBSP at least? Not sure if Webkit passes that down intact. > Source/WebCore/platform/graphics/harfbuzz-ng/HarfbuzzShaper.cpp:84 > +bool HarfbuzzShaper::isWordBreak(unsigned index) Maybe better named isWordEnd? > Source/WebCore/platform/graphics/harfbuzz-ng/HarfbuzzShaper.cpp:86 > + return index && isCodepointSpace(m_run[index]) && !isCodepointSpace(m_run[index - 1]); A bit harsh if m_run[index - 1] is a high-surrogate, but works. > Source/WebCore/platform/graphics/harfbuzz-ng/HarfbuzzShaper.cpp:104 > + if (isWordBreak(i)) What if multiple spaces are there in a row? As in "&nbsp;&nbsp;&nbsp;"? Currently they just get one share of padding instead of three. > Source/WebCore/platform/graphics/harfbuzz-ng/HarfbuzzShaper.cpp:117 > + ASSERT(offset < m_currentTextRun->numCharacters()); Is this surrogate-safe? > Source/WebCore/platform/graphics/harfbuzz-ng/HarfbuzzShaper.cpp:152 > + while (nextTextRun()) This and previous function change the run iterator without resetting it. Is that expected? > Source/WebCore/platform/graphics/harfbuzz-ng/HarfbuzzShaper.cpp:161 > + m_startIndexOfCurrentRun += m_currentTextRun->numCharacters(); Is this surrogate safe? > Source/WebCore/platform/graphics/harfbuzz-ng/HarfbuzzShaper.cpp:164 > + // a sngile font. Typo. > Source/WebCore/platform/graphics/harfbuzz-ng/HarfbuzzShaper.cpp:206 > + // Iterate through the glyphs in physical order, no need flipping for RTL. s/physical order/visual order/ ? > Source/WebCore/platform/graphics/harfbuzz-ng/HarfbuzzShaper.cpp:213 > + totalAdvance += determineWordBreakSpacing(glyphInfos[i].cluster); I like this! Very clever. > Source/WebCore/platform/graphics/harfbuzz-ng/HarfbuzzShaper.cpp:230 > + totalAdvance += m_letterSpacing; Shouldn't you add this before setting the position for this glyph? Ie. a few lines earlier? > Source/WebCore/platform/graphics/harfbuzz-ng/HarfbuzzShaper.cpp:250 > + ASSERT(glyphIndex < m_currentTextRun->numGlyphs()); If we do proper rounding, it is conceivable that glyphIndex == m_currentTextRun->numGlyphs() here. > Source/WebCore/platform/graphics/harfbuzz-ng/HarfbuzzShaper.cpp:263 > + // Iterate through the script runs in physical order, searching for the run covering the positions of interest. s/physical/visual/ here too? > Source/WebCore/platform/graphics/harfbuzz-ng/HarfbuzzShaper.cpp:279 > + fromX = startX; Don't you also need rtl handling here too? Like you do for toX. > Source/WebCore/platform/graphics/harfbuzz-ng/HarfbuzzTextRun.cpp:42 > +static inline double truncateFixedPointToInteger(hb_position_t value) Same comment as the previous instance. > Source/WebCore/platform/graphics/harfbuzz-ng/HarfbuzzTextRun.cpp:54 > + m_harfbuzzBuffer = hb_buffer_create(); Reusing buffer may be worthwhile, though I'm not sure. > Source/WebCore/platform/graphics/harfbuzz-ng/HarfbuzzTextRun.cpp:57 > + // TODO: Should call shape() here? Should set buffer script, direction, and language here. The auto-detect functionality is really not to be trusted... > Source/WebCore/platform/graphics/harfbuzz-ng/HarfbuzzTextRun.cpp:104 > + int advance = truncateFixedPointToInteger(positions[glyphIndex].x_advance); The way you do it, the truncation error will accumulate. You may want to keep lastX in non-truncated hb_position_t or something. > Source/WebCore/platform/graphics/harfbuzz-ng/HarfbuzzTextRun.cpp:111 > + return rtl() ? 0 : m_numGlyphs - 1; Should really return m_numGlyphs in ltr mode. Ie. *after* last glyph. > Source/WebCore/platform/graphics/harfbuzz-ng/HarfbuzzTextRun.cpp:121 > + position += truncateFixedPointToInteger(glyphPositions()[glyphIndex].x_advance); This needs more work. You should add the x_advance of the whole cluster, no just the first glyph. > Source/WebCore/platform/graphics/skia/GlyphPageTreeNodeSkia.cpp:54 > + // FIXME: Implement. If you are really keen you can add the character to a buffer and try applying the 'vrt2' and 'vert' features using the hb-ot-layout.h API...

Regarding the chromium patch: the mesa and other gl changes at the end are definitely noise and not intended.

*** This bug has been marked as a duplicate of bug 69826 ***