[webkit-reviews] review denied: [Bug 45423] [Gtk] Port tiled backing store : [Attachment 79136] Fixed merge conflict after source directory movement

[bugzilla-daemon at webkit.org]

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 79136: Fixed merge conflict after source directory movement
https://bugs.webkit.org/attachment.cgi?id=79136&action=review

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
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?