Created attachment 4652 [details] adds custom cursor support, fixes some image renderer issues
Comment on attachment 4652 [details] adds custom cursor support, fixes some image renderer issues 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.
Also, just FYI, SVG uses the same exact cursor property, and allow you to specify an arbitrary URI (just like your patch adds): http://www.w3.org/TR/SVG/interact.html#CursorProperty One difference is this uri might point to an SVG (or <cursor> element) which we'll need to convert into a pixmap and set as the cursorImage. I will probably just need to extend CSSImageValueImpl (or the imaging system below CSSImageValueImpl).
(In reply to comment #2) > 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. I think we need a compile time check, not just a runtime assert. I think I'll just leave that part of the code as-is. What bothered me was that some code was making the assumption about the constants and other code was not. > Should this: > + parsedValue = new > CSSImageValueImpl(DOMString(KURL(styleElement->baseURL().qstring(), > uri.qstring()).url()), styleElement); > > be: > parsedValue = new > CSSImageValueImpl(styleElement.getDocument().completeURL(uri), styleElement); > > instead? Maybe. I copied that code from CSS_PROP_LIST_STYLE_IMAGE, so we should make it the same in both places. > 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) > > ? No, because if the style is inherit or initial, the first macro will return and the second macro won't do anything at all. > Is this to avoid the obj-c message dispatch? > + return m_imageRenderer == nil || [m_imageRenderer isNull]; > Possibly unnecessary. No. If the renderer is nil, then isNull will return false. We need to check for both. > QSize will take an NSSize (and CGSize) directly, instead of: > + NSSize sz = [m_imageRenderer size]; > return QSize((int)sz.width, (int)sz.height); You're right. I believe i was the one who added the constructor to make a QSize from an NSSize, in fact, but that was after this code was written. I'm not sure it's mandatory to do that cleanup at this time, but maybe I'll do it. > 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? Yes.
I've read your comments. The only action item remaining is the completeURL call change.
Turns out the completeURL change would be wrong. The URL is relative to the style sheet, not the document the style sheet is attached to.
Good to know. Thanks for looking into this.