Bug 81785 – [GTK] Build fix for Accelerated Compositing with Clutter

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Joone Hur]

AcceleratedCompositingContext was introduced to isolate different accelerated compositing implementations: r104194(https://bugs.webkit.org/show_bug.cgi?id=75519).
However, Clutter implementation doesn't build with it.

[Joone Hur]

Created attachment 133060 [details]
Proposed Patch

[Martin Robinson]

Comment on attachment 133060 [details]
Proposed Patch

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

> Source/WebCore/platform/graphics/cairo/GraphicsContext3DCairo.cpp:39
>  #include "Extensions3DOpenGL.h"
>  #include "GraphicsContext3DPrivate.h"
>  #include "Image.h"
> +#include "NotImplemented.h"
>  #include "OpenGLShims.h"
>  #include "PlatformContextCairo.h"
>  #include "RefPtrCairo.h"

Clutter is going to need its own GraphicsContext3D right? Why not just add a stub?

> Source/WebKit/gtk/WebCoreSupport/AcceleratedCompositingContext.h:75
> -    OwnPtr<WebCore::GraphicsLayer> m_rootGraphicsLayer;
>      unsigned int m_syncTimerCallbackId;
>  
>  #if USE(CLUTTER)
> +    WebCore::GraphicsLayer* m_rootGraphicsLayer;
>      GtkWidget* m_rootLayerEmbedder;
>  #elif USE(TEXTURE_MAPPER_GL)
>      WebCore::GLContext* glContext();
>      WebCore::TextureMapperLayer* m_rootTextureMapperLayer;
> +    OwnPtr<WebCore::GraphicsLayer> m_rootGraphicsLayer;
>      OwnPtr<WebCore::TextureMapper> m_textureMapper;

Why can't this be shared?

[Joone Hur]

Comment on attachment 133060 [details]
Proposed Patch

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

>> Source/WebCore/platform/graphics/cairo/GraphicsContext3DCairo.cpp:39
>>  #include "RefPtrCairo.h"
> 
> Clutter is going to need its own GraphicsContext3D right? Why not just add a stub?

I will add GraphicsContext3DClutter later. This patch just fixes the build error.

>> Source/WebKit/gtk/WebCoreSupport/AcceleratedCompositingContext.h:75
>>      OwnPtr<WebCore::TextureMapper> m_textureMapper;
> 
> Why can't this be shared?

Currently, we just use the given layer for the root layer, but I will create a root layer to add the given layer as a child. 
Thanks you for your comments.

[Martin Robinson]

(In reply to comment #3)
> (From update of attachment 133060 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=133060&action=review
> 
> >> Source/WebCore/platform/graphics/cairo/GraphicsContext3DCairo.cpp:39
> >>  #include "RefPtrCairo.h"
> > 
> > Clutter is going to need its own GraphicsContext3D right? Why not just add a stub?
> 
> I will add GraphicsContext3DClutter later. This patch just fixes the build error.

I think it makes sense to add the stub now, since in the end the file will never be shared between the two.
> 
> >> Source/WebKit/gtk/WebCoreSupport/AcceleratedCompositingContext.h:75
> >>      OwnPtr<WebCore::TextureMapper> m_textureMapper;
> > 
> > Why can't this be shared?
> 
> Currently, we just use the given layer for the root layer, but I will create a root layer to add the given layer as a child. 

There's no need. Now that I understand why you are doing this, I don't think it needs to be changed.

[Joone Hur]

Comment on attachment 133060 [details]
Proposed Patch

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

>>>> Source/WebCore/platform/graphics/cairo/GraphicsContext3DCairo.cpp:39
>>>>  #include "RefPtrCairo.h"
>>> 
>>> Clutter is going to need its own GraphicsContext3D right? Why not just add a stub?
>> 
>> I will add GraphicsContext3DClutter later. This patch just fixes the build error.
> 
> I think it makes sense to add the stub now, since in the end the file will never be shared between the two.

Okay, I will add GraphicsContext3D stub to be built with Clutter.

[Joone Hur]

Created attachment 133737 [details]
Proposed Patch2

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/platform/graphics/clutter/GraphicsContext3DPrivate.h:34:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/WebCore/platform/graphics/clutter/GraphicsContext3DPrivate.h:40:  Missing space before {  [whitespace/braces] [5]
Source/WebCore/platform/graphics/clutter/GraphicsContext3DPrivate.h:43:  The parameter name "matrix" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 3 in 9 files


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

[Joone Hur]

Created attachment 133740 [details]
Proposed Patch3

Fixed style errors.

[Martin Robinson]

Comment on attachment 133740 [details]
Proposed Patch3

Thanks for cleaning up my mess!

[Joone Hur]

Comment on attachment 133740 [details]
Proposed Patch3

Thank you! :-)

[WebKit Review Bot]

Comment on attachment 133740 [details]
Proposed Patch3

Clearing flags on attachment: 133740

Committed r112141: <http://trac.webkit.org/changeset/112141>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.