Bug 60377

Created attachment 92593 [details] Fix the debug version plus test case I have attached the fix for test case. The comment was already there but not followed correctly. We do not need to put assert in useProgram as program can be null (according to the test case).

Comment on attachment 92593 [details] Fix the debug version plus test case Need a ChangeLog and test.

Created attachment 93319 [details] Second version of the fix Trying again after editing the changelog.

Created attachment 93320 [details] Test case

Attachment 93319 [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/WebKit2/ChangeLog:6: Line contains tab character. [whitespace/tab] [5] Source/WebCore/ChangeLog:6: Line contains tab character. [whitespace/tab] [5] Total errors found: 2 in 4 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 93319 [details] Second version of the fix Removing because of format failing.

Created attachment 93321 [details] Trying Fix again

Comment on attachment 93321 [details] Trying Fix again View in context: https://bugs.webkit.org/attachment.cgi?id=93321&action=review > Source/WebKit2/Shared/WebPreferencesStore.h:61 > - macro(WebGLEnabled, webGLEnabled, Bool, bool, false) \ > + macro(WebGLEnabled, webGLEnabled, Bool, bool, true) \ This enables WebGL for all WK2 ports, that kind of change should not be done behind a bug marked [Qt]. Not to mention that it has nothing to do with the failing test this bug is about.

(In reply to comment #8) > This enables WebGL for all WK2 ports, that kind of change should not be done behind a bug marked [Qt]. > Not to mention that it has nothing to do with the failing test this bug is about. I agree. But i put this in the patch just for everybody to enable webgl without changing the code. Any i take out this part from the patch.

Created attachment 93807 [details] Removing the webgl enabler for wk2

Comment on attachment 93807 [details] Removing the webgl enabler for wk2 You need to describe the change in the changelog, like why the assertion is not needed. Also, without changelog, it looks like you are fixing 2 unrelated things: -The assert which is Qt specific -The uniform name, which is common code. I would prefer 2 patches with the test for each change. Finally, how is this related to WebKit 2? Your changes only WebCore common code?

Nice to see that more people are contributing to this work :) Fix itself looks good but why you have added ASSERT()? How is it related fixing *this* particular bug?

(In reply to comment #12) > Fix itself looks good but why you have added ASSERT()? How is it related fixing *this* particular bug? I have not added but removed the assert because in debug mode compilation, the test case is giving assertion failed. As the test case itself mentions that it is ok to have program NULL, we do not need it in any case.

(In reply to comment #11) > (From update of attachment 93807 [details]) > You need to describe the change in the changelog, like why the assertion is not needed. Ok i will do so. > > Also, without changelog, it looks like you are fixing 2 unrelated things: Both are related to same test case. Thats why i created a single patch for it. Also both are one liners. so in my view creating a single patch makes sense. > -The assert which is Qt specific agreed. > -The uniform name, which is common code. agreed. I have not checked it with other platforms but i have mentioned the possibility that it might be reproducible on other platforms. I was just checking it on qt webkit2. > > I would prefer 2 patches with the test for each change. > > Finally, how is this related to WebKit 2? Your changes only WebCore common code? I had only tested with Webkit2 for Qt. Now i tested with qt webkit and got the same behaviour. so will remove wk2 from the summary.

Created attachment 93949 [details] Another patch for fix

Comment on attachment 93949 [details] Another patch for fix Putting for review.

Attachment 93949 [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:2446: Tab found; better to use spaces [whitespace/tab] [1] 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 93952 [details] Another patch for fix

Comment on attachment 93952 [details] Another patch for fix View in context: https://bugs.webkit.org/attachment.cgi?id=93952&action=review > Source/WebCore/ChangeLog:8 > + [Qt] fast/canvas/webgl/gl-uniform-arrays.html failing for Qt on Linux > + https://bugs.webkit.org/show_bug.cgi?id=60377 > + > + LayoutTests/fast/canvas/webgl/gl-uniform-arrays.html It would be nice to have a description of what was causing the problem and how this patch solves it.

Created attachment 94081 [details] More description about the issue in changelog

(In reply to comment #19) > (From update of attachment 93952 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=93952&action=review > > > Source/WebCore/ChangeLog:8 > > + [Qt] fast/canvas/webgl/gl-uniform-arrays.html failing for Qt on Linux > > + https://bugs.webkit.org/show_bug.cgi?id=60377 > > + > > + LayoutTests/fast/canvas/webgl/gl-uniform-arrays.html > > It would be nice to have a description of what was causing the problem and how this patch solves it. The problem was that we were truncating the name of the active uniform array without checking if it contains the array braces "[0]". As a result we were truncating the actual name of the active uniform. My patch checks for the array braces before taking them out. Also we do not need assert in useProgram as program can be null. I have added these comments to the changelog as well.

Comment on attachment 93952 [details] Another patch for fix Clearing that one because a new version has been uploaded.

Comment on attachment 94081 [details] More description about the issue in changelog View in context: https://bugs.webkit.org/attachment.cgi?id=94081&action=review > Source/WebCore/ChangeLog:10 > + for an array of active uniform, we can receive just a name without Uppercase for the first letter, it's a sentence. > Source/WebCore/ChangeLog:11 > + array braces. Currently i donot see any [0] in the active uniform arrays name missing space. > Source/WebCore/ChangeLog:12 > + and as a result we were trucating the the actual name of the active uniforms. typo again.

Created attachment 94622 [details] Fixing typos.

Comment on attachment 94622 [details] Fixing typos. View in context: https://bugs.webkit.org/attachment.cgi?id=94622&action=review > Source/WebCore/html/canvas/WebGLRenderingContext.cpp:2446 > - if (info.size > 1) > + if (info.size > 1 && info.name.endsWith("[0]")) Is "[0]" the only thing we should be stripping. What about "[1]"? This looks like a testable change, why is there no new test in this patch? > Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:-1448 > - ASSERT(program); This should be in a separate patch.

(In reply to comment #25) > (From update of attachment 94622 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=94622&action=review > > > Source/WebCore/html/canvas/WebGLRenderingContext.cpp:2446 > > - if (info.size > 1) > > + if (info.size > 1 && info.name.endsWith("[0]")) > > Is "[0]" the only thing we should be stripping. What about "[1]"? Yes, because if its an array the first element will be either just a name or it will end with [0]. There is no way it will end with [1]. > This looks like a testable change, why is there no new test in this patch? The test case that i have attached is actually failing without this change for qtwebkit. because we are truncating the last three characters without verifying if they are really [0]. "getActiveUniform" gives only "color" as the name for "info" even its an array. So we need to make sure we are not truncating the name of the uniform itself. > > > Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:-1448 > > - ASSERT(program); > > This should be in a separate patch. Ok. I have created two different patches now. But the same test case can be applied for both. So i have added it to the changelog.

Created attachment 94631 [details] Assenrtion not needed patch.

Created attachment 94633 [details] array name should be checked before removing the [0].

Created attachment 94636 [details] Another try for name checking patch

Comment on attachment 94636 [details] Another try for name checking patch Sorry again tab :(

Attachment 94636 [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/ChangeLog:9: Line contains tab character. [whitespace/tab] [5] 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 94639 [details] Another try for name checking patch

Created attachment 94640 [details] getting crazy. Trying again

Comment on attachment 94631 [details] Assenrtion not needed patch. Clearing flags on attachment: 94631 Committed r87208: <http://trac.webkit.org/changeset/87208>

Looks good. Thanks for catching this.

Comment on attachment 94640 [details] getting crazy. Trying again r=me based on zmo's lgtm.

The commit-queue encountered the following flaky tests while processing attachment 94640 [details]: inspector/debugger/debugger-breakpoints-not-activated-on-reload.html bug 55551 (authors: pfeldman@chromium.org and podivilov@chromium.org) java/lc3/JavaObject/JavaObjectToInt-002-n.html bug 58299 (author: ap@webkit.org) The commit-queue is continuing to process your patch.

Comment on attachment 94640 [details] getting crazy. Trying again Clearing flags on attachment: 94640 Committed r87451: <http://trac.webkit.org/changeset/87451>

All reviewed patches have been landed. Closing bug.

Revision r87208 cherry-picked into qtwebkit-2.2 with commit 122c9e6 <http://gitorious.org/webkit/qtwebkit/commit/122c9e6> Revision r87451 cherry-picked into qtwebkit-2.2 with commit 9682ccf <http://gitorious.org/webkit/qtwebkit/commit/9682ccf>