Summary: | Split RenderBlock::layoutInlineChildren into smaller functions | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | James Robinson <jamesr> | ||||||
Component: | New Bugs | Assignee: | James Robinson <jamesr> | ||||||
Status: | RESOLVED CONFIGURATION CHANGED | ||||||||
Severity: | Normal | CC: | abarth, ahmad.saleem792, ap, eric, hyatt, leviw, mitz, rniwa, simon.fraser, webkit.review.bot, zalan | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Other | ||||||||
OS: | OS X 10.5 | ||||||||
Bug Depends on: | 37114, 60052, 60080 | ||||||||
Bug Blocks: | 57779 | ||||||||
Attachments: |
|
Description
James Robinson
2010-03-31 19:06:49 PDT
Created attachment 52237 [details]
Patch
Hopefully this is slightly more readable and easy to edit. I've been careful not to change any logic, just moved code into new functions, added comments, and renamed a few variables that were confusingly named. Attachment 52237 [details] did not pass style-queue:
Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/rendering/RenderBlockLineLayout.cpp:817: One line control clauses should not use braces. [whitespace/braces] [4]
WebCore/rendering/RenderBlockLineLayout.cpp:867: One space before end of line comments [whitespace/comments] [5]
Total errors found: 2 in 3 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 52237 [details] Patch Good idea. > + struct FloatWithRect { Since all the helper functions are member functions, I don't see why we need to make the FloatWithRect struct public. Maybe this is left over from an earlier version of the patch? > + bool layoutReplacedElements(bool relayoutChildren, bool fullLayout, Vector<FloatWithRect>& floats); We don't need the argument name "floats" here. > + RootInlineBox* createLineBoxesForResolver(const InlineBidiResolver& resolver, const InlineIterator& currentPosition, > + bool firstLine, bool previousLineBrokeCleanly, BidiRun* trailingSpaceRun); We don't line up subsequent lines with the "(" from the first line in WebKit. We don't need the argument name "resolver" here and it's arguably we don't need the name "currentPosition" either. And for the actual function definition I also think that "currentPosition" is possibly a too-long name. I suggest just "position", which although ambiguous is shorter and probably clearer despite the ambiguity. > + void layoutRunsAndFloats(bool fullLayout, Vector<FloatWithRect>& floats, int& repaintTop, int& repaintBottom); We don't need the argument name "floats" here. > +bool RenderBlock::layoutReplacedElements(bool relayoutChildren, bool fullLayout, Vector<FloatWithRect>& floats) May want to consider marking this "used in only one place" function inline. Same with others. In some cases that results in smaller code size and slightly better performance. > + RenderObject* o = bidiFirst(this, 0, false); > + while (o) { It's good that you just moved the code and kept the changes to a minimum. Two things I would change in this code in the future would be using a word instead of the letter "o" and use a for loop instead of a while loop. > + lineBox = constructLine(resolver.runCount(), resolver.firstRun(), resolver.lastRun(), firstLine, > + !currentPosition.obj, currentPosition.obj && !currentPosition.pos ? currentPosition.obj : 0); We don't line up second lines under the "(" like this in WebKit code. If the length of the line bothers you than I suggest we use local variables for some of these expressions. One such expression would be "currentPosition.obj && !currentPosition.pos ? currentPosition.obj : 0". And I don't see why that needs "currentPosition.obj &&" in it at all. > + size_t floatCount = floats.size(); > + // Floats that did not have layout did not repaint when we laid them out. They would have > + // painted by now if they had moved, but if they stayed at (0, 0), they still need to be > + // painted. > + for (size_t i = 0; i < floatCount; ++i) { Not new, but it's quite change that this comment is tucked in between the floatCount line and the for line. I would move the comment up one line. Since this is a refactoring patch, I'm going to say review- due to the fact that it needlessly makes FloatWithRect public. Yes, making FloatWithRect public was from an earlier revision and not intentional. Thank you for reviewing, I'll upload an updated patch shortly. Created attachment 52357 [details]
Patch
Attachment 52357 [details] did not pass style-queue:
Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/rendering/RenderBlockLineLayout.cpp:817: One line control clauses should not use braces. [whitespace/braces] [4]
Total errors found: 1 in 3 files
If any of these errors are false positives, please file a bug against check-webkit-style.
I experimented with adding the 'inline' keyword to each of the called functions, but gcc still refused to inline any of them at -O3 citing "--param max-inline-insns-single limit reached" meaning it thought the function body was too large to consider inlining. I can add the keyword back if you think it might help on other platforms. Committed r57030: <http://trac.webkit.org/changeset/57030> Rollout landed in r57096. This regressed fast/repaint/line-flow-with-floats-9 in pixel mode. Reverted at http://trac.webkit.org/changeset/57096, will update the patch once I figure out how I broke stuff. Attachment 52357 [details] was posted by a committer and has review+, assigning to James Robinson for commit.
Comment on attachment 52357 [details]
Patch
Marking r- since this was reverted.
I'm re-writing James' patch in smaller pieces. I'll attach patches to dependent bugs. Well, I found the cause of the regression, James. See bug 60052 Is this bug now fixed given that layoutInlineChildren has been broken down into smaller pieces? http://trac.webkit.org/browser/trunk/Source/WebCore/rendering/RenderBlockLineLayout.cpp#L1103 (In reply to Ryosuke Niwa from comment #16) > Is this bug now fixed given that layoutInlineChildren has been broken down > into smaller pieces? > http://trac.webkit.org/browser/trunk/Source/WebCore/rendering/ > RenderBlockLineLayout.cpp#L1103 Is this required anymore because as mentioned by Ryosuke, this refactoring / simplification happened already? Thanks! We've been asking each other about whether this refactoring is done for 10+ years, I think it's time to stop. |