[Webkit-unassigned] [Bug 83045] Improve line breaking performance for complex text

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Jul 23 16:38:20 PDT 2012


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


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #153881|review?                     |review+
               Flag|                            |




--- Comment #13 from Darin Adler <darin at apple.com>  2012-07-23 16:38:24 PST ---
(From update of attachment 153881)
View in context: https://bugs.webkit.org/attachment.cgi?id=153881&action=review

> Source/WebCore/ChangeLog:11
> +        Currently RenderBlock::LineBreaker::nextLineBreak assumes that measuring individual words is as cheap
> +        as free. This is not the case when dealing with complex text, which benefits from laying out as much
> +        text as possible and by reusing that layout when feasible: by doing so this patch improves line
> +        breaking by 35% as measured with a simple test app.

Do we have data that indicates no additional cost when not using the complex text case?

> Source/WebCore/platform/graphics/Font.cpp:213
> +PassOwnPtr<TextLayout> Font::createLayout(RenderText*, float xPos, bool collapseWhiteSpace) const

Normally we’d leave out all the argument names here to avoid unused argument warnings.

> Source/WebCore/platform/graphics/Font.cpp:222
> +float Font::width(TextLayout&, unsigned from, unsigned to)

Here too. Leave out the argument names.

> Source/WebCore/platform/graphics/Font.cpp:224
> +    notImplemented();

I think we want ASSERT_NOT_REACHED() here rather than just notImplemented().

> Source/WebCore/platform/graphics/mac/ComplexTextController.cpp:50
> +    static Font fontWithNoWordSpacing(const Font& originalFont)

Seems this could be private. Or a non-member helper function outside the class.

> Source/WebCore/platform/graphics/mac/ComplexTextController.cpp:57
> +    static TextRun constructTextRun(RenderText* text, const Font& font, float xPos)

This could/should be private.

> Source/WebCore/platform/graphics/mac/ComplexTextController.cpp:70
> +        , m_controller(adoptPtr(new ComplexTextController(&m_font, m_run, true)))

The use of a boolean here leads to not-all-that-readable code. What does true mean? In WebKit we normally avoid this, or used named enums for them, to prevent that problem.

> Source/WebCore/platform/graphics/mac/ComplexTextController.cpp:76
> +        m_controller->advance(from, 0, true);

Same problem here with the boolean argument.

> Source/WebCore/platform/graphics/mac/ComplexTextController.cpp:78
> +        m_controller->advance(from + len, 0, true);

And here.

> Source/WebCore/platform/graphics/mac/ComplexTextController.cpp:94
> +    if (collapseWhiteSpace && TextLayout::isNeeded(text, *this))
> +        return adoptPtr(new TextLayout(text, *this, xPos));
> +    return nullptr;

It’s idiomatic in WebKit to use “early return”, which means we’d have the if statement reversed. Or use two separate if statements. The case where we actually create a TextLayout would be the one not indented.

> Source/WebCore/platform/graphics/mac/ComplexTextController.cpp:100
> +void Font::destroyLayout(TextLayout* layout)
> +{
> +    delete layout;
> +}

I’d have called this deleteLayout, for reasons that may be obvious here.

> Source/WebCore/platform/graphics/mac/ComplexTextController.h:97
> +        bool ltr() const { return m_ltr; }

Normally we’d name this isLTR() rather than ltr().

> Source/WebCore/platform/graphics/mac/ComplexTextController.h:119
> +        bool m_ltr;

Normally we’d name this m_isLTR rather than m_ltr.

> Source/WebCore/platform/graphics/mac/ComplexTextController.h:133
> +    Vector<unsigned, 8> m_runIndices;

We typically put a “why” comment near constants like this 8, to indicate where the number came from.

> Source/WebCore/platform/graphics/mac/ComplexTextController.h:137
> +    bool m_ltrOnly;

Normally we’d name this m_isLTROnly rather than m_ltrOnly.

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


More information about the webkit-unassigned mailing list