[webkit-reviews] review denied: [Bug 124394] -webkit-text-decoration-skip: ink is slow : [Attachment 217017] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Nov 15 09:46:15 PST 2013
Simon Fraser (smfr) <simon.fraser at apple.com> has denied Myles C. Maxfield
<mmaxfield at apple.com>'s request for review:
Bug 124394: -webkit-text-decoration-skip: ink is slow
https://bugs.webkit.org/show_bug.cgi?id=124394
Attachment 217017: Patch
https://bugs.webkit.org/attachment.cgi?id=217017&action=review
------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=217017&action=review
> Source/WebCore/platform/graphics/GraphicsContext.cpp:813
> +IntSize GraphicsContext::compatibleBufferPixelSize(const IntSize& size)
const
> {
> // Make the buffer larger if the context's transform is scaling it so we
need a higher
> // resolution than one pixel per unit. Also set up a corresponding scale
factor on the
> // graphics context.
>
> AffineTransform transform = getCTM(DefinitelyIncludeDeviceScale);
> - IntSize scaledSize(static_cast<int>(ceil(size.width() *
transform.xScale())), static_cast<int>(ceil(size.height() *
transform.yScale())));
> + return IntSize(static_cast<int>(ceil(size.width() *
transform.xScale())), static_cast<int>(ceil(size.height() *
transform.yScale())));
It's possibly confusing that this looks at GraphicsContext state to determine
buffer size. I think it would be better as a static function, passing in the
transform.
> Source/WebCore/platform/graphics/GraphicsContext.cpp:834
> + IntSize scaledSize = compatibleBufferPixelSize(size);
> + buffer.resetBuffer();
> + buffer.context()->scale(FloatSize(static_cast<float>(scaledSize.width())
/ size.width(),
> + static_cast<float>(scaledSize.height()) / size.height()));
Not related to this patch, but this scaling strategy means that sometimes the
scale in the buffer won't quite match the scale in the target context.
> Source/WebCore/rendering/InlineTextBox.cpp:1067
> + DEFINE_STATIC_LOCAL(OwnPtr<ImageBuffer>, imageBuffer, ());
> + DEFINE_STATIC_LOCAL(IntSize, imageBufferSize, ());
This means that a single rendering of a skip-ink underline is going to keep
hold of an ImageBuffer for the rest of the life of the process. I don't think
we want that. In some places (e.g. ShadowBlur) we use a timer to clear caches
like this; I think we should centralize this logic into one buffer cache
somewhere.
More information about the webkit-reviews
mailing list