[webkit-reviews] review denied: [Bug 94417] [GTK] [WebKit2] Use XComposite window for accelerated compositing : [Attachment 164565] Patch

[bugzilla-daemon at webkit.org]

Carlos Garcia Campos <cgarcia at igalia.com> has denied Martin Robinson
<mrobinson at webkit.org>'s request for review:
Bug 94417: [GTK] [WebKit2] Use XComposite window for accelerated compositing
https://bugs.webkit.org/show_bug.cgi?id=94417

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

------- Additional Comments from Carlos Garcia Campos <cgarcia at igalia.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=164565&action=review


Setting r- because it breaks the attached inspector.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:281
> +    priv->redirectedWindow = RedirectedXCompositeWindow::create(IntSize(1,
1));
> +    priv->readyToRenderAcceleratedCompositingResults = false;

These are now only defined when USE(TEXTURE_MAPPER_GL)

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:358
> +    webViewBase->priv->redirectedWindow->resize(IntSize(allocation->width,
allocation->height));

Ditto, this should be protected by #if USE(TEXTURE_MAPPER_GL) #endif block. I
think this breaks the inspector when attached to the view. you should probably
use the view size, not the whole window size.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:371
> +    GtkAllocation oldAllocation;
> +    gtk_widget_get_allocation(widget, &oldAllocation);

Since you are only interested in the size and not the position, you could
create an IntSize oldSize(gtk_widget_get_allocated_width(widget),
gtk_widget_get_allocated_height(widget));

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:375
> +    if (oldAllocation.width != allocation->width || oldAllocation.height !=
allocation->height) {

And you could create an IntRect for the new allocation and here compare oldSize
!= viewRect.size().

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:382
> +	   resizeWebKitWebViewBaseFromAllocation(webViewBase, allocation);

And here you pass the viewRect instead of the GtkAllocation and then you don't
need to create the viewRect in resizeWebKitWebViewBaseFromAllocation

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:865
> +    gpointer* webViewBasePointer =
static_cast<gpointer*>(fastMalloc(sizeof(gpointer)));
> +    g_object_add_weak_pointer(G_OBJECT(webViewBase), webViewBasePointer);
> +    *webViewBasePointer = webViewBase;
> +    g_timeout_add(1000 / 60,
reinterpret_cast<GSourceFunc>(queueAnotherDrawOfAcceleratedCompositingResults),
webViewBasePointer);

If you need something to be created and destroyed for the life of a GSource,
you can use g_time_add_full and provide a GDestroyNotify, that way you don't
need to care about destroying the data in the idle callback, and the data will
be destroyed even if the source is destroyed before the callback is called. In
this case I would use a helper struct, something like this:

struct DelayedQueueDraw {
    WebKitWebViewBase* view;
};

or a better name for the struct, I don't know.

static DelayedQueueDraw* delayed_queue_draw_create(WebKitWebViewBase* view)
{
    DelayedQueueDraw* delayedDraw = g_slice_new0(DelayedQueueDraw); // Or new
instead of g_slice
    delayedDraw->view = view;
    g_object_add_weak_pointer(G_OBJECT(webViewBase), &delayedDraw->view);
    return delayedDraw;
}

static void delayed_queue_draw_free(DelayedQueueDraw* delayedDraw)
{
    if (delayedDraw->view)
	g_object_remove_weak_pointer(G_OBJECT(delayedDraw->view),
&delayedDraw->view);
    g_slice_free(DelayedQueueDraw, delayedDraw);
}

then you can use g_timeout_add_full this way:

g_timeout_add_full(G_PRIORITY_DEFAULT, 1000 / 60,
reinterpret_cast<GSourceFunc>(queueAnotherDrawOfAcceleratedCompositingResults),
delayed_queue_draw_create(webViewBase), delayed_queue_draw_free);