[webkit-reviews] review denied: [Bug 6002] WebKit does not properly handle SVG <cursor> element : [Attachment 10468] Improved patch

[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 10468: Improved patch
http://bugzilla.opendarwin.org/attachment.cgi?id=10468&action=edit

------- Additional Comments from Eric Seidel <macdome at opendarwin.org>
A few things here.

I recently re-read the spec, and I realize now that the grammar requires one to
always specify a CSS cursor type, but that one can also optionally specify a
list of URIs before that cursor type.  One may not specify a list of cursor
types (which is unfortunate).

This however, leads itself to a nice design on our part.  RenderStyle::cursor()
can always hold the last CSS cursor type, and there can be a separate fallback
list (RenderStyle::cursors() in your patch) which is preferred over that cursor
type if available.

1. We can't to SVG <cursor> element lookup at parse time.  That breaks an js
insertion/modification/removal of <cursor> elements.

2. Might as well fix CSS3 hotspots while we're here... right now the code goes
out of its way to not support them. ;)

3. This should probably only be allowed during quirks mode:
+	     if (!value) { // no value after url list (MSIE 5 compatibility)

4.  This needs to use a setter:
+	     style->cursors() = parentStyle->cursors();

5.  I'm not sure storing a Vector of CSSCursorImageValues is a good idea. 
RenderStyles are really tight on space, and I think that Vector is at least 8
bytes in size (which is at least 4 bytes bigger than we need to be using here).
 See my patch for how I used a pointer to a singly-linked list instead.

I'm not sure it's a good idea to only do the image load at runtime.
+	     CachedImage* cimage =
cursors[i]->image(node->document()->docLoader());

This can be a const IntPoint&, although that's not critical:
+static NSCursor* createCustomCursor(Image* image, IntPoint hotspot)

So this is going to need at least one more round.