WebGLRenderingCOntext::getExtension does not check if compressed_texture_s3tc is supported before returning a WebGLExtension object.
Created attachment 172581 [details] proposed patch proposed patch
Is this patch up for review?
Yes, this patch is up for review. (In reply to comment #2) > Is this patch up for review?
Comment on attachment 172581 [details] proposed patch Please read http://www.webkit.org/coding/contributing.html
Created attachment 173662 [details] Patch
Attachment 173662 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/html/canvas/WebGLRenderingContext.cpp:2369: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 1 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 173674 [details] Patch
(In reply to comment #7) > Created an attachment (id=173674) [details] > Patch Do you mean this to be BlackBerry specific? If so you need to ifdef (PLATFORM(BB)) the change. If this is for all ports, the [BlackBerry] in the title is misleading, and we want non BB reviewers to look at it.
Created attachment 175016 [details] Patch
(In reply to comment #8) Yes, the patch is meant to be BlackBerry specific, and the change you suggested has been made. > (In reply to comment #7) > > Created an attachment (id=173674) [details] [details] > > Patch > > Do you mean this to be BlackBerry specific? If so you need to ifdef (PLATFORM(BB)) the change. If this is for all ports, the [BlackBerry] in the title is misleading, and we want non BB reviewers to look at it.
(In reply to comment #10) > (In reply to comment #8) > Yes, the patch is meant to be BlackBerry specific, and the change you suggested has been made. > > (In reply to comment #7) > > > Created an attachment (id=173674) [details] [details] [details] > > > Patch > > > > Do you mean this to be BlackBerry specific? If so you need to ifdef (PLATFORM(BB)) the change. If this is for all ports, the [BlackBerry] in the title is misleading, and we want non BB reviewers to look at it. Might be also good for other platforms?
(In reply to comment #11) We're not sure about the string that's being used to check whether s3tc is supported or not. We are using this website (http://www.opengl.org/registry/specs/EXT/texture_compression_s3tc.txt) as our basis for the string being passed in, but we're not a 100% sure and don't want other platforms to suffer because of this. > (In reply to comment #10) > > (In reply to comment #8) > > Yes, the patch is meant to be BlackBerry specific, and the change you suggested has been made. > > > (In reply to comment #7) > > > > Created an attachment (id=173674) [details] [details] [details] [details] > > > > Patch > > > > > > Do you mean this to be BlackBerry specific? If so you need to ifdef (PLATFORM(BB)) the change. If this is for all ports, the [BlackBerry] in the title is misleading, and we want non BB reviewers to look at it. > > Might be also good for other platforms?
(In reply to comment #12) > (In reply to comment #11) > We're not sure about the string that's being used to check whether s3tc is supported or not. We are using this website (http://www.opengl.org/registry/specs/EXT/texture_compression_s3tc.txt) as our basis for the string being passed in, but we're not a 100% sure and don't want other platforms to suffer because of this. > If a platform does support GL_EXT_texture_compression_s3tc but Extensions3D::supports(GL_EXT_texture_compression_s3tc) returns false, I would think it is a bug in that port, and should be fixed by people who maintain that port. I think this is actually a cross-platform fix. Can you create a layout-test and also get WebGL reviewers involved? class Extensions3D { public: virtual ~Extensions3D() {} // Supported extensions: // GL_EXT_texture_format_BGRA8888 // GL_EXT_read_format_bgra // GL_ARB_robustness // GL_ARB_texture_non_power_of_two / GL_OES_texture_npot // GL_EXT_packed_depth_stencil / GL_OES_packed_depth_stencil // GL_ANGLE_framebuffer_blit / GL_ANGLE_framebuffer_multisample // GL_OES_texture_float // GL_OES_standard_derivatives // GL_OES_rgb8_rgba8 // GL_OES_vertex_array_object // GL_ANGLE_translated_shader_source // GL_ARB_texture_rectangle (only the subset required to // implement IOSurface binding; it's recommended to support // this only on Mac OS X to limit the amount of code dependent // on this extension) // GL_EXT_texture_compression_dxt1 // GL_EXT_texture_compression_s3tc // GL_OES_compressed_ETC1_RGB8_texture // GL_IMG_texture_compression_pvrtc // EXT_texture_filter_anisotropic // GL_EXT_debug_marker // GL_CHROMIUM_copy_texture // GL_CHROMIUM_flipy // Takes full name of extension; for example, // "GL_EXT_texture_format_BGRA8888". virtual bool supports(const String&) = 0;
Comment on attachment 173674 [details] Patch No longer obsolete, since it's being considered for all platforms.
Comment on attachment 175016 [details] Patch Obsolete.
Comment on attachment 173674 [details] Patch Rejecting attachment 173674 [details] from review queue. akaushik@rim.com does not have reviewer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py. - If you do not have reviewer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags. - If you have reviewer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed). The commit-queue restarts itself every 2 hours. After restart the commit-queue will correctly respect your reviewer rights.
Created attachment 175248 [details] Patch
Comment on attachment 175248 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=175248&action=review Looks fair overall but needs a little more work. > Source/WebCore/html/canvas/WebGLRenderingContext.cpp:2382 > + && m_context->getExtensions()->supports("GL_EXT_texture_compression_s3tc")) { Use WebGLCompressedTextureS3TC::supported(WebGLRenderingContext*) instead to keep the checks here and WebGLRenderingContext::getSupportedExtensions symmetric.
Created attachment 175505 [details] Patch
Comment on attachment 175505 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=175505&action=review Sorry for the delay getting back to this. Looks good; r=me. Please make the requested cleanups. You can then add my name to the "Reviewed by" line by hand, upload a revised patch, and any committer can cq+ it. > LayoutTests/ChangeLog:4 > + LayoutTests: Remove this and surrounding blank lines. > LayoutTests/fast/canvas/webgl/webgl-compressed-texture-s3tc.html:1 > +<!DOCTYPE html> Please clean up the weird characters at the beginning of this line.
Created attachment 176808 [details] Patch
Comment on attachment 176808 [details] Patch Rejecting attachment 176808 [details] from commit-queue. rwlbuis@gmail.com does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py. - If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags. - If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed). The commit-queue restarts itself every 2 hours. After restart the commit-queue will correctly respect your committer rights.
Sorry, somehow this got lost. This issue was recently rediscovered in https://bugs.webkit.org/show_bug.cgi?id=109508 and the code change was made, so the attached patch will now have conflicts, but if you want to land the test, that would be useful.
Comment on attachment 176808 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=176808&action=review > Source/WebCore/html/canvas/WebGLRenderingContext.cpp:2382 > + if (equalIgnoringCase(name, "WEBKIT_WEBGL_compressed_texture_s3tc") > + && WebGLCompressedTextureS3TC::supported(this)) { Normally we’d just put this on one line.