[webkit-reviews] review granted: [Bug 17258] SVG uses erroneous cursor implementation : [Attachment 19074] Updated patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Nikolas Zimmermann
<zimmermann at kde.org>'s request for review:
Bug 17258: SVG uses erroneous cursor implementation
http://bugs.webkit.org/show_bug.cgi?id=17258

Attachment 19074: Updated patch
http://bugs.webkit.org/attachment.cgi?id=19074&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
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.