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

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has granted 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 87350: Fix regression - patch v3
https://bugs.webkit.org/attachment.cgi?id=87350&action=review

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

Seems reasonable.  I'm not sure I fully folowed the entire patch.  You might
want krit to take a second look.  Please consider my suggested cleanups.

Thsi seems like a ridiculous amount of code for SVG bidi.  I woudl expect we
could share more with HTML.

> Source/WebCore/rendering/svg/SVGRootInlineBox.cpp:278
> +	       SVGTextLayoutAttributes* firstAttributes = 0;
> +	       SVGTextLayoutAttributes* lastAttributes = 0;
> +	       unsigned attributesSize = attributes.size();
> +	       for (unsigned i = 0; i < attributesSize; ++i) {
> +		   SVGTextLayoutAttributes& currentAttributes =
attributes.at(i);
> +		   if (!firstAttributes && firstContext ==
currentAttributes.context())
> +		       firstAttributes = ¤tAttributes;
> +		   if (!lastAttributes && lastContext ==
currentAttributes.context())
> +		       lastAttributes = ¤tAttributes;
> +		   if (firstAttributes && lastAttributes)
> +		       break;
> +	       }

I would do this as a helper function which reutnred firstAttributes and
lastAttributes as refernece params.  And then you could easily name the locals
first and last and current and not worry about confusion. :)

> Source/WebCore/rendering/svg/SVGTextLayoutEngine.cpp:464
> +	   while (!aborted) {
> +	       if (!currentLogicalCharacterMetrics(logicalAttributes,
logicalMetrics)) {
> +		   if (!currentLogicalCharacterAttributes(logicalAttributes))

Just push this while into a function which returns a bool.  Then your if
aborted logic which follows is easier. :)