[webkit-reviews] review denied: [Bug 47019] [chromium] FontLinux performance improvement : [Attachment 69524] patch

[bugzilla-daemon at webkit.org]

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

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

This is looking pretty good.  Just a few changes and you'll get this in.

Thanks.

> WebCore/ChangeLog:8
> +	   Reduce te number of calls for normalization function because
converting

typo: te
s/for/for the/

> WebCore/ChangeLog:13
> +	   No new tests since there is no functionality change.

Is there any test that currently verifies this functionality?

If not, please add one. (Hopefully that won't be hard.)

> WebCore/platform/graphics/chromium/FontLinux.cpp:@
>  public:

Please update the copyright year at the top of the file. (Add 2010. Don't
delete the other years.)

Could you do a patch (without any other changes) to fix the layout of this
code?

These methods shouldn't be in the class itself as they shouldn't be inlined.

This would also help the diff'ing tool to pick up function names better.

> WebCore/platform/graphics/chromium/FontLinux.cpp:386
> +    const TextRun& getNormalizedTextRun(const TextRun& originalRun)

It would be nice it this function was static, since you are calling it from
within the initialization of the member variables in the constructor.

As the moment it is way too easy to use a member variable in here and that
shouldn't be done in this function or any that it calls (except for special
cases).

It looks like it only uses m_normalizedBuffer and m_normalizedRun, so those
could be passed in to it. (Or perhaps this method and these two member
variables should be split off into a little class that is called at this
point.)

> WebCore/platform/graphics/chromium/FontLinux.cpp:402
> +	   // Convert to NFC form if text has diacritical marks

s/if /if the /
Please add a period to the end of the comment.

> WebCore/platform/graphics/chromium/FontLinux.cpp:409
> +		  
icu::Normalizer::normalize(icu::UnicodeString(originalRun.characters(),

Why do we only need to call icu::Normalizer::normalize when
UBLOCK_COMBINING_DIACRITICAL_MARKS? 

Are there any other cases?

> WebCore/platform/graphics/chromium/FontLinux.cpp:410
> +				    originalRun.length()), UNORM_NFC, 0 /* no
options */,

alignment with ( of previous line.  (Note that you don't need to wrap the
line.)

> WebCore/platform/graphics/chromium/FontLinux.cpp:419
> +	   int normalizedBufferLen;

normalizedBufferLength

(Avoid abbreviations in WebKit code.)

> WebCore/platform/graphics/chromium/FontLinux.cpp:420
> +	   const UChar* srcText;

Use sourceText.

(Avoid abbreviations in WebKit code.)

> WebCore/platform/graphics/chromium/FontLinux.cpp:430
> +	   normSpaceAndMirrorChars(originalRun.rtl(), m_normalizedBuffer.get(),
srcText, normalizedBufferLen);

The ordering of parameters seems odd. It almost follows a memcpy ordering but
it also has a bool before them.
I would go with input/output ordering.

Like this
normSpaceAndMirrorChars(srcText, originalRun.rtl(), m_normalizedBuffer.get(),
normalizedBufferLen);

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

"norm" avoid abbreviation.

s/Space/Spaces/

> WebCore/platform/graphics/chromium/FontLinux.cpp:598
> +		   if (rtl)

Do "else if"

As opposed to

else {
    if