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

[bugzilla-request-daemon at opendarwin.org]

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

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

------- Additional Comments from Eric Seidel <macdome at opendarwin.org>
Storing a Vector on RenderStyle is not a good idea.  That uses at least 4 more
bytes than it needs to.  Storing a pointer to a vector would be OK, or using a
list, both would work (personally I'm partial to the list).

Creating a CSSValueList unconditionally, and then deleting it if not needed is
wasteful.

+	 CSSValueList* list = new CSSValueList;

A better method is to have an if in your while statement which checks
if (!list)
    list = new CSSValueList;

Copy-paste error, you set the X value of the hotspot twice:
+		     hotspot.setX(value->fValue);

Also, you don't properly error out when it's
cursor: url("foo.png") 1, auto;
a single number should be an error.

I still would argue that a missing last value should only be allowed in quirks
mode:

+	     if (!value) { // no value after url list (MSIE 5 compatibility)

+			
style->addCursor(primitiveValue->getStringValue().deprecatedString().mid(1));

This doesn't need to use a deprecatedString.  .substring(1) will do the same as
.mid(1)

Otherwise it looks fine.
Need one more round on this.