[Webkit-unassigned] [Bug 138093] [GTK] Move RedirectedXCompositeWindow from platform to WebKit2 layer

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=138093

--- Comment #8 from Martin Robinson <mrobinson at webkit.org> ---
Comment on attachment 240542
  --> https://bugs.webkit.org/attachment.cgi?id=240542
Cleanup diffs to make it easier to review

View in context: https://bugs.webkit.org/attachment.cgi?id=240542&action=review

I like the reorganization overall and think it's an improvement, but there are some issues I have with the new design.

> Source/WebKit2/UIProcess/gtk/RedirectedXCompositeWindow.cpp:143
> -    Display* display = GDK_DISPLAY_XDISPLAY(gdk_display_get_default());
> +    ASSERT(supportsXDamageAndXComposite(display));

I think it's a bug to put this into an ASSERT. This means that this check is only run for debug builds and will crash when run on an XServer that does not support these extensions. Instead we should fail gracefully in this case.

> Source/WebKit2/UIProcess/gtk/RedirectedXCompositeWindow.cpp:223
>      if (!m_needsNewPixmapAfterResize && m_surface)
> -        return m_surface.get();
> +        return;

One issue here is that you can no longer detect a failure to create the surface after a resize. I think that the old design handled this case better and this can now hide subtle failures.

> Source/WebKit2/UIProcess/gtk/RedirectedXCompositeWindow.cpp:267
> +void RedirectedXCompositeWindow::draw(cairo_t* cr)
>  {
> -    if (m_damageNotifyCallback)
> -        m_damageNotifyCallback(m_damageNotifyData);
> +    prepareSurfaceForDrawing();
> +    if (!m_surface)
> +        return;
> +
> +    cairo_set_source_surface(cr, m_surface.get(), 0, 0);
> +    cairo_fill(cr);
>  }

It's a bit odd to move the drawing operation here, and I greatly preferred it exterior to the class. Let the user of the interface decide how to use the surface.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20141030/64992d2a/attachment-0002.html>