<rdar://problem/12588963>

Created attachment 171143 [details] Proposed Change

Comment on attachment 171143 [details] Proposed Change Attachment 171143 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14618624 New failing tests: platform/chromium/virtual/gpu/fast/canvas/canvas-as-image-hidpi.html fast/canvas/canvas-as-image-hidpi.html

I think you're going to have to update ImageBufferSkia to pay attention to your new parameter; it's already got a member variable tracking its scale. Could we come up with a better name for the parameter and the enum values? I really can't tell reading the declaration or call sites what the new code is intended to do, or why you need to override the default parameter value.

(In reply to comment #4) > I think you're going to have to update ImageBufferSkia to pay attention to your new parameter; it's already got a member variable tracking its scale. > > Could we come up with a better name for the parameter and the enum values? I really can't tell reading the declaration or call sites what the new code is intended to do, or why you need to override the default parameter value. It has a member tracking the resolution, but it never scaled like ImageBufferCG. So it isn't handling HiDPI the same as CG. I can add a FIXME and file a bug for Skia. But I don't know Skia/Chromium to be able to fix it.

(In reply to comment #4) > Could we come up with a better name for the parameter and the enum values? I really can't tell reading the declaration or call sites what the new code is intended to do, or why you need to override the default parameter value. As far as the name goes I am open to ideas. But this was the best I could think of while keeping it terse.

Comment on attachment 171143 [details] Proposed Change View in context: https://bugs.webkit.org/attachment.cgi?id=171143&action=review > LayoutTests/ChangeLog:3 > + Test if -webkit-canvas in CSS uses the full backing store instead of alway 1x when rendering. Typo: always > LayoutTests/fast/canvas/canvas-as-image-hidpi.html:19 > + ctx.fillStyle = "rgb(200, 0, 0)"; > + ctx.fillRect(10, 10, 50, 50); > + > + ctx.fillStyle = "rgba(0, 0, 200, 0.5)"; > + ctx.fillRect(25, 25, 90, 90); I'm not sure this will actually test the change. This particular code drawn into 100x100 and then stretched would be indistinguishable from the 200x200 image. Or am I missing it? I can't think of a good way to test otherwise though. > Source/WebCore/ChangeLog:3 > + Make -webkit-canvas in CSS use the full backing store instead of alway 1x when rendering. Typo: always > Source/WebCore/platform/graphics/ImageBuffer.h:81 > + enum Scale { > + Scaled, > + Unscaled > + }; Should this be called ScaleBehavior or something? Scale is a slightly ambiguous word.

Comment on attachment 171143 [details] Proposed Change View in context: https://bugs.webkit.org/attachment.cgi?id=171143&action=review >> LayoutTests/fast/canvas/canvas-as-image-hidpi.html:19 >> + ctx.fillRect(25, 25, 90, 90); > > I'm not sure this will actually test the change. This particular code drawn into 100x100 and then stretched would be indistinguishable from the 200x200 image. Or am I missing it? > > I can't think of a good way to test otherwise though. I did run the test before and after the change and the edges were blurry before. >> Source/WebCore/platform/graphics/ImageBuffer.h:81 >> + }; > > Should this be called ScaleBehavior or something? Scale is a slightly ambiguous word. I can call it ScaleBehavior.

(In reply to comment #5) > (In reply to comment #4) > > I think you're going to have to update ImageBufferSkia to pay attention to your new parameter; it's already got a member variable tracking its scale. > > > > Could we come up with a better name for the parameter and the enum values? I really can't tell reading the declaration or call sites what the new code is intended to do, or why you need to override the default parameter value. > > It has a member tracking the resolution, but it never scaled like ImageBufferCG. So it isn't handling HiDPI the same as CG. I can add a FIXME and file a bug for Skia. But I don't know Skia/Chromium to be able to fix it. That's intentional, chrome is matching the safari/ios model, not the safari/mac one: http://www.html5rocks.com/en/tutorials/canvas/hidpi/