Bug 17258 – SVG uses erroneous cursor implementation

• Component: SVG
• Status: RESOLVED FIXED

[Nikolas Zimmermann]

SVG cursors are not well-integrated within the CSS(3) cursor support in WebCore. SVGCursorElement duplicates CSSCursorImageValue functionality and inherits from CachedResourceClient itself, handling remote-image acquisation on it's own.

RenderStyle's CursorData class holds "IntPoint hotSpot", "CachedImage* cursorImage", but SVG doesn't fill that structure with data, but instead stores "String cursorFragmentId" - a reference to a <cursor> element in the DOM. EventHandler (yes, page/EventHandler.cpp) uses getElementById to find the corresponding SVGCursorElement, to access the CachedImage pointer, stored in the SVGCursorElement.
Because of SVG's special logic, it involves lots of quirks all over WebCore.

As you can see the logic is backwards, and all of it should be fixed by integrating within the CSSCursorImageValue concept (-> save size in RenderStyle). Also fix all dynamic attribute updates of all <cursor> propreties (x / y / xlink:href). Needs a manual testcase: manual-test/svg-cursor-changes.svg - as DRT can not be used to test cursor image changes.

[Nikolas Zimmermann]

Created attachment 19018 [details]
Initial patch

No regressions.

[Eric Seidel (no email)]

Comment on attachment 19018 [details]
Initial patch

It looks like:
HashSet<RefPtr<SVGElement> > m_clients;
could easily cause cycles in the DOM, thus leaking whole subtrees.  I suggest making that a hashset of weak pointer, and then making it the client's responsibility to clear themselves when they go away (I also wonder how that would work with style sharing?  looks like you disable it.)

Otherwise the patch looks OK.

[Nikolas Zimmermann]

Created attachment 19074 [details]
Updated patch

Incorporated Erics comments, now properly deregistering cursor clients.

[Darin Adler]

Comment on attachment 19074 [details]
Updated patch

In general, I prefer the early exit style to the "nested if" style.

+    return !kurl.ref().isEmpty();

Should use the more efficient hasRef() function.

+        int x = static_cast<int>(ceilf(cursorElement->x().value()));

Why "ceil" rather than "round"? What's the general philosophy on this? I'd like to see us use a helper function to both ceil and convert to an integer rather than sprinkling these combinations of floating point and integer code throughout. How does this handle values that are out of the integer range?

+        KURL referencedUrl(element->document()->baseURL(), cursorElement->href().deprecatedString());

Why not use the completeURL function instead of rolling your own? Is there a reason you can't use it? Can we address that?

I think the spelling "Url" is ugly. We normally use "url" or "URL" in WebKit code.

+    if (!element || !element->isSVGElement() || !element->document())
+        return false;

There's no need to check !element->document(). That pointer can never be 0 for an Element.

+    SVGElement* m_client;

I think that "client" is a strange name for this. Could you explain what the purpose of the field is? Maybe we can come up with a better name.

-class CSSImageValue : public CSSPrimitiveValue, public CachedResourceClient {
+class CSSImageValue : public CSSPrimitiveValue,
+                      public CachedResourceClient {

I don't think this formatting change is an improvement. The line wasn't all that long.

 protected:
+    CachedImage* image(DocLoader*, const String& url);
+
     CachedImage* m_image;
     bool m_accessedImage;

Can one or more of m_image and m_accessedImage be private instead of protected?

+                    if (image->updateIfNeeded(m_element)) // Elements with SVG cursors are not allowed to share style.

This is a confusing comment. How does the boolean result of updateIfNeeded relate to SVG cursors? If that function returns true for SVG cursors, that's quite unclear.

+        HashSet<SVGElement*>::const_iterator it = m_clients.begin();
+        const HashSet<SVGElement*>::const_iterator end = m_clients.end();
+
+        for (; it != end; ++it)
+            (*it)->setChanged();

Can setChanged trigger code that might change the m_clients set? If so, then this code needs to account for that.

I think the for statement reads better if you initialize the iterator inside the loop. If you use a typedef for the iterator type you can prevent ifr from being such a long line that it's unreadable. I'm not a big fan of the const for variables like "end". Sure, it's not modified, but we don't set "const" on local variables that we don't modify. What makes this one so special?

I'm going to say review+, but you should consider making some of the changes I requested.

[Nikolas Zimmermann]

(In reply to comment #4)
> (From update of attachment 19074 [details] [edit])
> In general, I prefer the early exit style to the "nested if" style.
Same here, except if there's one condition to check, then I prefer the non-early exit style :-)
 
> +    return !kurl.ref().isEmpty();
> 
> Should use the more efficient hasRef() function.
Ah much better. Fixed.
 
> +        int x = static_cast<int>(ceilf(cursorElement->x().value()));
> 
> Why "ceil" rather than "round"? What's the general philosophy on this? I'd like
> to see us use a helper function to both ceil and convert to an integer rather
> than sprinkling these combinations of floating point and integer code
> throughout. How does this handle values that are out of the integer range?
Yeah, it's a very problematic case throughtout the svg/ code. Fixed to use round, makes more sense this way.
 
> +        KURL referencedUrl(element->document()->baseURL(),
> cursorElement->href().deprecatedString());
> 
> Why not use the completeURL function instead of rolling your own? Is there a
> reason you can't use it? Can we address that?
I'm not sure about that. I've just reused the existing logic from CSSParser, for the CSS_PROP_CURSOR case:

list->append(new CSSCursorImageValue(KURL(styleElement->baseURL().deprecatedString(), uri.deprecatedString()).string(), hotspot, styleElement));

Though I like the completeURL suggestion - didn't even know it existed. Fixed to use it.

> 
> I think the spelling "Url" is ugly. We normally use "url" or "URL" in WebKit
> code.
Agreed, variable is gone now anyway.

> 
> +    if (!element || !element->isSVGElement() || !element->document())
> +        return false;
> 
> There's no need to check !element->document(). That pointer can never be 0 for
> an Element.
Correct, fixed.
 
> +    SVGElement* m_client;
> 
> I think that "client" is a strange name for this. Could you explain what the
> purpose of the field is? Maybe we can come up with a better name.
Yeah, it's strange indeed: using "SVGElement* m_referencedElement" now, as the CSSCursorImageValue now holds a pointer to the SVG <cursor> element that the cursor CSS rule referenced.

> 
> -class CSSImageValue : public CSSPrimitiveValue, public CachedResourceClient {
> +class CSSImageValue : public CSSPrimitiveValue,
> +                      public CachedResourceClient {
> 
> I don't think this formatting change is an improvement. The line wasn't all
> that long.
Oh, I thought we'd use a new line per inherited class (it's done like this in svg/).

> 
>  protected:
> +    CachedImage* image(DocLoader*, const String& url);
> +
>      CachedImage* m_image;
>      bool m_accessedImage;
> 
> Can one or more of m_image and m_accessedImage be private instead of protected?
Both are needed for CSSCursorImageValue, though I could add getters/setters if you'd prefer that. Though I'm fine with the current concept..

> 
> +                    if (image->updateIfNeeded(m_element)) // Elements with SVG
> cursors are not allowed to share style.
> 
> This is a confusing comment. How does the boolean result of updateIfNeeded
> relate to SVG cursors? If that function returns true for SVG cursors, that's
> quite unclear.
Okay, I've named it "updateIfSVGCursorIsUsed".
 
> +        HashSet<SVGElement*>::const_iterator it = m_clients.begin();
> +        const HashSet<SVGElement*>::const_iterator end = m_clients.end();
> +
> +        for (; it != end; ++it)
> +            (*it)->setChanged();
> 
> Can setChanged trigger code that might change the m_clients set? If so, then
> this code needs to account for that.
Yes, but that would be delayed. It's guaruanteed that m_clients can't be mutated, by a setChanged() call - so it's safe as is.

> I think the for statement reads better if you initialize the iterator inside
> the loop. If you use a typedef for the iterator type you can prevent ifr from
> being such a long line that it's unreadable. I'm not a big fan of the const for
> variables like "end". Sure, it's not modified, but we don't set "const" on
> local variables that we don't modify. What makes this one so special?
Okay, fine with me - I'm just used to this pattern. Changed.
 
> I'm going to say review+, but you should consider making some of the changes I
> requested.
All requested changes are made. Thanks for the review! Landing tonite...

[Nikolas Zimmermann]

Landed in r30208.