instead of duplicating collapsing logic etc etc.
Created attachment 250017 [details] WIP -> EWS
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 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
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 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
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
Created attachment 250030 [details] WIP -> EWS
Created attachment 250076 [details] Patch
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.
Created attachment 250082 [details] Patch
(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 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 &.
Created attachment 250084 [details] Patch
Comment on attachment 250084 [details] Patch Clearing flags on attachment: 250084 Committed r182325: <http://trac.webkit.org/changeset/182325>
All reviewed patches have been landed. Closing bug.