[Webkit-unassigned] [Bug 6002] WebKit does not properly handle SVG <cursor> element

[bugzilla-daemon at opendarwin.org]

http://bugzilla.opendarwin.org/show_bug.cgi?id=6002


ap at nypop.com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #8441|review?                     |review-
               Flag|                            |




------- Comment #5 from ap at nypop.com  2006-05-21 05:00 PDT -------
(From update of attachment 8441)
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.


-- 
Configure bugmail: http://bugzilla.opendarwin.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.