[webkit-reviews] review granted: [Bug 47732] [chromium] FontLinux.cpp cleanup (move most methods in TextRunWorker from inlined) : [Attachment 70888] patch

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has granted Xiaomei Ji <xji at chromium.org>'s
request for review:
Bug 47732: [chromium]  FontLinux.cpp cleanup (move most methods in
TextRunWorker from inlined)
https://bugs.webkit.org/show_bug.cgi?id=47732

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

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

Please consider addressing my comments on landing.

> WebCore/platform/graphics/chromium/FontLinux.cpp:164
> +    TextRunWalker(const TextRun& run, unsigned startingX, const Font* font);


Remove param names that add no information: "run", "font".

> WebCore/platform/graphics/chromium/FontLinux.cpp:167
> +    bool isWordBreak(unsigned i, bool isRTL);

What is "i? In its current form, I'd remove it, but I have no idea what that
param is. Should it be called index?

> WebCore/platform/graphics/chromium/FontLinux.cpp:170
> +    void setPadding(int padding);

No need for "padding". (Since the method name is setPadding and it only takes
one parameter.)

> WebCore/platform/graphics/chromium/FontLinux.cpp:172
> +    void setBackwardsIteration(bool isBackwards);

Remove param name.