[webkit-reviews] review denied: [Bug 91940] [GTK] Add GraphicsLayerActor : [Attachment 165610] Patch

[bugzilla-daemon at webkit.org]

Joone Hur <joone at webkit.org> has denied Joone Hur <joone at webkit.org>'s request
for review:
Bug 91940: [GTK] Add GraphicsLayerActor
https://bugs.webkit.org/show_bug.cgi?id=91940

Attachment 165610: Patch
https://bugs.webkit.org/attachment.cgi?id=165610&action=review

------- Additional Comments from Joone Hur <joone at webkit.org>
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.