[webkit-reviews] review denied: [Bug 89888] use scale() instead of getCTM/normalizeTransform/setCTM for svg text decorations : [Attachment 149319] Patch

[bugzilla-daemon at webkit.org]

Dirk Schulze <krit at webkit.org> has denied Mike Reed <reed at google.com>'s request
for review:
Bug 89888: use scale() instead of getCTM/normalizeTransform/setCTM for svg text
decorations
https://bugs.webkit.org/show_bug.cgi?id=89888

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

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


In general I like to do this, instead of the "normalizeTransform" logic. I
reviewed the original patch and thought that it might be worth at that time.
But changed my mind. Even if we see rounding differences. I still think it is a
good idea if Niko looks over the patch as well.

r- because of the issues listed below. I am fine if someone else reviews a new
patch, since I won't be available for the next weeks.

> Source/WebCore/ChangeLog:8
> +
> +	   use scale() instead of getCTM/normalizeTransform/setCTM for svg text
decorations.
> +	   https://bugs.webkit.org/show_bug.cgi?id=89888
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   No new tests. Current layouttests exercise this code path.

A detailed comment is missing what you are doing and why you are doing it.

> Source/WebCore/rendering/svg/SVGInlineTextBox.cpp:618
> +	   context->save();

We have GraphicsContextStateSaver for save/restore. Can't this be used here?
Also, why are you using save and restore here? I can remember that for some
platforms it was quite an expensive operation. Can't you just unscale?

> Source/WebCore/rendering/svg/SVGInlineTextBox.cpp:623
> +	   context->restore();

Some lines later there is a condition and another restore  is called. This
seems dangerous, since no save was called before here.