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

bugzilla-request-daemon at opendarwin.org bugzilla-request-daemon at opendarwin.org
Sun Sep 10 19:56:35 PDT 2006

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

Attachment 10491: Another attempt

------- 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".

More information about the webkit-reviews mailing list