[webkit-reviews] review denied: [Bug 76446] RenderSVGRoot should inherit from RenderReplaced : [Attachment 122756] Patch v1

[bugzilla-daemon at webkit.org]

Dirk Schulze <krit at webkit.org> has denied Nikolas Zimmermann
<zimmermann at kde.org>'s request for review:
Bug 76446: RenderSVGRoot should inherit from RenderReplaced
https://bugs.webkit.org/show_bug.cgi?id=76446

Attachment 122756: Patch v1
https://bugs.webkit.org/attachment.cgi?id=122756&action=review

------- Additional Comments from Dirk Schulze <krit at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=122756&action=review


The pitch looks good to me. Just a  question to the code. But what about the
test results? That can't be true? r- because of the results.

> Source/WebCore/ChangeLog:14
> +	   It allows us to simplify the painting, as outlines, etc. are painted
by RenderReplaced now.
> +	   While I was it, speed up the local to border box computations by
caching the result.

We haven't done this? :P

> Source/WebCore/rendering/RenderReplaced.cpp:125
> +    if (paintInfo.phase != PaintPhaseForeground && paintInfo.phase !=
PaintPhaseSelection && !canHaveChildren())

Is that just a performance improvement? Why do you need it in your patch?

> LayoutTests/platform/mac/svg/custom/linking-a-03-b-all-expected.txt:8
> +	 RenderSVGText {text} at (100,9) size 188x14 contains 1 chunk(s)
> +	   RenderSVGInlineText {#text} at (0,0) size 188x14
> +	     chunk 1 text run 1 at (100.00,20.00) startOffset 0 endOffset 41
width 188.00: "Some circles with ids, for linking tests."

Why do we get these big changes?

> LayoutTests/platform/mac/svg/custom/linking-a-03-b-all-expected.txt:11
> +	 RenderSVGText {text} at (203,87) size 39x14 contains 1 chunk(s)
> +	   RenderSVGInlineText {#text} at (0,0) size 39x14

To be honest, I don#t understand why just RenderSVGText is affected?

>
LayoutTests/platform/mac/svg/custom/mouse-move-on-svg-container-expected.txt:14

> -selection start: position 0 of child 0 {#text} of child 3 {text} of child 1
{svg} of body
> -selection end:   position 24 of child 0 {#text} of child 3 {text} of child 1
{svg} of body
> +caret: position 0 of child 0 {#text} of child 3 {text} of child 1 {svg} of
body

Ouch, that looks like a regression. Please compare the image results. All text
selection changes are gone. Have you checked that you got a clean DRT
directory? Maybe one of your text patches caused that?