[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