[webkit-reviews] review denied: [Bug 99530] cssText for cursor property doesn't include hotspot : [Attachment 169070] Patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied 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 169070: Patch
https://bugs.webkit.org/attachment.cgi?id=169070&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=169070&action=review


Good catch, and a nice patch!

> Source/WebCore/ChangeLog:14
> +	   (WebCore):

This line is useless. Please edit generated part to be human readable when
prepare-ChangeLog gets it wrong.

> Source/WebCore/css/CSSCursorImageValue.cpp:87
> +    if (m_hotSpot.x() >= 0) {

Why are you only checking x coordinate? A hot spot like "0 10" would not be
printed.

I think that you need "m_hotSpot.x() || m_hotSpot.y()" here. Please also add
tests for "0 10", "10 0", and "0 0" (it would be interesting to check what
other browsers do with the latter).

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

When overriding a virtual function, we keep "virtual" for clarity, and also now
append OVERRIDE:

    virtual String customCssText() const OVERRIDE;

> LayoutTests/fast/css/cursor-parsing-expected.txt:22
> +PASS div.style.cssText is ""
> +PASS div.style.cssText is ""
> +PASS div.style.cssText is ""
> +PASS div.style.cssText is ""
> +PASS div.style.cssText is ""
> +PASS div.style.cssText is ""
> +PASS div.style.cssText is ""
> +PASS div.style.cssText is ""

It's acceptable to leave as is, but a better test would have human readable
results. E.g.:

PASS roundtrip("url(file:///foo.png) 12") is "".