[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


darin at apple.com changed:

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




------- Comment #21 from darin at apple.com  2006-09-10 12:58 PDT -------
(From update of attachment 10487)
+            if (uri.find("#") == 0) {

This is an inefficient way to figure out if the first character of a string is
a "#" that needlessly searches the entire string for "#" characters when there
are none. An efficient way would be to use uri[0] == '#' or
uri.startsWith("#").

+        if (!strict && value->id == CSS_VAL_HAND) { // MSIE 5 compatibility :/
+            id = CSS_VAL_POINTER;
+            valid_primitive = true;
+        } else if (value->id >= CSS_VAL_AUTO && value->id <= CSS_VAL_HELP)

How does the CSS_VAL_AUTO case here work? I don't see it setting id, and it
looks to me like code below in the if (valid_primitive) block isn't going to
work if id is not set.

I would fine the code easier to read without this else:

+        else
+            delete list;
+        if (!strict && value->id == CSS_VAL_HAND) { // MSIE 5 compatibility :/

Both the "delete list" and the "if (!strict" line run only if the if above
returns false, but it's hard to figure that out because of the else.

The RenderPath.cpp seems unrelated to cursors, so should be landed separately.

+                Element* e =
node->document()->getElementById((*cursors)[i].cursorFragmentId.deprecatedString());

What's the .deprecatedString() for here? I think that will just cause it to
copy the string an extra time or two!

Looks like there's a logic error. If SVG_SUPPORT is on and the tag does not
have the name "cursorTag", then cimage can be null, and we'll crash on the:

+            if (cimage->image()->isNull())

Why "WebCore::" in WebCore::SVGNames::cursorTag? The usual idiom is to put
"using namespace SVGNames" at the top of the file and then just say
"cursorTag", but in any case there's no reason to say WebCore:: here.

Maybe we could overload the Cursor constructor instead of adding an optional
parameter. That's more efficient when not passing the parameter, and eliminates
the need to include the IntPoint.h header in Cursor.h. Or maybe the hotSpot
parameter should not be optional -- I think that would be good, and in that
case there's no need to include IntPoint.h either (a forward declaration will
suffice).

+    RefPtr<CursorList> cursor_data;

I suggest cursorData instead.

+    Vector<CursorData> m_impl;

How about making that member private?

+    const CursorData &operator[](int i) const {
+        return m_impl[i];
+    }
+    const CursorData &operator[](int i) {
+        return m_impl[i];
+    }

If both overloads return a "const CursorData&" then we don't need overloading
(can just keep the const version). Also, the "&" should be next to
"CursorData", not "operator". I think we could call it m_vector instead of
m_impl.


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