|Summary:||[EFL] Add GraphicsContext3DEfl for WebGL and accelerated compositing|
|Product:||WebKit||Reporter:||Hyowon Kim <hw1008.kim>|
|Component:||WebKit EFL||Assignee:||Nobody <webkit-unassigned>|
|Severity:||Normal||CC:||gyuyoung.kim, gyuyoung.kim, kenneth, kevin.cs.oh, leandro, lucas.de.marchi, rakuco, ryuan.choi, sangseok.lim, tonikitoo, webkit.review.bot|
|Version:||528+ (Nightly build)|
|Bug Depends on:||62707, 62961|
|Bug Blocks:||62695, 62959|
Description Hyowon Kim 2011-06-15 02:40:29 PDT
patch will be added.
Comment 2 Hyowon Kim 2011-06-17 05:44:45 PDT
(In reply to comment #1) > Created an attachment (id=97589) [details] > Patch This patch adds the GraphicsContext3DInternal class using Evas_GL APIs. Evas_GL APIs allow to use OpenGL to render to specially set up evas_image_objects which act as render target surfaces. GraphicsContext3D in Efl port will be used for accelerated compositing as well as WebGL.
Comment 3 Leandro Pereira 2011-06-17 08:09:11 PDT
Comment on attachment 97589 [details] Patch Patch is *huge*; could you please split it into smaller, preferably less than 20kB parts?
Comment 5 Hyowon Kim 2011-06-19 23:05:02 PDT
(In reply to comment #4) > Created an attachment (id=97745) [details] > Patch The patch has split into three bugs. This patch contains only GraphicsContext3DEfl.cpp. - GraphicsContext3D.h : Bug 62959 - GraphicsContext3DInternal.h/cpp : Bug 62961 I'm looking forward to the review.
Comment 6 Raphael Kubo da Costa (:rakuco) 2011-06-20 07:06:39 PDT
Reviewing this depends on changes made to bug 62961, so there's not much to say about this one. Is there going to be another patch plugging these new files into the build system?
Comment 7 Hyowon Kim 2011-06-20 19:40:48 PDT
(In reply to comment #6) > Reviewing this depends on changes made to bug 62961, so there's not much to say about this one. > > Is there going to be another patch plugging these new files into the build system? Yes, I will upload another patch for adding new files to build-script files. Thanks.
Comment 9 Raphael Kubo da Costa (:rakuco) 2011-06-22 07:10:21 PDT
It looks mostly OK, it just needs to be updated when the final version of GraphicsContext3DInternal is approved. > Source/WebCore/platform/graphics/efl/GraphicsContext3DEfl.cpp:29 > +#include <wtf/text/CString.h> I don't think you need this header after you pass a String directly to Graphics
Comment 10 Leandro Pereira 2011-06-22 07:44:50 PDT
Comment on attachment 98116 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=98116&action=review This looks like a huge wrapper class; isn't it possible to share most of this code with other ports, as there are few references to Evas and such? > Source/WebCore/platform/graphics/efl/GraphicsContext3DEfl.cpp:35 > + bool renderDirectlyToEvasGLObject = (renderStyle == RenderDirectlyToHostWindow) ? true : false; No need to use a ternary operator here; comparisons already returns a boolean. > Source/WebCore/platform/graphics/efl/GraphicsContext3DEfl.cpp:142 > + m_internal->bufferData(target, size, /* data */ 0, usage); Is this comment really needed? > Source/WebCore/platform/graphics/efl/GraphicsContext3DEfl.cpp:167 > + m_internal->clearColor(r, g, b, a); I'd use more descriptive names here. > Source/WebCore/platform/graphics/efl/GraphicsContext3DEfl.cpp:177 > + m_internal->clearStencil(s); Ditto.
Comment 12 Hyowon Kim 2011-06-22 19:38:52 PDT
(In reply to comment #11) > Created an attachment (id=98298) [details] > Patch I updated this patch to reflect the comment #9 and #10.
Comment 13 Hyowon Kim 2011-06-22 20:02:56 PDT
(In reply to comment #10) > (From update of attachment 98116 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=98116&action=review > > This looks like a huge wrapper class; isn't it possible to share most of this code with other ports, as there are few references to Evas and such? Yes, most of this code are sharable. Source/WebCore/platform/graphics/GraphicsContext3D.h:934 // FIXME: ideally this would be used on all platforms. #if PLATFORM(CHROMIUM) || PLATFORM(QT) || PLATFORM(GTK) friend class GraphicsContext3DInternal; OwnPtr<GraphicsContext3DInternal> m_internal; #endif Chromium & QT already have their own GraphicsContext3DInternal class. How can you make it common? I have no concrete plans yet. Thank you for your comment :)
Comment 14 Raphael Kubo da Costa (:rakuco) 2011-06-27 05:17:08 PDT
Informal r+ from my side now that GraphicsContext3DInternal looks OK too.
Comment 15 Eric Seidel (no email) 2011-09-12 15:37:56 PDT
Comment on attachment 98298 [details] Patch rs=me.
Comment 16 WebKit Review Bot 2011-09-12 20:11:07 PDT
Comment on attachment 98298 [details] Patch Clearing flags on attachment: 98298 Committed r95002: <http://trac.webkit.org/changeset/95002>
Comment 17 WebKit Review Bot 2011-09-12 20:11:13 PDT
All reviewed patches have been landed. Closing bug.