[Webkit-unassigned] [Bug 122458] Simple line layout

[bugzilla-daemon at webkit.org]

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





--- Comment #32 from Andreas Kling <akling at apple.com>  2013-10-19 06:52:46 PST ---
(From update of attachment 214574)
View in context: https://bugs.webkit.org/attachment.cgi?id=214574&action=review

> Source/WebCore/editing/TextIterator.cpp:531
> +        while (runEnd < str.length() && (isCollapsibleWhitespace(str[runEnd]) || str[runEnd] == '\t'))
> +            ++runEnd;

You might be able to tighten this loop by putting the StringImpl in a local, since both length() and operator[] will do null checks.

> Source/WebCore/editing/TextIterator.cpp:547
> +        while (runEnd < str.length() && !isCollapsibleWhitespace(str[runEnd]))
> +            ++runEnd;

This loop would benefit as well.

> Source/WebCore/rendering/RenderBlock.cpp:3519
> +        if (!child->hasSelfPaintingLayer() && !child->isFloating() && child->nodeAtPoint(request, result, locationInContainer, childPoint, childHitTest))

I would do the !isFloating() check first, since that's just a bit test.

> Source/WebCore/rendering/RenderFullScreen.cpp:38
> -using namespace WebCore;
> +namespace WebCore {

wat. We should fix this separately :)

> Source/WebCore/rendering/RenderTreeAsText.cpp:253
> +            if (text.containingBlock()->isTableCell())
> +                y -= toRenderTableCell(o.containingBlock())->intrinsicPaddingBefore();

Should put containingBlock() in a local since it's nontrivial.

> Source/WebCore/rendering/RenderTreeAsText.cpp:545
> +    if (o.containingBlock()->isTableCell())
> +        y -= toRenderTableCell(o.containingBlock())->intrinsicPaddingBefore();

Ditto.

> Source/WebCore/rendering/RenderTreeAsText.cpp:598
> +            for (InlineTextBox* box = text.firstTextBox(); box; box = box->nextTextBox()) {

auto?

> Source/WebCore/rendering/RenderTreeAsText.cpp:604
> +        const SimpleLineLayout* simpleLineLayout = text.parent()->isRenderBlockFlow() ? toRenderBlockFlow(text.parent())->simpleLineLayout() : nullptr;
> +        if (simpleLineLayout) {

if (auto simpleLineLayout = ...)?

> Source/WebCore/rendering/ResolvedSimpleLines.h:131
> +    , m_string(toRenderText(*flow.firstChild()).text())

Does ResolvedSimpleLines really need to hold a strong ref on the text?

> Source/WebCore/rendering/SimpleLineLayout.cpp:270
> +        Line line;
> +        line.textOffset = lineStartOffset;
> +        line.textLength = lineEndOffset - lineStartOffset;
> +        line.width = lineWidth.committedWidth();
> +
> +        m_lines.append(line);

You could C++11'ize this:

m_lines.append({lineStartOffset, lineEndOffset - lineStartOffset, lineWidth.committedWidth()});

If m_lines is only going to be built once, the member should be something with less overhead than Vector.
You could even go all 2012 and make it a variable-sized object :)

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