Bug 60687 - [GTK] Replace GdkRectangle by cairo_rectangle_int_t
Summary: [GTK] Replace GdkRectangle by cairo_rectangle_int_t
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Joone Hur
URL:
Keywords: Gtk
Depends on:
Blocks: 45423
  Show dependency treegraph
 
Reported: 2011-05-11 21:38 PDT by Joone Hur
Modified: 2011-06-17 04:50 PDT (History)
5 users (show)

See Also:


Attachments
Proposed Patch (13.91 KB, patch)
2011-05-12 00:56 PDT, Joone Hur
gns: review-
Details | Formatted Diff | Diff
Proposed Patch2 (24.07 KB, patch)
2011-05-17 10:37 PDT, Joone Hur
mrobinson: review-
Details | Formatted Diff | Diff
Proposed Patch3 (24.27 KB, patch)
2011-05-19 10:22 PDT, Joone Hur
gns: review-
Details | Formatted Diff | Diff
Proposed Patch4 (24.69 KB, patch)
2011-05-29 02:14 PDT, Joone Hur
no flags Details | Formatted Diff | Diff
Proposed Patch5 (15.17 KB, patch)
2011-06-01 22:01 PDT, Joone Hur
mrobinson: review+
Details | Formatted Diff | Diff
Proposed Patch6 (13.47 KB, patch)
2011-06-17 04:09 PDT, Joone Hur
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joone Hur 2011-05-11 21:38:27 PDT
Starting with version 1.10, Cairo provides cairo_rectangle_t that is equivalent to GdkRectangle. Therefore, we can replace GdkRectangle by  cairo_rectangle_int_t if we use Cairo 1.10 or higher version.
Comment 1 Joone Hur 2011-05-12 00:56:07 PDT
Created attachment 93263 [details]
Proposed Patch
Comment 2 Martin Robinson 2011-05-12 08:59:02 PDT
Comment on attachment 93263 [details]
Proposed Patch

Won't this give us a hard dependency on newer versions of Cairo?
Comment 3 Joone Hur 2011-05-12 20:08:52 PDT
(In reply to comment #2)
> (From update of attachment 93263 [details])
> Won't this give us a hard dependency on newer versions of Cairo?

If we use more Cairo code, we can share more code between GTK+2 and GTK+3.
It allows us to write code a bit more simple and clean. 

Anyway, the tiled backing store patch needs this patch to work in GTK+2 and GTK+3, even other ports like WinCairo and EFL.
Comment 4 Carlos Garcia Campos 2011-05-12 23:33:18 PDT
GdkRectangle is already a typedef cairo_rectangle_int_t in GTK3, so we can use it in both GTK 2 and 3 without #ifdefs. We can add IntRectCairo.cpp anyway for ports using cairo but not gtk. And for me depending on Cairo 1.10 is not a problem but a must :-)
Comment 5 Gustavo Noronha (kov) 2011-05-13 07:05:51 PDT
Considering what Carlos said, and I think it makes sense to depend on cairo 1.10, should we remove IntRectGtk and use cairo_rectangle_int_t everywhere? I believe we had this discussion in a different bug where we decided we did not want to bump the cairo dependency isn't that right?
Comment 6 Martin Robinson 2011-05-13 08:44:27 PDT
(In reply to comment #5)
> Considering what Carlos said, and I think it makes sense to depend on cairo 1.10, should we remove IntRectGtk and use cairo_rectangle_int_t everywhere? I believe we had this discussion in a different bug where we decided we did not want to bump the cairo dependency isn't that right?

When I woke up this morning I was against requiring 1.10, but now I think it's time. Let's do it!
Comment 7 Joone Hur 2011-05-13 10:09:53 PDT
(In reply to comment #6)
> (In reply to comment #5)
> > Considering what Carlos said, and I think it makes sense to depend on cairo 1.10, should we remove IntRectGtk and use cairo_rectangle_int_t everywhere? I believe we had this discussion in a different bug where we decided we did not want to bump the cairo dependency isn't that right?
> 
> When I woke up this morning I was against requiring 1.10, but now I think it's time. Let's do it!

Okay, I will update this patch to remove IntRectGtk and use cairo_rectangle_int_t instead of GdkRectangle in other source code.
Comment 8 Gustavo Noronha (kov) 2011-05-16 05:39:32 PDT
Comment on attachment 93263 [details]
Proposed Patch

r-ing the patch because Joone will update it based on the comments
Comment 9 Joone Hur 2011-05-17 10:37:55 PDT
Created attachment 93793 [details]
Proposed Patch2

Here is the updated patch.
Thanks!
Comment 10 Martin Robinson 2011-05-17 10:50:54 PDT
Comment on attachment 93793 [details]
Proposed Patch2

View in context: https://bugs.webkit.org/attachment.cgi?id=93793&action=review

I'm not sure I understand this patch. While depending on a newer version of Cairo means that we can use cairo_rect_t and cairo_region_t, it doesn't mean we can use it with old versions of GTK+.

> Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:639
> +    gdk_cairo_region(cr, (GdkRegion*)reg);

Why are you casting here. Isn't this incorect on old versions of GTK+? I don't think it's right to cast between a cairo region and GdkRegion.

> Source/WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp:432
> -    if (gdk_rectangle_intersect(&area, &sourceRect, &moveRect)) {
> -        GdkRegion* moveRegion = gdk_region_rectangle(&moveRect);
> +    if (gdk_rectangle_intersect((GdkRectangle*)&area, (GdkRectangle*)&sourceRect, (GdkRectangle*)&moveRect)) {
> +        GdkRegion* moveRegion = gdk_region_rectangle((GdkRectangle*)&moveRect);
>          gdk_window_move_region(window, moveRegion, delta.width(), delta.height());

I have the same feeling here!
Comment 11 Joone Hur 2011-05-17 22:33:14 PDT
(In reply to comment #10)
> (From update of attachment 93793 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=93793&action=review
> 
> I'm not sure I understand this patch. While depending on a newer version of Cairo means that we can use cairo_rect_t and cairo_region_t, it doesn't mean we can use it with old versions of GTK+.
> 
> > Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:639
> > +    gdk_cairo_region(cr, (GdkRegion*)reg);
> 
> Why are you casting here. Isn't this incorect on old versions of GTK+? I don't think it's right to cast between a cairo region and GdkRegion.

Yes, this is incorrect on GTK+2. Instead, I think I can remove all GTK+ code in here.

> 
> > Source/WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp:432
> > -    if (gdk_rectangle_intersect(&area, &sourceRect, &moveRect)) {
> > -        GdkRegion* moveRegion = gdk_region_rectangle(&moveRect);
> > +    if (gdk_rectangle_intersect((GdkRectangle*)&area, (GdkRectangle*)&sourceRect, (GdkRectangle*)&moveRect)) {
> > +        GdkRegion* moveRegion = gdk_region_rectangle((GdkRectangle*)&moveRect);
> >          gdk_window_move_region(window, moveRegion, delta.width(), delta.height());
> 
> I have the same feeling here!

This is not the same case. I just did type cast cairo_rectangle_int_t to GdkRectangle.
Comment 12 Joone Hur 2011-05-19 10:22:08 PDT
Created attachment 94085 [details]
Proposed Patch3

Gtk+ code has been removed in the drawFocusRing method from GraphicsContextCairo.cpp
Comment 13 Gustavo Noronha (kov) 2011-05-25 06:19:10 PDT
Comment on attachment 94085 [details]
Proposed Patch3

View in context: https://bugs.webkit.org/attachment.cgi?id=94085&action=review

I like removing IntRectGtk, and bringing the code closer to a future removal of gtk2 support, but there are some convoluted casts which are unnecessary and we should use c++-style casts everywhere, so r- on these grounds.

> Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:-653
> -#if PLATFORM(GTK)
> -#ifdef GTK_API_VERSION_2
> -    GdkRegion* reg = gdk_region_new();
> -#else
> -    cairo_region_t* reg = cairo_region_create();
> -#endif
> -
> -    for (unsigned i = 0; i < rectCount; i++) {
> -#ifdef GTK_API_VERSION_2
> -        GdkRectangle rect = rects[i];
> -        gdk_region_union_with_rect(reg, &rect);
> -#else
> -        cairo_rectangle_int_t rect = rects[i];
> -        cairo_region_union_rectangle(reg, &rect);
> -#endif
> -    }
> -    gdk_cairo_region(cr, reg);
> -#ifdef GTK_API_VERSION_2
> -    gdk_region_destroy(reg);
> -#else
> -    cairo_region_destroy(reg);
> -#endif
> -#else

Removing this code seems unnecessary for our goals - just keeping it the way it is would work just fine. What do we gain by removing this (in addition to less #ifdef churn)?

> Source/WebCore/platform/gtk/GtkPluginWidget.cpp:55
> -    GdkRectangle rect = _rect;
> -    gdk_window_invalidate_rect(window, &rect, FALSE);
> +    cairo_rectangle_int_t rect = _rect;
> +    gdk_window_invalidate_rect(window, (GdkRectangle*)&rect, FALSE);

This is fine. GdkRectangle used to be a public struct, as is cairo_rectangle_int_t, as opposed to their region counterparts that are opaque, and they have the exact same size and offsets. But, you should use C++ cast convention here, so static_cast<GdkRectangle*>(&rect).

> Source/WebCore/platform/gtk/GtkPluginWidget.cpp:84
> -    event->expose.area = static_cast<GdkRectangle>(rect);
> +    cairo_rectangle_int_t rectangle = rect;
> +    event->expose.area = *(GdkRectangle*)&rectangle;

This seems to be unnecessarily convoluted. Why cast the address of the variable to a pointer just to derreference it?

> Source/WebCore/plugins/gtk/PluginViewGtk.cpp:565
> +    GtkAllocation allocation(*(GdkRectangle*)&delayedRect);

Same here.

> Source/WebKit/gtk/webkit/webkitwebview.cpp:679
> +        paintWebView(frame, priv->transparent, gc, static_cast<IntRect>(*(cairo_rectangle_int_t*)&event->area), paintRects);

And here.
Comment 14 Martin Robinson 2011-05-25 07:42:18 PDT
Comment on attachment 94085 [details]
Proposed Patch3

View in context: https://bugs.webkit.org/attachment.cgi?id=94085&action=review

>> Source/WebCore/platform/gtk/GtkPluginWidget.cpp:55
>> -    GdkRectangle rect = _rect;
>> -    gdk_window_invalidate_rect(window, &rect, FALSE);
>> +    cairo_rectangle_int_t rect = _rect;
>> +    gdk_window_invalidate_rect(window, (GdkRectangle*)&rect, FALSE);
> 
> This is fine. GdkRectangle used to be a public struct, as is cairo_rectangle_int_t, as opposed to their region counterparts that are opaque, and they have the exact same size and offsets. But, you should use C++ cast convention here, so static_cast<GdkRectangle*>(&rect).

With this change, I recommend fixing the naming of _rect too. It should be called coreRect or something similar.

> Source/WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp:431
> -    GdkRegion* invalidRegion = gdk_region_rectangle(&area);
> +    GdkRegion* invalidRegion = gdk_region_rectangle((GdkRectangle*)&area);
>  
> -    if (gdk_rectangle_intersect(&area, &sourceRect, &moveRect)) {
> -        GdkRegion* moveRegion = gdk_region_rectangle(&moveRect);
> +    if (gdk_rectangle_intersect((GdkRectangle*)&area, (GdkRectangle*)&sourceRect, (GdkRectangle*)&moveRect)) {
> +        GdkRegion* moveRegion = gdk_region_rectangle((GdkRectangle*)&moveRect);

Instead of casting everywhere, if you must cast, you could do it once at the beginning of the #ifdef.

I'm not sure I understand changes like this though. We still maintain GTK+ 2 support so using GdkRectangle makes sense, since it's defined to be cairo_rectangle_t by GDK itself. This change doesn't simplify the code or remove any lines...it actually makes it more complicated. I definitely support preparing for GTK+ 3, but what's the reasoning for this change? Couldn't it wait until we simply remove GTK+ 2 support?
Comment 15 Gustavo Noronha (kov) 2011-05-25 07:56:31 PDT
(In reply to comment #14)
> I'm not sure I understand changes like this though. We still maintain GTK+ 2 support so using GdkRectangle makes sense, since it's defined to be cairo_rectangle_t by GDK itself. This change doesn't simplify the code or remove any lines...it actually makes it more complicated. I definitely support preparing for GTK+ 3, but what's the reasoning for this change? Couldn't it wait until we simply remove GTK+ 2 support?

The idea was to remove IntRectGtk, but I don't really mind keeping it if you think makes sense. We can then just use cairo_rectangle_int_t unconditionally only on the parts where GdkRectangle is not necessary.
Comment 16 Joone Hur 2011-05-27 01:15:32 PDT
Comment on attachment 94085 [details]
Proposed Patch3

View in context: https://bugs.webkit.org/attachment.cgi?id=94085&action=review

>> Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:-653
>> -#else
> 
> Removing this code seems unnecessary for our goals - just keeping it the way it is would work just fine. What do we gain by removing this (in addition to less #ifdef churn)?

If we keep this part of code, we have to add casting code here. Instead, it seems possible to remove GTK+ code if we use Cairo 1.10.
Isn't it good to remove platform specific code here if possible?

>>> Source/WebCore/platform/gtk/GtkPluginWidget.cpp:55
>>> +    gdk_window_invalidate_rect(window, (GdkRectangle*)&rect, FALSE);
>> 
>> This is fine. GdkRectangle used to be a public struct, as is cairo_rectangle_int_t, as opposed to their region counterparts that are opaque, and they have the exact same size and offsets. But, you should use C++ cast convention here, so static_cast<GdkRectangle*>(&rect).
> 
> With this change, I recommend fixing the naming of _rect too. It should be called coreRect or something similar.

kov, we can use not static_cast but reinterpret_cast here, because static_cast just can convert between two related types like casting a base-class pointer to a derived-class pointer, and vice-versa. It seems not consider the size. So I will change this casting code as follows:
 gdk_window_invalidate_rect(window, reinterpret_cast<GdkRectangle*>(&rect), FALSE);

Martin, it makes sense, so I will rename _rect to coreRect.

>> Source/WebCore/platform/gtk/GtkPluginWidget.cpp:84
>> +    event->expose.area = *(GdkRectangle*)&rectangle;
> 
> This seems to be unnecessarily convoluted. Why cast the address of the variable to a pointer just to derreference it?

Pointer type of structure can be only casted, so it needs to dereference it. Anyway, I will change this code as follows:
 event->expose.area = *reinterpret_cast<GdkRectangle*>(&rectangle);

>> Source/WebKit/gtk/webkit/webkitwebview.cpp:679
>> +        paintWebView(frame, priv->transparent, gc, static_cast<IntRect>(*(cairo_rectangle_int_t*)&event->area), paintRects);
> 
> And here.

I will update this code as follows:

cairo_rectangle_int_t rect = *reinterpret_cast<cairo_rectangle_int_t*>(&event->area);
paintWebView(frame, priv->transparent, gc, static_cast<IntRect>(rect), paintRects);
Comment 17 Joone Hur 2011-05-29 02:14:13 PDT
Created attachment 95284 [details]
Proposed Patch4

I applied my above comment to this patch.
Comment 18 Gustavo Noronha (kov) 2011-05-31 06:19:04 PDT
(In reply to comment #16)
> > Removing this code seems unnecessary for our goals - just keeping it the way it is would work just fine. What do we gain by removing this (in addition to less #ifdef churn)?
> 
> If we keep this part of code, we have to add casting code here. Instead, it seems possible to remove GTK+ code if we use Cairo 1.10.
> Isn't it good to remove platform specific code here if possible?

No, there is no need to touch this code at all, not even to add casts. There was probably a reason for having this platform-specific branch here, so unless we understand why or what we would gain (other than less ifdef churn) I don't see a reason to mess with it.

> kov, we can use not static_cast but reinterpret_cast here, because static_cast just can convert between two related types like casting a base-class pointer to a derived-class pointer, and vice-versa. It seems not consider the size. So I will change this casting code as follows:
>  gdk_window_invalidate_rect(window, reinterpret_cast<GdkRectangle*>(&rect), FALSE);

Sure, the important thing is to use the C++-style casts to be style-compliant =).

> Martin, it makes sense, so I will rename _rect to coreRect.

I have a suggestion, though, which will be in line with what Martin Robinson is saying. Let's keep IntRectGtk, since this is complicating the code more than it should. Let's use cairo_rectangle_int_t where possible and where GdkRectangles are necessary you just use them.
Comment 19 Joone Hur 2011-06-01 22:01:43 PDT
Created attachment 95720 [details]
Proposed Patch5

I updated the patch with the changes suggested by kov, Thanks!
Comment 20 Gustavo Noronha (kov) 2011-06-16 07:09:34 PDT
Comment on attachment 95720 [details]
Proposed Patch5

Looks good to me, but I'd prefer to have Martin give the last go here =)
Comment 21 Martin Robinson 2011-06-16 07:51:35 PDT
Comment on attachment 95720 [details]
Proposed Patch5

This looks fine except that I think you should remove the OwnPtr and IntRect changes until they are actually used. It's weird to check them in as dead code. I think this patch is fine just bumping the Cairo dependency and then depending on it in a few remaining places. Please make these changes before landing.
Comment 22 Joone Hur 2011-06-17 04:09:02 PDT
Created attachment 97576 [details]
Proposed Patch6

I've removed the OwnPtr change, but we still need the IntRect change(support for cairo_rectangle_int_t) to work with Gtk+3.
Comment 23 WebKit Review Bot 2011-06-17 04:50:11 PDT
Comment on attachment 97576 [details]
Proposed Patch6

Clearing flags on attachment: 97576

Committed r89133: <http://trac.webkit.org/changeset/89133>
Comment 24 WebKit Review Bot 2011-06-17 04:50:17 PDT
All reviewed patches have been landed.  Closing bug.