[Webkit-unassigned] [Bug 45423] [Gtk] Port tiled backing store
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Jan 25 14:51:11 PST 2011
https://bugs.webkit.org/show_bug.cgi?id=45423
Martin Robinson <mrobinson at webkit.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #80038|review? |review-
Flag| |
--- Comment #33 from Martin Robinson <mrobinson at webkit.org> 2011-01-25 14:51:10 PST ---
(From update of attachment 80038)
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.
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list