[webkit-reviews] review denied: [Bug 6002] WebKit does not properly handle SVG <cursor> element : [Attachment 10491] Another attempt

[bugzilla-request-daemon at opendarwin.org]

Darin Adler <darin at apple.com> has denied Darin Adler <darin at apple.com>'s
request for review:
Bug 6002: WebKit does not properly handle SVG <cursor> element
http://bugzilla.opendarwin.org/show_bug.cgi?id=6002

Attachment 10491: Another attempt
http://bugzilla.opendarwin.org/attachment.cgi?id=10491&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
Some nitpicks:

+	     for (unsigned i = 0;i < cursors->size();++i)

Missing spaces after the semicolons.

+	 if (list) {
+	     list->append(value);
+	     return list;
+	 }
+	 ASSERT(value);

The ASSERT(value) should go before the if.

+		     if (primitiveValue->getStringValue().find("#") == 0)

You fixed one of these, but not the other!

+	 Cursor(Image*, IntPoint hotspot);

Should be const IntPoint& instead of IntPoint, and in fact some of the
implementations, like the CursorQt.cpp ones, are like that.

+	 inherited.access()->cursorData = new CursorList();

It's slightly nicer to just do "new CursorList" in a case like this, I think.

+class CursorList : public Shared<CursorList>
+{

Brace should be on the same line as the class declaration.

+    void setCursors(CursorList*);

This should take a PassRefPtr<CursorList> since it takes ownership of the
passed-in object.

+    CachedImage* cursorImage; // weak pointer, the CSSValueImage takes care of
deleting cursorImage

I'm a little concerned about this. What's the guarantee that the RenderStyle
won't outlast the CSSValueImage? Is there any other RenderStyle code that uses
this ownership model?

I'm going to say review- because of the find() and because of my previous
comment about what I called the "CSS_VAL_AUTO case".