Bug 138093 – [GTK] Move RedirectedXCompositeWindow from platform to WebKit2 layer

• Component: Platform
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

It's only used by WebKitWebViewBase.

[Carlos Garcia Campos]

Created attachment 240480 [details]
Patch

[WebKit Commit Bot]

Attachment 240480 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:434:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Martin Robinson]

Comment on attachment 240480 [details]
Patch

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

> Source/WebCore/platform/gtk/RedirectedXCompositeWindow.cpp:-189
> -    if (m_needsContext == CreateGLContext) {
> -        context()->waitNative();
> -        // This swap is based on code in Chromium. It tries to work-around a bug in the Intel drivers
> -        // where a swap is necessary to ensure the front and back buffers are properly resized.
> -        if (context() == GLContext::getCurrent())
> -            context()->swapBuffers();
> -    }
> -

I'm not sure that it is appropriate to remove this workaround in this patch. Please move the file and then make separate changes (like the cool anonymous function) in separate patches. I'm worried about introducing hidden regressions.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:-75
> -#if USE(TEXTURE_MAPPER_GL) && PLATFORM(X11)
> -#include <WebCore/RedirectedXCompositeWindow.h>
> -#endif

I think we need to maintain at least PLATFORM(X11) here to deal with Wayland.

[Carlos Garcia Campos]

(In reply to comment #3)
> Comment on attachment 240480 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=240480&action=review
> 
> > Source/WebCore/platform/gtk/RedirectedXCompositeWindow.cpp:-189
> > -    if (m_needsContext == CreateGLContext) {
> > -        context()->waitNative();
> > -        // This swap is based on code in Chromium. It tries to work-around a bug in the Intel drivers
> > -        // where a swap is necessary to ensure the front and back buffers are properly resized.
> > -        if (context() == GLContext::getCurrent())
> > -            context()->swapBuffers();
> > -    }
> > -
> 
> I'm not sure that it is appropriate to remove this workaround in this patch.

That's dead code, since the CreateGLContext mode is not used at all.

> Please move the file and then make separate changes (like the cool anonymous
> function) in separate patches. I'm worried about introducing hidden
> regressions.

I can make a diff of the file.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:-75
> > -#if USE(TEXTURE_MAPPER_GL) && PLATFORM(X11)
> > -#include <WebCore/RedirectedXCompositeWindow.h>
> > -#endif
> 
> I think we need to maintain at least PLATFORM(X11) here to deal with Wayland.

It's done in the header already, we usually try to avoid #ifdefs around header includes, by adding the ifdefs in the headers, in this case, the header was already protected.

[Carlos Garcia Campos]

Created attachment 240541 [details]
Updated patch explaining all the cleanups in the changelog

[Carlos Garcia Campos]

Created attachment 240542 [details]
Cleanup diffs to make it easier to review

[WebKit Commit Bot]

Attachment 240541 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:434:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Martin Robinson]

Comment on attachment 240542 [details]
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.

[Carlos Garcia Campos]

Comment on attachment 240542 [details]
Cleanup diffs to make it easier to review

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

>> Source/WebKit2/UIProcess/gtk/RedirectedXCompositeWindow.cpp:143
>> +    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.

No, it's not. The thing is that previously we had the constructor private, and you could only create the object using the create factory method. Now we are removing the factory methods in WebKit in favor of using std::make_unqiue, but in this particular case, the factory method is required, because it calls supportsXDamageAndXComposite() to return NULL in case xserver doesn't support the required extensions. I still moved to use std::unique_ptr and std::make_unique, but keeping the factory method to check the xserver extensions. But std::make_unique requires the constructor to be public, so this assert is a way to make sure that the caller has used the factory method to create the object. I can remove the ASSERT here, and make the constructor private again, by not using make_unique and returning a std::unique_ptr<RedirectedXCompositeWindow>(new RedirectedXCompositeWindow()); directly.

>> Source/WebKit2/UIProcess/gtk/RedirectedXCompositeWindow.cpp:267
>>  }
> 
> 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.

Ok. I made this to simplify the caller code, but it's also true that it may only fit in the current caller code.

[Carlos Garcia Campos]

(In reply to comment #8)
> Comment on attachment 240542 [details]
> 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.

Note that current code, doesn't event check the returned value is not NULL before using it :-)

[Carlos Garcia Campos]

Created attachment 240665 [details]
Updated patch according to review comments

Changes:

  - Removed the confusing ASSERT in the constructor by leaving the constructor private to ensure the object can only be created by the factory method that already checks if xserver extensions are supported.
  - Moved the draw code back to the web view.
  - Renamed prepareSurfaceForDrawing as just surface() that returns the surface and made it public again.

[WebKit Commit Bot]

Attachment 240665 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:434:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Carlos Garcia Campos]

Committed r176016: <http://trac.webkit.org/changeset/176016>