Bug 77308

Created attachment 124554 [details] Patch

Does this patch have any relation with the one in bug 62961?

you should remove fast/canvas/webgl from Skipped file.

(In reply to comment #2) > Does this patch have any relation with the one in bug 62961? A little. I also checked the patch, but it looks too old and is not valid anymore. If someone wants to implement WebGL with EVAS_GL backend like the patch, it's a better idea to add a new option to choose gl backend in my opinion.

Does it mean Samsung (or anyone else, for that matter) is not working on that approach anymore and I can close that bug report?

(In reply to comment #5) > Does it mean Samsung (or anyone else, for that matter) is not working on that approach anymore and I can close that bug report? No, it doesn't. Using evasgl is still a valid approach for WebGL and it could be one option to choose. I believe Samsung(or anyone else) is going to contribute it near soon. But on my side, current implementation of GraphicsContext3DEfl has something wrong. First of all, GraphicsContext3DInternal is not a valid class name. Second, OpenGL API wrapper is useless for supporting WebGL if we follow the way of other ports. Anyway I think the patch for bug 62961 is needed to revise. This patch is just an another option that we can choose to support WebGL.

(In reply to comment #3) > you should remove fast/canvas/webgl from Skipped file. Which file do you mean? In here, LayoutTests/platform/efl/Skipped?

yup (In reply to comment #7) > (In reply to comment #3) > > you should remove fast/canvas/webgl from Skipped file. > > Which file do you mean? In here, LayoutTests/platform/efl/Skipped?

(In reply to comment #3) > you should remove fast/canvas/webgl from Skipped file. And please make sure the tests are passing :)

Created attachment 124747 [details] Patch

Attachment 124747 [details] did not pass style-queue: Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9 Updating OpenSource First, rewinding head to replay your work on top of it... Applying: Fix compilation errors on build-webkit --debug --no-workers on mac. Using index info to reconstruct a base tree... Falling back to patching base and 3-way merge... Auto-merging LayoutTests/ChangeLog CONFLICT (content): Merge conflict in LayoutTests/ChangeLog Auto-merging LayoutTests/platform/qt/Skipped CONFLICT (content): Merge conflict in LayoutTests/platform/qt/Skipped Auto-merging Source/WebCore/ChangeLog CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog Failed to merge in the changes. Patch failed at 0001 Fix compilation errors on build-webkit --debug --no-workers on mac. When you have resolved this problem run "git rebase --continue". If you would prefer to skip this patch, instead run "git rebase --skip". To restore the original branch and stop rebasing run "git rebase --abort". rebase refs/remotes/origin/master: command returned error: 1 Died at Tools/Scripts/update-webkit line 164. If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 124747 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=124747&action=review You should send a test version of the patch setting ENABLE_WEBGL to ON so we can check the build passes. > Source/WebCore/CMakeLists.txt:2209 > + platform/graphics/GraphicsContext3D.cpp Why? It's not sorted anymore. > Source/WebKit/efl/ewk/ewk_view.cpp:651 > + priv->pageSettings->setWebGLEnabled(true); What happens if you pass true here but have ENABLE_WEBGL set to OFF (the default configuration)?

Comment on attachment 124747 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=124747&action=review Thank you for review. :) AFAIK, WebGL has remained experimental. We usually disable that kind of feature as the default configuration. Same situation in GTK, MAC port and build-webkit. In addition, above style alarm looks false... :p >> Source/WebCore/CMakeLists.txt:2209 >> + platform/graphics/GraphicsContext3D.cpp > > Why? It's not sorted anymore. Sorry, I couldn't get the point. 'p' < 'r', right? >> Source/WebKit/efl/ewk/ewk_view.cpp:651 >> + priv->pageSettings->setWebGLEnabled(true); > > What happens if you pass true here but have ENABLE_WEBGL set to OFF (the default configuration)? In that case, we can't create webgl context, so that we wouldn't see any webgl image.

Comment on attachment 124747 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=124747&action=review Just a couple tiny style nits. Please fix them before landing the patch. > Source/WebCore/platform/graphics/efl/DrawingBufferEfl.cpp:40 > +DrawingBuffer::DrawingBuffer(GraphicsContext3D* context, > + const IntSize& size, > + bool multisampleExtensionSupported, > + bool packedDepthStencilExtensionSupported, > + bool separateBackingTexture) In WebKit we usually keep parameter lists on one line. > Source/WebCore/platform/graphics/efl/DrawingBufferEfl.cpp:66 > + // create a texture to render into This comment should start with a capital letter and end with a period.

Comment on attachment 124747 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=124747&action=review >>> Source/WebCore/CMakeLists.txt:2209 >>> + platform/graphics/GraphicsContext3D.cpp >> >> Why? It's not sorted anymore. > > Sorry, I couldn't get the point. 'p' < 'r', right? You need to take casing into account. >>> Source/WebKit/efl/ewk/ewk_view.cpp:651 >>> + priv->pageSettings->setWebGLEnabled(true); >> >> What happens if you pass true here but have ENABLE_WEBGL set to OFF (the default configuration)? > > In that case, we can't create webgl context, so that we wouldn't see any webgl image. So enabling WebGL in the source code even if WebGL is disabled in the build won't do any harm?

Let me repeat that you should send a test version of the patch setting ENABLE_WEBGL to ON so we can check the build passes. By the way, have you checked that the layout tests you have enabled pass?

(In reply to comment #16) > Let me repeat that you should send a test version of the patch setting ENABLE_WEBGL to ON so we can check the build passes. By the way, have you checked that the layout tests you have enabled pass? If the feature is disabled during build, the test script should be smart enough to not run the tests.

(In reply to comment #17) > If the feature is disabled during build, the test script should be smart enough to not run the tests. I meant running the tests locally (there's no bot running EFL layout tests yet).

(In reply to comment #16) > Let me repeat that you should send a test version of the patch setting ENABLE_WEBGL to ON so we can check the build passes. Hm.. I'm a little confused. Do you want that I send you the patch setting WEBGL ON personally and leave WEBGL OFF as the default? Or we should set ENABLE_WEBGL ON as the default though the feature is experimental? > By the way, have you checked that the layout tests you have enabled pass? No. I haven't yet. Let me do it after updating the patch.

(In reply to comment #19) > Hm.. I'm a little confused. Do you want that I send you the patch setting WEBGL ON personally and leave WEBGL OFF as the default? Or we should set ENABLE_WEBGL ON as the default though the feature is experimental? The former -- just send the patch with the default toggled _without_ r? and cq? and manually ask the EWS bots to run, so that the EFL bot tries to build the patch with the enabled code path and we can check it works fine.

Comment on attachment 124747 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=124747&action=review Thank you all. >>>> Source/WebCore/CMakeLists.txt:2209 >>>> + platform/graphics/GraphicsContext3D.cpp >>> >>> Why? It's not sorted anymore. >> >> Sorry, I couldn't get the point. 'p' < 'r', right? > > You need to take casing into account. Excuse me, Could you tell me in detail? Do you want my explanation why I sorted files to build in ChangeLog? or Want to relocate GraphicsContext3D.cpp before DrawingBuffer.cpp? >> Source/WebCore/platform/graphics/efl/DrawingBufferEfl.cpp:40 >> + bool separateBackingTexture) > > In WebKit we usually keep parameter lists on one line. Done. But if you think DrawingBufferGtk.cpp is also needed to modify like this, let me know. :) >> Source/WebCore/platform/graphics/efl/DrawingBufferEfl.cpp:66 >> + // create a texture to render into > > This comment should start with a capital letter and end with a period. ditto. >>>> Source/WebKit/efl/ewk/ewk_view.cpp:651 >>>> + priv->pageSettings->setWebGLEnabled(true); >>> >>> What happens if you pass true here but have ENABLE_WEBGL set to OFF (the default configuration)? >> >> In that case, we can't create webgl context, so that we wouldn't see any webgl image. > > So enabling WebGL in the source code even if WebGL is disabled in the build won't do any harm? Right. No problem to build and run.

(In reply to comment #19) > (In reply to comment #16) > > Let me repeat that you should send a test version of the patch setting ENABLE_WEBGL to ON so we can check the build passes. > > Hm.. I'm a little confused. Do you want that I send you the patch setting WEBGL ON personally and leave WEBGL OFF as the default? Or we should set ENABLE_WEBGL ON as the default though the feature is experimental? > > > By the way, have you checked that the layout tests you have enabled pass? > No. I haven't yet. Let me do it after updating the patch. I've run DRT locally, but nothing special regarding webgl test cases.

(In reply to comment #21) > >> Sorry, I couldn't get the point. 'p' < 'r', right? > > > > You need to take casing into account. > > Excuse me, Could you tell me in detail? Do you want my explanation why I sorted files to build in ChangeLog? or Want to relocate GraphicsContext3D.cpp before DrawingBuffer.cpp? You just need to move GraphicsContext3D.cpp back to its original position in CMakeLists.txt: 'p' < 'r', but 'G' < 'g'.

(In reply to comment #22) > I've run DRT locally, but nothing special regarding webgl test cases. Do you mean the webgl tests you have removed from the skipped list are all passing? If so, OK from my side.

Created attachment 125482 [details] Patch

Created attachment 125488 [details] Patch

(In reply to comment #23) > (In reply to comment #21) > > >> Sorry, I couldn't get the point. 'p' < 'r', right? > > > > > > You need to take casing into account. > > > > Excuse me, Could you tell me in detail? Do you want my explanation why I sorted files to build in ChangeLog? or Want to relocate GraphicsContext3D.cpp before DrawingBuffer.cpp? > > You just need to move GraphicsContext3D.cpp back to its original position in CMakeLists.txt: 'p' < 'r', but 'G' < 'g'. O.K. I see. I was confused, since following lines in OptionsEfl.cmake. I have thought we don't care of uppercase/lowcase. >html/MediaFragmentURIParser.cpp >html/shadow/MediaControlElements.cpp >html/TimeRanges.cpp I also fixed it. :)

Comment on attachment 125488 [details] Patch Assuming all the raised issues have been fixed, cq+'ing the patch.

Comment on attachment 125488 [details] Patch Clearing flags on attachment: 125488 Committed r106772: <http://trac.webkit.org/changeset/106772>

All reviewed patches have been landed. Closing bug.