[webkit-reviews] review denied: [Bug 53980] REGRESSION (r68976): Incorrect bidi rendering in SVG text : [Attachment 87321] Fix regression - patch

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Nikolas Zimmermann
<zimmermann at kde.org>'s request for review:
Bug 53980: REGRESSION (r68976): Incorrect bidi rendering in SVG text
https://bugs.webkit.org/show_bug.cgi?id=53980

Attachment 87321: Fix regression - patch
https://bugs.webkit.org/attachment.cgi?id=87321&action=review

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

> Source/WebCore/rendering/InlineFlowBox.cpp:1293
> +    while (leaf) {
> +	   if (leaf->bidiLevel() > maxLevel)
> +	       maxLevel = leaf->bidiLevel();
> +	   if (leaf->bidiLevel() < minLevel)
> +	       minLevel = leaf->bidiLevel();
> +	   leafBoxesInLogicalOrder.append(leaf);
> +	   leaf = leaf->nextLeafChild();
> +    }

This easier to write as a for loop using min/max.

> Source/WebCore/rendering/InlineFlowBox.cpp:1313
> +	       while (it != end) {
> +		   if ((*it)->bidiLevel() >= minLevel)
> +		       break;
> +		   ++it;

See bug https://bugs.webkit.org/show_bug.cgi?id=57341.	It's a shame you're
copying this code out of BidiResolver.

> Source/WebCore/rendering/svg/SVGRootInlineBox.cpp:237
> +static inline void reverseInlineBoxRangeAndValueListsIfNeeded(void*
userData, Vector<InlineBox*>::iterator first, Vector<InlineBox*>::iterator
last)

I'm confused why we ever need to use a void*...

> Source/WebCore/rendering/svg/SVGRootInlineBox.cpp:243
> +	   if (first == last || first == --last)

So you want this to conditionally decrement last?  This seems very easy to
mis-read.

> Source/WebCore/rendering/svg/SVGRootInlineBox.cpp:291
> +		   swapItems(xValuesFirst, xValuesLast, firstBoxPosition,
lastBoxPosition);

Seems we should write a swapItems which takes a SVGTextLayoutAttributes* and
then we don't need quite so much copy/paste code here.