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

Tue Jun 12 13:12:34 PDT 2012

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

Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #141204|review?, commit-queue?      |review+, commit-queue-
               Flag|                            |

--- Comment #8 from Darin Adler <darin at apple.com>  2012-06-12 13:12:33 PST ---
(From update of attachment 141204)
View in context: https://bugs.webkit.org/attachment.cgi?id=141204&action=review

This generally looks good to me. There are some stylistic issues that should be fixed before landing. I’m going to say r=me but I’d also like the patch to state what performance results we achieved with this change.

Someone even more expert with this part of WebKit might have some other more substantive comments.

> Source/WebCore/ChangeLog:24
> +        (WTF): Define deleteOwnedPtr for TextLayout as calling through destroyLayout; relying on operator delete is not workable as the class does not exist on all platforms.

Normally what we’d do is put a platform #if around the line of code that defines the data member of type OwnPtr<TextLayout>.

> Source/WebCore/ChangeLog:26
> +        (WebCore): Implement no-op TextLayout wrappers for non-Mac platforms.

I’m not sure this is the right approach.

> Source/WebCore/platform/graphics/Font.cpp:33
> +#if !PLATFORM(MAC)
> +#include "NotImplemented.h"
> +#endif

Conditional includes normally go in a separate paragraph after the main includes.

Alternatively, I think it’s OK to include this unconditionally.

> Source/WebCore/platform/graphics/Font.cpp:47
> +template <> void deleteOwnedPtr<WebCore::TextLayout>(WebCore::TextLayout* ptr)

Might be useful to put ac moment here explaining why we did this. I think the answer is that it allows use to compile the destruction of OwnPtr<TextLayout> in source files that don’t have access to the TextLayout class definition.

I’m not sure it’s worth it -- we haven’t done this for any other classes in WebCore -- but it’s probably OK.

> Source/WebCore/platform/graphics/Font.h:316
> +template <> void deleteOwnedPtr<WebCore::TextLayout>(WebCore::TextLayout* ptr);

Should omit the argument name here to match usual WebKit coding style.

> Source/WebCore/platform/graphics/mac/ComplexTextController.cpp:53
> +        TextRun run = RenderBlock::constructTextRun(text, font, text->characters(), text->textLength(), text->style());
> +        Font::CodePath codePathToUse = font.codePath(run);
> +        return codePathToUse == Font::Complex;

Might be more readable to eliminate the two local variables. Or you could at least omit the second one.

> Source/WebCore/platform/graphics/mac/ComplexTextController.cpp:57
> +        : m_font(font)

I suggest adding a helper function that returns a font with word spacing set to 0 so we can initialize this.

> Source/WebCore/platform/graphics/mac/ComplexTextController.cpp:58
> +        , m_run(0, 0)

Then we can initialize this properly as well. I suggest making a helper function to do that so we don’t have to first initialize it to (0, 0) just to overwrite it a moment later.

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

Then this could be done with initialization too. This should use an OwnPtr and adoptPtr.

> Source/WebCore/platform/graphics/mac/ComplexTextController.cpp:73
> +    ~TextLayout()
> +    {
> +        delete m_controller;
> +    }

This function wouldn’t be necessary if we used OwnPtr.

> Source/WebCore/platform/graphics/mac/ComplexTextController.cpp:86
> +    TextRun m_run;

Do we need to keep the TextRun around after making the ComplexTextController? I think I can see why we might need to keep m_font around, but not clear on why we have to keep m_run around.

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

On further reflection, I think we should just put the TextLayout class in a header so we don’t need this function.

> Source/WebCore/platform/graphics/mac/ComplexTextController.cpp:111
> +float Font::width(TextLayout& layout, unsigned from, unsigned to)
> +{
> +    return layout.width(from, to);
> +}

Or this function.

> Source/WebCore/platform/graphics/mac/ComplexTextController.cpp:485
> +            const ComplexTextRun& complexTextRun = *m_complexTextRuns[r];
> +            leftmostGlyph += complexTextRun.glyphCount();

I don’t think you need the local variable here.

> Source/WebCore/platform/graphics/mac/ComplexTextController.cpp:493
> +            unsigned r = m_runIndices.last();

I don’t think this local variable helps readability.

> Source/WebCore/platform/graphics/mac/ComplexTextController.cpp:494
> +            const ComplexTextRun& currentTextRun = *m_complexTextRuns[r];

I suggest naming this local variable just "run" rather than "currentTextRun".

> Source/WebCore/platform/graphics/mac/ComplexTextController.cpp:495
> +            offset = currentTextRun.stringLocation() + currentTextRun.indexEnd();

If there was a helper function that did this addition, either a member function or a free function, then we would not need the local variable.

> Source/WebCore/platform/graphics/mac/ComplexTextController.cpp:498
> +        for (unsigned q = 0; q < runCount; ++q) {

We normally name these loop variables i in WebKit.

> Source/WebCore/platform/graphics/mac/ComplexTextController.cpp:499
> +            const ComplexTextRun& complexTextRun = *m_complexTextRuns[q];

More of the same.

> Source/WebCore/platform/graphics/mac/ComplexTextController.cpp:500
> +            if (offset == complexTextRun.stringLocation() + complexTextRun.indexBegin()) {

More of the same.

> Source/WebCore/platform/graphics/mac/ComplexTextController.cpp:507
> +    unsigned r = m_runIndices[m_currentRun];

We typically use words rather than letters for variable names in WebKit, with only one or two customary exceptions.

Maybe this could be named currentRunIndex?

> Source/WebCore/platform/graphics/mac/ComplexTextController.cpp:508
> +    for (unsigned q = 0; q < r; ++q) {

We normally name these loop variables i in WebKit.

> Source/WebCore/platform/graphics/mac/ComplexTextController.cpp:510
> +        const ComplexTextRun& complexTextRun = *m_complexTextRuns[q];
> +        leftmostGlyph += complexTextRun.glyphCount();

More of the same.

> Source/WebCore/platform/graphics/mac/ComplexTextController.cpp:519
> +        const ComplexTextRun& currentTextRun = *m_complexTextRuns[m_currentRun++];
> +        leftmostGlyph += currentTextRun.glyphCount();

Again, I think this would read better without the local variable.

> Source/WebCore/platform/graphics/mac/ComplexTextController.cpp:546
> +    unsigned r = indexOfCurrentRun(leftmostGlyph);

Should be named leftmostGlyphIndex.

> Source/WebCore/platform/graphics/mac/ComplexTextController.cpp:552
>          unsigned g = ltr ? m_glyphInCurrentRun : glyphCount - 1 - m_glyphInCurrentRun;
> +        unsigned k = leftmostGlyph + g;

Sad to see the existing code used "g" and "k" for these. I don’t recommend changing that at this time, though.

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

Where did the number 16 come from? I guess the vectors above all say 64 with no comment saying where that came from.

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