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

Tue Jun 23 17:47:01 PDT 2009

David Levin <levin at chromium.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 31430: /tmp/patch
https://bugs.webkit.org/attachment.cgi?id=31430&action=review

------- Additional Comments from David Levin <levin at chromium.org>
Many of my comments are style related and I'm doing an r- because there are
quite a few to fix up.

Even though I have tried to verify the logic, it would be great if you got
someone on the chromium team more familiar with text handling to look over the
code.  Perhaps jshin or playmobil would be good choices.

> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog

> +2009-06-16  Adam Langley  <agl at google.com>
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   Chromium: Add complex text support on Linux.

Add link to bug here.

> +	   (WebCore::GetGlyphMetrics):
> +	   (WebCore::GetFontMetric):
> +	   (WebCore::):
This appears to be a prepare-Changelog goof, so just delete it (or fix it if
you figure out what function this should have been).

> diff --git a/WebCore/platform/graphics/chromium/FontLinux.cpp
b/WebCore/platform/graphics/chromium/FontLinux.cpp

> +extern HB_Error harfbuzzSkiaGetTable(void* voidface, const HB_Tag tag,
HB_Byte* buffer, HB_UInt* len);

I'd omit the param name "tag" since it doesn't appear to add any info.

> +class TextRunWalker {
> +public:
> +    TextRunWalker(const TextRun& run, SkPaint* paint, unsigned startingX,
const FontPlatformData* font)
> +	   : m_run(run)
> +	   , m_paint(paint)

Why does this store m_paint and then never use it?

> +	   m_item.face = HB_NewFace(const_cast<FontPlatformData*>(font),
harfbuzzSkiaGetTable);
> +	   m_item.font = (HB_FontRec*) fastMalloc(sizeof(HB_FontRec));

Use c++ style casts instead of c style casts.  This place and several others.

> +
> +    void setBackwardsIteration(bool is_backwards)
is_backwards 
useCasingLikeThis :)
.

> +	   return m_item.glyphs
> +	       && m_item.attributes
> +	       && m_item.advances
> +	       && m_item.offsets
> +	       && m_glyphs16
> +	       && m_xpos;

This looks rather narrow to me. (You can got beyond 80 columns if desired but
if you find this more readable for this item leave it as is.)

>
> +
> +	       // We overflowed our arrays. Resize and retry.
> +	       if (!expandGlyphArrays())
> +		   return false;
> +	   }
> +
> +	   HB_Fixed adv_sum = 0;
Two problems with "adv_sum"
1. abbrvtn
2. theCasing

> +	   for (unsigned i = 0; i < m_item.num_glyphs; ++i) {
> +	       m_glyphs16[i] = m_item.glyphs[i];
> +	       adv_sum += m_item.advances[i] >> 6;
> +	   }

6 is scattered through this code.  (dividing by 64.)  I'm not sure why.
It would be good to create a static const int/unsigned for this which will help
explain what this is by its name and get rid of the 6's all over the place.

> +    uint16_t* m_glyphs16;  // a vector of 16-bit glyph ids
One space before end of line comments.
Also use sentences (or at least a sentence like format). Start with capital and
end with period.

> +    SkScalar* m_xpos;  // a vector of x positions for each glyph
> +    SkPaint* const m_paint;
> +    ssize_t m_indexOfNextScriptRun;	// indexes the script run in |m_run|
> +    const unsigned m_startingX;
> +    unsigned m_offsetX;
> +    unsigned m_pixelWidth;  // width (in px) of the current script run
> +    unsigned m_numCodePoints;  // code points in current script run
> +    unsigned m_maxGlyphs;
> +    bool m_iterateBackwards;
> +};
> +
> +void Font::drawComplexText(GraphicsContext* gc, const TextRun& run,
>			      const FloatPoint& point, int from, int to) const
>  {
> -    notImplemented();
> +    if (run.length() == 0)
Use
    if (run.length())

(avoid comparisons to 0)

> +	 return;
> +
> +    const FontPlatformData& font = fontDataForCharacters(run.characters(),
run.length())->fontDataForCharacter(' ')->platformData();
> +    PlatformGraphicsContext* pgc = gc->platformContext();

Avoid abbreviations "pgc". 
Maybe graphicsContext instead?

> +    bool stroke = (textMode & cTextStroke)
> +		  && pgc->getStrokeStyle() != NoStroke
> +		  && pgc->getStrokeThickness() > 0;
Why not put this all on one line?
(If you leave it like this, then the continuation lines should only be indented
4 spaces.)

> +
> +    TextRunWalker walker(run, stroke ? &strokePaint : &fillPaint, point.x(),
&font);

It looks like fill and stroke may both be true?  Why is only one passed into to
this class? 
(Actually I would just get rid of that parameter from TextRunWalker.)

> +		       return basePosition + j;
> +	       }
> +
> +	       ASSERT(false);
s/ ASSERT(false);/ASSERT_NOT_REACHED();/

>  
> +// Return the rectangle for selecting the given range of code-points in the
> +// TextRun.

This is an odd line break.  In WebKit code, you are certainly not limited to 80
columns

>  FloatRect Font::selectionRectForComplexText(const TextRun& run,
>					       const IntPoint& point, int h,
"h" use unabbreviated names.

>
> +    // the position in question might be just after the text
Use sentence formatting.

> +++ b/WebCore/platform/graphics/chromium/HarfbuzzSkia.cpp

> + * Copyright (c) 2009, Google Inc. All rights reserved.

> +#include <harfbuzz-shaper.h>

Previously you did #include "harfbuzz-shaper.h"

> +
> +static HB_Fixed SkiaScalarToHarfbuzzFixed(SkScalar v)

What does "v" represent?  (Please change the variable name to something
better.)

> +static HB_Bool StringToGlyphs(HB_Font hbFont, const HB_UChar16* chars,
hb_uint32 len, HB_Glyph* glyphs, hb_uint32* glyphsSize, HB_Bool isRTL)

Avoid abbreviations.  Change variable names to something like:
character
length

> +	   memcpy(&value, reinterpret_cast<uint8_t*>(glyphs) + sizeof(uint16_t)
* i, sizeof(uint16_t));

reinterpret_cast<uint8_t*>(glyphs) + sizeof(uint16_t) * i

This looks wrong, but I get it. What do you think about this instead?

reinterpret_cast<uint16_t*>(glyphs) + i

> +static void GlyphsToAdvances(HB_Font hbFont, const HB_Glyph* glyphs,
hb_uint32 numGlyphs, HB_Fixed* advances, int flags)

flags appears unused.  Is that a bug?

> +	   // We use a memcpy to avoid breaking strict aliasing rules.
> +	   memcpy(&value, reinterpret_cast<uint8_t*>(advances) + sizeof(float)
* i, sizeof(float));

Same comment about "reinterpret_cast<uint8_t*>(advances) + sizeof(float) * i"
as before.

> +    font->setupPaint(&paint);
> +    paint.setTextEncoding(SkPaint::kUTF16_TextEncoding);
> +
> +    uint16_t* glyphs16 = (uint16_t*) fastMalloc(sizeof(uint16_t) * len);

Why the use of fastMalloc vs plain old new?
And if you switch to new, it would be good to adopt OwnPtr (from wtf/OwnPtr.h).
You can think of this like a scoped_ptr.

> +    for (int i = 0; i < numGlyphs; ++i) {
> +	   if (glyphs16[i] == 0) {

Avoid comparisons to 0.

> +	       canRender = false;

> +
> +static HB_Error GetOutlinePoint(HB_Font hbFont, HB_Glyph glyph, int flags,
hb_uint32 point, HB_Fixed* xpos, HB_Fixed* ypos, hb_uint32* nPoints)

s/xpos/xPos/
s/ypos/yPos/

What is nPoints?  It looks like an abbreviation.

> +    SkPath path;
> +    paint.getTextPath(&glyph16, sizeof(glyph16), 0, 0, &path);
> +    int numPoints = path.getPoints(NULL, 0);

Use 0 instead of NULL.

> +static HB_Fixed GetFontMetric(HB_Font hbFont, HB_FontMetric metric)
> +{
...
> +    switch (metric) {
> +    case HB_FontAscent:
> +	   return SkiaScalarToHarfbuzzFixed(-skiaMetrics.fAscent);
> +    default:
> +	   return 0;
> +    }

This looks odd to me.  Why a switch instead of an "if"? (since there is only
one case statement)