Summary: | findNextLineBreak splits InlineIterator into 3 pieces | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ryosuke Niwa <rniwa> | ||||
Component: | Layout and Rendering | Assignee: | 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
Ryosuke Niwa
2011-05-03 16:32:30 PDT
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> |