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

[bugzilla-request-daemon at opendarwin.org]

Darin Adler <darin at apple.com> has denied Darin Adler <darin at apple.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 10487: Another go
http://bugzilla.opendarwin.org/attachment.cgi?id=10487&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
+	     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.deprecatedStrin
g());

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.