[webkit-reviews] review denied: [Bug 25068] [Chromium] Complex text support for Chromium Linux : [Attachment 31811] patch

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Adam Langley <agl at chromium.org>'s
request for review:
Bug 25068: [Chromium] Complex text support for Chromium Linux
https://bugs.webkit.org/show_bug.cgi?id=25068

Attachment 31811: patch
https://bugs.webkit.org/attachment.cgi?id=31811&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
God I hate C.

Style:
 322	 bool NextRTLScriptRun()
 352	 bool NextLTRScriptRun()

Why copy/paste here?

 571	     if (fromX == -1 && from < walker.numCodePoints()) {
 572		 // |from| is within this script run. So we index the clusters
log to
 573		 // find which glyph this code-point contributed to and find
its x
 574		 // position.
 575		 int glyph = walker.logClusters()[from];
 576		 fromX = base + walker.xPositions()[glyph];
 577		 fromAdvance = walker.advances()[glyph];
 578	     } else
 579		 from -= walker.numCodePoints();
 580 
 581	     if (toX == -1 && to < walker.numCodePoints()) {
 582		 int glyph = walker.logClusters()[to];
 583		 toX = base + walker.xPositions()[glyph];
 584		 toAdvance = walker.advances()[glyph];
 585	     } else
 586		 to -= walker.numCodePoints();

Again:

 593	 // The position in question might be just after the text.
 594	 const int rightEdge = base;
 595	 if (fromX == -1 && !from)
 596	     fromX = leftEdge;
 597	 else if (walker.rtl())
 598	    fromX += truncateFixedPointToInteger(fromAdvance);
 599 
 600	 if (toX == -1 && !to)
 601	     toX = rightEdge;
 602	 else if (!walker.rtl())
 603	     toX += truncateFixedPointToInteger(toAdvance);

Style:
 51 static HB_Fixed SkiaScalarToHarfbuzzFixed(SkScalar value)
 57 static HB_Bool StringToGlyphs(HB_Font hbFont, const HB_UChar16* characters,
hb_uint32 length, HB_Glyph* glyphs, hb_uint32* glyphsSize, HB_Bool isRTL)
 79 static void GlyphsToAdvances(HB_Font hbFont, const HB_Glyph* glyphs,
hb_uint32 numGlyphs, HB_Fixed* advances, int flags)
 105 static HB_Bool CanRender(HB_Font hbFont, const HB_UChar16* characters,
hb_uint32 length)


Style:
197 HB_FontClass harfbuzzSkiaClass = {
 198   StringToGlyphs,
 199   GlyphsToAdvances,
 200   CanRender,
 201   GetOutlinePoint,
 202   GetGlyphMetrics,
 203   GetFontMetric,
 204 };

This would probably have been easier in smaller pieces?  This code is pretty
ugly.  Mostly because it's C... and hard enough to stomach in small chunks. 
Silly manual memory management.

I still don't really know what this all is.  But at some level it doesn't
matter.  This is your port, and I don't need to be a gate keeper.  Please post
a new patch with fixed style though.