Bug 60687 – [GTK] Replace GdkRectangle by cairo_rectangle_int_t

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Joone Hur]

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.

[Joone Hur]

Created attachment 93263 [details]
Proposed Patch

[Martin Robinson]

Comment on attachment 93263 [details]
Proposed Patch

Won't this give us a hard dependency on newer versions of Cairo?

[Joone Hur]

(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.

[Carlos Garcia Campos]

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 :-)

[Gustavo Noronha (kov)]

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?

[Martin Robinson]

(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!

[Joone Hur]

(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.

[Gustavo Noronha (kov)]

Comment on attachment 93263 [details]
Proposed Patch

r-ing the patch because Joone will update it based on the comments

[Joone Hur]

Created attachment 93793 [details]
Proposed Patch2

Here is the updated patch.
Thanks!

[Martin Robinson]

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!

[Joone Hur]

(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.

[Joone Hur]

Created attachment 94085 [details]
Proposed Patch3

Gtk+ code has been removed in the drawFocusRing method from GraphicsContextCairo.cpp

[Gustavo Noronha (kov)]

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.

[Martin Robinson]

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?

[Gustavo Noronha (kov)]

(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.

[Joone Hur]

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);

[Joone Hur]

Created attachment 95284 [details]
Proposed Patch4

I applied my above comment to this patch.

[Gustavo Noronha (kov)]

(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.

[Joone Hur]

Created attachment 95720 [details]
Proposed Patch5

I updated the patch with the changes suggested by kov, Thanks!

[Gustavo Noronha (kov)]

Comment on attachment 95720 [details]
Proposed Patch5

Looks good to me, but I'd prefer to have Martin give the last go here =)

[Martin Robinson]

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.

[Joone Hur]

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.

[WebKit Review Bot]

Comment on attachment 97576 [details]
Proposed Patch6

Clearing flags on attachment: 97576

Committed r89133: <http://trac.webkit.org/changeset/89133>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.