[webkit-reviews] review granted: [Bug 91940] [GTK] Add GraphicsLayerActor : [Attachment 178788] Patch

[bugzilla-daemon at webkit.org]

Gustavo Noronha (kov) <gns at gnome.org> has granted 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 178788: Patch
https://bugs.webkit.org/attachment.cgi?id=178788&action=review

------- Additional Comments from Gustavo Noronha (kov) <gns at gnome.org>
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.