Extend coverage by adding <br> support to multi renderer simple line layout. <div> foobar<br> </div>
Created attachment 242128 [details] WIP patch
Attachment 242128 [details] did not pass style-queue: ERROR: Source/WebCore/rendering/SimpleLineLayoutFlowContents.cpp:30: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 1 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 242128 [details] WIP patch Attachment 242128 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/6036930961604608 Number of test failures exceeded the failure limit.
Created attachment 242129 [details] Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-10 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
Comment on attachment 242128 [details] WIP patch Attachment 242128 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/4695646833475584 Number of test failures exceeded the failure limit.
Created attachment 242130 [details] Archive of layout-test-results from webkit-ews-05 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-05 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Simple line layout's renderer dump needs adjustment.
Created attachment 242188 [details] WIP patch This needs quite bit of a rebaseline.
Created attachment 249341 [details] Patch
WIP patch.
Comment on attachment 249341 [details] Patch Attachment 249341 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/5457275296677888 New failing tests: fast/parser/open-comment-in-textarea.html http/tests/navigation/javascriptlink-frames.html http/tests/navigation/postredirect-goback1.html http/tests/navigation/postredirect-basic.html
Created attachment 249348 [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 249341 [details] Patch Attachment 249341 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/5039804475506688 New failing tests: fast/parser/open-comment-in-textarea.html http/tests/navigation/javascriptlink-frames.html http/tests/navigation/postredirect-goback1.html http/tests/navigation/postredirect-basic.html
Created attachment 249349 [details] Archive of layout-test-results from ews104 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
Created attachment 249378 [details] Patch
WIP patch -> EWS.
Created attachment 249426 [details] Patch
Created attachment 249466 [details] Patch
Created attachment 249502 [details] Patch
Comment on attachment 249502 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=249502&action=review r=me with comments > Source/WebCore/rendering/RenderLineBreak.cpp:182 > + if (auto layout = simpleLineLayout(*this)) auto* layout (here and elsewhere). We like like to document pointers these days. > Source/WebCore/rendering/RenderLineBreak.cpp:202 > + if (auto layout = simpleLineLayout(*this)) { etc. > Source/WebCore/rendering/SimpleLineLayout.cpp:358 > + m_trailingWhitespaceLength -= (current.isCollapsed() ? 1 : current.end() - current.start()); > + m_trailingWhitespaceWidth -= current.width(); You could put !ASSERT_DISABLED around these. > Source/WebCore/rendering/SimpleLineLayout.cpp:362 > + ASSERT(!m_trailingWhitespaceLength); > + ASSERT(!m_trailingWhitespaceWidth); Then do m_trailingWhitespaceLength = 0 m_trailingWhitespaceWidth = 0 after these to avoid unnecessary work in release build. > Source/WebCore/rendering/SimpleLineLayout.cpp:461 > + // <br> always produces a run. (required by testing output) > + if (currentFragment.type() == TextFragmentIterator::TextFragment::HardLineBreak) > + line.appendFragmentAndCreateRunIfNeeded(currentFragment, runs); :( > Source/WebCore/rendering/SimpleLineLayout.cpp:476 > +static TextFragmentIterator::TextFragment firstFragment(TextFragmentIterator& textFragmentIterator, LineState& currentLine, const LineState& previousLine, Layout::RunVector& runs) Could 'textFragmentIterator' argument get a more informative name? What does it represent here? Why is the argument non-const reference? The function both modifies the argument and returns a value? Can they differ? Does this function modify currentLine and runs too? This seems surprising since the function name indicates nothing like that. > Source/WebCore/rendering/SimpleLineLayout.cpp:495 > // Check if we need to skip the leading whitespace. > - if (style.collapseWhitespace) { > - while (firstFragment.type() == TextFragmentIterator::TextFragment::Whitespace) > - firstFragment = textFragmentIterator.nextTextFragment(); > - } > + firstFragment = skipWhitespaceIfNeeded(firstFragment, textFragmentIterator); Comment doesn't add much value. > Source/WebCore/rendering/SimpleLineLayoutTextFragmentIterator.cpp:80 > +struct EndOfSegmentSetter { > + EndOfSegmentSetter(std::function<void()> exitFunction) > + : scopeExit(WTF::move(exitFunction)) > + { > + } > + > + ~EndOfSegmentSetter() > + { > + scopeExit(); > + } > +private: > + std::function<void()> scopeExit; > +}; > + > TextFragmentIterator::TextFragment TextFragmentIterator::nextTextFragment(float xPosition) > { > // A fragment can either be > - // 1. new line character when preserveNewline is on (not considered as whitespace) or > + // 1. line break when <br> is present or preserveNewline is on (not considered as whitespace) or > // 2. whitespace (collasped, non-collapsed multi or single) or > // 3. non-whitespace characters. > - // 4. empty, indicating content end. > + // 4. content end. > + EndOfSegmentSetter segmentEndSetterOnExit([this]() { > + m_atEndOfSegment = (m_currentSegment == m_flowContents.end()) | (m_position == m_currentSegment->end); > + }); This seems rather eccentric. How about just factoring into two functions where the first one calls the second one and then does the end-of-function actions? > LayoutTests/ChangeLog:33 > + This patch enables RenderBlockFlows to use simple line layout on text content when <br> is present. > + Simple text with <br> is a fairly common pattern on discussion(forum)-like web pages. This patch reduces memory usage > + and speeds up layout for such content. > + > + Reviewed by NOBODY (OOPS!). > + > + * fast/text/simple-line-with-br-expected.html: Added. > + * fast/text/simple-line-with-br.html: Added. > + * platform/mac-mavericks/fast/parser/open-comment-in-textarea-expected.txt: > + * platform/mac-mavericks/http/tests/navigation/javascriptlink-frames-expected.txt: > + * platform/mac/fast/css/text-overflow-ellipsis-bidi-expected.txt: > + * platform/mac/fast/dom/focus-contenteditable-expected.txt: > + * platform/mac/fast/forms/range/slider-padding-expected.txt: Added. > + * platform/mac/fast/forms/textarea-scroll-height-expected.txt: > + * platform/mac/fast/parser/open-comment-in-textarea-expected.txt: > + * platform/mac/fast/text/international/bidi-layout-across-linebreak-expected.txt: > + * platform/mac/fast/text/svg-font-face-with-kerning-expected.txt: Added. > + * platform/mac/http/tests/navigation/javascriptlink-frames-expected.txt: > + * platform/mac/http/tests/navigation/postredirect-basic-expected.txt: > + * platform/mac/http/tests/navigation/postredirect-goback1-expected.txt: > + * platform/mac/printing/single-line-must-not-be-split-into-two-pages-expected.txt: > + * platform/mac/svg/wicd/test-rightsizing-b-expected.txt: > + * platform/mac/tables/mozilla/bugs/bug106795-expected.txt: > + * platform/mac/tables/mozilla/bugs/bug1224-expected.txt: > + * platform/mac/tables/mozilla/bugs/bug131020-expected.txt: > + * platform/mac/tables/mozilla/bugs/bug131020_iframe-expected.txt: > + * platform/mac/tables/mozilla/bugs/bug1430-expected.txt: > + * platform/mac/tables/mozilla/bugs/bug16252-expected.txt: You should add some comments about of the types of rebaselinings you are doing here and why.
Created attachment 250328 [details] Patch
(In reply to comment #20) > Comment on attachment 249502 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=249502&action=review > > r=me with comments > > > Source/WebCore/rendering/RenderLineBreak.cpp:182 > > + if (auto layout = simpleLineLayout(*this)) > > auto* layout (here and elsewhere). We like like to document pointers these > days. > > > Source/WebCore/rendering/RenderLineBreak.cpp:202 > > + if (auto layout = simpleLineLayout(*this)) { > > etc. Fixed. > > > Source/WebCore/rendering/SimpleLineLayout.cpp:358 > > + m_trailingWhitespaceLength -= (current.isCollapsed() ? 1 : current.end() - current.start()); > > + m_trailingWhitespaceWidth -= current.width(); > > You could put !ASSERT_DISABLED around these. > > > Source/WebCore/rendering/SimpleLineLayout.cpp:362 > > + ASSERT(!m_trailingWhitespaceLength); > > + ASSERT(!m_trailingWhitespaceWidth); > > Then do > > m_trailingWhitespaceLength = 0 > m_trailingWhitespaceWidth = 0 > > after these to avoid unnecessary work in release build. Fixed. > > > Source/WebCore/rendering/SimpleLineLayout.cpp:461 > > + // <br> always produces a run. (required by testing output) > > + if (currentFragment.type() == TextFragmentIterator::TextFragment::HardLineBreak) > > + line.appendFragmentAndCreateRunIfNeeded(currentFragment, runs); > > :( > > > Source/WebCore/rendering/SimpleLineLayout.cpp:476 > > +static TextFragmentIterator::TextFragment firstFragment(TextFragmentIterator& textFragmentIterator, LineState& currentLine, const LineState& previousLine, Layout::RunVector& runs) > > Could 'textFragmentIterator' argument get a more informative name? What does > it represent here? > > Why is the argument non-const reference? The function both modifies the > argument and returns a value? Can they differ? We call the TextFragmentIterator as textFragmentIterator across these functions. I am happy to change its name if we can come up with a better one. The input here is the iterator and the return value is the first valid fragment on this line. The argument is non-const reference, because we call .nextFragment() on the iterator until we find the first valid fragment. > > Does this function modify currentLine and runs too? This seems surprising > since the function name indicates nothing like that. :( Yes and it's because of the <br> test output requirement that you commented as ':(' above. When we skip a <br> as part of the first fragment logic, we actually need to create a run for this <br> to accommodate test output requirements. I'll try to refactor it in a follow-up patch (if possible). > > > Source/WebCore/rendering/SimpleLineLayout.cpp:495 > > // Check if we need to skip the leading whitespace. > > - if (style.collapseWhitespace) { > > - while (firstFragment.type() == TextFragmentIterator::TextFragment::Whitespace) > > - firstFragment = textFragmentIterator.nextTextFragment(); > > - } > > + firstFragment = skipWhitespaceIfNeeded(firstFragment, textFragmentIterator); > > Comment doesn't add much value. > Fixed. > > Source/WebCore/rendering/SimpleLineLayoutTextFragmentIterator.cpp:80 > > +struct EndOfSegmentSetter { > > + EndOfSegmentSetter(std::function<void()> exitFunction) > > + : scopeExit(WTF::move(exitFunction)) > > + { > > + } > > + > > + ~EndOfSegmentSetter() > > + { > > + scopeExit(); > > + } > > +private: > > + std::function<void()> scopeExit; > > +}; > > + > > TextFragmentIterator::TextFragment TextFragmentIterator::nextTextFragment(float xPosition) > > { > > // A fragment can either be > > - // 1. new line character when preserveNewline is on (not considered as whitespace) or > > + // 1. line break when <br> is present or preserveNewline is on (not considered as whitespace) or > > // 2. whitespace (collasped, non-collapsed multi or single) or > > // 3. non-whitespace characters. > > - // 4. empty, indicating content end. > > + // 4. content end. > > + EndOfSegmentSetter segmentEndSetterOnExit([this]() { > > + m_atEndOfSegment = (m_currentSegment == m_flowContents.end()) | (m_position == m_currentSegment->end); > > + }); > > This seems rather eccentric. How about just factoring into two functions > where the first one calls the second one and then does the end-of-function > actions? Fixed. > > > LayoutTests/ChangeLog:33 > > + This patch enables RenderBlockFlows to use simple line layout on text content when <br> is present. > > + Simple text with <br> is a fairly common pattern on discussion(forum)-like web pages. This patch reduces memory usage > > + and speeds up layout for such content. > > + > > + Reviewed by NOBODY (OOPS!). > > + > > + * fast/text/simple-line-with-br-expected.html: Added. > > + * fast/text/simple-line-with-br.html: Added. > > + * platform/mac-mavericks/fast/parser/open-comment-in-textarea-expected.txt: > > + * platform/mac-mavericks/http/tests/navigation/javascriptlink-frames-expected.txt: > > + * platform/mac/fast/css/text-overflow-ellipsis-bidi-expected.txt: > > + * platform/mac/fast/dom/focus-contenteditable-expected.txt: > > + * platform/mac/fast/forms/range/slider-padding-expected.txt: Added. > > + * platform/mac/fast/forms/textarea-scroll-height-expected.txt: > > + * platform/mac/fast/parser/open-comment-in-textarea-expected.txt: > > + * platform/mac/fast/text/international/bidi-layout-across-linebreak-expected.txt: > > + * platform/mac/fast/text/svg-font-face-with-kerning-expected.txt: Added. > > + * platform/mac/http/tests/navigation/javascriptlink-frames-expected.txt: > > + * platform/mac/http/tests/navigation/postredirect-basic-expected.txt: > > + * platform/mac/http/tests/navigation/postredirect-goback1-expected.txt: > > + * platform/mac/printing/single-line-must-not-be-split-into-two-pages-expected.txt: > > + * platform/mac/svg/wicd/test-rightsizing-b-expected.txt: > > + * platform/mac/tables/mozilla/bugs/bug106795-expected.txt: > > + * platform/mac/tables/mozilla/bugs/bug1224-expected.txt: > > + * platform/mac/tables/mozilla/bugs/bug131020-expected.txt: > > + * platform/mac/tables/mozilla/bugs/bug131020_iframe-expected.txt: > > + * platform/mac/tables/mozilla/bugs/bug1430-expected.txt: > > + * platform/mac/tables/mozilla/bugs/bug16252-expected.txt: > > You should add some comments about of the types of rebaselinings you are > doing here and why. Fixed.
Comment on attachment 250328 [details] Patch Clearing flags on attachment: 250328 Committed r182536: <http://trac.webkit.org/changeset/182536>
All reviewed patches have been landed. Closing bug.
This change made accessibility/table-sections.html time out on all debug bots.
Zalan asked me to roll out the patch for now. Will do.
Re-opened since this is blocked by bug 143523
Created attachment 250479 [details] Patch
Comment on attachment 250479 [details] Patch Clearing flags on attachment: 250479 Committed r182620: <http://trac.webkit.org/changeset/182620>
Zalan, this one test is still broken on Windows after this patch, and after all the result updates: printing/single-line-must-not-be-split-into-two-pages.html
(In reply to comment #31) > Zalan, this one test is still broken on Windows after this patch, and after > all the result updates: > printing/single-line-must-not-be-split-into-two-pages.html It looks like the Windows bot is emitting an extra EOL at the end of the test file. Maybe this should just be handled as a windows-specific test expectation? Zalan, do you think this extra newline is significant?