The ANGLE snapshot in WebKit is pretty old. The latest revision of ANGLE has many bug fixes and improvements. Since ANGLE is in a very active state of development, it would also be nice to have an automated tool to update the WebKit snapshot.
Also, the current implementation in Mac port handling ANGLE is both inefficient and buggy. Look at WebKit/chromium/src/WebGraphicsContext3DDefaultImpl.cpp for comparison.
Any status on this? Getting *really* out of date now and making it difficult to keep changes working with WebKit.
I have no problem with updating ANGLE, I just haven't had the time. Please feel free to snag it, or I will get to it in a couple of weeks or so...
I'll give it a shot!
Since ANGLE interfaces changed, you need to fix WebCore/platform/graphics/ANGLEWebkitBridge.cpp and also shader related functions in WebCore/platform/graphics/opengl/GraphicsContext3DOpenGL.cpp. You can look at http://src.chromium.org/viewvc/chrome/trunk/src/webkit/gpu/webgraphicscontext3d_in_process_impl.cc?view=log. The implementation ins GraphicsContext3DOpenGL.cpp should be very similar.
Created attachment 79053 [details] ANGLE r533 This updates ANGLE to r533. Tested against the WebGL conformance tests and several of the WebGL samples - all seem to work OK. Only unique changes were in WebCore to support API changes made to ANGLE since the last snapshot several months ago.
Comment on attachment 79053 [details] ANGLE r533 View in context: https://bugs.webkit.org/attachment.cgi?id=79053&action=review Looks good in general. Thanks for taking care of this long-standing task. > Source/WebCore/platform/graphics/ANGLEWebKitBridge.cpp:85 > + char* logBuffer = new char[logSize]; Should use OwnArrayPtr<char> here. > Source/WebCore/platform/graphics/ANGLEWebKitBridge.cpp:98 > + char* translationBuffer = new char[translationLength]; Should use OwnArrayPtr<char> here.
Comment on attachment 79053 [details] ANGLE r533 Ben, seconded; thanks for taking care of this long outstanding task. Could you please address Mo's review comments and upload a revised patch? I'll help you get it committed.
Created attachment 79343 [details] ANGLE r533 - revised patch This patch applies the review feedback and uses OwnArrayPtr<char> instead of raw character buffers.
Attachment 79343 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/ThirdParty/ANGLE/ANGLE.xcodeproj/pr..." exit_code: 1 Last 3072 characters of output: ANGLE/src/compiler/glslang_tab.cpp:4699: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/ThirdParty/ANGLE/src/compiler/glslang_tab.cpp:4706: Place brace on its own line for function definitions. [whitespace/braces] [4] Source/ThirdParty/ANGLE/src/compiler/glslang_tab.cpp:4706: glslang_parse is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Source/ThirdParty/ANGLE/src/compiler/Initialize.h:20: The parameter name "type" adds no information, so it should be removed. [readability/parameter_name] [5] Source/ThirdParty/ANGLE/src/compiler/Initialize.h:20: The parameter name "spec" adds no information, so it should be removed. [readability/parameter_name] [5] Source/ThirdParty/ANGLE/src/compiler/Initialize.h:21: The parameter name "resources" adds no information, so it should be removed. [readability/parameter_name] [5] Source/ThirdParty/ANGLE/src/compiler/Initialize.h:28: The parameter name "type" adds no information, so it should be removed. [readability/parameter_name] [5] Source/ThirdParty/ANGLE/src/compiler/Initialize.h:28: The parameter name "spec" adds no information, so it should be removed. [readability/parameter_name] [5] Source/ThirdParty/ANGLE/src/compiler/Initialize.h:29: The parameter name "resources" adds no information, so it should be removed. [readability/parameter_name] [5] Source/ThirdParty/ANGLE/src/compiler/Initialize.h:32: The parameter name "resources" adds no information, so it should be removed. [readability/parameter_name] [5] Source/ThirdParty/ANGLE/src/compiler/Initialize.h:33: The parameter name "extensionBehavior" adds no information, so it should be removed. [readability/parameter_name] [5] Source/ThirdParty/ANGLE/src/libGLESv2/Fence.h:9: #ifndef header guard has wrong style, please use: WTF_Fence_h [build/header_guard] [5] Source/ThirdParty/ANGLE/src/libGLESv2/Fence.h:16: Alphabetical sorting problem. [build/include_order] [4] Source/ThirdParty/ANGLE/src/libGLESv2/Fence.h:19: This { should be at the end of the previous line [whitespace/braces] [4] Source/ThirdParty/ANGLE/src/libGLESv2/Fence.h:22: This { should be at the end of the previous line [whitespace/braces] [4] Source/ThirdParty/ANGLE/src/libGLESv2/Fence.h:43: One space before end of line comments [whitespace/comments] [5] Source/ThirdParty/ANGLE/src/compiler/glslang_tab.h:41: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/ThirdParty/ANGLE/src/compiler/glslang_tab.h:133: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/ThirdParty/ANGLE/src/compiler/glslang_tab.h:231: Extra space for operator ! [whitespace/operators] [4] Source/ThirdParty/ANGLE/src/compiler/glslang_tab.h:234: This { should be at the end of the previous line [whitespace/braces] [4] Source/ThirdParty/ANGLE/src/compiler/glslang_tab.h:267: Tab found; better to use spaces [whitespace/tab] [1] Total errors found: 3817 in 97 files If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #10) > Attachment 79343 [details] did not pass style-queue: > > Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/ThirdParty/ANGLE/ANGLE.xcodeproj/pr..." exit_code: 1 So, yeah, ANGLE doesn't pass the style check. But the unique changes I made under WebCore do pass (so all errors are from ANGLE/).
(In reply to comment #11) > (In reply to comment #10) > > Attachment 79343 [details] [details] did not pass style-queue: > > > > Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/ThirdParty/ANGLE/ANGLE.xcodeproj/pr..." exit_code: 1 > > So, yeah, ANGLE doesn't pass the style check. But the unique changes I made under WebCore do pass (so all errors are from ANGLE/). Not a problem for the time being. Mo, does this update look good to you?
Looks good. Thank you.
Comment on attachment 79343 [details] ANGLE r533 - revised patch Thanks, Mo. Let's see whether the commit queue can handle this monster patch.
Comment on attachment 79343 [details] ANGLE r533 - revised patch Rejecting attachment 79343 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-4', 'apply-..." exit_code: 2 Last 500 characters of output: rc/libEGL/libEGL.cpp patching file Source/ThirdParty/ANGLE/src/libEGL/Config.cpp patching file Source/ThirdParty/ANGLE/src/libEGL/Display.cpp patching file Source/ThirdParty/ANGLE/src/libEGL/Surface.cpp patching file Source/ThirdParty/ANGLE/src/libEGL/Display.h patching file Source/ThirdParty/ANGLE/src/libEGL/Surface.h patching file Source/ThirdParty/ANGLE/ChangeLog Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Kenneth Russell', u'--..." exit_code: 1 Full output: http://queues.webkit.org/results/7550178
(In reply to comment #15) > (From update of attachment 79343 [details]) > Rejecting attachment 79343 [details] from commit-queue. Hmmm... The log snippet doesn't seem to show anything useful. Being my first submission, what exactly is the error here? What should I try next?
(In reply to comment #16) > (In reply to comment #15) > > (From update of attachment 79343 [details] [details]) > > Rejecting attachment 79343 [details] [details] from commit-queue. > > Hmmm... The log snippet doesn't seem to show anything useful. Being my first submission, what exactly is the error here? What should I try next? I'm not sure what the error is. I'm in the process of applying your patch to a local workspace and committing it by hand.
Yeah, our error reporting from svn-apply is bad. :(
Committed r76091: <http://trac.webkit.org/changeset/76091>
It turns out there were tabs in three autogenerated files: glslang_lex.cpp, glslang_tab.cpp and glslang_tab.h in Source/ThirdParty/ANGLE/src/compiler/. I added the allow-tabs property to these and was able to commit the patch by hand. I'm not sure whether this was actually the failure that caused the commit queue to fail to process the patch.
Note: the Mac Release builds broke with this patch; one-liner fix committed in http://trac.webkit.org/changeset/76093 , being upstreamed under http://codereview.appspot.com/4043042 .
(In reply to comment #21) > Note: the Mac Release builds broke with this patch; one-liner fix committed in http://trac.webkit.org/changeset/76093 , being upstreamed under http://codereview.appspot.com/4043042 . Interesting - I did a build-webkit (set to release) and didn't hit this error - so I don't make the same mistake again, how did you build?
(In reply to comment #22) > (In reply to comment #21) > > Note: the Mac Release builds broke with this patch; one-liner fix committed in http://trac.webkit.org/changeset/76093 , being upstreamed under http://codereview.appspot.com/4043042 . > > Interesting - I did a build-webkit (set to release) and didn't hit this error - so I don't make the same mistake again, how did you build? This may have been a Leopard-only build failure. I didn't build Release mode (on Snow Leopard) until after committing the patch. The only way to have caught this in advance therefore would have been to either use the commit queue (which didn't work) or to have built on Leopard.
It turns out to also have been necessary to remove the steps from ANGLE.xcodeproj which generated the grammar from the glslang.l/y files. The generated sources were already checked in and the version of these tools on Leopard was too old to handle one of the constructs in the grammar file. See http://trac.webkit.org/changeset/76095 .