[webkit-reviews] review denied: [Bug 45423] [Gtk] Port tiled backing store : [Attachment 68178] First try: Add a compile time option and implement the logic

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has denied Julien Chaffraix
<jchaffraix at webkit.org>'s request for review:
Bug 45423: [Gtk] Port tiled backing store
https://bugs.webkit.org/show_bug.cgi?id=45423

Attachment 68178: First try: Add a compile time option and implement the logic
https://bugs.webkit.org/attachment.cgi?id=68178&action=review

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=68178&action=review

This patch looks good, in general, but I think I'd rather see this implemented
in terms of the Cairo API as much as possible. We really want to avoid
GdkPixmap/GdkPixbuf as much as possible and especially deprecated APIs like
GdkRegion.

> WebCore/platform/graphics/Tile.h:82
> +    GdkPixmap* m_buffer;
> +    GdkRegion* m_dirtyRegion;

Instead of using GdkPixmap, I'd like to see a GRefPtr<cairo_surface_t> here.
GdkRegion is deprecated in recent GDK releases (replaced by cairo regions), so
you'll either have to figure out some version independent method or use
#ifdefs.

> WebCore/platform/graphics/gtk/TileGtk.cpp:61
> +	   Color color1(checkerColor1);
> +	   Color color2(checkerColor2);
> +	   float red1, green1, blue1, alpha1;
> +	   float red2, green2, blue2, alpha2;
> +	   color1.getRGBA(red1, green1, blue1, alpha1);
> +	   color2.getRGBA(red2, green2, blue2, alpha2);
> +	   for (unsigned y = 0; y < checkerSize; y += checkerSize / 2) {
> +	       bool alternate = y % checkerSize;
> +	       for (unsigned x = 0; x < checkerSize; x += checkerSize / 2) {
> +		   cairo_rectangle(cr, x, y, checkerSize / 2, checkerSize /2);
> +		   if (alternate)
> +		       cairo_set_source_rgb(cr, red1, green1, blue1);
> +		   else
> +		       cairo_set_source_rgb(cr, red2, green2, blue2);
> +		   cairo_fill(cr);
> +		   alternate = !alternate;

I think it might be cleaner to use the stippling approach that Benjamin Otte
oulined here: http://blogs.gnome.org/otte/2010/07/27/rendering-cleanup/

> WebCore/platform/graphics/gtk/TileGtk.cpp:119
> +	   if (!m_buffer) {
> +	       m_backBuffer = gdk_pixmap_new(0,
> +				  m_backingStore->m_tileSize.width(),
> +				  m_backingStore->m_tileSize.height(),
> +				 
gdk_visual_get_depth(gdk_visual_get_system()));
> +	   } else {

WebKit code style requires many block types with only one statement to omit the
curly braces.

> WebCore/platform/graphics/gtk/TileGtk.cpp:154
> +    if (m_buffer)
> +	   g_object_unref(m_buffer);

The use of GRefPtr would elminate the need to do this.

> WebCore/platform/graphics/gtk/TileGtk.cpp:170
> +		      target.height());
> +

Line lengths in the project can be ~120+ characters. So this should be:
    IntRect source((target.x() - m_rect.x()), (target.y() - m_rect.y()),
target.height());

> WebCore/platform/graphics/gtk/TileGtk.cpp:178
> +    cairo_t* cairoContextSource = gdk_cairo_create(m_buffer);
> +    cairo_set_source_surface(
> +	   cairoContext,
> +	   cairo_get_target(cairoContextSource),
> +	   target.x() - source.x(),
> +	   target.y() - source.y());

Using a Cairo surface as the buffer will eliminate some of this and allow you
do just do something like:
cairo_set_source_surface(cairoContxt, m_buffer.get(), target.x() - source.x(),
target.y() - source.y());