[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