Bug 44354 - Cairo and EFL port shouldn't depend on glib.
Summary: Cairo and EFL port shouldn't depend on glib.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Martin Robinson
URL:
Keywords:
: 44561 (view as bug list)
Depends on:
Blocks:
 
Reported: 2010-08-20 14:13 PDT by Rafael Antognolli
Modified: 2010-08-26 15:43 PDT (History)
8 users (show)

See Also:


Attachments
Patch (49.57 KB, patch)
2010-08-20 19:21 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 Rafael Antognolli 2010-08-20 14:13:49 PDT
Since the commit http://trac.webkit.org/changeset/65530 these ports now depend on glib. That's because of the specialised reference pointers GRefPtr (that is used for glib stuff) that were added for cairo.

Is it possible to do the same kind of specialised pointer but using just RefPtr? I couldn't find any place in WebCore code that does this. If it's not, could we just avoid using this, or maybe just ifdef the code? I can send a patch for this if necessary.
Comment 1 Martin Robinson 2010-08-20 14:45:47 PDT
In this case, it appears that WebKitGTK+ is loading a stale copy of the inspector files for the tests. I should have a patch up shortly. Sorry for the noise!
Comment 2 Martin Robinson 2010-08-20 14:45:59 PDT
Whoops. Wrong bug.
Comment 3 Martin Robinson 2010-08-20 19:21:30 PDT
Created attachment 65015 [details]
Patch
Comment 4 Martin Robinson 2010-08-20 19:31:51 PDT
GRefPtr needs to be independent of GLib so that ports with other platform-specific reference-counted types can use it. I've attached a patch that effectively renames it PlatformRefPtr. I'm not stuck on the name and feel it's a bit long, but this should fix the build.
Comment 5 Gustavo Noronha (kov) 2010-08-23 07:57:03 PDT
Comment on attachment 65015 [details]
Patch

hrrmm... I liked the original idea of doing a typedef PlatformRefPtr GRefPtr, what happened to it? How about PRefPtr? Naming this might be a good thing to take to the mailing list, I think. Also, clever trick with GVariant. I dislike that but I guess the hit on build performance is big enough that it outweights my dislike =(
Comment 6 Martin Robinson 2010-08-23 08:10:50 PDT
(In reply to comment #5)
> (From update of attachment 65015 [details])
> hrrmm... I liked the original idea of doing a typedef PlatformRefPtr GRefPtr, what happened to it?

I found that I couldn't do the typedef, because C++ doesn't support making a typedef of a templated type (you can only typdef a specific specialization).

> How about PRefPtr? Naming this might be a good thing to take to the mailing list, I think. 

I'm fine with PRefPtr or even leaving it as GRefPtr, if others like it. The renaming is fairly easy. I'll make a post!

> Also, clever trick with GVariant. I dislike that but I guess the hit on build performance is big enough that it outweights my dislike =(

I was inspired to do this, because not using glib header in the include was one of the original conditions of the r+ of the original GRefPtr patch. I still think the best solution involves precopiled prefix headers thought! :)
Comment 7 Martin Robinson 2010-08-25 00:51:36 PDT
*** Bug 44561 has been marked as a duplicate of this bug. ***
Comment 8 Xan Lopez 2010-08-25 10:38:45 PDT
I think this makes sense in general, and I don't have any problem with the name in the Patch (PlatRefPtr would also be OK). If it's still here tomorrow (my) morning I'll review it for real.
Comment 9 Gustavo Noronha (kov) 2010-08-25 10:43:55 PDT
Comment on attachment 65015 [details]
Patch

OK, let's move forward! =)
Comment 10 Martin Robinson 2010-08-25 11:00:25 PDT
Comment on attachment 65015 [details]
Patch

Clearing flags on attachment: 65015

Committed r66024: <http://trac.webkit.org/changeset/66024>
Comment 11 Martin Robinson 2010-08-25 11:00:31 PDT
All reviewed patches have been landed.  Closing bug.
Comment 12 Darin Adler 2010-08-25 18:03:32 PDT
I think it’s a strange choice to rename GRefPtr to PlatformRefPtr. I don’t understand what the “platform” is in this case.
Comment 13 Gustavo Noronha (kov) 2010-08-26 13:28:58 PDT
(In reply to comment #12)
> I think it’s a strange choice to rename GRefPtr to PlatformRefPtr. I don’t understand what the “platform” is in this case.

The reasoning is we began using GRefPtr for non-glib-specific ref-counted types in backends used by several ports that ont necessarily use glib (in this case Cairo). The name 'Platform' comes from these types being platform-specific (or rather port specific). We could keep GRefPtr as it was for glib types, and add a CairoRefPtr that would be cairo-specific , but that seemed wasteful.
Comment 14 Martin Robinson 2010-08-26 13:44:25 PDT
I should also say that I'm open to suggestions for renaming this template. It seems that it may one day combine with JSRetainPtr in which case PlatformRefPtr makes less sense.
Comment 15 Darin Adler 2010-08-26 14:01:27 PDT
(In reply to comment #13)
> The reasoning is we began using GRefPtr for non-glib-specific ref-counted types in backends used by several ports that ont necessarily use glib (in this case Cairo). The name 'Platform' comes from these types being platform-specific (or rather port specific). We could keep GRefPtr as it was for glib types, and add a CairoRefPtr that would be cairo-specific , but that seemed wasteful.

So you’re saying there’s no chance that a single port might some day want to use GLib and Cairo as well?
Comment 16 Gustavo Noronha (kov) 2010-08-26 14:42:23 PDT
(In reply to comment #15)
> So you’re saying there’s no chance that a single port might some day want to use GLib and Cairo as well?

Not really. GTK+ currently uses both glib and cairo, I meant some other ports (like the redistributable windows port and EFL) use only Cairo, so GRefPtr did not make sense for them as a name.
Comment 17 Darin Adler 2010-08-26 15:26:54 PDT
Either we can make a version of RefPtr that automatically knows how to do the right thing, or we should have a pointer named CairoRefPtr. This PlatformRefPtr name concept makes no sense to me. I must be missing something.
Comment 18 Martin Robinson 2010-08-26 15:39:49 PDT
Brent Fulgham had made another patch which just specialized RefPtr for cairo types. Does it make sense to simply remove PlatformRefPtr/GRefPtr and to just rely on template specializations of RefPtr?
Comment 19 Martin Robinson 2010-08-26 15:43:58 PDT
Here's a link to the original series of comments about why this was implemented separately from RefPtr (in this case, it was for the GLib OwnPtr, which resulted in a separate GLib RefPtr later on): https://bugs.webkit.org/show_bug.cgi?id=20483#c2