As the GraphicsContext3D instance is created by TextureMapperGL, this patch removes the uses of GraphicsContext3D for AcceleratedCompositingContext and adds an EFL specific context using Evas_GL.
Created attachment 225831 [details] Patch
Comment on attachment 225831 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=225831&action=review > Source/WebKit/efl/ChangeLog:3 > + [EFL] Replace GraphicsContext3D with Evas_GL in AcceleratedCompositingContext. Could you explain why we need to replace GraphicsContext3D with Evas_GL ? Any benefit ? > Source/WebKit/efl/WebCoreSupport/AcceleratedCompositingContextEfl.cpp:69 > + int width = 0; Please use Evas_Coord to be used by evas_object_geometry_get() e.g. Evas_Coord width, height; > Source/WebKit/efl/WebCoreSupport/AcceleratedCompositingContextEfl.h:44 > + bool resize(const IntSize&); Why should this function be public ?
Created attachment 225851 [details] Patch
(In reply to comment #2) > (From update of attachment 225831 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=225831&action=review > > > Source/WebKit/efl/ChangeLog:3 > > + [EFL] Replace GraphicsContext3D with Evas_GL in AcceleratedCompositingContext. > > Could you explain why we need to replace GraphicsContext3D with Evas_GL ? Any benefit ? TextureMapperGL has its own GraphicsContext3D created by createForCurrentGLContext(). So AcceleratedCompositingContext doesn't need to create and set it to TextureMapperGL anymore. But, we should do 'makeCurrentContext' by using a platform specific context before createForCurrentGLContext(). This patch removes GraphicsContext3D related codes in AcceleratedCompositingContext and adds EvasGLContext/Surface to it instead. > > Source/WebKit/efl/WebCoreSupport/AcceleratedCompositingContextEfl.cpp:69 > > + int width = 0; > > Please use Evas_Coord to be used by evas_object_geometry_get() > > e.g. Evas_Coord width, height; Done. > > Source/WebKit/efl/WebCoreSupport/AcceleratedCompositingContextEfl.h:44 > > + bool resize(const IntSize&); > > Why should this function be public ? If webview size is changed, we will have to resize the EvasGLSurface.
Comment on attachment 225851 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=225851&action=review > TextureMapperGL has its own GraphicsContext3D created by createForCurrentGLContext(). So AcceleratedCompositingContext doesn't need to create and set it to TextureMapperGL anymore. But, we should do 'makeCurrentContext' by using a platform specific context before createForCurrentGLContext(). It would be good if you write this reason to ChangeLog. This patch removes GraphicsContext3D related codes in AcceleratedCompositingContext and adds EvasGLContext/Surface to it instead. > Source/WebKit/efl/WebCoreSupport/AcceleratedCompositingContextEfl.h:44 > + bool resize(const IntSize&); > > > Source/WebKit/efl/WebCoreSupport/AcceleratedCompositingContextEfl.h:44 > > > + bool resize(const IntSize&); > > > > Why should this function be public ? > > If webview size is changed, we will have to resize the EvasGLSurface. I don't see well. So, you may call it from other places in future, right ? > Source/WebKit/efl/WebCoreSupport/AcceleratedCompositingContextEfl.h:61 > + OwnPtr<Evas_GL> m_evasGL; Please use c++11 (e.g. std::unique_ptr<>) instead of OwnPtr.
Created attachment 225944 [details] Patch
(In reply to comment #5) > It would be good if you write this reason to ChangeLog. Done. > > > > Source/WebKit/efl/WebCoreSupport/AcceleratedCompositingContextEfl.h:44 > > > > + bool resize(const IntSize&); > > > > > > Why should this function be public ? > > > > If webview size is changed, we will have to resize the EvasGLSurface. > > I don't see well. So, you may call it from other places in future, right ? Right. > > Source/WebKit/efl/WebCoreSupport/AcceleratedCompositingContextEfl.h:61 > > + OwnPtr<Evas_GL> m_evasGL; > > Please use c++11 (e.g. std::unique_ptr<>) instead of OwnPtr. I think we should file a new bug to replace OwnPtr<> with unique_ptr<> for EFL objects. I can't use unique_ptr<> for Evas_GL right now, because we have to add a custom deleter like deleteOwnedPtr() for OwnPtr<>. Fixing this is out of this patch's scope, don't you think?
Created attachment 225946 [details] Patch
Comment on attachment 225946 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=225946&action=review > Source/WebKit/efl/WebCoreSupport/AcceleratedCompositingContextEfl.cpp:65 > + m_evasGLContext = WebKit::EvasGLContext::create(m_evasGL.get()); Hmm, at least, I think that we should fix it before landed this. EvasGLContext is in WebCore/ I believe that EvasGLContext should be in WebCore namespace.
Comment on attachment 225946 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=225946&action=review r- based on review comments. > Source/WebKit/efl/WebCoreSupport/AcceleratedCompositingContextEfl.h:61 > + OwnPtr<Evas_GL> m_evasGL; OwnPtr is being replaced by C+11 (e.g. std::unique_ptr) So, it would be good if you start to use it in this patch. Loot at the meta bug. Convert OwnPtr and PassOwnPtr uses to std::unique_ptr https://bugs.webkit.org/show_bug.cgi?id=128007
(In reply to comment #7) > > > Source/WebKit/efl/WebCoreSupport/AcceleratedCompositingContextEfl.h:61 > > > + OwnPtr<Evas_GL> m_evasGL; > > > > Please use c++11 (e.g. std::unique_ptr<>) instead of OwnPtr. > > I think we should file a new bug to replace OwnPtr<> with unique_ptr<> for EFL objects. > I can't use unique_ptr<> for Evas_GL right now, > because we have to add a custom deleter like deleteOwnedPtr() for OwnPtr<>. > Fixing this is out of this patch's scope, don't you think? Ah, I missed to read this comment. If there is additional work to use it, please file a new bug for it.
> > Source/WebKit/efl/WebCoreSupport/AcceleratedCompositingContextEfl.cpp:65 > > + m_evasGLContext = WebKit::EvasGLContext::create(m_evasGL.get()); > > Hmm, at least, I think that we should fix it before landed this. > > EvasGLContext is in WebCore/ > I believe that EvasGLContext should be in WebCore namespace. Good point. Done. #129797
(In reply to comment #11) > (In reply to comment #7) > > > > > Source/WebKit/efl/WebCoreSupport/AcceleratedCompositingContextEfl.h:61 > > > > + OwnPtr<Evas_GL> m_evasGL; > > > > > > Please use c++11 (e.g. std::unique_ptr<>) instead of OwnPtr. > > > > I think we should file a new bug to replace OwnPtr<> with unique_ptr<> for EFL objects. > > I can't use unique_ptr<> for Evas_GL right now, > > because we have to add a custom deleter like deleteOwnedPtr() for OwnPtr<>. > > Fixing this is out of this patch's scope, don't you think? > > Ah, I missed to read this comment. If there is additional work to use it, please file a new bug for it. I've added another bug for it. bug 129853
Created attachment 226087 [details] Patch
Comment on attachment 226087 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=226087&action=review LGTM except for my small comment. > Source/WebKit/efl/WebCoreSupport/AcceleratedCompositingContextEfl.h:44 > + bool resize(const IntSize&); I think it would be good to keep it as *private* until using by other classes.
Committed r165314: <http://trac.webkit.org/changeset/165314>
Comment on attachment 226087 [details] Patch clearing flags. I landed manually after moved resize() to private section like gyuyoung mentioned.