Closed Bug 286304 Opened 19 years ago Closed 14 years ago

[mac] implement SetCursor(imgIContainer* cursor) so CSS cursor: url(...) works

Categories

(Core :: Widget: Cocoa, enhancement)

All
macOS
enhancement
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: Biesinger, Assigned: lordpixel)

References

()

Details

(Keywords: dev-doc-complete, platform-parity, student-project, Whiteboard: p-safari)

Attachments

(1 file, 6 obsolete files)

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.
*** Bug 339113 has been marked as a duplicate of this bug. ***
Summary: [mac] implement SetCursor(imgIContainer* cursor) → [mac] implement SetCursor(imgIContainer* cursor) so CSS cursor: url(...) works
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.
Status: NEW → ASSIGNED
Assignee: sfraser_bugs → jmt
Status: ASSIGNED → NEW
Attached patch First attempt at fix. (obsolete) — Splinter Review
Please give feedback on style / approach / correctness / anything else, haven't done a patch this large in a while.
URL contains a test case for a custom cursor, I'll try to scrounge up a better one.
Assignee: jmt → joshmoz
Component: Widget: Mac → Widget: Cocoa
QA Contact: mac → cocoa
Attachment #268345 - Flags: review?(joshmoz)
Assignee: joshmoz → jmt
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!
Attachment #268345 - Flags: review?(joshmoz) → review-
Attached patch Updated patch, based on review. (obsolete) — Splinter 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.
Attachment #268345 - Attachment is obsolete: true
(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
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.
James, you still thinking to do any more with this, or can I give it to a student?
Keywords: student-project
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.
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?
Attachment #283168 - Attachment is obsolete: true
Attachment #419757 - Flags: review?(joshmoz)
Taking...
Assignee: jmt → lordpixel
Status: NEW → ASSIGNED
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!
Attachment #419757 - Flags: review?(joshmoz) → review-
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.
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.
Attachment #419757 - Attachment is obsolete: true
Attachment #424510 - Flags: review?(joshmoz)
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.
Attachment #424510 - Attachment is obsolete: true
Attachment #424538 - Flags: review?(joshmoz)
Attachment #424510 - Flags: review?(joshmoz)
Keywords: dev-doc-needed
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.
Attachment #424538 - Flags: review?(joshmoz) → review+
Hardware: PowerPC → All
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.
Hardware: All → PowerPC
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.
Blocks: 544704
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?
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.
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?
Attachment #424538 - Attachment is obsolete: true
Attachment #430903 - Flags: review?(joshmoz)
Attachment #430903 - Flags: review?(joshmoz) → review-
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.
Good catch. That was actually in the previous version, so I guess it's a good thing it didn't get checked in.
Attachment #430903 - Attachment is obsolete: true
Attachment #430984 - Flags: review?(joshmoz)
Attachment #430984 - Flags: review?(joshmoz) → review+
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?
pushed to mozilla-central

http://hg.mozilla.org/mozilla-central/rev/b0a64e39c7c0
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Hardware: PowerPC → All
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.
Depends on: 554713
This change was already documented, but I've cleaned up the doc slightly.
You need to log in before you can comment on or make changes to this bug.