[Webkit-unassigned] [Bug 47019] [chromium] FontLinux performance improvement

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=47019


David Levin <levin at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #71229|review?                     |review+, commit-queue-
               Flag|                            |




--- Comment #6 from David Levin <levin at chromium.org>  2010-10-20 19:41:26 PST ---
(From update of attachment 71229)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.