Bug 81785 - [GTK] Build fix for Accelerated Compositing with Clutter
Summary: [GTK] Build fix for Accelerated Compositing with Clutter
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Joone Hur
URL:
Keywords:
Depends on:
Blocks: 73767 105699
  Show dependency treegraph
 
Reported: 2012-03-21 08:37 PDT by Joone Hur
Modified: 2012-12-23 17:19 PST (History)
4 users (show)

See Also:


Attachments
Proposed Patch (6.21 KB, patch)
2012-03-21 09:27 PDT, Joone Hur
no flags Details | Formatted Diff | Diff
Proposed Patch2 (19.31 KB, patch)
2012-03-25 23:54 PDT, Joone Hur
no flags Details | Formatted Diff | Diff
Proposed Patch3 (19.31 KB, patch)
2012-03-26 00:15 PDT, Joone Hur
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joone Hur 2012-03-21 08:37:46 PDT
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.
Comment 1 Joone Hur 2012-03-21 09:27:31 PDT
Created attachment 133060 [details]
Proposed Patch
Comment 2 Martin Robinson 2012-03-21 10:26:43 PDT
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?
Comment 3 Joone Hur 2012-03-21 22:34:21 PDT
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.
Comment 4 Martin Robinson 2012-03-22 07:49:09 PDT
(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.
Comment 5 Joone Hur 2012-03-25 23:53:19 PDT
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.
Comment 6 Joone Hur 2012-03-25 23:54:28 PDT
Created attachment 133737 [details]
Proposed Patch2
Comment 7 WebKit Review Bot 2012-03-25 23:58:17 PDT
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.
Comment 8 Joone Hur 2012-03-26 00:15:12 PDT
Created attachment 133740 [details]
Proposed Patch3

Fixed style errors.
Comment 9 Martin Robinson 2012-03-26 08:29:22 PDT
Comment on attachment 133740 [details]
Proposed Patch3

Thanks for cleaning up my mess!
Comment 10 Joone Hur 2012-03-26 12:16:18 PDT
Comment on attachment 133740 [details]
Proposed Patch3

Thank you! :-)
Comment 11 WebKit Review Bot 2012-03-26 12:41:58 PDT
Comment on attachment 133740 [details]
Proposed Patch3

Clearing flags on attachment: 133740

Committed r112141: <http://trac.webkit.org/changeset/112141>
Comment 12 WebKit Review Bot 2012-03-26 12:42:03 PDT
All reviewed patches have been landed.  Closing bug.