[webkit-reviews] review denied: [Bug 60687] [GTK] Replace GdkRectangle by cairo_rectangle_int_t : [Attachment 94085] Proposed Patch3

[bugzilla-daemon at webkit.org]

Gustavo Noronha (kov) <gns at gnome.org> has denied Joone Hur
<joone.hur at collabora.co.uk>'s request for review:
Bug 60687: [GTK] Replace GdkRectangle by cairo_rectangle_int_t
https://bugs.webkit.org/show_bug.cgi?id=60687

Attachment 94085: Proposed Patch3
https://bugs.webkit.org/attachment.cgi?id=94085&action=review

------- Additional Comments from Gustavo Noronha (kov) <gns at gnome.org>
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.