Bug 143351 – Simple line layout: Use pre-computed simple line runs to produce innerText content.

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[alan]

instead of duplicating collapsing logic etc etc.

[alan]

Created attachment 250017 [details]
WIP -> EWS

[WebKit Commit Bot]

Attachment 250017 [details] did not pass style-queue:


ERROR: Source/WebCore/editing/TextIterator.cpp:569:  An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 250017 [details]
WIP -> EWS

Attachment 250017 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4564631037149184

New failing tests:
fast/text/mark-matches-rendering-simple-lines.html

[Build Bot]

Created attachment 250021 [details]
Archive of layout-test-results from ews100 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Build Bot]

Comment on attachment 250017 [details]
WIP -> EWS

Attachment 250017 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6217626711752704

New failing tests:
fast/text/mark-matches-rendering-simple-lines.html

[Build Bot]

Created attachment 250023 [details]
Archive of layout-test-results from ews106 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[alan]

Created attachment 250030 [details]
WIP -> EWS

[alan]

Created attachment 250076 [details]
Patch

[Antti Koivisto]

Comment on attachment 250076 [details]
Patch

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

> Source/WebCore/editing/TextIterator.cpp:546
> +    if (const SimpleLineLayout::Layout* layout = renderer.simpleLineLayout()) {

auto*

> Source/WebCore/editing/TextIterator.cpp:574
> +        // Skip to m_offset position.
> +        auto run = range.begin();
> +        auto end = range.end();
> +        while (run != end && (*run).end() <= (static_cast<unsigned>(m_offset) + m_previousTextLengthInFlow))
> +            ++run;
> +        if (run == end) {
> +            // Collapsed trailing whitespace.
> +            m_offset = endPosition;
> +            m_lastTextNodeEndedWithCollapsedSpace = true;
> +            return true;
> +        }

I think the code would be clearer if you had

auto it = range.begin();

and then

auto& run = *it;

for the following code.

> Source/WebCore/editing/TextIterator.cpp:583
> +        const auto& simpleRun = run.simpleRun();
> +        ASSERT(simpleRun.end - simpleRun.start <= rendererText.length());
> +        // contentStart skips leading whitespace.
> +        unsigned contentStart = std::max<unsigned>(m_offset, simpleRun.start - m_previousTextLengthInFlow);
> +        unsigned contentEnd = std::min(endPosition, simpleRun.end - m_previousTextLengthInFlow);

simpleRun() (which return SimpleLineLayout::Run) is not meant to be used outside the resolver code (and probably shouldn't be public). If something is missing from the RunResolver API should be added there instead.

If you have 

auto& run = *it;

then run. run.start() == it.simpleRun().start etc

so you can just use run.start() and run.end() here and don't need simpleRun().

This reminds me that we should really have only one class called Run and rename one of them to something else.

> Source/WebCore/editing/TextIterator.cpp:607
> +        m_nextRunNeedsWhitespace = simpleRun.isEndOfLine && contentEnd < endPosition && renderer.style().isCollapsibleWhiteSpace(rendererText[contentEnd]);

isEndOfLine is missing from RunResolver::Run but you should just add it there instead of accessing simpleRun.

[alan]

Created attachment 250082 [details]
Patch

[alan]

(In reply to comment #9)
> Comment on attachment 250076 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=250076&action=review
> 
> > Source/WebCore/editing/TextIterator.cpp:546
> > +    if (const SimpleLineLayout::Layout* layout = renderer.simpleLineLayout()) {
> 
> auto*
Fixed.

> 
> > Source/WebCore/editing/TextIterator.cpp:574
> > +        // Skip to m_offset position.
> > +        auto run = range.begin();
> > +        auto end = range.end();
> > +        while (run != end && (*run).end() <= (static_cast<unsigned>(m_offset) + m_previousTextLengthInFlow))
> > +            ++run;
> > +        if (run == end) {
> > +            // Collapsed trailing whitespace.
> > +            m_offset = endPosition;
> > +            m_lastTextNodeEndedWithCollapsedSpace = true;
> > +            return true;
> > +        }
> 
> I think the code would be clearer if you had
> 
> auto it = range.begin();
> 
> and then
> 
> auto& run = *it;
> 
> for the following code.
Fixed.

> 
> > Source/WebCore/editing/TextIterator.cpp:583
> > +        const auto& simpleRun = run.simpleRun();
> > +        ASSERT(simpleRun.end - simpleRun.start <= rendererText.length());
> > +        // contentStart skips leading whitespace.
> > +        unsigned contentStart = std::max<unsigned>(m_offset, simpleRun.start - m_previousTextLengthInFlow);
> > +        unsigned contentEnd = std::min(endPosition, simpleRun.end - m_previousTextLengthInFlow);
> 
> simpleRun() (which return SimpleLineLayout::Run) is not meant to be used
> outside the resolver code (and probably shouldn't be public). If something
> is missing from the RunResolver API should be added there instead.
> 
> If you have 
> 
> auto& run = *it;
> 
> then run. run.start() == it.simpleRun().start etc
> 
> so you can just use run.start() and run.end() here and don't need
> simpleRun().
Fixed.

> 
> This reminds me that we should really have only one class called Run and
> rename one of them to something else.
Will address it separately.
> 
> > Source/WebCore/editing/TextIterator.cpp:607
> > +        m_nextRunNeedsWhitespace = simpleRun.isEndOfLine && contentEnd < endPosition && renderer.style().isCollapsibleWhiteSpace(rendererText[contentEnd]);
> 
> isEndOfLine is missing from RunResolver::Run but you should just add it
> there instead of accessing simpleRun.
Fixed.

[Antti Koivisto]

Comment on attachment 250082 [details]
Patch

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

> Source/WebCore/editing/TextIterator.cpp:579
> +        const auto& run = *it;

Actually I think this should be "auto run = *it" (with or without const) as *it returns a temporary. Not sure why this complies with &.

[alan]

Created attachment 250084 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 250084 [details]
Patch

Clearing flags on attachment: 250084

Committed r182325: <http://trac.webkit.org/changeset/182325>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.