Bug 91940 – [GTK] Add GraphicsLayerActor

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Joone Hur]

GraphicsLayerActor is a ClutterActor, which is owned by GraphicsLayer.
It will be used to render composited layers.

[Joone Hur]

Created attachment 153803 [details]
Proposed Patch

[WebKit Review Bot]

Attachment 153803 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.h:70:  parent_class is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.h:80:  graphics_layer_actor_get_type is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:85:  graphics_layer_actor_class_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:120:  graphics_layer_actor_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 4 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Joone Hur]

Created attachment 153808 [details]
Proposed Patch2

[WebKit Review Bot]

Attachment 153808 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.h:71:  parent_class is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.h:81:  graphics_layer_actor_get_type is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:86:  graphics_layer_actor_class_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:121:  graphics_layer_actor_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 4 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Rodrigo Moya]

Created attachment 156700 [details]
Patch that adds GraphicsLayerActor and uses it in GraphicsLayerClutter

Joone,

I have been recently assigned to work on thus, and just learnt about this bug, so here's the patch for adding GraphicsLayerActor that I have prepared. I just had a review from Gustavo, so there are a couple things I need to fix, but I thought it'd be better to let you know about it ASAP, so that we can coordinate this work.

This patch has GraphicsLayerActor with the animation bits removed, the basic stuff in GraphicsLayerClutter to use the GraphicsLayerActor instead of ClutterRectangle, and PlatformClutterLayerClient.

[Joone Hur]

Created attachment 165610 [details]
Patch

[Gustavo Noronha (kov)]

Comment on attachment 165610 [details]
Patch

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

Quick pass on the code, bunch of nits.

> Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:41
> +    // Guard against changing size while allocating.

Think this comment is not necessary.

> Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:54
> +    gfloat anchorX;
> +    gfloat anchorY;
> +    gfloat anchorZ;

We usually try to use regular types where a glib-specific type is not required.

> Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:90
> +    GParamSpec* pspec;

Should be declared along with the first time a GParamSpec is created, bellow.

> Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:136
> +    priv->scrollX = 0.0;
> +    priv->scrollY = 0.0;
> +
> +    priv->translateX = 0.0;
> +    priv->translateY = 0.0;
> +
> +    priv->matrix = 0;

Think we don't need to zero members.

> Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:261
> +    gfloat translateX = priv->scrollX + priv->translateX;
> +    gfloat translateY = priv->scrollY + priv->translateY;

Ditto about the types.

> Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:307
> +static void graphicsLayerActorPick(ClutterActor* actor, const ClutterColor* pick)

What I did recently on webkit-clutter, though I believe not in GraphicsLayerActor was to just call the paint function for picking, since what we want is really to have them be the same.

> Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:473
> +    if (!priv->surface)
> +        g_message("surface is null");

Remove this, turn into an ASSERT or LOG?

> Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.h:78
> +
> +    /* padding for future expansion */
> +    void (*_padding_0)(void);
> +    void (*_padding_1)(void);
> +    void (*_padding_2)(void);
> +    void (*_padding_3)(void);
> +    void (*_padding_4)(void);

Padding is not really needed given we're using GrahicsLayerActor as a private object, it can change ABI for now.

[Joone Hur]

Comment on attachment 165610 [details]
Patch

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

>> Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:41
>> +    // Guard against changing size while allocating.
> 
> Think this comment is not necessary.

Done.

>> Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:54
>> +    gfloat anchorZ;
> 
> We usually try to use regular types where a glib-specific type is not required.

Done.

>> Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:90
>> +    GParamSpec* pspec;
> 
> Should be declared along with the first time a GParamSpec is created, bellow.

Done.

>> Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:136
>> +    priv->matrix = 0;
> 
> Think we don't need to zero members.

Done.

>> Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:261
>> +    gfloat translateY = priv->scrollY + priv->translateY;
> 
> Ditto about the types.

Done.

>> Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:307
>> +static void graphicsLayerActorPick(ClutterActor* actor, const ClutterColor* pick)
> 
> What I did recently on webkit-clutter, though I believe not in GraphicsLayerActor was to just call the paint function for picking, since what we want is really to have them be the same.

I think we don't need to handle the pick signal in GraphicsLayerActor so I will remove this callback function.

>> Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:473
>> +        g_message("surface is null");
> 
> Remove this, turn into an ASSERT or LOG?

ASSERT would be good.

>> Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.h:78
>> +    void (*_padding_4)(void);
> 
> Padding is not really needed given we're using GrahicsLayerActor as a private object, it can change ABI for now.

Done.

[Joone Hur]

Created attachment 167473 [details]
Patch

[WebKit Review Bot]

Attachment 167473 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:100:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:101:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:102:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:109:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:110:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:111:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:177:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:224:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:226:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:227:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:229:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:230:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:231:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:232:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:233:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:235:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:291:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:292:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:363:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:364:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:365:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:366:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 22 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Joone Hur]

Created attachment 178788 [details]
Patch

[Gustavo Noronha (kov)]

Comment on attachment 178788 [details]
Patch

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

LGTM

> Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:203
> +        // FIXME: some pages make us create very big layers, which
> +        // exceeds cairo's limits, check for that here.
> +        if (textureBox.x2 > MAX_IMAGE_SIZE) {
> +            g_warning("Layer %s has a width that exceeds cairo's limits: %.2f", clutter_actor_get_name(self), textureBox.x2);
> +            textureBox.x2 = MAX_IMAGE_SIZE;
> +        }
> +
> +        if (textureBox.y2 > MAX_IMAGE_SIZE) {
> +            g_warning("Layer %s has a height that exceeds cairo's limits: %.2f", clutter_actor_get_name(self), textureBox.y2);
> +            textureBox.y2 = MAX_IMAGE_SIZE;
> +        }

This workaround was added by me, but I believe this was only being the case because of a bug that was fixed afterwards, since I never saw this warning again. I'd suggest removing it and we can readd this workaround if needed (though I believe it won't be).

> Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:349
> +    priv->texture = clutter_cairo_texture_new(width > 0 ? width : 1, height > 0 ? height : 1);

We should port this to use the new Clutter content/canvas API.

> Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:357
> +static void graphicsLayerActorDrawLayerContents(ClutterActor* actor, GraphicsContext& context)

This is called drawLayerContents in WebKit Clutter, since it's not an exported function I don't see a reason to have the prefix, better keep it named the same as in the original, so it's easier to move forward.

> Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:409
> +/* 
> + * public API
> + */

It's not really public in the usual sense, so I'd remove this comment. Also, there are some exported symbols, such as New, that are not bellow this comment, so it's misleading.

[Joone Hur]

Created attachment 178806 [details]
Patch

[Joone Hur]

Comment on attachment 178806 [details]
Patch

I got r+ with the previous patch from kov

[WebKit Review Bot]

Comment on attachment 178806 [details]
Patch

Clearing flags on attachment: 178806

Committed r137319: <http://trac.webkit.org/changeset/137319>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.