[webkit-reviews] review granted: [Bug 100608] Canvas webkitBackingStorePixelRatio does not change on resize reset : [Attachment 171133] Proposed Change

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Timothy Hatcher <timothy at apple.com>'s
request for review:
Bug 100608: Canvas webkitBackingStorePixelRatio does not change on resize reset
https://bugs.webkit.org/show_bug.cgi?id=100608

Attachment 171133: Proposed Change
https://bugs.webkit.org/attachment.cgi?id=171133&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=171133&action=review


> Source/WebCore/ChangeLog:6
> +	   The backing store was not being recreated using the current page
pixel ratio
> +	   when a resize reset occurred.

What is a “resize reset”?

> Source/WebCore/html/HTMLCanvasElement.cpp:265
> +    IntSize newSize = IntSize(w, h);

Could just use construction syntax here instead of repeating IntSize.

> Source/WebCore/html/HTMLCanvasElement.cpp:308
> +    // NOTE: High-DPI canvas requires the platform-specific ImageBuffer
implementation to respect
> +    // the resolutionScale parameter.

I do not understand this comment that you moved here. How does this comment
help someone reading this function?

> Source/WebCore/html/HTMLCanvasElement.h:85
>      void setSize(const IntSize& newSize)
>      { 
> -	   if (newSize == size())
> +	   if (newSize == size() && targetDeviceScaleFactor() ==
m_deviceScaleFactor)

It seems really strange that if a canvas is in a window that’s moved from one
screen to another, the device scale factor is updated only when the canvas’s
size changes. Is that really the design?

I don’t like the design of this setSize function. Before the change, the
function was like changing width and height with the DOM, but optimized in two
ways:

1) If the size did not change, it skipped the reset. Setting width and height
through the DOM does not have that kind of optimization.
2) If the size did change, then it did one reset rather than the two you’d get
if you set both width and height through the DOM.

For me, (1) does not seem like a good thing, It’s strange that this setSize
function has this optimization, but setting the width and height DOM attributes
or content attributes does not. On the other hand, to me (2) seems purely good
because it boosts performance but otherwise is not detectable.

What code calls this function? Are those callers really relying on behavior
(1)? If they are not, then we could just remove the early return rather than
adding the mysterious scale factor check here.