Bug 73319 - [GTK] Initial implementation of Accelerated Compositing using Clutter
: [GTK] Initial implementation of Accelerated Compositing using Clutter
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: WebKit Gtk
: 528+ (Nightly build)
: Unspecified Linux
: P2 Normal
Assigned To: Joone Hur
:
Depends on: 73298 73458
Blocks: 73767 74087 105699
  Show dependency treegraph
 
Reported: 2011-11-29 07:19 PST by Joone Hur
Modified: 2012-12-23 17:19 PST (History)
7 users (show)

See Also:


Attachments
Proposed Patch (19.20 KB, application/octet-stream)
2011-12-03 09:04 PST, Joone Hur
no flags Details
Proposed Patch2 (19.20 KB, patch)
2011-12-03 09:07 PST, Joone Hur
gns: commit‑queue-
Details | Formatted Diff | Diff
Proposed Patch3 (19.17 KB, patch)
2011-12-03 10:52 PST, Joone Hur
no flags Details | Formatted Diff | Diff
Proposed Patch4 (19.01 KB, patch)
2011-12-03 11:15 PST, Joone Hur
no flags Details | Formatted Diff | Diff
Proposed Patch5 (18.57 KB, patch)
2011-12-04 01:56 PST, Joone Hur
mrobinson: review-
Details | Formatted Diff | Diff
Proposed Patch6 (16.83 KB, patch)
2011-12-04 10:19 PST, Joone Hur
webkit.review.bot: commit‑queue-
Details | Formatted Diff | Diff
Proposed Patch7 (20.36 KB, patch)
2011-12-05 09:49 PST, Joone Hur
gns: review+
Details | Formatted Diff | Diff
Updated patch (20.16 KB, patch)
2011-12-08 23:56 PST, Joone Hur
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joone Hur 2011-11-29 07:19:01 PST
This is an initial implementation of Accelerated Compositing for WebKitGtk+.
A GtkClutterEmbed will be used to embed Clutter Actors inside WebKitWebView.
Comment 1 Joone Hur 2011-12-03 09:04:09 PST
Created attachment 117759 [details]
Proposed Patch
Comment 2 Joone Hur 2011-12-03 09:07:34 PST
Created attachment 117761 [details]
Proposed Patch2
Comment 3 Gustavo Noronha (kov) 2011-12-03 09:16:03 PST
Comment on attachment 117761 [details]
Proposed Patch2

Attachment 117761 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10728339
Comment 4 Gustavo Noronha (kov) 2011-12-03 10:07:59 PST
Comment on attachment 117761 [details]
Proposed Patch2

View in context: https://bugs.webkit.org/attachment.cgi?id=117761&action=review

> Source/WebKit/gtk/ChangeLog:6
> +        This patch allows to add a GtkClutterEmbed to embed Clutter Actors in WebKitWebView and set the root GraphicsLayer to WebKitWebView when Accelerated Compositing needs to be enabled.

This line looks a bit too long, split it around "set the root"? =)

> Source/WebKit/gtk/webkit/webkitwebview.cpp:4904
> +    g_return_if_fail(WEBKIT_IS_WEB_VIEW(webView));

make this an ASSERT() I guess, g_return_if_fail should only be used in public API.

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

This is bad =(. We would need to initialize clutter ourselves I think, somehow, otherwise we'll break lots of stuff. Should be OK for the early stages though.

> configure.ac:1094
> +   AC_SUBST(CAIRO_CFLAGS)
> +   AC_SUBST(CAIRO_LIBS)

Why do we need cairo checks here? Can't we use the existing ones?
Comment 5 Joone Hur 2011-12-03 10:52:18 PST
Created attachment 117767 [details]
Proposed Patch3
Comment 6 Joone Hur 2011-12-03 10:56:33 PST
(In reply to comment #5)
> Created an attachment (id=117767) [details]
> Proposed Patch3

Sorry, I didn't apply Kov's comments to this patch.
I will update the patch again. Thanks!
Comment 7 Joone Hur 2011-12-03 11:15:13 PST
Created attachment 117770 [details]
Proposed Patch4

I updated the patch with the changes suggested by Kov.
Comment 8 Gustavo Noronha (kov) 2011-12-04 01:25:31 PST
Comment on attachment 117770 [details]
Proposed Patch4

View in context: https://bugs.webkit.org/attachment.cgi?id=117770&action=review

The patch looks good to me except for those comments, I'd like to hear Martin's opinion on the general approach before we go ahead.

> Source/WebCore/platform/graphics/clutter/GraphicsLayerClutter.h:46
> +    ClutterActor* m_layer; // The main layer

I'd remove this comment.

> Source/WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp:936
> +    // Currently, we only support CSS 3D Transform

Missing period.

> Source/WebKit/gtk/webkit/webkitwebview.cpp:849
> +

Unnecessary change? =)

> Source/WebKit/gtk/webkit/webkitwebview.cpp:4913
> +    ClutterActor* stage = gtk_clutter_embed_get_stage (GTK_CLUTTER_EMBED (priv->rootLayerEmbedder));

Spaces before (.

> Source/WebKit/gtk/webkit/webkitwebview.cpp:4942
> +    // Detach the root layer from the hosting view");

There's "); at the end instead of a period.

> Source/WebKit/gtk/webkit/webkitwebview.cpp:4949
> +    return TRUE;

I'm wondering, why do we have this boolean return value if we always return TRUE? =)
Comment 9 Joone Hur 2011-12-04 01:55:51 PST
Comment on attachment 117770 [details]
Proposed Patch4

View in context: https://bugs.webkit.org/attachment.cgi?id=117770&action=review

>> Source/WebCore/platform/graphics/clutter/GraphicsLayerClutter.h:46
>> +    ClutterActor* m_layer; // The main layer
> 
> I'd remove this comment.

Done.

>> Source/WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp:936
>> +    // Currently, we only support CSS 3D Transform
> 
> Missing period.

Done.

>> Source/WebKit/gtk/webkit/webkitwebview.cpp:849
>> +
> 
> Unnecessary change? =)

Done.

>> Source/WebKit/gtk/webkit/webkitwebview.cpp:4913
>> +    ClutterActor* stage = gtk_clutter_embed_get_stage (GTK_CLUTTER_EMBED (priv->rootLayerEmbedder));
> 
> Spaces before (.

Done.

>> Source/WebKit/gtk/webkit/webkitwebview.cpp:4942
>> +    // Detach the root layer from the hosting view");
> 
> There's "); at the end instead of a period.

Done.

>> Source/WebKit/gtk/webkit/webkitwebview.cpp:4949
>> +    return TRUE;
> 
> I'm wondering, why do we have this boolean return value if we always return TRUE? =)

Yes, we don't need to have the return value,so I've removed it.
Comment 10 Joone Hur 2011-12-04 01:56:32 PST
Created attachment 117786 [details]
Proposed Patch5
Comment 11 Martin Robinson 2011-12-04 05:54:44 PST
Comment on attachment 117786 [details]
Proposed Patch5

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)
Comment 12 Joone Hur 2011-12-04 10:19:09 PST
Created attachment 117796 [details]
Proposed Patch6

I applied Martin's comments to this patch.
Thanks!
Comment 13 WebKit Review Bot 2011-12-04 11:02:34 PST
Comment on attachment 117796 [details]
Proposed Patch6

Attachment 117796 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10729543

New failing tests:
svg/custom/linking-uri-01-b.svg
Comment 14 Joone Hur 2011-12-05 09:49:21 PST
Created attachment 117891 [details]
Proposed Patch7

GRefPtrClutter is added to this patch.
Comment 15 Gustavo Noronha (kov) 2011-12-06 05:47:06 PST
Comment on attachment 117891 [details]
Proposed Patch7

View in context: https://bugs.webkit.org/attachment.cgi?id=117891&action=review

Please fix those comments before landing!

> Source/WebCore/GNUmakefile.am:580
> +	-I$(srcdir)/Source/WebCore/platform/clutter

Missing a \

> Source/WebKit/gtk/webkit/webkitwebview.cpp:4913
> +        core(webView)->mainFrame()->view()->syncCompositingStateIncludingSubframes();

Don't we need to call this also after adding the new layer to the stage?

> Source/WebKit/gtk/webkit/webkitwebview.cpp:4949
> +static gboolean webViewSyncLayers(gpointer data)
> +{
> +    WebKitWebView* webView = WEBKIT_WEB_VIEW(data);
> +    core(webView)->mainFrame()->view()->syncCompositingStateIncludingSubframes();
> +
> +    return FALSE;
>  }

Maybe wrap this in #if USE(CLUTTER) so that it won't give us warnings when building without.
Comment 16 Gustavo Noronha (kov) 2011-12-06 11:13:06 PST
btw, I noticed while testing this that we require clutter >= 1.8.3, that isn't really needed, I bet, since I only have 1.8.2 which is the latest packaged in Debian, please change that as well =)
Comment 17 Joone Hur 2011-12-08 23:32:50 PST
Comment on attachment 117891 [details]
Proposed Patch7

View in context: https://bugs.webkit.org/attachment.cgi?id=117891&action=review

>> Source/WebCore/GNUmakefile.am:580
>> +	-I$(srcdir)/Source/WebCore/platform/clutter
> 
> Missing a \

Done.

>> Source/WebKit/gtk/webkit/webkitwebview.cpp:4913
>> +        core(webView)->mainFrame()->view()->syncCompositingStateIncludingSubframes();
> 
> Don't we need to call this also after adding the new layer to the stage?

I've removed this with the IF statement because there seems no case to run this code. Usually, the rootGraphicsLayer is removed when the webViewDetachRootGraphicsLayer is called.

>> Source/WebKit/gtk/webkit/webkitwebview.cpp:4949
>>  }
> 
> Maybe wrap this in #if USE(CLUTTER) so that it won't give us warnings when building without.

Done.

> configure.ac:289
> +CLUTTER_REQUIRED_VERSION=1.8.3

Changed to 1.8.2
Comment 18 Joone Hur 2011-12-08 23:56:24 PST
Created attachment 118539 [details]
Updated patch
Comment 19 WebKit Review Bot 2011-12-09 01:41:06 PST
Comment on attachment 118539 [details]
Updated patch

Clearing flags on attachment: 118539

Committed r102448: <http://trac.webkit.org/changeset/102448>
Comment 20 WebKit Review Bot 2011-12-09 01:41:12 PST
All reviewed patches have been landed.  Closing bug.