Created attachment 97734 [details] Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=97734&action=review > Source/WebCore/platform/graphics/efl/GraphicsContext3DInternal.cpp:61 > + m_evasGL = ewk_view_evas_gl_create(ewkView); Should we call ewk api to create evas GL? > Source/WebCore/platform/graphics/efl/GraphicsContext3DInternal.cpp:83 > + m_context = evas_gl_context_create(m_evasGL, shareContext); > + if (!m_context) > + return false; > + > + if (!createSurface(ewkView, bRenderDirectlyToEvasGLObject)) > + return false; If we can fail to create here, I think that we should create evas gl. Is it right?

I think this patch is too big. Is it possible to split this patch into smaller one ?

(In reply to comment #3) > I think this patch is too big. Is it possible to split this patch into smaller one ? I have already heard the same comment about the size of my patch from Leandro. (Bug 62709) GraphicsContext3DInternal is big because it contains wrapper functions to all OpenGL(ES) functions. I have no idea how to split it.

(In reply to comment #4) > (In reply to comment #3) > > I think this patch is too big. Is it possible to split this patch into smaller one ? > > I have already heard the same comment about the size of my patch from Leandro. > (Bug 62709) > GraphicsContext3DInternal is big because it contains wrapper functions to all OpenGL(ES) functions. I have no idea how to split it. I'm not sure if my case can be adjusted your patch. Anyway, when I have a huge patch, I make a patch which has dummy functions first. Then, I make next patches which implement the dummy functions step by step.

(In reply to comment #2) > View in context: https://bugs.webkit.org/attachment.cgi?id=97734&action=review > > > Source/WebCore/platform/graphics/efl/GraphicsContext3DInternal.cpp:61 > > + m_evasGL = ewk_view_evas_gl_create(ewkView); > > Should we call ewk api to create evas GL? > > > Source/WebCore/platform/graphics/efl/GraphicsContext3DInternal.cpp:83 > > + m_context = evas_gl_context_create(m_evasGL, shareContext); > > + if (!m_context) > > + return false; > > + > > + if (!createSurface(ewkView, bRenderDirectlyToEvasGLObject)) > > + return false; > > If we can fail to create here, I think that we should create evas gl. > Is it right? s/create evas gl/clear evas gl/ anyway, this is my mistake after discussed. please ignore it.

Created attachment 97769 [details] Patch

(In reply to comment #6) > (In reply to comment #2) > > View in context: https://bugs.webkit.org/attachment.cgi?id=97734&action=review > > > > > Source/WebCore/platform/graphics/efl/GraphicsContext3DInternal.cpp:61 > > > + m_evasGL = ewk_view_evas_gl_create(ewkView); > > > > Should we call ewk api to create evas GL? I didn't know about evas_object_evas_get(). I have replaced the following code 1 with the code 2. (1) m_evasGL = ewk_view_evas_gl_create(ewkView); (2) Evas* evas = evas_object_evas_get(ewkView); m_evasGL = evas_gl_create(evas); Thank you very very much :) > > > Source/WebCore/platform/graphics/efl/GraphicsContext3DInternal.cpp:83 > > > + m_context = evas_gl_context_create(m_evasGL, shareContext); > > > + if (!m_context) > > > + return false; > > > + > > > + if (!createSurface(ewkView, bRenderDirectlyToEvasGLObject)) > > > + return false; > > > > If we can fail to create here, I think that we should create evas gl. > > Is it right? > > s/create evas gl/clear evas gl/ > anyway, this is my mistake after discussed. please ignore it.

View in context: https://bugs.webkit.org/attachment.cgi?id=97769&action=review > Source/WebCore/platform/graphics/efl/GraphicsContext3DInternal.cpp:59 > + Evas_Object* ewkView = static_cast<Evas_Object*>(hostWindow->platformPageClient()); how do you think view instead of ewkView. > Source/WebCore/platform/graphics/efl/GraphicsContext3DInternal.cpp:70 > + Evas_GL_Context* shareContext = 0; shareContext or sharedContext? Which one is right? > Source/WebCore/platform/graphics/efl/GraphicsContext3DInternal.cpp:74 > + GraphicsContext3D* compositingContext = ewk_view_accelerated_compositing_context_get(ewkView); Is it public api? If not, does we need to guard this to ACCELEARTED_COMPOSITING macro? > Source/WebCore/platform/graphics/efl/GraphicsContext3DInternal.cpp:89 > +bool GraphicsContext3DInternal::createSurface(Evas_Object* ewkView, bool renderDirectlyToEvasGLObject) If createSurface is only for initialize, does we need to separate it? > Source/WebCore/platform/graphics/efl/GraphicsContext3DInternal.cpp:98 > + evas_object_geometry_get(ewkView, &x, &y, &w, &h); I think that we can move this into if (renderDirectlyToEvasGLObject). And evas_object_geometry_get will clear values to 0 although it failed. > Source/WebCore/platform/graphics/efl/GraphicsContext3DInternal.cpp:100 > + int surfaceWidth = 0, surfaceHeight = 0; I prefered these meaningful name, but can we share this with w, h. > Source/WebCore/platform/graphics/efl/GraphicsContext3DInternal.cpp:106 > + } else > + surfaceWidth = surfaceHeight = 1; How about initializing above values to 1 instead of else statement. Other codes looks just bypassed from WebKit's to evas_gl_api's.

Created attachment 97781 [details] Patch

(In reply to comment #10) > Created an attachment (id=97781) [details] > Patch I updated this patch to reflect the comment #9 From Ryuan Choi. Thanks again Ryuan.

Informal r- from my side: > Source/WebCore/platform/graphics/efl/GraphicsContext3DInternal.cpp:2 > + Copyright (C) 2009-2011 Samsung Electronics This file is new, so the copyright should start in 2011. > Source/WebCore/platform/graphics/efl/GraphicsContext3DInternal.cpp:57 > +bool GraphicsContext3DInternal::initialize(GraphicsContext3D::Attributes attrs, HostWindow* hostWindow, bool bRenderDirectlyToEvasGLObject) I'd rather if instead of having a public constructor and this method you could have a static create() method which took care of creating and initializing the object automatically. > Source/WebCore/platform/graphics/efl/GraphicsContext3DInternal.cpp:144 > +void GraphicsContext3DInternal::bindAttribLocation(Platform3DObject program, GC3Duint index, const char* name) I think it makes more sense to make name a String, and call .utf8().data() here. > Source/WebCore/platform/graphics/efl/GraphicsContext3DInternal.cpp:394 > + char* name = static_cast<char*>(malloc(maxNameLength * sizeof(char))); sizeof(char) is always 1. Manually managing the memory allocated here should not be necessary. You can use, for example, an OwnArrayPtr: OwnArrayPtr<char> name = adoptArrayPtr(new char[maxNameLength]); > Source/WebCore/platform/graphics/efl/GraphicsContext3DInternal.cpp:431 > + char* name = static_cast<char*>(malloc(maxNameLength * sizeof(char))); Ditto. > Source/WebCore/platform/graphics/efl/GraphicsContext3DInternal.cpp:486 > + ListHashSet<GC3Denum>::iterator iter = m_syntheticErrors.begin(); I think it's clearer to do GC3Denum err = m_syntheticErrors.first(); m_syntheticErrors.remove(m_syntheticErrors.begin()); return err; > Source/WebCore/platform/graphics/efl/GraphicsContext3DInternal.cpp:545 > + char* log = static_cast<char*>(malloc(logLength * sizeof(char))); Same comment about manual memory management. > Source/WebCore/platform/graphics/efl/GraphicsContext3DInternal.cpp:579 > + char* log = static_cast<char*>(malloc(logLength * sizeof(char))); Ditto. > Source/WebCore/platform/graphics/efl/GraphicsContext3DInternal.cpp:601 > + char* log = static_cast<char*>(malloc(logLength * sizeof(char))); Ditto. > Source/WebCore/platform/graphics/efl/GraphicsContext3DInternal.cpp:1113 > + return 0; Missing notImplemented()? > Source/WebCore/platform/graphics/efl/GraphicsContext3DInternal.h:2 > + Copyright (C) 2009-2011 Samsung Electronics This file is new, so the copyright should start in 2011.

Comment on attachment 97781 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=97781&action=review > Source/WebCore/platform/graphics/efl/GraphicsContext3DInternal.cpp:2 > + Copyright (C) 2009-2011 Samsung Electronics Remvoe 2009. Let's only use 2011.

Created attachment 98115 [details] Patch

(In reply to comment #14) > Created an attachment (id=98115) [details] > Patch I updated this patch to reflect the comment #12 from Raphael. Raphael, thank you for your interest and comments.

(In reply to comment #12) > Informal r- from my side: > > > Source/WebCore/platform/graphics/efl/GraphicsContext3DInternal.cpp:57 > > +bool GraphicsContext3DInternal::initialize(GraphicsContext3D::Attributes attrs, HostWindow* hostWindow, bool bRenderDirectlyToEvasGLObject) > > I'd rather if instead of having a public constructor and this method you could have a static create() method which took care of creating and initializing the object automatically. > GraphicsContext3DInternal::initialize() calls many Evas_GL's create functions. If one of them fails, initialize() returns false, and then the GraphicsContext3DInternal instance will be destroyed. According to your comment about a static create() function, I made the following codes. static PassOwnPtr<GraphicsContext3DInternal> GraphicsContext3DInternal::create(a, b, c) { OwnPtr<GraphicsContext3DInternal> internal = adoptPtr(new GraphicsContext3DInternal()); if (!internal->initialize(a, b, c)) return 0; return internal.release(); } But it couldn't return 0 due to the following compile error. error: conversion from 'int' to non-scalar type 'WTF::PassOwnPtr<WebCore::GraphicsContext3DInternal>' requested So creating and initailizing a instance at once in the static GraphicsContext3DInternal::create() function seems to be difficult because of its return type. GraphicsContext3DInternal instances are created only in GraphicsContext3D::create(), so I think that current implementation might be good.

Comment on attachment 98115 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=98115&action=review When you define functions, you need to adhere consistency in parameter name. AFAIK, we have to adhere WebKit coding style rule in WebCore, > Source/WebCore/platform/graphics/efl/GraphicsContext3DInternal.h:57 > + void blendFuncSeparate(GC3Denum srcRGB, GC3Denum dstRGB, GC3Denum srcAlpha, GC3Denum dstAlpha); you use "A" upper case in srcAlpha. > Source/WebCore/platform/graphics/efl/GraphicsContext3DInternal.h:70 > + void copyTexImage2D(GC3Denum target, GC3Dint level, GC3Denum internalformat, GC3Dint x, GC3Dint y, GC3Dsizei width, GC3Dsizei height, GC3Dint border); In internalformat, you don't use upper case for "f".

(In reply to comment #16) > (In reply to comment #12) > > Informal r- from my side: > > > > > Source/WebCore/platform/graphics/efl/GraphicsContext3DInternal.cpp:57 > > > +bool GraphicsContext3DInternal::initialize(GraphicsContext3D::Attributes attrs, HostWindow* hostWindow, bool bRenderDirectlyToEvasGLObject) > > > > I'd rather if instead of having a public constructor and this method you could have a static create() method which took care of creating and initializing the object automatically. > > > GraphicsContext3DInternal::initialize() calls many Evas_GL's create functions. > If one of them fails, initialize() returns false, and then the GraphicsContext3DInternal instance will be destroyed. > According to your comment about a static create() function, I made the following codes. > static PassOwnPtr<GraphicsContext3DInternal> GraphicsContext3DInternal::create(a, b, c) > { > OwnPtr<GraphicsContext3DInternal> internal = adoptPtr(new GraphicsContext3DInternal()); > if (!internal->initialize(a, b, c)) > return 0; > return internal.release(); > } > But it couldn't return 0 due to the following compile error. > error: conversion from 'int' to non-scalar type 'WTF::PassOwnPtr<WebCore::GraphicsContext3DInternal>' requested > So creating and initailizing a instance at once in the static GraphicsContext3DInternal::create() function seems to be difficult because of its return type. > GraphicsContext3DInternal instances are created only in GraphicsContext3D::create(), so I think that current implementation might be good. Sorry, I should have used "return nullptr;" instead of 0!!!

(In reply to comment #18) > Sorry, I should have used "return nullptr;" instead of 0!!! I'm glad you've figured it out :) I'm looking forward to another patch.

Created attachment 98304 [details] Patch

(In reply to comment #19) > (In reply to comment #18) > > Sorry, I should have used "return nullptr;" instead of 0!!! > > I'm glad you've figured it out :) I'm looking forward to another patch. You are so kind :) I updated this patch to reflect your comment about using the static create function.

It looks good to me now; informal r+ from my side.

How are you testing this code? are you enabling webgl in build-webkit?

Created attachment 128908 [details] Patch

(In reply to comment #23) > How are you testing this code? are you enabling webgl in build-webkit? I'm sorry for the delay in my response. I've actually finished the implementation about accelerated compositing using texmap and GC3D already. I will summit another patch soon, in order to add GC3D files to webkit-efl build when ACCELERATED_COMPOSITING is used. Thanks for your interest.

Comment on attachment 128908 [details] Patch Attachment 128908 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11633543 New failing tests: inspector/protocol/console-agent.html

i think we are already using GTK GraphicsContext3DInternal: https://bugs.webkit.org/show_bug.cgi?id=77296 I think you guys should try to synchronize what you are doing. (In reply to comment #26) > (From update of attachment 128908 [details]) > Attachment 128908 [details] did not pass chromium-ews (chromium-xvfb): > Output: http://queues.webkit.org/results/11633543 > > New failing tests: > inspector/protocol/console-agent.html

Created attachment 128939 [details] re-upload

Created attachment 128947 [details] re-upload2

(In reply to comment #27) > i think we are already using GTK GraphicsContext3DInternal: https://bugs.webkit.org/show_bug.cgi?id=77296 > > I think you guys should try to synchronize what you are doing. > Bug 77296 makes it shareable to use GC3DPrivate with GLX backend. But, this patch is a EFL platform specific using Evas_GL backend. (As you know, the initialization of GLX is a lot different from Evas_GL's.) Evas_GL could make it easy to render to Evas and provide options to take advantage of performance gains. And I talked with 'ChangSeok Oh' who's the author of Bug 77296. He agreed that GC3DPrivate in efl port should be seperated from it in glx. Thanks!

Comment on attachment 128947 [details] re-upload2 Rejecting attachment 128947 [details] from commit-queue. New failing tests: inspector/protocol/console-agent.html Full output: http://queues.webkit.org/results/11634005

Comment on attachment 128947 [details] re-upload2 Try cq+ once more because I think that complained message is not related to this bug.

Comment on attachment 128947 [details] re-upload2 Clearing flags on attachment: 128947 Committed r109061: <http://trac.webkit.org/changeset/109061>

All reviewed patches have been landed. Closing bug.