[webkit-reviews] review granted: [Bug 83045] Improve line breaking performance for complex text : [Attachment 141204] Rebased patch.

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Jun 12 13:12:33 PDT 2012


Darin Adler <darin at apple.com> has granted nholbrook at apple.com's request for
review:
Bug 83045: Improve line breaking performance for complex text
https://bugs.webkit.org/show_bug.cgi?id=83045

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

------- Additional Comments from Darin Adler <darin at apple.com>
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.


More information about the webkit-reviews mailing list