[webkit-reviews] review denied: [Bug 99493] CSS cursor property should support webkit-image-set : [Attachment 174876] Patch

[bugzilla-daemon at webkit.org]

Beth Dakin <bdakin at apple.com> has denied Rick Byers <rbyers at chromium.org>'s
request for review:
Bug 99493: CSS cursor property should support webkit-image-set
https://bugs.webkit.org/show_bug.cgi?id=99493

Attachment 174876: Patch
https://bugs.webkit.org/attachment.cgi?id=174876&action=review

------- Additional Comments from Beth Dakin <bdakin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=174876&action=review


This is very close to r+! I just have a few minor comments I would like you to
address first.

> Source/WebCore/css/CSSCursorImageValue.cpp:166
> +	   if (m_imageValue->isImageValue())

Doesn't this need to be an 'else if' to avoid overriding m_image if it is SVG?

> Source/WebCore/css/CSSParser.cpp:1875
> +	   // [<image> [<x> <y>]?,]*  [ auto | crosshair | default | pointer |
progress | move | e-resize | ne-resize |

I'm not convinced we should make this change. We don't use <image> anywhere
else in this class, so it's a little confusing.

> Source/WebCore/page/EventHandler.cpp:157
> +const double minimumCursorScale = 0.001;

Why is this the minimum scale?

> Source/WebCore/page/EventHandler.cpp:1450
> +		       cimage =
static_cast<StyleCachedImage*>(image)->cachedImage();

I know this variable name was already here and is not your fault…but cimage is
such a terrible name! Maybe change it to cursorImage while your changing this
code anyway?

> Source/WebCore/page/EventHandler.cpp:1453
> +		       StyleCachedImageSet* imageSet =
static_cast<StyleCachedImageSet*>(image);

This cast shouldn't be necessary. imageScaleFactor() is implemented as a
virtual function on StyleImage in order to avoid this. I'm guessing that you
need it though because of the call to cachedImage() which is not implemented on
StyleImage. I think you should make cachedImage() virtual and add an
implementation to StyleImage that returns null.

> Source/WebCore/page/EventHandler.cpp:1469
> +	       // Limit the size of cursors (in ui pixels) so that they cannot
be

Capitalize UI.

> Source/WebCore/page/EventHandler.cpp:1475
> +	       Image* rimage = cimage->imageForRenderer(renderer);

I would just call this image.