use scale() instead of getCTM/normalizeTransform/setCTM for svg text decorations
Created attachment 149302 [details] Patch
Why wouldn't all platforms want this path? Why does Skia?
Why: Skia-specific: The current code does not play well if we try to record/playback the drawing commands, since the getCTM() is done once, and may not be the same matrix that is present during subsequent playback. General: The old code introduces a fair amount of complexity and heuristics (e.g. why do we normalize only around identity, and not other integral scale factors or integral translates?) If other platforms want to opt-in to this change, I presume they will either remove the check, or add their platform to the list.
Comment on attachment 149302 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=149302&action=review > Source/WebCore/rendering/svg/SVGInlineTextBox.cpp:652 > + context->save(); if we consider save/restore() lightweight, I'd prefer the save/restore pair outside the conditional so that only one "scalingFactor != 1" check is needed.
I can make that change. My original CL attempted to keep the existing flow as much as possible. I do think save/restore are very efficient in skia, so it should not be a perf issue there.
always calling save/restore might be another reason to make this Skia specific, unless the other platforms feel the same way about their relative perf cost...
I don't know what the current view on the cost of GraphicsContext::save/restore is.
I'd prefer if we just removed the "old" code and used scale instead, if that's perceived as the better way. We can re-visit adding the "old" code back for other platforms if scale is too slow. The perf bots will answer these questions (but I doubt it matters). So my suggestion would be to just change the code instead of adding yet another if-def.
I'll give it a try, and hope there are rebaselines on other platforms...
Created attachment 149319 [details] Patch
patch #2 removes #ifdefs
PTAL
Comment on attachment 149319 [details] Patch Am I reading correctly that this patch basically undoes this change: http://trac.webkit.org/changeset/78704/trunk/Source/WebCore/rendering/svg/SVGInlineTextBox.cpp? Do you anticipate that this will cause rendering differences between 64 and 32 bit machines (for skia or otherwise)?
I would think it would, though zimmermann might comment on that. This is why I thought my change should be per-platform.
I see. I can understand your desire for this to be per-platform then. I'd like to give Niko a day to comment. He's working from Germany last I knew.
Comment on attachment 149319 [details] Patch 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.
(In reply to comment #16) > (From update of attachment 149319 [details]) > 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? Too numerically unstable. That is why we have save/restore. > > > 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. This is from code that has always been here. The restore is balance a hidden save inside applyShadowToGraphicsContext().
Created attachment 149516 [details] Patch
Updated comment, and replied to Dirk's comments. PTAL
Comment on attachment 149516 [details] Patch If there are no regressions, r=me. Reverting the mess is just right, it didn't fix anything after all, and was my rude attempt to get consistent results between 32bit/64bit mac machines, but as it effectively breaks things for Chromium, it should just go away.
Comment on attachment 149516 [details] Patch Clearing flags on attachment: 149516 Committed r121343: <http://trac.webkit.org/changeset/121343>
All reviewed patches have been landed. Closing bug.
*** Bug 89506 has been marked as a duplicate of this bug. ***