[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 #69524|review?                     |review-
               Flag|                            |




--- Comment #2 from David Levin <levin at chromium.org>  2010-10-13 23:49:54 PST ---
(From update of attachment 69524)
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

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