Bug 91940 - [GTK] Add GraphicsLayerActor
Summary: [GTK] Add GraphicsLayerActor
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Joone Hur
URL:
Keywords:
Depends on:
Blocks: 73767 105699
  Show dependency treegraph
 
Reported: 2012-07-22 07:18 PDT by Joone Hur
Modified: 2012-12-23 17:19 PST (History)
9 users (show)

See Also:


Attachments
Proposed Patch (30.91 KB, patch)
2012-07-23 08:20 PDT, Joone Hur
no flags Details | Formatted Diff | Diff
Proposed Patch2 (31.10 KB, patch)
2012-07-23 09:04 PDT, Joone Hur
no flags Details | Formatted Diff | Diff
Patch that adds GraphicsLayerActor and uses it in GraphicsLayerClutter (70.60 KB, patch)
2012-08-06 08:42 PDT, Rodrigo Moya
no flags Details | Formatted Diff | Diff
Patch (34.00 KB, patch)
2012-09-25 07:23 PDT, Joone Hur
no flags Details | Formatted Diff | Diff
Patch (32.85 KB, patch)
2012-10-07 00:39 PDT, Joone Hur
no flags Details | Formatted Diff | Diff
Patch (31.38 KB, patch)
2012-12-11 06:03 PST, Joone Hur
no flags Details | Formatted Diff | Diff
Patch (30.72 KB, patch)
2012-12-11 07:57 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 2012-07-22 07:18:41 PDT
GraphicsLayerActor is a ClutterActor, which is owned by GraphicsLayer.
It will be used to render composited layers.
Comment 1 Joone Hur 2012-07-23 08:20:06 PDT
Created attachment 153803 [details]
Proposed Patch
Comment 2 WebKit Review Bot 2012-07-23 08:22:39 PDT
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.
Comment 3 Joone Hur 2012-07-23 09:04:07 PDT
Created attachment 153808 [details]
Proposed Patch2
Comment 4 WebKit Review Bot 2012-07-23 09:08:11 PDT
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.
Comment 5 Rodrigo Moya 2012-08-06 08:42:41 PDT
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.
Comment 6 Joone Hur 2012-09-25 07:23:28 PDT
Created attachment 165610 [details]
Patch
Comment 7 Gustavo Noronha (kov) 2012-10-03 06:33:19 PDT
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.
Comment 8 Joone Hur 2012-10-07 00:33:37 PDT
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.
Comment 9 Joone Hur 2012-10-07 00:39:24 PDT
Created attachment 167473 [details]
Patch
Comment 10 WebKit Review Bot 2012-10-07 00:42:44 PDT
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.
Comment 11 Joone Hur 2012-12-11 06:03:20 PST
Created attachment 178788 [details]
Patch
Comment 12 Gustavo Noronha (kov) 2012-12-11 06:27:37 PST
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.
Comment 13 Joone Hur 2012-12-11 07:57:32 PST
Created attachment 178806 [details]
Patch
Comment 14 Joone Hur 2012-12-11 08:01:22 PST
Comment on attachment 178806 [details]
Patch

I got r+ with the previous patch from kov
Comment 15 WebKit Review Bot 2012-12-11 08:21:11 PST
Comment on attachment 178806 [details]
Patch

Clearing flags on attachment: 178806

Committed r137319: <http://trac.webkit.org/changeset/137319>
Comment 16 WebKit Review Bot 2012-12-11 08:21:16 PST
All reviewed patches have been landed.  Closing bug.