Bug 91940

Created attachment 153803 [details] Proposed Patch

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.

Created attachment 153808 [details] Proposed Patch2

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.

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.

Created attachment 165610 [details] Patch

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 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.

Created attachment 167473 [details] Patch

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.

Created attachment 178788 [details] Patch

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.

Created attachment 178806 [details] Patch

Comment on attachment 178806 [details] Patch I got r+ with the previous patch from kov

Comment on attachment 178806 [details] Patch Clearing flags on attachment: 178806 Committed r137319: <http://trac.webkit.org/changeset/137319>

All reviewed patches have been landed. Closing bug.