Bug 60687

Created attachment 93263 [details] Proposed Patch

Comment on attachment 93263 [details] Proposed Patch Won't this give us a hard dependency on newer versions of Cairo?

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

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

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?

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

(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 on attachment 93263 [details] Proposed Patch r-ing the patch because Joone will update it based on the comments

Created attachment 93793 [details] Proposed Patch2 Here is the updated patch. Thanks!

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!

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

Created attachment 94085 [details] Proposed Patch3 Gtk+ code has been removed in the drawFocusRing method from GraphicsContextCairo.cpp

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 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?

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

Created attachment 95284 [details] Proposed Patch4 I applied my above comment to this patch.

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

Created attachment 95720 [details] Proposed Patch5 I updated the patch with the changes suggested by kov, Thanks!

Comment on attachment 95720 [details] Proposed Patch5 Looks good to me, but I'd prefer to have Martin give the last go here =)

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.

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 on attachment 97576 [details] Proposed Patch6 Clearing flags on attachment: 97576 Committed r89133: <http://trac.webkit.org/changeset/89133>

All reviewed patches have been landed. Closing bug.