[webkit-reviews] review denied: [Bug 50365] Glyphs in vertical text tests are rotated 90 degrees clockwise on Chromium Linux and Windows : [Attachment 75343] A patch to make Chromium Linux render vertical text correctly

[bugzilla-daemon at webkit.org]

Kent Tamura <tkent at chromium.org> has denied Koan-Sin Tan
<koansin.tan at gmail.com>'s request for review:
Bug 50365: Glyphs in vertical text tests are rotated 90 degrees clockwise on
Chromium Linux and Windows
https://bugs.webkit.org/show_bug.cgi?id=50365

Attachment 75343: A patch to make Chromium Linux render vertical text correctly
https://bugs.webkit.org/attachment.cgi?id=75343&action=review

------- Additional Comments from Kent Tamura <tkent at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=75343&action=review

I'm not familiar with this area.  So I make some comments on patch style.

* Please add a ChangeLog entry. WebCore/ChangeLog.
* Please make a patch at the top level directory.  You made the patch at
WebKit/WebCore, should make at WebKit/.

You can use WebKitTools/Scripts/webkit-patch to make an acceptable patch.

> platform/graphics/chromium/FontLinux.cpp:106
> +	       pos2[i].set(x+SkFloatToScalar(adv[i].width()), y);
> +	       pos3[i].set(x+SkFloatToScalar(adv[i].width()),
y-SkFloatToScalar(adv[i].width())); 

Add spaces around operators such as + -

> platform/graphics/chromium/FontLinux.cpp:132
> +		   canvas->drawTextOnPath(glyphs+i, 2, path, 0, paint);

Add spaces around +

> platform/graphics/chromium/FontLinux.cpp:136
> +	   } else {
> +	       canvas->drawPosText(glyphs, numGlyphs << 1, pos, paint);
> +	   }

Remove { and }

> platform/graphics/skia/GlyphPageTreeNodeSkia.cpp:72
> +	       for (int i=0; i < bufferLength; i++) {
> +		   hb_buffer_add_glyph(buffer, glyphs[i], 0, i);
> +	       }

Add spaces around =.
Remove { and }

> platform/graphics/skia/GlyphPageTreeNodeSkia.cpp:80
> +	       HB_GSUB_Select_Script(hb_face->gsub, 
> +			   HB_MAKE_TAG('D', 'F', 'L', 'T'), &script_index);
> +	       HB_GSUB_Select_Feature(hb_face->gsub,
> +			   HB_MAKE_TAG('v', 'e', 'r', 't'), script_index,
> +			    0xffff, &feature_index);

You don't need wrap these lines. WebKit style allow looooong lines.

> platform/graphics/skia/GlyphPageTreeNodeSkia.cpp:90
> +		   for (int i=0; i < bufferLength; i++) {
> +		       glyphs[i] = (Glyph) buffer->out_string[i].gindex;
> +		   }

Remove { and }.
We prefer C++-style cast.