Bug 60082

Summary: findNextLineBreak splits InlineIterator into 3 pieces
Product: WebKit Reporter: Ryosuke Niwa <rniwa>
Component: Layout and RenderingAssignee: Ryosuke Niwa <rniwa>
Status: RESOLVED FIXED    
Severity: Enhancement CC: eric, hyatt, jamesr, leviw, mitz, simon.fraser
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 57779    
Attachments:
Description Flags
cleanup eric: review+

Ryosuke Niwa
Reported 2011-05-03 16:32:30 PDT
There's a FIXME in findNextLineBreak that says: // FIXME: It is error-prone to split the position object out like this. // Teach this code to work with objects instead of this split tuple. We should fix this.
Attachments
cleanup (32.87 KB, patch)
2011-05-03 16:51 PDT, Ryosuke Niwa 		eric: review+
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Ryosuke Niwa
Comment 1 2011-05-03 16:51:04 PDT
Created attachment 92167 [details] cleanup
Eric Seidel (no email)
Comment 2 2011-05-03 16:58:38 PDT
Comment on attachment 92167 [details] cleanup View in context: https://bugs.webkit.org/attachment.cgi?id=92167&action=review I think this makes the code uglier, but more obviously shows what was wrong with the old code. Do you plan to have a follow-up to clean up more of this? > Source/WebCore/rendering/InlineIterator.h:241 > +inline void InlineIterator::fastIncrementInTextNode() Seems the normal increment() call wants to call this? > Source/WebCore/rendering/RenderBlockLineLayout.cpp:1818 > + while (current.m_obj) { Do we not have an object() function to call instead? Seems new code shouldn't be grabbing m_ members. > Source/WebCore/rendering/RenderBlockLineLayout.cpp:1819 > + RenderObject* next = bidiNext(this, current.m_obj); I think it's OK to store o as a local to reduce the code churn here. > Source/WebCore/rendering/RenderBlockLineLayout.cpp:1998 > + lBreak.moveToStartOf(current.m_obj); Wow. I now have some clue what this line does! Maybe geting rid of "o" in this change is the right thing to do! > Source/WebCore/rendering/RenderBlockLineLayout.cpp:2138 > - lBreak.moveTo(o, pos, nextBreakable); > + lBreak.moveTo(current.m_obj, current.m_pos, current.m_nextBreakablePosition); Sigh. This is nearly the same as lBreak = current, but not quite (due to root).
Ryosuke Niwa
Comment 3 2011-05-03 17:06:21 PDT
Comment on attachment 92167 [details] cleanup View in context: https://bugs.webkit.org/attachment.cgi?id=92167&action=review >> Source/WebCore/rendering/InlineIterator.h:241 >> +inline void InlineIterator::fastIncrementInTextNode() > > Seems the normal increment() call wants to call this? Sure. >> Source/WebCore/rendering/RenderBlockLineLayout.cpp:1818 > > Do we not have an object() function to call instead? Seems new code shouldn't be grabbing m_ members. No :( >> Source/WebCore/rendering/RenderBlockLineLayout.cpp:1998 > > Wow. I now have some clue what this line does! Maybe geting rid of "o" in this change is the right thing to do! I believe so.
Ryosuke Niwa
Comment 4 2011-05-03 17:38:10 PDT
Committed r85695: <http://trac.webkit.org/changeset/85695>
Note You need to log in before you can comment on or make changes to this bug.