Make SVG images for custom CSS cursors appear sharp on retina displays

Ensure using an SVG image as a custom CSS cursor gives a sharp image on a
retina display. Rasterize the SVG image using the current device pixel ratio
before the Cursor is converted to a WebCursor.

BUG=325565
TEST=Check http://jsbin.com/aboVovOw on a retina display, cursor should not be blurry

[pdr., 2013-12-04 20:55:40]

At a high level, I think we're fixing this at the wrong level. For example, in
your testcase, if you replace the 28x28 svg image with a 28x28 png image, it
will also be blurry. In addition, Firefox seems to also render the cursor image
blurry.

If we want to not cause web authors to do the 2x trick with svg images, I think
we should fix this down in nativeImageForCurrentFrame. This would require fixing
other callers (e.g., drag images, etc), but would be much cleaner from a code
perspective. Unfortunately, if all other browsers require the 2x trick, we will
end up drawing 4x the pixels if users want this to be sharp on firefox.

https://codereview.chromium.org/105363002/diff/1/Source/web/ChromeClientImpl.cpp
File Source/web/ChromeClientImpl.cpp (right):

https://codereview.chromium.org/105363002/diff/1/Source/web/ChromeClientImpl....
Source/web/ChromeClientImpl.cpp:719:
setCursor(WebCursorInfo(rasterizeSVGCursor(cursor,
m_webView->deviceScaleFactor())));
Is this codepath only used for SVG cursors?

[Evan Wallace, 2013-12-04 21:31:09]

For context, I'm also working on getting this working in Firefox:
https://bugzilla.mozilla.org/show_bug.cgi?id=888689. It makes sense that SVG
cursors would be sharp and bitmap cursors would be blurry since that's what
already happens inside a page. It does involve rendering 4x the area but so does
rendering an SVG image inside a page.

I initially investigated adding a scale factor to nativeImageForCurrentFrame()
but it seemed like a much more invasive change, which lead me to this approach.
I can rework the patch to use nativeImageForCurrentFrame instead, but that
seemed to involve wrapping a StyleImage to bundle the imageScaleFactor along
with it, similar to StyleCachedImageSet. The problem I had then was that
CSSCursorImageValue doesn't know if the resource will be an svg until it's
loaded (which is after the StyleImage has been created), so it doesn't know
whether or not to wrap it. The imageScaleFactor must only be bundled for svg
images and not for bitmap images. Is this the right direction to investigate?

https://codereview.chromium.org/105363002/diff/1/Source/web/ChromeClientImpl.cpp
File Source/web/ChromeClientImpl.cpp (right):

https://codereview.chromium.org/105363002/diff/1/Source/web/ChromeClientImpl....
Source/web/ChromeClientImpl.cpp:719:
setCursor(WebCursorInfo(rasterizeSVGCursor(cursor,
m_webView->deviceScaleFactor())));
On 2013/12/04 20:55:40, pdr wrote:
> Is this codepath only used for SVG cursors?

Yes, rasterizeSVGCursor() will only rasterize the cursor if it contains an SVG
image (if isSVGImage() returns true).

[pdr., 2013-12-04 21:43:43]

On 2013/12/04 21:31:09, Evan Wallace wrote:
> For context, I'm also working on getting this working in Firefox:
> https://bugzilla.mozilla.org/show_bug.cgi?id=888689. It makes sense that SVG
> cursors would be sharp and bitmap cursors would be blurry since that's what
> already happens inside a page. It does involve rendering 4x the area but so
does
> rendering an SVG image inside a page.

Wow :) This is excellent. I don't have my Windows machine in the office but I'm
curious
if you know what happens on IE?

> 
> I initially investigated adding a scale factor to nativeImageForCurrentFrame()
> but it seemed like a much more invasive change, which lead me to this
approach.
> I can rework the patch to use nativeImageForCurrentFrame instead, but that
> seemed to involve wrapping a StyleImage to bundle the imageScaleFactor along
> with it, similar to StyleCachedImageSet. The problem I had then was that
> CSSCursorImageValue doesn't know if the resource will be an svg until it's
> loaded (which is after the StyleImage has been created), so it doesn't know
> whether or not to wrap it. The imageScaleFactor must only be bundled for svg
> images and not for bitmap images. Is this the right direction to investigate?

I'd recommend adding an optional enum parameter to nativeImageForCurrentFrame
that's
something like:
enum ImageDimensions {
  ImageDefault,
  WithDeviceScale,
};
(please pick a better name :)

You can set the default to be ImageDefault so you don't have to update all
callsites.
You can then just call nativeImageForCurrentFrame(WithDeviceScale) and do the
page scale
calculation in SVGImage::nativeImageForCurrentFrame(...).

If you're not familiar with the pattern of using enums for bools (I wasn't),
Document::updateLayoutIgnorePendingStylesheets(RunPostLayoutTasks =
RunPostLayoutTasksAsyhnchronously) shows an example of this pattern.

[Evan Wallace, 2013-12-05 10:30:04]

On 2013/12/04 21:43:43, pdr wrote:
> Wow :) This is excellent. I don't have my Windows machine in the office but
I'm
> curious
> if you know what happens on IE?

Yes, unfortunately IE's cursor support is severely lacking. Custom cursors don't
work at all unless they are in the Microsoft-specific *.cur format. IE also
doesn't support other nice custom cursor features like data URIs.

> I'd recommend adding an optional enum parameter to nativeImageForCurrentFrame
> that's
> something like:
> enum ImageDimensions {
>   ImageDefault,
>   WithDeviceScale,
> };
> (please pick a better name :)
> 
> You can set the default to be ImageDefault so you don't have to update all
> callsites.
> You can then just call nativeImageForCurrentFrame(WithDeviceScale) and do the
> page scale
> calculation in SVGImage::nativeImageForCurrentFrame(...).
> 
> If you're not familiar with the pattern of using enums for bools (I wasn't),
> Document::updateLayoutIgnorePendingStylesheets(RunPostLayoutTasks =
> RunPostLayoutTasksAsyhnchronously) shows an example of this pattern.

I tried augmenting nativeImageForCurrentFrame() but it still seems like a pretty
invasive change for such a small fix. Right now conversion from a WebCore::Image
to a blink::WebImage always uses PreferDeviceScale because WebImage.h doesn't
include Image.h and I wasn't sure if it was a good idea to change the public API
just for that.

[pdr., 2013-12-06 06:06:42]

On 2013/12/05 10:30:04, Evan Wallace wrote:
> On 2013/12/04 21:43:43, pdr wrote:
> > Wow :) This is excellent. I don't have my Windows machine in the office but
> I'm
> > curious
> > if you know what happens on IE?
> 
> Yes, unfortunately IE's cursor support is severely lacking. Custom cursors
don't
> work at all unless they are in the Microsoft-specific *.cur format. IE also
> doesn't support other nice custom cursor features like data URIs.
> 
> > I'd recommend adding an optional enum parameter to
nativeImageForCurrentFrame
> > that's
> > something like:
> > enum ImageDimensions {
> >   ImageDefault,
> >   WithDeviceScale,
> > };
> > (please pick a better name :)
> > 
> > You can set the default to be ImageDefault so you don't have to update all
> > callsites.
> > You can then just call nativeImageForCurrentFrame(WithDeviceScale) and do
the
> > page scale
> > calculation in SVGImage::nativeImageForCurrentFrame(...).
> > 
> > If you're not familiar with the pattern of using enums for bools (I wasn't),
> > Document::updateLayoutIgnorePendingStylesheets(RunPostLayoutTasks =
> > RunPostLayoutTasksAsyhnchronously) shows an example of this pattern.
> 
> I tried augmenting nativeImageForCurrentFrame() but it still seems like a
pretty
> invasive change for such a small fix. Right now conversion from a
WebCore::Image
> to a blink::WebImage always uses PreferDeviceScale because WebImage.h doesn't
> include Image.h and I wasn't sure if it was a good idea to change the public
API
> just for that.

I agree, this looks pretty messy. I apologize for sending you down that path :/

I have another idea, but I'm not sure of the implications. You could try
plumbing the device scale into SVGImage (like you did in the latest patch) and
then using it for the ImageBuffer::create call, which takes a resolutionScale
parameter. Ideally, this would create a 2x ImageBuffer that acts like it has the
original dimensions.

[pdr., 2013-12-14 23:52:53]

+rbyers

This is a tough one. I still don't like the first approach (I think this should
be handled at a different layer) but the second patch doesn't seem right either.

I feel like we need to bring in some experts. Rick, you're the author of a large
Cursor patch and are probably the most knowledgeable about this area; can you
share your thoughts on how to solve this problem? At a high level, I don't think
SVG should be special cased here, but I don't see a nice way to make that work.

[Rick Byers, 2013-12-18 06:31:52]

On 2013/12/14 23:52:53, pdr wrote:
> +rbyers
> 
> This is a tough one. I still don't like the first approach (I think this
should
> be handled at a different layer) but the second patch doesn't seem right
either.
> 
> I feel like we need to bring in some experts. Rick, you're the author of a
large
> Cursor patch and are probably the most knowledgeable about this area; can you
> share your thoughts on how to solve this problem? At a high level, I don't
think
> SVG should be special cased here, but I don't see a nice way to make that
work.

Yeah, I wrote the support for high-dpi cursors a year ago (with
-webkit-image-set), see http://crbug.com/136034.  But I totally neglected SVG
cursors at the time (except for adding some minimal test cases for them). 
Thanks for pushing on this Evan - I agree that it should be possible to specify
cursors with SVG and have them use the native resolution.

I'm no expert on Image stuff, but I've spent a couple hours trying to page this
back in and form an opinion.  As you've probably seen, the way it works for
-webkit-image-set cursors is that CSSCursorImageValue::cachedImage creates a
StyleImage for the current device scale factor.  With SVG images the StyleImage
doesn't need to change (necessarily) depending on the device scale factor.

What makes it so that an SVG image inline in the HTML is rasterized according to
the current device scale factor, but when used for a Cursor it's rasterized with
a fixed scale factor of 1?  Is it just that converting an Image to a WebImage
uses Image::nativeImageForCurrentFrame which doesn't take device scale factor
into account, but inline images use Image::draw* which does?  Are there other
uses of SVGImage::nativeImageForCurrentFrame which similarly don't correctly
support high-dpi scenarios?  Perhaps there should be a special version of
nativeImageForCurrentFrame on SVGImage which takes a desired scale factor as an
argument (since SVGImage is the only type of Image that doesn't have a fixed
scale)?  Then we could pass the current deviceScaleFactor into the WebCursorInfo
ctor and have it rasterize the SVG according to that scale.  That's _almost_
what the original patch is doing, except it seems hacky to pre-rasterize and
generate a new Cursor with a bitmap image.  I don't think it's crazy to
special-case SVG in WebCursorInfo since WebCursorInfo details (somewhat
implicitly) with rasterizing an Image to a bitmap and isn't going to want to
apply the device-scale to bitmap images.

I guess that's the other option though - WebCursorInfo could always include a
bitmap in device pixels and then not need a scale factor at all.  Then we'd
rasterize ALL Images using the current device scale factor and not need to
special-case SVG (but it would only be SVG that would scale gracefully).  This
would send some unnecessary bits across the IPC channel for the common case of
bitmap images, but otherwise I think it's basically identical to what we do
today (where it's up to the browser size to scale the image appropriately). 
I.e. maybe the fundamental problem here is that I chose to scale cursor images
to match the device scale in the browser process, after we've already had to
rasterize vector images for communication across IPC (I think this was motivated
by WebKit port compatibility at the time, no longer an issue in blink).  If we
did this scale-to-device-pixels just before the rasterization step in blink then
things would just fall out nicely. 

Thoughts?

Also this of course needs a test case.  See
fast/events/mouse-cursor-image-set.html for reference.