Description
zalan
2014-09-30 15:57:16 PDT
Created attachment 238974 [details]
Patch
Attachment 238974 [details] did not pass style-queue:
ERROR: Source/WebCore/rendering/SimpleLineLayout.cpp:319: Please declare enum bitfields as unsigned integral types. [runtime/enum_bitfields] [5]
ERROR: Source/WebCore/rendering/SimpleLineLayout.cpp:321: Please declare enum bitfields as unsigned integral types. [runtime/enum_bitfields] [5]
ERROR: Source/WebCore/rendering/SimpleLineLayout.cpp:579: Multi line control clauses should use braces. [whitespace/braces] [4]
Total errors found: 3 in 16 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 238975 [details]
SimpleLineLayout.cpp as the patch diff is far from perfect.
Added the entire file to have a better view on the changeset. (as it completely changes the parsing functions)
Comment on attachment 238974 [details] Patch Attachment 238974 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/6623868777136128 New failing tests: tables/mozilla_expected_failures/bugs/bug2479-5.html Created attachment 238981 [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 238974 [details] Patch Attachment 238974 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/5255022971453440 New failing tests: tables/mozilla_expected_failures/bugs/bug2479-5.html Created attachment 238984 [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
Created attachment 238986 [details]
Patch
Attachment 238986 [details] did not pass style-queue:
ERROR: Source/WebCore/rendering/SimpleLineLayout.cpp:319: Please declare enum bitfields as unsigned integral types. [runtime/enum_bitfields] [5]
ERROR: Source/WebCore/rendering/SimpleLineLayout.cpp:321: Please declare enum bitfields as unsigned integral types. [runtime/enum_bitfields] [5]
Total errors found: 2 in 17 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 238986 [details] Patch Attachment 238986 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/5220023282958336 New failing tests: tables/mozilla_expected_failures/bugs/bug2479-5.html Created attachment 238993 [details]
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-15 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
Comment on attachment 238986 [details] Patch Attachment 238986 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/5116903265665024 New failing tests: tables/mozilla_expected_failures/bugs/bug2479-5.html Created attachment 238994 [details]
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-07 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Created attachment 239028 [details]
Patch
Attachment 239028 [details] did not pass style-queue:
ERROR: Source/WebCore/rendering/SimpleLineLayout.cpp:319: Please declare enum bitfields as unsigned integral types. [runtime/enum_bitfields] [5]
ERROR: Source/WebCore/rendering/SimpleLineLayout.cpp:321: Please declare enum bitfields as unsigned integral types. [runtime/enum_bitfields] [5]
Total errors found: 2 in 16 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 239039 [details]
SimpleLineLayout.cpp to have a better view on the changeset as the diff is far from being perfect.
I'll put the rebaseline change to separate patch. Comment on attachment 239028 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=239028&action=review Very nice, r=me > Source/WebCore/ChangeLog:19 > + The patch changes the behavior of overflow-wrap: break-word. Currently > + we wrap words at arbitrary positions regardless whether there's a valid wrapping > + position earlier on the line, while this patch checks it and wraps accordingly. > + It is inline with Firefox current behavior. This patch changes simple layout only. How about the line boxes? > Source/WebCore/ChangeLog:43 > + * rendering/SimpleLineLayout.cpp: > + (WebCore::SimpleLineLayout::skipWhitespace): > + (WebCore::SimpleLineLayout::computeLineLeft): > + (WebCore::SimpleLineLayout::TextFragment::TextFragment): > + (WebCore::SimpleLineLayout::TextFragment::operator=): > + (WebCore::SimpleLineLayout::TextFragment::isEmpty): > + (WebCore::SimpleLineLayout::LineState::LineState): > + (WebCore::SimpleLineLayout::LineState::commitAndCreateRun): > + (WebCore::SimpleLineLayout::LineState::addUncommitted): > + (WebCore::SimpleLineLayout::LineState::addUncommittedWhitespace): > + (WebCore::SimpleLineLayout::LineState::jumpTo): > + (WebCore::SimpleLineLayout::LineState::width): > + (WebCore::SimpleLineLayout::LineState::fits): > + (WebCore::SimpleLineLayout::LineState::removeCommittedTrailingWhitespace): > + (WebCore::SimpleLineLayout::removeTrailingWhitespace): > + (WebCore::SimpleLineLayout::initializeLine): > + (WebCore::SimpleLineLayout::splitFragmentToFitThisLine): > + (WebCore::SimpleLineLayout::nextFragment): > + (WebCore::SimpleLineLayout::createLineRuns): > + (WebCore::SimpleLineLayout::updateLineConstrains): > + (WebCore::SimpleLineLayout::createTextRuns): Some function level comments would be nice to explain the refactoring. > Source/WebCore/rendering/SimpleLineLayout.cpp:471 > +static TextFragment splitFragmentToFitThisLine(float availableWidth, TextFragment& fragmentToSplit, bool keepAtLeastOneCharacter, const RenderText& textRenderer, const CharacterType* text, 'This' doesn't add anything. fragmentToSplit could be the first parameter since it is being operated on. > Source/WebCore/rendering/SimpleLineLayout.cpp:489 > + // Simple binary search to find out what fits the current line. > + // FIXME: add surrogate pair support. > + unsigned left = fragmentToSplit.start; > + unsigned right = fragmentToSplit.end - 1; // We can ignore the last character. It surely does not fit. > + float width = 0; > + while (left < right) { Nice optimization! > Source/WebCore/rendering/SimpleLineLayout.cpp:521 > + TextFragment current; current->fragment perhaps > Source/WebCore/rendering/SimpleLineLayout.cpp:563 > + TextFragment currentFragment = nextFragment(line.end, lineBreakIterator, style, text, textLength, line.width(), textRenderer); 'current' doesn't really add much, maybe just 'fragment' > Source/WebCore/rendering/SimpleLineLayout.cpp:571 > + bool firstFragment = !line.width(); isFirstFragment > Source/WebCore/rendering/SimpleLineLayout.cpp:575 > + else // No need to add the new line fragment if there's already content on the line. We are about to close this line anyway. Better to put the comment on the next line (and add {}) > Source/WebCore/rendering/SimpleLineLayout.cpp:585 > + } else if (firstFragment) // Non-breakable non-whitespace first fragment. Add it to the current line. -it overflows though. Here too > Source/WebCore/rendering/SimpleLineLayout.cpp:587 > + else // Non-breakable non-whitespace fragment when there's already a fragment on the line. Push it to the next line. And here. > Source/WebCore/rendering/SimpleLineLayout.cpp:620 > +static void updateLineConstrains(RenderBlockFlow& flow, float& availableWidth, float& logicalLeftOffset) const RenderBlockFlow& Comment on attachment 239028 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=239028&action=review > PerformanceTests/Layout/line-layout-simple.html:187 > + <div style="text-align: justify"> > + <h4>ARGUMENTUM</h4> It would be good to land the performance test change first separately an wait a bit for the bots to get some results with the updated test before landing the code changes. Comment on attachment 239028 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=239028&action=review > Source/WebCore/rendering/SimpleLineLayout.cpp:311 > + TextFragment(const TextFragment& f) > + : start(f.start) > + , isCollapsedWhitespace(f.isCollapsedWhitespace) > + , end(f.end) > + , isWhitespaceOnly(f.isWhitespaceOnly) > + , isBreakable(f.isBreakable) > + , mustBreak(f.mustBreak) > + , width(f.width) > + { > } > -} > > -template <typename CharacterType> > -void createTextRuns(Layout::RunVector& runs, unsigned& lineCount, RenderBlockFlow& flow, RenderText& textRenderer) > -{ > - const Style style(flow.style()); > + TextFragment& operator=(const TextFragment& f) > + { > + start = f.start; > + end = f.end; > + isCollapsedWhitespace = f.isCollapsedWhitespace; > + isWhitespaceOnly = f.isWhitespaceOnly; > + isBreakable = f.isBreakable; > + mustBreak = f.mustBreak; > + width = f.width; > + return *this; > + } Do you actually need these? The implicit copy constructor/operator should work right? Committed r175048: <http://trac.webkit.org/changeset/175048> (In reply to comment #21) > Committed r175048: <http://trac.webkit.org/changeset/175048> ^^ performance test change only. Created attachment 240505 [details]
Patch
Attachment 240505 [details] did not pass style-queue:
ERROR: Source/WebCore/rendering/SimpleLineLayout.cpp:296: Please declare enum bitfields as unsigned integral types. [runtime/enum_bitfields] [5]
ERROR: Source/WebCore/rendering/SimpleLineLayout.cpp:298: Please declare enum bitfields as unsigned integral types. [runtime/enum_bitfields] [5]
Total errors found: 2 in 9 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 240515 [details]
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-01 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Created attachment 240537 [details]
Patch
Attachment 240537 [details] did not pass style-queue:
ERROR: Source/WebCore/rendering/SimpleLineLayout.cpp:296: Please declare enum bitfields as unsigned integral types. [runtime/enum_bitfields] [5]
ERROR: Source/WebCore/rendering/SimpleLineLayout.cpp:298: Please declare enum bitfields as unsigned integral types. [runtime/enum_bitfields] [5]
Total errors found: 2 in 9 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 240537 [details] Patch Clearing flags on attachment: 240537 Committed r175259: <http://trac.webkit.org/changeset/175259> All reviewed patches have been landed. Closing bug. This patch broke tables/mozilla_expected_failures/bugs/bug2479-5.html on Mavericks. Updated the results in r175329 - the change was the same as on Yosemite, so I'm assuming that it's good. It's surprising to have so many test result changes from a performance patch. Is the behavior changed explained somewhere? I looked though bugzilla comments, but couldn't find an explanation. (In reply to comment #30) > This patch broke tables/mozilla_expected_failures/bugs/bug2479-5.html on > Mavericks. > > Updated the results in r175329 - the change was the same as on Yosemite, so > I'm assuming that it's good. > > It's surprising to have so many test result changes from a performance > patch. Is the behavior changed explained somewhere? I looked though bugzilla > comments, but couldn't find an explanation. If you look at the nature of the expected result changes, you'll see it's all about removing unnecessary (empty) text runs. Not creating empty runs has not that much of an impact on measurable performance, but the same time creating them is just a waste. Thanks for the r175329 change. |