Bug 99530

Created attachment 169070 [details] Patch

Comment on attachment 169070 [details] Patch 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 "".

Created attachment 169264 [details] Patch

Comment on attachment 169264 [details] Patch Thanks Alexey! Updated, please take another look. >> Source/WebCore/ChangeLog:14 >> + (WebCore): > > This line is useless. Please edit generated part to be human readable when prepare-ChangeLog gets it wrong. Done >> 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). Note this is >=, so 0 is valid, but (-1,-1) (our sentinel for no hotspot) isn't. In platform/Cursor.cpp we ignore any hotspot that's outside the bounds of the image. (0,0) is actually a reasonable and presumably common hotspot. I've added comments and tests to make this more clear. Anything else I can do? >> 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; This function is actually non-virtual. >> LayoutTests/fast/css/cursor-parsing-expected.txt:22 >> +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 "". Agreed - updated.

Ping, Alexey - how does this look now?

Comment on attachment 169264 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=169264&action=review > Source/WebCore/css/CSSCursorImageValue.cpp:89 > + if (m_hotSpot.x() >= 0 && m_hotSpot.y() >= 0) { Yep, now the check is fine. > LayoutTests/fast/css/cursor-parsing-expected.txt:29 > +PASS roundtripCssRule("cursor: url(file:///foo.png) -2 3;") is "cursor: url(file:///foo.png);" Checking (3 -2) and (-2, -3) (I mean only the value signs here) is actually a good idea to make the test set full.

Created attachment 169950 [details] Patch

(In reply to comment #6) > (From update of attachment 169264 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=169264&action=review > > > Source/WebCore/css/CSSCursorImageValue.cpp:89 > > + if (m_hotSpot.x() >= 0 && m_hotSpot.y() >= 0) { > > Yep, now the check is fine. > > > LayoutTests/fast/css/cursor-parsing-expected.txt:29 > > +PASS roundtripCssRule("cursor: url(file:///foo.png) -2 3;") is "cursor: url(file:///foo.png);" > > Checking (3 -2) and (-2, -3) (I mean only the value signs here) is actually a good idea to make the test set full. Thanks, done.

Comment on attachment 169950 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=169950&action=review > Source/WebCore/css/CSSCursorImageValue.cpp:91 > + result.append(String::number(m_hotSpot.x())); Should use appendNumber, not append(String::number()).

Created attachment 169955 [details] tweak based on cr feedback from darin@

(In reply to comment #9) > (From update of attachment 169950 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=169950&action=review > > > Source/WebCore/css/CSSCursorImageValue.cpp:91 > > + result.append(String::number(m_hotSpot.x())); > > Should use appendNumber, not append(String::number()). done, thanks!

Comment on attachment 169955 [details] tweak based on cr feedback from darin@ 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.

(In reply to comment #12) > (From update of attachment 169955 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=169955&action=review > > Patch is OK, but there is a little room for improvement. Thanks! > > 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? No, as far as I can see there's nothing in the specification that talks about what should happen when these values are invalid. I don't know what the original thinking was - it looks like this code has been around for awhile. > 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. Yes, that sounds better to me too. I was shying away from actually changing parser behavior in this CL since that could potentially break sites (if I recall, the hotspot check downstream will ignore any cursor with an invalid hotspot, rather than just ignore the hotspot as is done with the negative check in the parser). How about a file a separate bug for this? To me changing the parser behavior is really a separate issue from fixing the serialization of the existing parser behavior. > > 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. This isn't a virtual override - just a plain overload. CSSValue::cssText calls it directly, so it must be public. We use this pattern in a bunch of other places, eg. CSSImageSetValue, WebKitCSSTransformValue, etc. > > 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? I agree this was odd - my intention here was just to document the existing parsing behavior. Good question about other browsers - I just did a quick test (http://jsfiddle.net/rbyers/AYHmy/): Firefox 16.0.1: preserves and serializes arbitrary hotspot values as you suggest (but seems to have a bug on Windows where the hot spot isn't actually applied) IE 10: doesn't support hot-spot being specified in CSS so not relevant > > 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. Agreed. These need to be absolute URLs in order for the test results to be stable (since relative urls are converted to absolute). I assumed we didn't want URLs in layout tests that would trigger network activity, hence file. I could post-process the text to trim the absolute URLs, but parsing this string at all in the test seems error-prone (potential to mask certain types of bugs where the string doesn't match the format I expect). I've added a comment explaining this. Is there something better I should be doing? > > LayoutTests/fast/css/cursor-parsing.html:58 > > +testInvalidCursorRule('nonexistant'); > > The word nonexistent is misspelled here. Thanks, done.

Created attachment 169968 [details] a couple more tweaks based on cr feedback from darin@

(In reply to comment #13) > (In reply to comment #12) > > (From update of attachment 169955 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=169955&action=review > > > > Patch is OK, but there is a little room for improvement. > > Thanks! > > > > 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? > > No, as far as I can see there's nothing in the specification that talks about what should happen when these values are invalid. I don't know what the original thinking was - it looks like this code has been around for awhile. > > > 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. > > Yes, that sounds better to me too. I was shying away from actually changing parser behavior in this CL since that could potentially break sites (if I recall, the hotspot check downstream will ignore any cursor with an invalid hotspot, rather than just ignore the hotspot as is done with the negative check in the parser). How about a file a separate bug for this? To me changing the parser behavior is really a separate issue from fixing the serialization of the existing parser behavior. > > > > 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. > > This isn't a virtual override - just a plain overload. CSSValue::cssText calls it directly, so it must be public. We use this pattern in a bunch of other places, eg. CSSImageSetValue, WebKitCSSTransformValue, etc. > > > > 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? > > I agree this was odd - my intention here was just to document the existing parsing behavior. Good question about other browsers - I just did a quick test (http://jsfiddle.net/rbyers/AYHmy/): > > Firefox 16.0.1: preserves and serializes arbitrary hotspot values as you suggest (but seems to have a bug on Windows where the hot spot isn't actually applied) By the way, I also tested Firefox 16.0.1 on Linux. It works perfectly there. In particular: any out-of-range values (including negative) are preserved in serialization, but clamped to the image boundaries. And I was wrong about the behavior for out-of-range hotspot values - we either reset to (0,0) (on ports using USE(LAZY_NATIVE_CURSOR) - see determineHotSpot in Cursor.cpp) or leave it up to the port (chromium clamps to the image boundaries in WebCursor::ClampHotspot). I was thinking of the code in selectCursor that drops any large cursors (>128 width or height). So it seems low risk to preserve negative values and rely on this existing out-of-range code. It's a non-trivial amount of plumbing though - today we pass just IntPtrs around across a few places (CursorData, CSSCursorImageValue and Cursor including each port-specific implementation), I'd have to add a bool to all those places. Would you like me to change that in this CL, or just file a separate lower-priority bug? > IE 10: doesn't support hot-spot being specified in CSS so not relevant > > > > 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. > > Agreed. These need to be absolute URLs in order for the test results to be stable (since relative urls are converted to absolute). I assumed we didn't want URLs in layout tests that would trigger network activity, hence file. I could post-process the text to trim the absolute URLs, but parsing this string at all in the test seems error-prone (potential to mask certain types of bugs where the string doesn't match the format I expect). I've added a comment explaining this. Is there something better I should be doing? > > > > LayoutTests/fast/css/cursor-parsing.html:58 > > > +testInvalidCursorRule('nonexistant'); > > > > The word nonexistent is misspelled here. > > Thanks, done.

Comment on attachment 169968 [details] a couple more tweaks based on cr feedback from darin@ Attachment 169968 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14486684 New failing tests: fast/css/cursor-parsing.html

Comment on attachment 169968 [details] a couple more tweaks based on cr feedback from darin@ Attachment 169968 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14484713 New failing tests: fast/css/cursor-parsing.html

(In reply to comment #15) > To me changing the parser behavior is really a separate issue from fixing the serialization of the existing parser behavior. This serialization is making what was previously hidden internal behavior have a visible effect. Before, I believe the parser behavior was not detectable, just a hidden implementation detail. That’s why I’m asking you to consider it now. > So it seems low risk to preserve negative values and rely on this existing out-of-range code. It's a non-trivial amount of plumbing though - today we pass just IntPtrs around across a few places (CursorData, CSSCursorImageValue and Cursor including each port-specific implementation), I'd have to add a bool to all those places. Would you like me to change that in this CL, or just file a separate lower-priority bug? While I think it’s fine for you to work on this in another bug report, I think you’re mistaken that we’d have to change things in so many places. I think that only CSSCursorImageValue needs the boolean notion of hot spot specified. For all the other sites we could pass in -1,-1 when the hot spot is unspecified, taking advantage of the existing code path and acting the way the parser does now. Later might want classes like CursorData and Cursor to have a concept of “no hot spot” for clarity and perhaps so some obscure edge cases worked better, but I think that’s a different issue and need not have as high a priority as being able to round trip through CSS. I think that only CSSCursorImageValue and the cursor parsing section of CSSParser.cpp would need to change.

Comment on attachment 169968 [details] a couple more tweaks based on cr feedback from darin@ OK, but please deal with those test failures.

Created attachment 170148 [details] round-trip parsing of negative hotspot values

(In reply to comment #18) > (In reply to comment #15) > > To me changing the parser behavior is really a separate issue from fixing the serialization of the existing parser behavior. > > This serialization is making what was previously hidden internal behavior have a visible effect. Before, I believe the parser behavior was not detectable, just a hidden implementation detail. That’s why I’m asking you to consider it now. Good point, I agree. > > So it seems low risk to preserve negative values and rely on this existing out-of-range code. It's a non-trivial amount of plumbing though - today we pass just IntPtrs around across a few places (CursorData, CSSCursorImageValue and Cursor including each port-specific implementation), I'd have to add a bool to all those places. Would you like me to change that in this CL, or just file a separate lower-priority bug? > > While I think it’s fine for you to work on this in another bug report, I think you’re mistaken that we’d have to change things in so many places. > > I think that only CSSCursorImageValue needs the boolean notion of hot spot specified. For all the other sites we could pass in -1,-1 when the hot spot is unspecified, taking advantage of the existing code path and acting the way the parser does now. > > Later might want classes like CursorData and Cursor to have a concept of “no hot spot” for clarity and perhaps so some obscure edge cases worked better, but I think that’s a different issue and need not have as high a priority as being able to round trip through CSS. > > I think that only CSSCursorImageValue and the cursor parsing section of CSSParser.cpp would need to change. Yes, thank you. I came to the same conclusion last night as well. Plumbing the has-hotspot flag all the way down is perhaps the right way to fix bug 100059, but that's port-specific.

Ping - is this still R+ after the last tweak?

Created attachment 171487 [details] Mark as reviewed by darin, last patch was only making a minor change he suggested after he r+ an earlier version

Comment on attachment 171487 [details] Mark as reviewed by darin, last patch was only making a minor change he suggested after he r+ an earlier version Clearing flags on attachment: 171487 Committed r132966: <http://trac.webkit.org/changeset/132966>

All reviewed patches have been landed. Closing bug.