[Webkit-unassigned] [Bug 45423] [Gtk] Port tiled backing store

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=45423


Martin Robinson <mrobinson at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #79136|review?                     |review-
               Flag|                            |




--- Comment #30 from Martin Robinson <mrobinson at webkit.org>  2011-01-19 15:17:13 PST ---
(From update of attachment 79136)
View in context: https://bugs.webkit.org/attachment.cgi?id=79136&action=review

Looks good. r- because we can no longer use GdkPixmap.

> Source/WebCore/platform/graphics/Tile.h:44
> +typedef struct _GdkRegion ScreenRegion;

If you instead used

typedef GOwnPtr<GdkRegion> ScreenRegion;
and
typedef OwnPtr<cairo_region_t> ScreenRegion;

and then implemented the necessary specializations in RefPtrCairo.h and GOwnPtrGtk.h, you might be able to avoid manually calling *_region_destroy in TileGtk.cpp.

> Source/WebCore/platform/graphics/TiledBackingStore.cpp:46
>  {
> +    setTileSize(m_tileSize);
>  }

You don't mention in the ChangeLog why this is necessary.

> Source/WebCore/platform/graphics/gtk/TileGtk.cpp:38
> +#include <GOwnPtr.h>
> +#include <GRefPtr.h>

It's safer to include "GOwnPtrGtk.h" and "GRefPtrGtk.h" here. I think we should enforce this for all GTK+ specific code.

> Source/WebCore/platform/graphics/gtk/TileGtk.cpp:47
> +static const unsigned checkerColor1 = 0xff555555;
> +static const unsigned checkerColor2 = 0xffaaaaaa;

Foreground and background, instead of 1 and 2 maybe? I'm not sure here.

> Source/WebCore/platform/graphics/gtk/TileGtk.cpp:54
> +    static GdkPixmap* pixmap = 0;
> +
> +    if (pixmap) 
> +        return pixmap;

Super nit: I'd rather see this bit in one clump and then the work below separated out.

> Source/WebCore/platform/graphics/gtk/TileGtk.cpp:55
> +    pixmap = gdk_pixmap_new(0, checkerSize, checkerSize, gdk_visual_get_depth(gdk_visual_get_system()));

I'm pretty sure that gdk_pixmap_new has been removed from GDK 3.x. You should just use a Cairo surface here.

> Source/WebCore/platform/graphics/gtk/TileGtk.cpp:57
> +    GraphicsContext context(cr);

I'd rather see this done manually with Cairo than bringing GraphicsContext into the mix.

> Source/WebCore/platform/graphics/gtk/TileGtk.cpp:179
> +    cairo_t* cr = context->platformContext();
> +    gdk_cairo_set_source_pixmap(cr, checkeredPixmap(), 0, 0);

If you wish you use a GdkPixmap here you would want to do cairo_surface_create_similar, I believe.

> Source/WebKit/gtk/ChangeLog:28
> +        (renderFromTiledBackingStore): Added to calculate the document dirty rect taking the scroll offsets and invoke the tile paint api.
> +        (paintWebView): Use the tile store if enabled when painting.
> +        (webkit_web_view_update_settings): Exposed a new property enable-tiled-backing-store on the webkitwebsettings object.
> +        (webkit_web_view_settings_notify):
> +
>  2011-01-16  Adam Barth  <abarth at webkit.org>

Please break ChangeLog lines similarly to other entries in the file.

> Source/WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp:459
> +#if ENABLE(TILED_BACKING_STORE)
> +void ChromeClient::delegatedScrollRequested(const IntSize& delta)
> +{
> +}
> +#endif

Please put this with ChromeClient::visibleRectForTiledBackingStore() to reduce the number of #ifdefs.

> Source/WebKit/gtk/WebCoreSupport/ChromeClientGtk.h:155
> +        virtual WebCore::IntRect visibleRectForTiledBackingStore() const;

Is it necessary to specify WebCore:: here?

> Source/WebKit/gtk/webkit/webkitwebsettings.cpp:926
> +     * Whether to enable the the tiled backing store feature.
> +     * With the tiled backing store enabled, the content of the web page are cached to bitmap tiles.
> +     * These bitmap tiles are created and deleted on-demand as the viewport moves over the web page.
> +     * Enabling tiling can significantly speed up painting heavy operations like scrolling, but it 

Very nice! Thank you.

> Source/WebKit/gtk/webkit/webkitwebsettings.cpp:936
> +                                                         _("Whether Tiled Backing Store should be enabled"),

Should Tiled Backing Store be capitalized?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.