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

[bugzilla-request-daemon at opendarwin.org]

Alexey Proskuryakov <ap at nypop.com> has denied Alexey Proskuryakov
<ap at nypop.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 8441: First attempt
http://bugzilla.opendarwin.org/attachment.cgi?id=8441&action=edit

------- Additional Comments from Alexey Proskuryakov <ap at nypop.com>
I think I have enough comments to make a first round of a review :)

+	 if (style->cursors())
+	     return new
CSSPrimitiveValue(style->cursors()->cursor_image->url(),
CSSPrimitiveValue::CSS_URI);

In Firefox, the computed style includes the whole list, not just one URI (I
tested with a bookmarklet from
<http://www.squarefree.com/bookmarklets/webdevel.html>).

+class CSSCursorImageValue : public CSSImageValue

We usually have one class per file.

+	 //  [<uri>,]* [ auto | crosshair | default | pointer | progress | move
| e-resize | ne-resize |
...
+	     valueList->next(); // comma
+	     value = valueList->next();

It's probably OK not to support CSS3 hotspots at first, but here, the parser
would just get confused by them, which isn't good at all. Also, is it possible
to move the parsing logic to CSSGrammar.y?

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

Nitpick: I think custom cursors were implemented in MSIE 6.

+struct CursorInfo {
+    ~CursorInfo();
+    void clearCursor();
+    CachedImage *cursor_image;
+#if SVG_SUPPORT
+    IntPoint cursor_hotspot;
+#endif
+    struct CursorInfo *next;
+};

Special-casing SVG doesn't seem necessary for dealing with the hotspot (even if
we don't support CSS3 hotspots, the parser can just pass {0,0}).

-      cursor_image( o.cursor_image ), font( o.font ),
+      cursor_info( o.cursor_info ), font( o.font ),

StyleInheritedData didn't own cursor_image, but it owns cursor_info, so just
copying a pointer isn't enough.

+void RenderStyle::addCursor( CachedImage *v )

Style violation - we don't use extra spaces in braces (same issue elsewhere).


All fallback cursors are being loaded and decoded here, even if only the first
one gets used. I am not sure if this can be considered acceptable, but it's
definitely not good.

Of course, this change needs extensive test cases.