[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


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:

    GOwnPtr<GdkRegion> m_dirtyRegion;
    GOwnPtr<GdkRegion> m_dirtyRegion;
    OwnPtr<cairo_region_t> m_dirtyRegion;

> 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