Last Comment Bug 286304 - [mac] implement SetCursor(imgIContainer* cursor) so CSS cursor: url(...) works
: [mac] implement SetCursor(imgIContainer* cursor) so CSS cursor: url(...) works
Status: RESOLVED FIXED
p-safari
: dev-doc-complete, pp, student-project
Product: Core
Classification: Components
Component: Widget: Cocoa (show other bugs)
: Trunk
: All Mac OS X
: -- enhancement with 10 votes (vote)
: ---
Assigned To: Andrew Thompson
:
: Markus Stange [:mstange]
Mentors:
http://www.worldtimzone.com/mozilla/t...
: 339113 450025 470367 (view as bug list)
Depends on: 554713
Blocks: 544704
  Show dependency treegraph
 
Reported: 2005-03-15 14:30 PST by Christian :Biesinger (don't email me, ping me on IRC)
Modified: 2010-08-27 10:42 PDT (History)
27 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
First attempt at fix. (8.33 KB, patch)
2007-06-14 05:26 PDT, James Turner
jaas: review-
Details | Diff | Splinter Review
Updated patch, based on review. (7.30 KB, patch)
2007-10-02 04:56 PDT, James Turner
no flags Details | Diff | Splinter Review
Implements custom cursors on OS X, proposes centralized image conversion functions (12.64 KB, patch)
2010-01-01 12:19 PST, Andrew Thompson
jaas: review-
Details | Diff | Splinter Review
Version 2 of the patch addressing Josh's comments (30.49 KB, patch)
2010-01-31 11:27 PST, Andrew Thompson
no flags Details | Diff | Splinter Review
Version 3, doesn't set the cursor if it hasn't changed (similar to Windows code_ (31.33 KB, patch)
2010-01-31 19:49 PST, Andrew Thompson
jaas: review+
Details | Diff | Splinter Review
Merge with changes from bug 548097 (34.22 KB, patch)
2010-03-06 19:59 PST, Andrew Thompson
jaas: review-
Details | Diff | Splinter Review
Add missing -release Josh spotted (34.25 KB, patch)
2010-03-07 12:10 PST, Andrew Thompson
jaas: review+
Details | Diff | Splinter Review

Description Christian :Biesinger (don't email me, ping me on IRC) 2005-03-15 14:30:20 PST
to support urls for CSS cursor properties, nsIWidget::SetCursor(imgIContainer*)
needs to be implemented for the window.

bug 38447 implemented this for windows and gtk2.
Comment 1 David Baron :dbaron: ⌚️UTC-10 2006-05-24 08:44:52 PDT
*** Bug 339113 has been marked as a duplicate of this bug. ***
Comment 2 James Turner 2007-03-31 09:38:36 PDT
I'm working on this, got something that works but have currently screwed up the conversion from gfxIImageFrame / nsIImage to an NSBitmapImageRep : I was using code stolen from widget/src/cocoa/nsClipboard.mm but I'm getting pixel dirt. Will attach a patch once I sort out the format conversion.
Comment 3 James Turner 2007-06-14 05:26:32 PDT
Created attachment 268345 [details] [diff] [review]
First attempt at fix.

Please give feedback on style / approach / correctness / anything else, haven't done a patch this large in a while.
Comment 4 James Turner 2007-06-14 05:27:59 PDT
URL contains a test case for a custom cursor, I'll try to scrounge up a better one.
Comment 5 Josh Aas 2007-08-24 11:16:28 PDT
Comment on attachment 268345 [details] [diff] [review]
First attempt at fix.

James - thanks for the patch, sorry it took me so long to get to reviewing it!

The first thing I'm wondering is if you can take some of the image conversion code that you got from the clipboard and stick it in the nsCocoaUtils file to share between nsChildView and nsClipboard.

+static nsresult ConvertImageToNSImageRep(nsIImage* aImage, NSImage* aNSImage)

Using a type ("NSImage") in the argument name for "aNSImage" makes this hard to read. Please change the argument name to "cocoaImage" or "nativeImage" or something like that.

+  if ((stride % 4 != 0) || (height < 1) || (width < 1)) 
+  {

We always put the opening brace on the same line as the "if" statement. This needs to be fixed in multiple places.

+  // We have to reorder data to have alpha last because only Tiger can handle
+  // alpha being first.
+  PRUint32 imageLength = ((stride * height) / 4);
+  for (PRUint32 i = 0; i < imageLength; i++) {
+    PRUint32 pixel = imageData[i];
+    reorderedData[i] = CFSwapInt32HostToBig((pixel << 8) | (pixel >> 24));
+  }

We only support Tiger and up now, so this shouldn't be necessary any more. See bug 393336 which is about fixing that in the clipboard code.

+  [aNSImage addRepresentation:imageRep];
+  [imageRep autorelease];

There is no reason to autorelease the image rep - just release it. It has already been retained by aNSImage.

 - (void) setFrame: (int) aFrameIndex
-{
+{  

You're adding spaces after the bracket here.

Thanks again for doing this!
Comment 6 James Turner 2007-10-02 04:56:58 PDT
Created attachment 283168 [details] [diff] [review]
Updated patch, based on review.

Patch incorporating review feedback. Unfortunately, the code cannot easily be shared with the clipboard code (or take advantage of the fact that CG can deal with the endian/swapping issues for us) because we need an NSImage, not a CGImage. So I've opted for clarity instead. The memory handling is now safe - we pass a NULL planes array in, let NSImageRep allocate storage, and then copy into it.
Comment 7 Stuart Morgan 2007-10-02 07:41:46 PDT
(In reply to comment #6)
> Unfortunately, the code cannot easily be shared with the clipboard
> code (or take advantage of the fact that CG can deal with the
> endian/swapping issues for us) because we need an NSImage, not a CGImage.

It's pretty straightforward to get an NSImage once you have a CGImage; see
http://mxr.mozilla.org/mozilla/source/camino/src/embedding/CHBrowserView.mm#761
Comment 8 James Turner 2007-10-02 09:35:44 PDT
I'm ambivalent about whether or not it's better to go via the CGImage path, and then convert (but thank you for point out the approach). In the end it's going to be about the same amount of code regardless I think. Josh, if you feel strongly about this, please let me know.
Comment 9 Kevin Brosnan 2008-08-10 08:20:43 PDT
*** Bug 450025 has been marked as a duplicate of this bug. ***
Comment 10 Kevin Brosnan 2009-02-02 01:28:04 PST
*** Bug 470367 has been marked as a duplicate of this bug. ***
Comment 11 David Humphrey (:humph) 2009-11-12 11:34:55 PST
James, you still thinking to do any more with this, or can I give it to a student?
Comment 12 Andrew Thompson 2009-12-24 08:22:58 PST
Hey, so I just sat and implemented this, utilizing the code in nsMenuItemIconX.mm, which looks suspiciously like the code in the patch above from the clipboard code... Which shows you why you should (i) never copy and paste and (ii) always search bugzilla before writing code

So, we have ourselves one or two copies of this image conversion code, with varying vintages.

Let me take a look at moving this into a utility file, and also at the patch above which I think supports animation (mine doesn't). If I can merge them all, I'll try to get this to the point where it is landable.
Comment 13 Andrew Thompson 2010-01-01 12:19:28 PST
Created attachment 419757 [details] [diff] [review]
Implements custom cursors on OS X, proposes centralized image conversion functions

In this patch:

* image conversion functionality is placed in nsCocoaUtils. If this is checked in, I will file bugs and convert the clipboard and menu icon code so we have one common implementation

* animation is not supported. In the 2.0 tree, imgIContainer's API does not allow us to get arbitrary frames from an animation... it wants to drive the animation internally

* please look at the memory management closely

* You'll note that every time the SetCursor method is called, the image is converted and the cursor set, even if it is the same image over and over again (as indeed it will be as the mouse moves around)

To solve that I'd have to introduce an instance variable for the imgIContainer... do you think that is worth it... and if I did that should I use an nsComPtr to hold the value?
Comment 14 Andrew Thompson 2010-01-01 12:20:09 PST
Taking...
Comment 15 Josh Aas 2010-01-09 19:06:41 PST
Comment on attachment 419757 [details] [diff] [review]
Implements custom cursors on OS X, proposes centralized image conversion functions

Some indentation in your patch uses 4 spaces, please use 2. There are also lots of unnecessary spaces at the ends of lines that are not empty.

+  //limit cursor size to 128x128 to avoid DoS attacks
+  PRInt32 width = frame->Height();
+  PRInt32 stride = frame->Stride();
+  PRInt32 height = frame->Width();
+  if ((stride % 4 != 0) || (height < 1) || (width < 1)) {
+    return NS_ERROR_FAILURE;
+  }  

Please put a space between "//" and the first character of a comment.

Why are you confident in this stride assumption and the later assumptions about the format of the data? Is that guaranteed to be the same and documented somewhere?

Why did you write the DoS prevention comment here but you don't check that here? (you check it somewhere else)

+nsresult nsCocoaUtils::CreateCGImageFromImageContainer(imgIContainer *aImage, PRUint32 aWhichFrame, CGImageRef *aResult)
...
+  return NS_OK;

In this function you might want to return an error code based on whether or not *aResult is NULL.

+  *aResult = [[NSImage alloc] initWithSize: imageRect.size];

In Gecko Obj-C code we don't put a space between ":" and the argument in a method call.

+  NS_OBJC_BEGIN_TRY_ABORT_BLOCK_NSRESULT;
+  PRInt32 width = ::CGImageGetWidth(aInputImage);
...
+  return NS_OK;
+  NS_OBJC_END_TRY_ABORT_BLOCK_NSRESULT;

We always put an empty newline after the start exception handler macro and before the end macro.

+  if ( NS_FAILED(rv) || !imageRef ) {

No spaces around the contents of the "if" statement, "if (foo)".

+  CGContextDrawImage(imageContext, *(CGRect*)&imageRect, aInputImage);

All calls to Mac OS X C functions should have a "::" prefixed, you only did this some of the time.

@interface nsCursorManager : NSObject
{
  @private
  NSMutableDictionary *mCursors;
  nsCursor mCurrentCursor;
  nsMacCursor *mCurrentMacCursor;
}

I think we should rename "mCurrentCursor" to "mCurrentGeckoCursor" so all of this code is easier to read.

+  nsMacCursor *mCurrentMacCursor;

Are you sure you need this strong reference? It seems like you can do the same stuff by accessing NSCursor's "currentCursor" and nsCocoaCursor's "mLastSetCocoaCursor".

+  //if the hotstop is nonsensical, make it 0,0

"hotspot" not "hotstop"

+  aHotspotX = (aHotspotX < 0 || aHotspotX > (PRUint32) width - 1) ? 0 : aHotspotX;
+  aHotspotY = (aHotspotY < 0 || aHotspotY > (PRUint32) height - 1) ? 0 : aHotspotY;

Are you sure that Gecko's idea of 0,0 matches up with Cocoa's? I think NSCursor uses a flipped coordinate system but please double check that this works correctly.

Thanks for working on this!
Comment 16 Andrew Thompson 2010-01-14 19:10:24 PST
Thanks for the review. Some replies...

- I cleaned up the whitespace and other cosmetic stuff. 

>Why are you confident in this stride assumption and the later assumptions about
>the format of the data? Is that guaranteed to be the same and documented
>somewhere?

- Actually, yes: http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/public/gfxImageSurface.h#44 for one. Also similar image conversion code exists in nsMenuItemIconX.mm and nsClipboard.mm.

>Are you sure that Gecko's idea of 0,0 matches up with Cocoa's? I think NSCursor
>uses a flipped coordinate system but please double check that this works
>correctly.

Yes, NSCursor is flipped, meaning it is "normal" with y=0 at the top.

>Are you sure you need this strong reference? It seems like you can do the same
>stuff by accessing NSCursor's "currentCursor" and nsCocoaCursor's
>"mLastSetCocoaCursor".

- Well, all the predefined cursors are held onto using static references in the cursor cache when they are created. But the custom cursors are created on the fly, so I think I need to hold a strong reference to the instance of nsMacCursor. What I probably could do is move the nsCursor (mCurrentCursor) into nsMacCursor.h, because I agree having both instance variables is ugly. Give me a couple more days and I'll see if there's anything I can do.
Comment 17 Andrew Thompson 2010-01-31 11:27:19 PST
Created attachment 424510 [details] [diff] [review]
Version 2 of the patch addressing Josh's comments

Josh - to your point about holding instance variables... what I did in this patch was put the current nsCursor (i.e. the value of the enum) inside nsMacCursor. i.e. I pass the current nsCursor to the nsMacCursor constructor and it stores it in a field so I can later do [mCurrentCursor type].

This at least cleans up the mess of having two current cursor variables in nsCursorManager. There's a bit of repetitive code to set everything up, but once that is done the API is cleaner to use.

Fundamentally I think we have to retain an instance of nsMacCursor because we're now creating them when a custom URI is passed in.

I still think the biggest issue is that I don't detect when the same instance of imgIContainer* cursor is passed into SetCursor. I am looking at the code in the windows implementation and thinking I should copy it. http://mxr.mozilla.org/mozilla-central/source/widget/src/windows/nsWindow.cpp#1958 

Please take a look at the patch I attached and I will roll in that Windows code along with your review comments.
Comment 18 Andrew Thompson 2010-01-31 19:49:22 PST
Created attachment 424538 [details] [diff] [review]
Version 3, doesn't set the cursor if it hasn't changed (similar to Windows code_

Sorry for 2 patches in 1 day, but I added the functionality from the Windows implementation that checks if the custom image for the cursor has changed or not before setting it.
Comment 19 Josh Aas 2010-02-01 13:47:55 PST
Comment on attachment 424538 [details] [diff] [review]
Version 3, doesn't set the cursor if it hasn't changed (similar to Windows code_

+      return [nsMacCursor cursorWithCursor:[NSCursor arrowCursor] type: aCursor];

There are still a bunch of added lines in this patch that have spaces between ":" and the actual arguments in obj-c calls and signatures. Don't worry about conforming with existing code style in this way.

+  //if the hotspot is nonsensical, make it 0,0

Still a few places with no space between "//" and the start of the comment.

Other than that little stuff it looks good, thanks.
Comment 20 Andrew Thompson 2010-02-02 18:44:50 PST
I cleaned up the whitespace... actually I fixed all of it in nsMacCursor and nsCursorManager to be consistent.

OK - process questions

i) does this need super-review and if so who?
ii) should I attach another version of the patch with the whitespace cleanup? Seems redundant and I don't want to force you to r+ it again... but I'll follow your lead.
Comment 21 Josh Aas 2010-02-02 18:58:16 PST
You're not changing any APIs so you don't need sr, and I don't need to see the whitespace-adjusted patch. Maybe to keep things clear you could land all the whitespace changes first and then land your real patch.
Comment 22 Smokey Ardisson (offline for a while; not following bugs - do not email) 2010-03-06 17:03:33 PST
lordpixel, are you going to land this, or attach code changes and whitespace changes patches for someone else to land, or is there some other hold-up here I'm not seeing?
Comment 23 Andrew Thompson 2010-03-06 17:37:18 PST
There's no particular holdup other than the general rule the tree has to be open and I am supposed to have 4 hours to wait for everything to build.

My family life doesn't give me many such opportunities and so far tinderbox hasn't been cooperating when I have had one.

Give me 48 hours to see if I can make the whitespace patch. After that point, it'd make more sense if someone else commits it. Same for #544704.
Comment 24 Andrew Thompson 2010-03-06 19:59:50 PST
Created attachment 430903 [details] [diff] [review]
Merge with changes from bug 548097

Oops, spoke to soon. Had to merge patch with changes from bug 548097.

Looking at the patch again I just don't see a lot of noise from whitespace. I am disinclined to turn this into two checkins.

Josh: same story... I apologize for making you do it again, but I've had to update the patch to take account of other changes in nsCococaUtils.h/mm. Can you review please?
Comment 25 Josh Aas 2010-03-07 09:19:57 PST
Comment on attachment 430903 [details] [diff] [review]
Merge with changes from bug 548097

>+  NSImage *cursorImage;
>+  nsresult rv = nsCocoaUtils::CreateNSImageFromImageContainer(aCursorImage, imgIContainer::FRAME_FIRST, &cursorImage);

This will leak, it is never released. Later on you initialize an NSCursor with it but the NSCursor will retain again if it wants to keep the image.
Comment 26 Andrew Thompson 2010-03-07 12:10:32 PST
Created attachment 430984 [details] [diff] [review]
Add missing -release Josh spotted

Good catch. That was actually in the previous version, so I guess it's a good thing it didn't get checked in.
Comment 27 Josh Aas 2010-03-11 07:01:16 PST
Do you want me to check your patch in for you? If so do you want a name to go with your email address in the commit entry?
Comment 28 Josh Aas 2010-03-11 08:46:50 PST
pushed to mozilla-central

http://hg.mozilla.org/mozilla-central/rev/b0a64e39c7c0
Comment 29 Andrew Thompson 2010-03-12 20:12:23 PST
Thanks. I was going to ask you to do this if I didn't get to it this weekend.

I've also added my real name.
Comment 30 Eric Shepherd [:sheppy] 2010-08-27 10:42:28 PDT
This change was already documented, but I've cleaned up the doc slightly.

Note You need to log in before you can comment on or make changes to this bug.