[webkit-reviews] review denied: [Bug 45423] [Gtk] Port tiled backing store : [Attachment 80038] Updated patch with Martin's changes and support for GTK+3.0

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Jan 25 14:51:10 PST 2011


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

Attachment 80038: Updated patch with Martin's changes and support for GTK+3.0
https://bugs.webkit.org/attachment.cgi?id=80038&action=review

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

Looking good! I think this should go one more round, but it's very close. We'll
need another r+ before it can land though (since it includes an API change).

> Source/WebCore/ChangeLog:1
> +2011-01-25  Julien Chaffraix <jchaffraix at codeaurora.org>, Zaheer Ahmad
<zahmad at codeaurora.org>, and Joone Hur <joone.hur at collabora.co.uk>

I think usually this is FirstName LastName<space><space><email>. I'm not sure
if this is necessary, but I wouldn't be surprised if the tools don't handle
this format properly.

> Source/WebCore/ChangeLog:8
> +	   This is the GTK port of tiling mechanism implemented in 35146

This sentence is missing a period. Also, it might be better to rephrase this.
"Add the GTK+ port of the tiling..."

> Source/WebCore/ChangeLog:13
> +	   (WebCore::TiledBackingStore::TiledBackingStore): Call setTileSize to
apply the default tile size.

I'm still not sure why this is necessary for GTK+ and not for other ports. This
file is platform-independent. What is different to require this?

> Source/WebCore/platform/graphics/Tile.h:90
> +    ScreenRegion m_dirtyRegion;

Since you only use ScreenRegion in one place, I think it makes sense to pull
the #ifdefs down here like so:

#if PLATFORM(GTK)
    GOwnPtr<GdkRegion> m_dirtyRegion;
#ifdef GTK_API_VERSION_2
    GOwnPtr<GdkRegion> m_dirtyRegion;
#else
    OwnPtr<cairo_region_t> m_dirtyRegion;
#endif
#endif

> Source/WebCore/platform/graphics/cairo/OwnPtrCairo.cpp:56
> +template <> void deleteOwnedPtr<cairo_region_t>(cairo_region_t* ptr)
> +{
> +    if (ptr)
> +	   cairo_region_destroy(ptr);
> +}

Since cairo_region_t is new in 1.10 this needs to be guarded by #if
CAIRO_VERSION >= CAIRO_VERSION_ENCODE(1, 10, 0). You may need to provide dummy
struct implementation to fulfill the typedef for older Cairo versions. Maybe
not, because the template is never instantiated for old versions. I can test
that if you need me to.

> Source/WebCore/platform/graphics/gtk/TileGtk.cpp:52
> +static cairo_surface_t* checkeredPixmap()

You should change the name of this method now that it no longer returns a
pixmap.

> Source/WebCore/platform/graphics/gtk/TileGtk.cpp:149
> +#ifdef GTK_API_VERSION_2
> +    int rectCount;
> +    GOwnPtr<GdkRectangle> rects;
> +    gdk_region_get_rectangles(m_dirtyRegion.get(), &rects.outPtr(),
&rectCount);
> +    m_dirtyRegion.clear();
> +    m_dirtyRegion.set(gdk_region_new());
> +#else
> +    int rectCount = cairo_region_num_rectangles(m_dirtyRegion.get());
> +    GOwnPtr<GdkRectangle> rects(g_new(GdkRectangle, rectCount));
> +    for (int i = 0; i < rectCount; i++)
> +	   cairo_region_get_rectangle(m_dirtyRegion.get(), i, rects.get()+i);
> +    m_dirtyRegion.clear();
> +    m_dirtyRegion.set(cairo_region_create());
> +#endif
> +
> +    RefPtr<cairo_t> cr = adoptRef(cairo_create(m_buffer.get()));
> +    GraphicsContext context(cr.get());
> +    context.translate(-m_rect.x(), -m_rect.y());

I think it would be better to use a vector of IntRects here. See how it's done
in webkit_web_view_expose_event and webkit_web_view_draw. We probably want to
isolate the code in the #ifdefs into a static function that just returns a
Vector of IntRects.


More information about the webkit-reviews mailing list