Bug 143351 - Simple line layout: Use pre-computed simple line runs to produce innerText content.
Summary: Simple line layout: Use pre-computed simple line runs to produce innerText co...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: zalan
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-04-02 16:18 PDT by zalan
Modified: 2015-04-03 11:28 PDT (History)
4 users (show)

See Also:


Attachments
WIP -> EWS (7.42 KB, patch)
2015-04-02 16:20 PDT, zalan
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-mavericks (543.63 KB, application/zip)
2015-04-02 17:10 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews106 for mac-mavericks-wk2 (586.78 KB, application/zip)
2015-04-02 17:16 PDT, Build Bot
no flags Details
WIP -> EWS (7.43 KB, patch)
2015-04-02 19:19 PDT, zalan
no flags Details | Formatted Diff | Diff
Patch (8.69 KB, patch)
2015-04-03 08:47 PDT, zalan
no flags Details | Formatted Diff | Diff
Patch (9.57 KB, patch)
2015-04-03 09:38 PDT, zalan
no flags Details | Formatted Diff | Diff
Patch (9.57 KB, patch)
2015-04-03 10:00 PDT, zalan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description zalan 2015-04-02 16:18:46 PDT
instead of duplicating collapsing logic etc etc.
Comment 1 zalan 2015-04-02 16:20:56 PDT
Created attachment 250017 [details]
WIP -> EWS
Comment 2 WebKit Commit Bot 2015-04-02 16:23:16 PDT
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.
Comment 3 Build Bot 2015-04-02 17:10:50 PDT
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
Comment 4 Build Bot 2015-04-02 17:10:53 PDT
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
Comment 5 Build Bot 2015-04-02 17:16:41 PDT
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
Comment 6 Build Bot 2015-04-02 17:16:44 PDT
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
Comment 7 zalan 2015-04-02 19:19:23 PDT
Created attachment 250030 [details]
WIP -> EWS
Comment 8 zalan 2015-04-03 08:47:43 PDT
Created attachment 250076 [details]
Patch
Comment 9 Antti Koivisto 2015-04-03 09:15:14 PDT
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.
Comment 10 zalan 2015-04-03 09:38:42 PDT
Created attachment 250082 [details]
Patch
Comment 11 zalan 2015-04-03 09:39:51 PDT
(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.
Comment 12 Antti Koivisto 2015-04-03 09:55:53 PDT
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 &.
Comment 13 zalan 2015-04-03 10:00:35 PDT
Created attachment 250084 [details]
Patch
Comment 14 WebKit Commit Bot 2015-04-03 11:28:47 PDT
Comment on attachment 250084 [details]
Patch

Clearing flags on attachment: 250084

Committed r182325: <http://trac.webkit.org/changeset/182325>
Comment 15 WebKit Commit Bot 2015-04-03 11:28:51 PDT
All reviewed patches have been landed.  Closing bug.