[webkit-reviews] review denied: [Bug 73319] [GTK] Initial implementation of Accelerated Compositing using Clutter : [Attachment 117786] Proposed Patch5

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has denied Joone Hur
<joone.hur at collabora.co.uk>'s request for review:
Bug 73319: [GTK] Initial implementation of Accelerated Compositing using
Clutter
https://bugs.webkit.org/show_bug.cgi?id=73319

Attachment 117786: Proposed Patch5
https://bugs.webkit.org/attachment.cgi?id=117786&action=review

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=117786&action=review


General comments: I'm seeing a lot of this code over and over in the AC
implementations banging around. It would be nice to abstract it away with a
class. In fact, I'm working on such a thing now. ;)

Some it picky comments follow, but I think there are a few more serious issues
as well.

> Source/WebCore/ChangeLog:5
> +	   [GTK] Initial implementation of Accelerated Compositing using
Clutter
> +
> +	   https://bugs.webkit.org/show_bug.cgi?id=73319

Extra line here.

> Source/WebCore/ChangeLog:9
> +	   No new tests added as this patch doesn't affect any functionality.

This comment seems innaccurate. You're adding a new feature, so it does add new
functionality. Here you could describe whether or not the AC layout tests run
or whether the feature is complete enough to run the tests.

> Source/WebCore/platform/graphics/clutter/GraphicsLayerClutter.cpp:31
>  
>  #if USE(ACCELERATED_COMPOSITING)
> +
>  #include "GraphicsLayerClutter.h"

Accidental change?

> Source/WebCore/platform/graphics/clutter/GraphicsLayerClutter.cpp:46
> +    // ClutterRectangle will be used to show the debug border.
> +    m_layer = clutter_rectangle_new();

This should be an initializer field.

> Source/WebCore/platform/graphics/clutter/GraphicsLayerClutter.cpp:51
>  GraphicsLayerClutter::~GraphicsLayerClutter()
>  {
>  }

You are leaking m_layer, I think.

> Source/WebCore/platform/graphics/clutter/GraphicsLayerClutter.h:47
> +    ClutterActor* m_layer;

Can you use a smart pointer here?

> Source/WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp:919
> -    webViewSetRootGraphicsLayer(m_webView, rootLayer);
> +    if (rootLayer)
> +	   webViewSetRootGraphicsLayer(m_webView, rootLayer);
> +    else
> +	   webViewDetachRootGraphicsLayer(m_webView);
>  }

This patch doesn't seem to be against trunk.  In Alex's implementation
webViewSetRootGraphicsLayer handles the detach case.

> Source/WebKit/gtk/webkit/webkitwebview.cpp:3324
> +    priv->rootGraphicsLayer = NULL;
> +    priv->rootLayerEmbedder = NULL;

These pointers are initialized to zero by default by GLib aren't they?

> Source/WebKit/gtk/webkit/webkitwebview.cpp:4916
> +	   priv->rootGraphicsLayer = NULL;

NULL should be 0.

> Source/WebKit/gtk/webkit/webkitwebview.cpp:4919
> +	   Frame* frame = core(webView)->mainFrame();
> +	   frame->view()->syncCompositingStateIncludingSubframes();

Could be one line.

> Source/WebKit/gtk/webkit/webkitwebview.cpp:4926
> +	   ClutterColor stage_color = { 0xFF, 0xFF, 0xFF, 0xFF };

stage_color -> stageColor.

> Source/WebKit/gtk/webkit/webkitwebview.cpp:4953
> +    WebKitWebView* webView = WEBKIT_WEB_VIEW(data);
> +    Frame* frame = core(webView)->mainFrame();
> +    frame->view()->syncCompositingStateIncludingSubframes();

I think it makes sense for this to be one line as well.

> Tools/GtkLauncher/main.c:386
> +#ifdef WTF_USE_CLUTTER
> +    gtk_clutter_init(&argc, &argv);
> +#else

I suppose there is no way to enforce this for embedders. Is there a way to
check if clutter is initialized and not attempt to use it if gtk_init was
called instead of gtk_clutter_init?

WTF_USE_CLUTTER should be USE(CLUTTER)