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

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

------- Additional Comments from Darin Adler <darin at apple.com>
+    NSPoint hotSpotPoint = {hotspot.x(), hotspot.y()};
+    return [[NSCursor alloc] initWithImage:img hotSpot:hotSpotPoint];

You don't need the local variable. IntPoint objects already know how to convert
themselves into NSPoint objects if you pass them to an expression that expects
an NSPoint.

+    void addCursor(PassRefPtr<CSSCursorImageValue> v);

This would read better without the "v" -- no need to name the parameter if
you're not using it here and if the name isn't helpful for documentation.

+    if (style && style->cursors()) {
+	 Vector<RefPtr<CSSCursorImageValue> > cursors = style->cursors();
+	 for (unsigned i = 0;i < cursors.size();++i) {
+	     CachedImage* cimage =
cursors[i].get()->image(node->document()->docLoader());
+	     if (!cimage)
+		  continue;
+	     if (cimage->image()->isNull())
+		 break;
+	     else if (!cimage->isErrorImage())
+		 return Cursor(cimage->image(), cursors[i].get()->hotspot());
+	 };
+    }

There's a spurious semicolon here.

Also, formatting trouble with an extra space before "continue" and missing
spaces after the semicolons in the if statement.

There's no need for an else after  the break.

I don't understand why a null CachedImage means continue, but an image that's
"isNull" means break, and an image that's an error image means continue. That's
confusing.

cursors[i].get()->image can just be cursors[i]->image -- you only need the
get() if you're passing to something that takes a raw pointer. You don't need
it just to use the pointer. Same for cursors[i].get()->hotspot().

Why use the term "info" in cursorInfo. Maybe cursors is the best name for the
property.

Could change cursors so it returns a const reference to the Vector. That way
you won't have to copy the vector just to access the cursors.

Do we really need a reference-counted class for the cursors? I think it might
be better to have the vector hold a struct with a RefPtr<CSSImageValue> plus an
IntPoint for the hot spot. That struct would not need to be a while class and
have a source file to itself. What do you think?

+	     for (int i = 0; i < len; i++)
+	     {

Braces should go on the same line as the for statement.

+		 if (!item->isPrimitiveValue()) continue;

If statements should not be packed into one line like this.

-    case CSS_PROP_RESIZE: // none | both | horizontal | vertical | inherit
-	 if (id == CSS_VAL_NONE || id == CSS_VAL_BOTH || id ==
CSS_VAL_HORIZONTAL || id == CSS_VAL_VERTICAL)
-	     valid_primitive = true;
-	 break;

Please don't roll out our new resize property!

+	     for (unsigned i = 0;i < cursors.size();++i) {
+		 list->append(new
CSSPrimitiveValue(cursors[i].get()->image(node->document()->docLoader())->url()
, CSSPrimitiveValue::CSS_URI));
+	     }

We usually don't put braces around single line for statements either (like
single-line if statements).

No test cases attached to the patch.

Looking good!