[webkit-reviews] review granted: [Bug 47019] [chromium] FontLinux performance improvement : [Attachment 71229] path

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has granted Xiaomei Ji <xji at chromium.org>'s
request for review:
Bug 47019: [chromium] FontLinux performance improvement
https://bugs.webkit.org/show_bug.cgi?id=47019

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

------- Additional Comments from David Levin <levin at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=71229&action=review

Please consider addressing the feedback when you land this.

Thanks.

> WebCore/platform/graphics/chromium/FontLinux.cpp:231
> +    static void normalizeSpacesAndMirrorChars(const UChar*, bool, UChar*,
int);

Ideally there would be parameter names here that indicated what each of these
parameters should be. (Use the parameter names from your function below.)

The WebKit rule for parameter names is remove them if they don't add any
information.

> WebCore/platform/graphics/chromium/FontLinux.cpp:232
> +    static const TextRun& getNormalizedTextRun(const TextRun&,
OwnPtr<TextRun>&, OwnArrayPtr<UChar>&);

Add parameter names here too.

> WebCore/platform/graphics/chromium/FontLinux.cpp:444
> +

Extra blank line.

> WebCore/platform/graphics/chromium/FontLinux.cpp:449
> + 

Extra blank line.

> WebCore/platform/graphics/chromium/FontLinux.cpp:598
> +void TextRunWalker::normalizeSpacesAndMirrorChars(const UChar* source, bool
rtl, UChar* destination, int length)

It would be nice if the ordering of these functions followed the ordering in
the class. (You could just swap them in the class.) This isn't required but it
is nice.

> WebCore/platform/graphics/chromium/FontLinux.cpp:610
> +		   character = u_charMirror(character);

The indentation is way off here. Perhaps there is a tab?

> LayoutTests/platform/chromium/fast/text/font-linux-normalize.html:3
> +<script src=../editing.js language="JavaScript" type="text/JavaScript"
></script>

It doesn't look like you use anything from editing.js.

If you don't, then don't include it.

> LayoutTests/platform/chromium/fast/text/font-linux-normalize.html:40
> +    //assertEqual("devanagari + a with diaeresis", string, "\u0958\u0909
\u00e4");

Why is this commented out? Maybe you should just remove it.