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.
Created attachment 92167 [details] cleanup
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).
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.
Committed r85695: <http://trac.webkit.org/changeset/85695>