[webkit-reviews] review denied: [Bug 57340] REGRESSION(r74971): Selection doesn't work correctly in BiDi Text : [Attachment 107118] Fixed qt build

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied 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 107118: Fixed qt build
https://bugs.webkit.org/attachment.cgi?id=107118&action=review

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


> Source/WebCore/editing/FrameSelection.cpp:170
> +static void adjustEndpointsAtBidiBoundary(VisiblePosition& base,
VisiblePosition& extent)
> +{
> +    RenderedPosition basePosition(base);
> +    RenderedPosition extentPosition(extent);

I still would flip this argument naming.  Argument as visibleBase,
visibleExtent and locals as base, extent.

> Source/WebCore/editing/RenderedPosition.cpp:112
> +InlineBox* RenderedPosition::prevLeafChild() const
> +{
> +    if (m_prevLeafChild == uncachedInlineBox())
> +	   m_prevLeafChild = m_inlineBox->prevLeafChild();
> +    return m_prevLeafChild;
> +}
> +
> +InlineBox* RenderedPosition::nextLeafChild() const
> +{
> +    if (m_nextLeafChild == uncachedInlineBox())
> +	   m_nextLeafChild = m_inlineBox->nextLeafChild();
> +    return m_nextLeafChild;
> +}

Why do we need these caches?  Did this show up on a benchmark?	If they didn't,
I'd rather not take the security risk associated with weak pointers.

> 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);

This might be more clear written out using locals?  I'm not sure.  as-is this
is unreadable to my eyes.

Also, is this some sort of equivalence? WHy the ||?  Should this be a
separately named equivalence function? (like how Darin long ago suggested for
comparing Position objects)?  == seems wrongly overloaded for this use.

> Source/WebCore/editing/RenderedPosition.cpp:147
> +    ASSERT_NOT_REACHED();
> +    return RenderedPosition();

I wonder if you can just remove this and the compiler will do the right thing?

> Source/WebCore/editing/RenderedPosition.h:89
> +    static InlineBox* uncachedInlineBox() { return
reinterpret_cast<InlineBox*>(1); }

You might want to add a comment here about why you chose 1. (cause it's on the
null page).

> Source/WebCore/editing/RenderedPosition.h:91
> +    static InlineBox* uncachedInlineBox() { return
reinterpret_cast<InlineBox*>(1); }
> +    mutable InlineBox* m_prevLeafChild;
> +    mutable InlineBox* m_nextLeafChild;

I almost wonder if these shouldn't be their own separate struct.  Some sort of
SiblingCache.