[webkit-reviews] review granted: [Bug 5689] add support for CSS "custom cursors" (cursor images) : [Attachment 4652] adds custom cursor support, fixes some image renderer issues

[bugzilla-request-daemon at opendarwin.org]

Eric Seidel <macdome at opendarwin.org> has granted Darin Adler
<darin at apple.com>'s request for review:
Bug 5689: add support for CSS "custom cursors" (cursor images)
http://bugzilla.opendarwin.org/show_bug.cgi?id=5689

Attachment 4652: adds custom cursor support, fixes some image renderer issues
http://bugzilla.opendarwin.org/attachment.cgi?id=4652&action=edit

------- Additional Comments from Eric Seidel <macdome at opendarwin.org>
Wow, what a great cleanup.

I will warn you, this change is great:
+	 if (style->cursorImage())
+	     return new CSSPrimitiveValueImpl(style->cursorImage()->url(),
CSSPrimitiveValue::CSS_URI);
+	 return new CSSPrimitiveValueImpl(CSS_VAL_AUTO + style->cursor() -
CURSOR_AUTO);

but I just had to go through and remove a bunch of those same assumptions from
KSVG2 (going the other way, during css parsing) when landing on top of our DOM.
 Best IMO would be to add some sort of ASSERT like:
ASSERT((style->cursor() > CSS_AUTO) && (style->cursor() < CURSOR_HELP))
Whenever such assumptions are made when converting back and forth from ints to
enums.

Should this:
+		 parsedValue = new
CSSImageValueImpl(DOMString(KURL(styleElement->baseURL().qstring(),
uri.qstring()).url()), styleElement);

be:
parsedValue = new
CSSImageValueImpl(styleElement.getDocument().completeURL(uri), styleElement);

instead?

Shouldn't this:

+	 if (isInherit) {
+	     style->setCursor(parentStyle->cursor());
+	     style->setCursorImage(parentStyle->cursorImage());
+	     return;
+	 }
+	 if (isInitial) {
+	     style->setCursor(RenderStyle::initialCursor());
+	     style->setCursorImage(0);
+	     return;
+	 }

just be:

HANDLE_INHERIT_AND_INITIAL(cursor, Cursor)
HANDLE_INHERIT_AND_INITIAL(cursorImage, CursorImage)

?  Seems easier to add RenderStyle::initialCursorImage() if that was the
blocking factor.

Is this to avoid the obj-c message dispatch?
+    return m_imageRenderer == nil || [m_imageRenderer isNull];
Possibly unnecessary.

QSize will take an NSSize (and CGSize) directly, instead of:
+    NSSize sz = [m_imageRenderer size];
     return QSize((int)sz.width, (int)sz.height);

What an awful method name: QPixmap::xForm()


I'm not sure I understand why you have this:
+	     if ([view respondsToSelector:@selector(documentCursor)] &&
cur.handle() == [view documentCursor])
+		 break;

Is that to work around a bug in AppKit where setting the same cursor twice does
extra work?

So the patch looks fine.  IMO It doesn't need another round of review assuming
you respond to comments above.