Bug 47732 - [chromium] FontLinux.cpp cleanup (move most methods in TextRunWorker from inlined)
Summary: [chromium] FontLinux.cpp cleanup (move most methods in TextRunWorker from in...
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
Depends on:
Blocks: 47019
  Show dependency treegraph
Reported: 2010-10-15 12:28 PDT by Xiaomei Ji
Modified: 2010-10-18 11:06 PDT (History)
5 users (show)

See Also:

patch (34.12 KB, patch)
2010-10-15 12:35 PDT, Xiaomei Ji
levin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Xiaomei Ji 2010-10-15 12:28:11 PDT
All function members in TextRunWorker are defined as inlined.
Most of them should not.
Comment 1 Xiaomei Ji 2010-10-15 12:35:45 PDT
Created attachment 70888 [details]
Comment 2 David Levin 2010-10-16 12:07:18 PDT
Comment on attachment 70888 [details]

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.
Comment 3 Xiaomei Ji 2010-10-18 10:41:41 PDT
Committed r69972: <http://trac.webkit.org/changeset/69972>
Comment 4 WebKit Review Bot 2010-10-18 11:06:43 PDT
http://trac.webkit.org/changeset/69972 might have broken Qt Windows 32-bit Release