Bug 60377 - [Qt] fast/canvas/webgl/gl-uniform-arrays.html failing for Qt on Linux
Summary: [Qt] fast/canvas/webgl/gl-uniform-arrays.html failing for Qt on Linux
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Linux
: P5 Normal
Assignee: Nobody
URL:
Keywords: Qt, QtTriaged
Depends on:
Blocks:
 
Reported: 2011-05-06 09:48 PDT by Idrees
Modified: 2011-05-27 10:53 PDT (History)
9 users (show)

See Also:


Attachments
Fix the debug version plus test case (1.40 KB, patch)
2011-05-06 09:52 PDT, Idrees
eric: review-
Details | Formatted Diff | Diff
Second version of the fix (3.51 KB, patch)
2011-05-12 11:53 PDT, Idrees
no flags Details | Formatted Diff | Diff
Test case (9.92 KB, text/html)
2011-05-12 11:55 PDT, Idrees
no flags Details
Trying Fix again (3.52 KB, patch)
2011-05-12 12:01 PDT, Idrees
kling: review-
Details | Formatted Diff | Diff
Removing the webgl enabler for wk2 (2.16 KB, patch)
2011-05-17 12:28 PDT, Idrees
benjamin: review-
Details | Formatted Diff | Diff
Another patch for fix (2.23 KB, patch)
2011-05-18 11:26 PDT, Idrees
no flags Details | Formatted Diff | Diff
Another patch for fix (2.23 KB, patch)
2011-05-18 11:34 PDT, Idrees
no flags Details | Formatted Diff | Diff
More description about the issue in changelog (2.67 KB, patch)
2011-05-19 10:03 PDT, Idrees
menard: review-
Details | Formatted Diff | Diff
Fixing typos. (2.67 KB, patch)
2011-05-24 09:03 PDT, Idrees
kling: review-
Details | Formatted Diff | Diff
Assenrtion not needed patch. (1.33 KB, patch)
2011-05-24 10:04 PDT, Idrees
no flags Details | Formatted Diff | Diff
array name should be checked before removing the [0]. (1.85 KB, patch)
2011-05-24 10:05 PDT, Idrees
no flags Details | Formatted Diff | Diff
Another try for name checking patch (1.85 KB, patch)
2011-05-24 10:21 PDT, Idrees
no flags Details | Formatted Diff | Diff
Another try for name checking patch (4.58 KB, patch)
2011-05-24 10:25 PDT, Idrees
no flags Details | Formatted Diff | Diff
getting crazy. Trying again (1.85 KB, patch)
2011-05-24 10:28 PDT, Idrees
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Idrees 2011-05-06 09:48:50 PDT
fast/canvas/webgl/gl-uniform-arrays.html failing for webkit2 on qt (linux). 

Also the debug version is giving assertion failing. The bug might be reproducible on other platforms as well.
Comment 1 Idrees 2011-05-06 09:52:36 PDT
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 2 Eric Seidel (no email) 2011-05-11 19:43:55 PDT
Comment on attachment 92593 [details]
Fix the debug version plus test case

Need a ChangeLog and test.
Comment 3 Idrees 2011-05-12 11:53:48 PDT
Created attachment 93319 [details]
Second version of the fix

Trying again after editing the changelog.
Comment 4 Idrees 2011-05-12 11:55:03 PDT
Created attachment 93320 [details]
Test case
Comment 5 WebKit Review Bot 2011-05-12 11:57:01 PDT
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 6 Idrees 2011-05-12 12:00:45 PDT
Comment on attachment 93319 [details]
Second version of the fix

Removing because of format failing.
Comment 7 Idrees 2011-05-12 12:01:48 PDT
Created attachment 93321 [details]
Trying Fix again
Comment 8 Andreas Kling 2011-05-17 12:13:13 PDT
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.
Comment 9 Idrees 2011-05-17 12:27:07 PDT
(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.
Comment 10 Idrees 2011-05-17 12:28:03 PDT
Created attachment 93807 [details]
Removing the webgl enabler for wk2
Comment 11 Benjamin Poulain 2011-05-18 07:18:24 PDT
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?
Comment 12 Jarkko Sakkinen 2011-05-18 07:43:40 PDT
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?
Comment 13 Idrees 2011-05-18 10:12:23 PDT
(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.
Comment 14 Idrees 2011-05-18 11:15:14 PDT
(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.
Comment 15 Idrees 2011-05-18 11:26:33 PDT
Created attachment 93949 [details]
Another patch for fix
Comment 16 Idrees 2011-05-18 11:27:14 PDT
Comment on attachment 93949 [details]
Another patch for fix

Putting for review.
Comment 17 WebKit Review Bot 2011-05-18 11:31:07 PDT
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.
Comment 18 Idrees 2011-05-18 11:34:54 PDT
Created attachment 93952 [details]
Another patch for fix
Comment 19 Luiz Agostini 2011-05-18 16:05:39 PDT
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.
Comment 20 Idrees 2011-05-19 10:03:14 PDT
Created attachment 94081 [details]
More description about the issue in changelog
Comment 21 Idrees 2011-05-19 10:11:11 PDT
(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 22 Alexis Menard (darktears) 2011-05-23 16:04:05 PDT
Comment on attachment 93952 [details]
Another patch for fix

Clearing that one because a new version has been uploaded.
Comment 23 Alexis Menard (darktears) 2011-05-23 16:05:42 PDT
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.
Comment 24 Idrees 2011-05-24 09:03:00 PDT
Created attachment 94622 [details]
Fixing typos.
Comment 25 Andreas Kling 2011-05-24 09:19:56 PDT
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.
Comment 26 Idrees 2011-05-24 10:03:14 PDT
(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.
Comment 27 Idrees 2011-05-24 10:04:28 PDT
Created attachment 94631 [details]
Assenrtion not needed patch.
Comment 28 Idrees 2011-05-24 10:05:39 PDT
Created attachment 94633 [details]
array name should be checked before removing the [0].
Comment 29 Idrees 2011-05-24 10:21:47 PDT
Created attachment 94636 [details]
Another try for name checking patch
Comment 30 Idrees 2011-05-24 10:23:35 PDT
Comment on attachment 94636 [details]
Another try for name checking patch

Sorry again tab :(
Comment 31 WebKit Review Bot 2011-05-24 10:24:28 PDT
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.
Comment 32 Idrees 2011-05-24 10:25:35 PDT
Created attachment 94639 [details]
Another try for name checking patch
Comment 33 Idrees 2011-05-24 10:28:33 PDT
Created attachment 94640 [details]
getting crazy. Trying again
Comment 34 WebKit Commit Bot 2011-05-24 14:47:39 PDT
Comment on attachment 94631 [details]
Assenrtion not needed patch.

Clearing flags on attachment: 94631

Committed r87208: <http://trac.webkit.org/changeset/87208>
Comment 35 Zhenyao Mo 2011-05-26 13:35:31 PDT
Looks good.  Thanks for catching this.
Comment 36 Andreas Kling 2011-05-26 13:37:20 PDT
Comment on attachment 94640 [details]
getting crazy. Trying again

r=me based on zmo's lgtm.
Comment 37 WebKit Commit Bot 2011-05-26 16:57:03 PDT
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 38 WebKit Commit Bot 2011-05-26 16:58:49 PDT
Comment on attachment 94640 [details]
getting crazy. Trying again

Clearing flags on attachment: 94640

Committed r87451: <http://trac.webkit.org/changeset/87451>
Comment 39 WebKit Commit Bot 2011-05-26 16:58:57 PDT
All reviewed patches have been landed.  Closing bug.
Comment 40 Ademar Reis 2011-05-27 10:53:31 PDT
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>