[webkit-reviews] review granted: [Bug 57340] REGRESSION(r74971): Selection doesn't work correctly in BiDi Text : [Attachment 108221] fixes the bug

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Sep 21 16:52:17 PDT 2011

Eric Seidel <eric at webkit.org> has granted Ryosuke Niwa <rniwa at webkit.org>'s
request for review:
Bug 57340: REGRESSION(r74971): Selection doesn't work correctly in BiDi Text

Attachment 108221: fixes the bug

------- Additional Comments from Eric Seidel <eric at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=108221&action=review

This in general seems fine.  the == operator is dangerous, I think.  I'm
convinced that the caching approach is a good one, but you really need to add
some sort of WARNING at the top of RenderPosition explaining that it is not
safe across layouts (or render tree modifications in general).

> Source/WebCore/editing/RenderedPosition.cpp:118
> +    return (m_renderer == other.m_renderer && m_inlineBox ==
other.m_inlineBox && m_offset == other.m_offset)
> +	   || (atLeftmostOffsetInBox() && other.atRightmostOffsetInBox() &&
prevLeafChild() == other.m_inlineBox)
> +	   || (atRightmostOffsetInBox() && other.atLeftmostOffsetInBox() &&
nextLeafChild() == other.m_inlineBox);

I still think this should be an explicit isEquivalent call, or something other
than ==.  Other parts of code (HashTable for example) would expect == to not be
this loose.

More information about the webkit-reviews mailing list