Bug 28346 - [GTK] Support for cursors that are images
Summary: [GTK] Support for cursors that are images
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk
Depends on: 28345
Blocks:
  Show dependency treegraph
 
Reported: 2009-08-15 19:28 PDT by Martin Robinson
Modified: 2009-08-24 11:15 PDT (History)
4 users (show)

See Also:


Attachments
Support for images as cursors (1.29 KB, patch)
2009-08-15 19:28 PDT, Martin Robinson
no flags Details | Formatted Diff | Diff
Patch with manual test added (2.47 KB, patch)
2009-08-15 20:08 PDT, Martin Robinson
no flags Details | Formatted Diff | Diff
Updated patch ready for review (3.28 KB, patch)
2009-08-17 23:40 PDT, Martin Robinson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Robinson 2009-08-15 19:28:52 PDT
Created attachment 34911 [details]
Support for images as cursors

GTK does not yet seem to support cursors that are images. I've attached a patch which adds this, but it does not seem to work correctly because of #28345.
Comment 1 Martin Robinson 2009-08-15 19:30:48 PDT
Just to clarify: currently this patch creates cursors which have a lots of artifacts. This can be observed by applying this patch and visiting: http://www.worldtimzone.com/mozilla/testcase/css3cursors.html
Comment 2 Jan Alonzo 2009-08-15 19:43:41 PDT
(In reply to comment #1)
> Just to clarify: currently this patch creates cursors which have a lots of
> artifacts. This can be observed by applying this patch and visiting:
> http://www.worldtimzone.com/mozilla/testcase/css3cursors.html

Hi Martin. Is it possible to add a test for this similar to the one above?
Comment 3 Martin Robinson 2009-08-15 20:08:42 PDT
Created attachment 34912 [details]
Patch with manual test added

I've added a manual test for this issue in WebCore/manual-tests/gtk/cursor-image.html
Comment 4 Dirk Schulze 2009-08-16 09:06:03 PDT
You need te set the review flag to get this patch reviewed.
Comment 5 Martin Robinson 2009-08-17 23:40:48 PDT
Created attachment 35022 [details]
Updated patch ready for review

I had planned to wait for the blocking issue to be resolved, but I've taken your cue and prepared the patch for review.
Comment 6 Eric Seidel (no email) 2009-08-18 14:58:15 PDT
Comment on attachment 35022 [details]
Updated patch ready for review

Why is the memory management correct here?  Do we need to release/destroy this cursor ever?

I take it "getGdkPixbuf()" causes the pixbuf to be reffed?
Comment 7 Martin Robinson 2009-08-18 15:12:00 PDT
(In reply to comment #6)
> (From update of attachment 35022 [details])
> Why is the memory management correct here?  Do we need to release/destroy this
> cursor ever?
> 
> I take it "getGdkPixbuf()" causes the pixbuf to be reffed?

getGdkPixbuf() constructs a new GdkPixbuf* from a cairo_surface_t*. Its reference count will be initialized to 1. gdk_cursor_new_from_pixbuf will copy bytes from a GdkPixbuf* as can be seen here: http://google.com/codesearch/p?hl=en&sa=N&cd=2&ct=rc#83-cTy5ZCbw/gtk+-2.4.12/gdk/x11/gdkcursor-x11.c&q=gdk_cursor_new_from_pixbuf&l=387

Since the GdkPixbuf was merely an intermediary between the cairo_surface_t and the GdkPointer, it is no longer needed. gdk_pixbuf_unref will decrease the reference count of the GdkPixbuf from 1 to 0, freeing the memory and preventing a leak.
Comment 8 Martin Robinson 2009-08-18 15:17:00 PDT
I should clarify the removal of the original gdk_cursor_ref(...) as well. Previously the stub method increased the reference count of a GdkCursor created via another Cursor constructor. This patch constructs an entirely new GdkCursor instead, which should already have a reference count of 1.
Comment 9 Gustavo Noronha (kov) 2009-08-23 16:01:58 PDT
Comment on attachment 35022 [details]
Updated patch ready for review

Good to see this moving forward =).
Comment 10 Eric Seidel (no email) 2009-08-23 16:02:56 PDT
Comment on attachment 35022 [details]
Updated patch ready for review

Rejecting patch 35022 from commit-queue.  This patch will require manual commit.

Patch https://bugs.webkit.org/attachment.cgi?id=35022 from bug 28346 failed to download and apply.
Comment 11 Gustavo Noronha (kov) 2009-08-24 07:56:15 PDT
Comment on attachment 35022 [details]
Updated patch ready for review

Landed as r47712. I had overlooked the fact that I could not determine the origin of the .cur file. I replaced the one you provided with a cell.cur from Mozilla sources, that I know is licensed in a good way.
Comment 12 Eric Seidel (no email) 2009-08-24 11:15:33 PDT
Sadly svn-apply can't handle git binary patches:
patch: **** Only garbage was found in the patch input.
patch -p0 "WebCore/manual-tests/gtk/resources/redcursor.cur" returned 2.  Pass --force to ignore patch failures.
Logging in as eric@webkit.org...
Rejecting patch 35022 from commit-queue.  This patch will require manual commit.

bug 26830.