[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]

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
https://bugs.webkit.org/show_bug.cgi?id=57340

Attachment 108221: fixes the bug
https://bugs.webkit.org/attachment.cgi?id=108221&action=review

------- 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.