[webkit-reviews] review granted: [Bug 60082] findNextLineBreak splits InlineIterator into 3 pieces : [Attachment 92167] cleanup
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue May 3 16:58:37 PDT 2011
Eric Seidel <eric at webkit.org> has granted Ryosuke Niwa <rniwa at webkit.org>'s
request for review:
Bug 60082: findNextLineBreak splits InlineIterator into 3 pieces
https://bugs.webkit.org/show_bug.cgi?id=60082
Attachment 92167: cleanup
https://bugs.webkit.org/attachment.cgi?id=92167&action=review
------- Additional Comments from Eric Seidel <eric at webkit.org>
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).
More information about the webkit-reviews
mailing list