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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Oct 18 09:23:20 PDT 2013


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





--- Comment #25 from Simon Fraser (smfr) <simon.fraser at apple.com>  2013-10-18 09:22:03 PST ---
(From update of attachment 214574)
View in context: https://bugs.webkit.org/attachment.cgi?id=214574&action=review

> Source/WebCore/dom/Position.cpp:650
> +            textRenderer->ensureInlineBoxes();

It's unclear how callers can know that they should call ensureInlineBoxes()

> Source/WebCore/editing/TextIterator.cpp:524
> +    if (renderer->simpleLineLayout()) {

Might be easier to read as usesSimpleLineLayout() or hasSimpleLineLayout(). It's a bit odd for an object to be called SimpleLineLayout.

> Source/WebCore/rendering/RenderBlock.h:793
>      unsigned m_hasMarginBeforeQuirk : 1; // Note these quirk values can't be put in RenderBlockRareData since they are set too frequently.
>      unsigned m_hasMarginAfterQuirk : 1;
>      unsigned m_beingDestroyed : 1;
>      unsigned m_hasMarkupTruncation : 1;
>      unsigned m_hasBorderOrPaddingLogicalWidthChanged : 1;
> +    unsigned m_forceLineboxInlineLayout : 1;

These can be bools, I think.

> Source/WebCore/rendering/RenderBlockFlow.cpp:2448
> +    m_forceLineboxInlineLayout = true;
> +
> +    if (!m_simpleLineLayout)
> +        return;
> +    m_simpleLineLayout = nullptr;

It's odd for something called ensureFoo to blow away existing state and recompute it. This function should be called updateFoo or recomputeFoo or something.

> Source/WebCore/rendering/RenderText.cpp:283
> +    // FIXME: These will go away when simple layout can do everything.

But then it will no longer be simple?

> Source/WebCore/rendering/RenderText.cpp:308
> +    const_cast<RenderText&>(*this).ensureInlineBoxes();

Again, not sure how I'd know, when writing new code, that I had to call ensureInlineBoxes().

> Source/WebCore/rendering/RenderText.cpp:1200
> +    if (auto layout = simpleLineLayout())
> +        return layout->caretMinOffset();
>      return m_lineBoxes.caretMinOffset();

Shame we can't turn m_lineBoxes into some virtual thing that does both.

> Source/WebCore/rendering/ResolvedSimpleLines.h:38
> +class ResolvedSimpleLines {

The "resolved" doesn't really communicate anything I think.

> Source/WebCore/rendering/SimpleLineLayout.cpp:298
> +    if (paintInfo.phase != PaintPhaseForeground)
> +        return;

You can probably bail if painting is disabled (updateControlTints).

-- 
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