Fixed cursor hotspot SVG override and made CSSCursorImageValue constant

Fixed cursor hotspot SVG override and made CSSCursorImageValue constant

Move the hotspot logic out of CSSCursorImageValue to keep it as a
constant object (except for the image cache). This is prework to make
all StyleBuilderFunctions take a const CSSValue&.

This also fixes a bug where the hotspot of an SVG cursor element was
being overridden by the element's specified hotspot even when the
hotspot was specified in CSS. Now the hotspot is only overridden if it
is not specified in the CSS, but is specified in the SVG.

Note: When getting the computed style for SVG cursors, Firefox always
returns the URL to the SVG element, and doesn't support intrinsic
hotspot definition.

Relevant Spec: https://drafts.csswg.org/css-ui/#cursor

BUG=526586
Committed: https://crrev.com/eae6561b90f59fb5bc26c0817c511aa5e667c428
Cr-Commit-Position: refs/heads/master@{#397061}

[sashab, 2016-05-27 07:31:46]

sashab@chromium.org changed reviewers:
+ timloh@chromium.org

[sashab, 2016-05-27 07:31:46]

Test wip

[Timothy Loh, 2016-05-30 01:38:35]

https://codereview.chromium.org/2018923002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/svg/css/cursor-hotspot-override-from-css.svg
(right):

https://codereview.chromium.org/2018923002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/svg/css/cursor-hotspot-override-from-css.svg:33:
if (getComputedStyle(cursorWithXYOnSVGOnly).cursor ==
"url(\"file:///icon1.ico\") 10 20, auto")
probably worthwhile to have a test or two where you check the inline style (i.e.
element.style.cursor)

https://codereview.chromium.org/2018923002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/CSSCursorImageValue.h (right):

https://codereview.chromium.org/2018923002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/CSSCursorImageValue.h:65: // True if the URL
has a fragment identifier (in which case it probably
isn't the function name descriptive enough already? :S

https://codereview.chromium.org/2018923002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/resolver/StyleBuilderCustom.cpp (right):

https://codereview.chromium.org/2018923002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/resolver/StyleBuilderCustom.cpp:206: //
Override the hot spot if it wasn't specified in the CSS but is specified in the
SVG.
It's not really overriding anything

https://codereview.chromium.org/2018923002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/resolver/StyleBuilderCustom.cpp:216: 
don't we need to set hotSpotSpecified to true somewhere here? at least the
current code does that

https://codereview.chromium.org/2018923002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/resolver/StyleBuilderCustom.cpp:217:
image->clearCachedImageIfNotURL(element->document().completeURL(cursorElement->href()->currentValue()->value()));
probably clearer if this moved to the top of this if statement, and if outer if
was pulled out to here so we call clearCachedImage conditionally.

[sashab, 2016-05-31 04:22:37]

https://codereview.chromium.org/2018923002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/svg/css/cursor-hotspot-override-from-css.svg
(right):

https://codereview.chromium.org/2018923002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/svg/css/cursor-hotspot-override-from-css.svg:33:
if (getComputedStyle(cursorWithXYOnSVGOnly).cursor ==
"url(\"file:///icon1.ico\") 10 20, auto")
On 2016/05/30 at 01:38:34, Timothy Loh wrote:
> probably worthwhile to have a test or two where you check the inline style
(i.e. element.style.cursor)

Oops lol I meant to be testing that the whole time. Why was I looking at
computed style lol... Probably don't need to test computed style

Oh well made 8 tests lol

https://codereview.chromium.org/2018923002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/CSSCursorImageValue.h (right):

https://codereview.chromium.org/2018923002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/CSSCursorImageValue.h:65: // True if the URL
has a fragment identifier (in which case it probably
On 2016/05/30 at 01:38:34, Timothy Loh wrote:
> isn't the function name descriptive enough already? :S

Done

https://codereview.chromium.org/2018923002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/resolver/StyleBuilderCustom.cpp (right):

https://codereview.chromium.org/2018923002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/resolver/StyleBuilderCustom.cpp:206: //
Override the hot spot if it wasn't specified in the CSS but is specified in the
SVG.
On 2016/05/30 at 01:38:34, Timothy Loh wrote:
> It's not really overriding anything

Changed to 'set'

https://codereview.chromium.org/2018923002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/resolver/StyleBuilderCustom.cpp:216: 
On 2016/05/30 at 01:38:34, Timothy Loh wrote:
> don't we need to set hotSpotSpecified to true somewhere here? at least the
current code does that

Hhhhmmm maybe that's why my tests weren't working... lol

https://codereview.chromium.org/2018923002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/resolver/StyleBuilderCustom.cpp:217:
image->clearCachedImageIfNotURL(element->document().completeURL(cursorElement->href()->currentValue()->value()));
On 2016/05/30 at 01:38:34, Timothy Loh wrote:
> probably clearer if this moved to the top of this if statement, and if outer
if was pulled out to here so we call clearCachedImage conditionally.

Yup agreed done

[sashab, 2016-05-31 04:53:28]

Changed test to use harness :)

[Timothy Loh, 2016-05-31 05:04:35]

I guess we should land the other patch first and rebase this one on top of it :)

https://codereview.chromium.org/2018923002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/svg/css/cursor-hotspot-override-from-css.html
(right):

https://codereview.chromium.org/2018923002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/svg/css/cursor-hotspot-override-from-css.html:16:
// TODO(sashab): There is currently no way to check whether the correct
All the expectations in the test file should be the correct values. In some
cases this means you'll have to check in a failing expectation. BTW could you
check and mention how FF handles these cases?

https://codereview.chromium.org/2018923002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/CSSCursorImageValue.cpp (left):

https://codereview.chromium.org/2018923002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/CSSCursorImageValue.cpp:160: String
CSSCursorImageValue::cachedImageURL()
would be nicer if you left the functions in the same place (order here doesn't
really need to match the header file)

https://codereview.chromium.org/2018923002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/resolver/StyleBuilderCustom.cpp (right):

https://codereview.chromium.org/2018923002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/resolver/StyleBuilderCustom.cpp:217: if
(image->cachedImageURL() !=
element->document().completeURL(cursorElement->href()->currentValue()->value()))
I think you missed when I commented earlier, this if statement should move
upwards to before we calculate the hotspot for clarify :)

[sashab, 2016-05-31 05:24:12]

This patch keeps getting better and better :)

https://codereview.chromium.org/2018923002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/CSSCursorImageValue.cpp (left):

https://codereview.chromium.org/2018923002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/CSSCursorImageValue.cpp:160: String
CSSCursorImageValue::cachedImageURL()
On 2016/05/31 at 05:04:35, Timothy Loh wrote:
> would be nicer if you left the functions in the same place (order here doesn't
really need to match the header file)

Done, also renamed clearCachedImage() back to clearImageResource() which is what
it was called before the change :)

https://codereview.chromium.org/2018923002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/resolver/StyleBuilderCustom.cpp (right):

https://codereview.chromium.org/2018923002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/resolver/StyleBuilderCustom.cpp:217: if
(image->cachedImageURL() !=
element->document().completeURL(cursorElement->href()->currentValue()->value()))
On 2016/05/31 at 05:04:35, Timothy Loh wrote:
> I think you missed when I commented earlier, this if statement should move
upwards to before we calculate the hotspot for clarify :)

Oh yup cool, done

[sashab, 2016-05-31 05:40:00]

Description was changed from

==========
Fixed cursor hotspot SVG override and made CSSCursorImageValue constant

Move the hotspot logic out of CSSCursorImageValue to keep it as a
constant object (except for the image cache). This is prework to make
all StyleBuilderFunctions take a const CSSValue&.

This also fixes a bug where the hotspot of an SVG cursor element was
being overridden by the element's specified hotspot even when the
hotspot was specified in CSS. Now the hotspot is only overridden if it
is not specified in the CSS, but is specified in the SVG.

BUG=526586
==========

to

==========
Fixed cursor hotspot SVG override and made CSSCursorImageValue constant

Move the hotspot logic out of CSSCursorImageValue to keep it as a
constant object (except for the image cache). This is prework to make
all StyleBuilderFunctions take a const CSSValue&.

This also fixes a bug where the hotspot of an SVG cursor element was
being overridden by the element's specified hotspot even when the
hotspot was specified in CSS. Now the hotspot is only overridden if it
is not specified in the CSS, but is specified in the SVG.

Note: When getting the computed style for SVG cursors, Firefox always
returns the URL to the SVG element, and doesn't support intrinsic
hotspot definition.

Relevant Spec: https://drafts.csswg.org/css-ui/#cursor

BUG=526586
==========

[Timothy Loh, 2016-06-01 02:54:28]

lgtm

[sashab, 2016-06-01 04:53:56]

The CQ bit was checked by sashab@chromium.org

[commit-bot: I haz the power, 2016-06-01 04:54:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2018923002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2018923002/120001

[commit-bot: I haz the power, 2016-06-01 05:02:54]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-06-01 05:04:00]

Description was changed from

==========
Fixed cursor hotspot SVG override and made CSSCursorImageValue constant

Move the hotspot logic out of CSSCursorImageValue to keep it as a
constant object (except for the image cache). This is prework to make
all StyleBuilderFunctions take a const CSSValue&.

This also fixes a bug where the hotspot of an SVG cursor element was
being overridden by the element's specified hotspot even when the
hotspot was specified in CSS. Now the hotspot is only overridden if it
is not specified in the CSS, but is specified in the SVG.

Note: When getting the computed style for SVG cursors, Firefox always
returns the URL to the SVG element, and doesn't support intrinsic
hotspot definition.

Relevant Spec: https://drafts.csswg.org/css-ui/#cursor

BUG=526586
==========

to

==========
Fixed cursor hotspot SVG override and made CSSCursorImageValue constant

Move the hotspot logic out of CSSCursorImageValue to keep it as a
constant object (except for the image cache). This is prework to make
all StyleBuilderFunctions take a const CSSValue&.

This also fixes a bug where the hotspot of an SVG cursor element was
being overridden by the element's specified hotspot even when the
hotspot was specified in CSS. Now the hotspot is only overridden if it
is not specified in the CSS, but is specified in the SVG.

Note: When getting the computed style for SVG cursors, Firefox always
returns the URL to the SVG element, and doesn't support intrinsic
hotspot definition.

Relevant Spec: https://drafts.csswg.org/css-ui/#cursor

BUG=526586

Committed: https://crrev.com/eae6561b90f59fb5bc26c0817c511aa5e667c428
Cr-Commit-Position: refs/heads/master@{#397061}
==========

[commit-bot: I haz the power, 2016-06-01 05:04:01]

Patchset 7 (id:??) landed as
https://crrev.com/eae6561b90f59fb5bc26c0817c511aa5e667c428
Cr-Commit-Position: refs/heads/master@{#397061}