[webkit-reviews] review denied: [Bug 40663] Modernize SVG Text code, following the HTML design : [Attachment 58990] Updated patch

[bugzilla-daemon at webkit.org]

Dirk Schulze <krit at webkit.org> has denied Nikolas Zimmermann
<zimmermann at kde.org>'s request for review:
Bug 40663: Modernize SVG Text code, following the HTML design
https://bugs.webkit.org/show_bug.cgi?id=40663

Attachment 58990: Updated patch
https://bugs.webkit.org/attachment.cgi?id=58990&action=review

------- Additional Comments from Dirk Schulze <krit at webkit.org>
WebCore/rendering/RenderBlock.h:232
 +	// Only used by RenderSVGText, which explicitely overrides
RenderBlock::layoutBlock(), do NOT use for anything else.
Can you surround it with #if ENABLE(SVG)?

WebCore/rendering/RenderBlockLineLayout.cpp:750
 +			    if (!isSVGRootInlineBox)
Can you use:
#if ENABLE(SVG)
    if (lineBox->isSVGRootInlineBox())
#endif
	computeHorizontalPositionsForLine(lineBox, firstLine,
resolver.firstRun(), trailingSpaceRun, end.atEnd(), textBoxDataMap);

instead?

WebCore/rendering/RenderSVGInlineText.cpp: 
 +	// FIXME: That's not sufficient. The localCaretRect function is also
used for selection.
Do or don't we need caret rects for selecting texts?

WebCore/rendering/RenderSVGText.cpp:185
 +	FloatRect boundingBox = objectBoundingBox();
Can you name it strokeRect or strokeBoundaries please? This would match the
naming in other renderers.

WebCore/rendering/RenderSVGText.cpp:207
 +   
No need for this white space :-)

WebCore/rendering/SVGInlineFlowBox.cpp:44
 +	FloatRect boundingBox = boxRenderer->repaintRectInLocalCoordinates();
same. boundingBox is not accurate enough, use repaintRect here

WebCore/rendering/SVGInlineTextBox.cpp:310
 +	    ASSERT_NOT_REACHED();
I think this the assertion should be removed. At the moment our default
fallback is black. This will change and if fill or stroke are not valid, we
shouldn't draw text/decorations/under-/over-/through-line at all. 
WebCore/rendering/SVGInlineTextBox.cpp:414
 +	ASSERT_NOT_REACHED();
IIRC there are also other decorations like blink. Maybe you shouldn't use the
assert here.

I did not check the pixel results yet, but the DRT looking at the DRT results,
the text moves more than 30px sometimes. You checked all DRT tests manually
with such a huge change?

I would like to look a second time on the patch, once you replied to this
review.