Bug 142579 – Simple line layout: Split fragments on renderer boundary on the fly.

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[alan]

This is in transition to remove FlowContents::segmentForPosition dependency. -which is required to support <br>.

[alan]

Created attachment 248444 [details]
Patch

[alan]

Created attachment 248615 [details]
Patch

[Antti Koivisto]

Comment on attachment 248615 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=248615&action=review

> Source/WebCore/rendering/SimpleLineLayout.cpp:254
> +        // Check if this fragment is a continuaiton of a text. Is such cases, this fragment is considred to be the first on the line

spelling 'considred'

> Source/WebCore/rendering/SimpleLineLayout.cpp:263
>      void appendFragment(const TextFragmentIterator::TextFragment& fragment, Layout::RunVector& runs)

Maybe rename to indicate this also creates runs.

> Source/WebCore/rendering/SimpleLineLayoutTextFragmentIterator.h:108
> +    FlowContents::Iterator m_flowContentsIterator;

This could use a name that describes its purpose.

[Antti Koivisto]

Comment on attachment 248615 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=248615&action=review

> Source/WebCore/rendering/SimpleLineLayoutTextFragmentIterator.cpp:65
> +    if (m_position == (*m_flowContentsIterator).end)

You should probably add operator-> to FlowContents::Iterator. These are pretty awkward. Something like 

if (m_position == m_currentSegment->end)

would read better.

> Source/WebCore/rendering/SimpleLineLayoutTextFragmentIterator.h:64
> +        bool isCollapsable() const { return m_isCollapsable; }

"collapsible"

[Antti Koivisto]

Comment on attachment 248615 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=248615&action=review

> Source/WebCore/rendering/SimpleLineLayout.cpp:326
> +    void revert(unsigned length, float width, Layout::RunVector& runs)

As you mentioned model based on commits might be nicer in the future.

[alan]

(In reply to comment #5)
> Comment on attachment 248615 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=248615&action=review
> 
> > Source/WebCore/rendering/SimpleLineLayout.cpp:326
> > +    void revert(unsigned length, float width, Layout::RunVector& runs)
> 
> As you mentioned model based on commits might be nicer in the future.

Actually I think I should do it as part of this patch. It greatly reduces complexity both at LineState::removeLastNonWhitespace() and at LineState::isFirstFragment().

[alan]

Created attachment 248803 [details]
Patch

[Antti Koivisto]

Comment on attachment 248803 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=248803&action=review

> Source/WebCore/rendering/SimpleLineLayout.cpp:338
> +    TextFragmentIterator::TextFragment revertUncommitted(Layout::RunVector& runs)
>      {
> -        ASSERT(runs.size());
> -        Run& lastRun = runs.last();
> -        lastRun.logicalRight -= m_trailingWhitespaceWidth;
> -        lastRun.end -= m_trailingWhitespaceLength;
> -        if (lastRun.start == lastRun.end)
> -            runs.removeLast();
> +        ASSERT(m_fragments.size());
> +        unsigned revertLength = 0;
> +        float revertWidth = 0;
> +        while (m_fragments.size()) {
> +            const auto& current = m_fragments.last();
> +            if (current == m_lastComitted)
> +                break;
> +            revertLength += current.end() - current.start();
> +            revertWidth += current.width();
> +            m_fragments.removeLast();
> +        }
> +        m_runsWidth -= revertWidth;
> +        if (revertLength)
> +            revertRuns(runs, revertLength, revertWidth);
> +        return m_lastComitted;
> +    }

It is bit strange that there is concept of "uncommitting" but there is no commit() function. This also does a lot of work whereas you would usually expect rollback to do almost nothing while commit() does all the work.

It is also unclear why some places use term "revert" and others "uncommit". Is there some difference?

> Source/WebCore/rendering/SimpleLineLayout.cpp:468
> +    do {

while(true)

> Source/WebCore/rendering/SimpleLineLayout.cpp:476
> +        nextFragment = textFragmentIterator.nextTextFragment();
> +    } while (true);

how do we know there is always nextFragment?

> Source/WebCore/rendering/SimpleLineLayoutFlowContents.h:59
> +        Iterator operator+(unsigned value) const;

This seems to be only used in one place with +1. Maybe that case cold be handled by

auto nextSegment = segment;
++ nextSegment;

> Source/WebCore/rendering/SimpleLineLayoutTextFragmentIterator.cpp:160
> +            const auto nextSegment = m_currentSegment + 1;

See the comment in the iterator.

[alan]

Created attachment 248861 [details]
Patch

[alan]

(In reply to comment #8)
> Comment on attachment 248803 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=248803&action=review
> 
> > Source/WebCore/rendering/SimpleLineLayout.cpp:338
> > +    TextFragmentIterator::TextFragment revertUncommitted(Layout::RunVector& runs)
> >      {
> > -        ASSERT(runs.size());
> > -        Run& lastRun = runs.last();
> > -        lastRun.logicalRight -= m_trailingWhitespaceWidth;
> > -        lastRun.end -= m_trailingWhitespaceLength;
> > -        if (lastRun.start == lastRun.end)
> > -            runs.removeLast();
> > +        ASSERT(m_fragments.size());
> > +        unsigned revertLength = 0;
> > +        float revertWidth = 0;
> > +        while (m_fragments.size()) {
> > +            const auto& current = m_fragments.last();
> > +            if (current == m_lastComitted)
> > +                break;
> > +            revertLength += current.end() - current.start();
> > +            revertWidth += current.width();
> > +            m_fragments.removeLast();
> > +        }
> > +        m_runsWidth -= revertWidth;
> > +        if (revertLength)
> > +            revertRuns(runs, revertLength, revertWidth);
> > +        return m_lastComitted;
> > +    }
> 
> It is bit strange that there is concept of "uncommitting" but there is no
> commit() function. This also does a lot of work whereas you would usually
> expect rollback to do almost nothing while commit() does all the work.
> 
> It is also unclear why some places use term "revert" and others "uncommit".
> Is there some difference?
I misused the committed/uncommitted terms. Changed to "last complete fragment".   

> 
> > Source/WebCore/rendering/SimpleLineLayout.cpp:468
> > +    do {
> 
> while(true)
> 
> > Source/WebCore/rendering/SimpleLineLayout.cpp:476
> > +        nextFragment = textFragmentIterator.nextTextFragment();
> > +    } while (true);
> 
> how do we know there is always nextFragment?
There's always a 'content-end' fragment.

> 
> > Source/WebCore/rendering/SimpleLineLayoutFlowContents.h:59
> > +        Iterator operator+(unsigned value) const;
> 
> This seems to be only used in one place with +1. Maybe that case cold be
> handled by
> 
> auto nextSegment = segment;
> ++ nextSegment;
> 
> > Source/WebCore/rendering/SimpleLineLayoutTextFragmentIterator.cpp:160
> > +            const auto nextSegment = m_currentSegment + 1;
> 
> See the comment in the iterator.
Done.

[Antti Koivisto]

Comment on attachment 248861 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 248861 [details]
Patch

Clearing flags on attachment: 248861

Committed r181667: <http://trac.webkit.org/changeset/181667>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

This caused ASan violations in SimpleLineLayout::revertRuns on multiple tests. Will e-mail Zalan a link to detailed results, and will roll out.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 142812

[alan]

Committed r181692: <http://trac.webkit.org/changeset/181692>