[webkit-reviews] review granted: [Bug 99530] cssText for cursor property doesn't include hotspot : [Attachment 169955] tweak based on cr feedback from darin@

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Rick Byers <rbyers at chromium.org>'s
request for review:
Bug 99530: cssText for cursor property doesn't include hotspot
https://bugs.webkit.org/show_bug.cgi?id=99530

Attachment 169955: tweak based on cr feedback from darin@
https://bugs.webkit.org/attachment.cgi?id=169955&action=review

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


Patch is OK, but there is a little room for improvement.

> Source/WebCore/css/CSSCursorImageValue.cpp:88
> +    // Negative values are ignored as outside the bounds of the image, and
so
> +    // used to indicate no hotspot.

I understand that negative values are outside the bounds of the image. But
large positive values can be outside the bounds of the image, too. This comment
does not explain the reason behind the “don’t serialize” behavior here. Is
there anything helpful in the specification that makes it clear what we should
do for edge cases such as when one value is negative and the other is not?

I suspect that a correct implementation would require that we store the lack of
a hot spot separately, explicitly, rather than using a special value to
indicate there is no hot spot. And that a correct implementation would
correctly re-serialize even negative numbers.

> Source/WebCore/css/CSSCursorImageValue.h:44
> +    String customCssText() const;

This should be:

    virtual String customCSSText() const OVERRIDE;

Also, it would be good to make this override a private function. I can’t think
of a real reason to support calling this function directly on a
CSSCursorImageValue.

> LayoutTests/fast/css/cursor-parsing-expected.txt:31
> +PASS roundtripCssRule("cursor: url(file:///foo.png) -2 3;") is "cursor:
url(file:///foo.png);"
> +PASS roundtripCssRule("cursor: url(file:///foo.png) 2 -3;") is "cursor:
url(file:///foo.png);"
> +PASS roundtripCssRule("cursor: url(file:///foo.png) -2 -3;") is "cursor:
url(file:///foo.png);"

Is this really expected behavior? How do other browsers do on this test?

> LayoutTests/fast/css/cursor-parsing.html:49
> +testCursorRule('url(file:///foo.png)');   //  IE compatibility 

The use of file URLs in all these test cases is curious. If the test could use
any arbitrary URL it seems a bit strange to chose a file URL since they are
special in so many ways.

> LayoutTests/fast/css/cursor-parsing.html:58
> +testInvalidCursorRule('nonexistant');

The word nonexistent is misspelled here.