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)
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
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
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.
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
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
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.
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.
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
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.
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.
2014-09-30 16:29 PDT, alan
2014-09-30 16:33 PDT, alan
2014-09-30 17:30 PDT, Build Bot
2014-09-30 18:14 PDT, Build Bot
2014-09-30 19:25 PDT, alan
2014-09-30 21:14 PDT, Build Bot
2014-09-30 21:18 PDT, Build Bot
2014-10-01 08:09 PDT, alan
2014-10-01 12:10 PDT, alan
2014-10-27 13:53 PDT, alan
2014-10-27 17:36 PDT, Build Bot
2014-10-28 07:27 PDT, alan