Bug 40175

A mistake. For 4.2, should generate INVALID_OPERATION and return 0.

Created attachment 57926 [details] patch

Comment on attachment 57926 [details] patch Looks good to me. Upon first glance it looked like some of the checks might be redundant, but it turns out that OpenGL 3.3 accepts more enums for calls like glCheckFramebufferStatus than OpenGL ES 2.0 does, so all of the checks you've added are needed. The test case looks good.

Comment on attachment 57926 [details] patch rs=me.

Comment on attachment 57926 [details] patch Rejecting patch 57926 from commit-queue. Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--ignore-tests', 'compositing', '--quiet']" exit_code: 1 Last 500 characters of output: ing Java tests make: Nothing to be done for `default'. Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests Skipped list contained 'compositing/iframes/composited-iframe.html', but no file of that name could be found Testing 19071 test cases. fast/dom/Window/window-property-descriptors.html -> failed Exiting early after 1 failures. 6789 tests run. 130.08s total testing time 6788 test cases (99%) succeeded 1 test case (<1%) had incorrect layout 1 test case (<1%) had stderr output Full output: http://webkit-commit-queue.appspot.com/results/3175098

Comment on attachment 57926 [details] patch Rejecting patch 57926 from commit-queue. zmo@google.com does not have committer permissions according to http://trac.webkit.org/browser/trunk/WebKitTools/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 WebKitTools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed). Due to bug 30084 the commit-queue will require a restart after your change. Please contact eseidel@chromium.org to request a commit-queue restart. After restart the commit-queue will correctly respect your committer rights.

Comment on attachment 57926 [details] patch Rejecting patch 57926 from commit-queue. Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--ignore-tests', 'compositing', '--quiet']" exit_code: 1 Last 500 characters of output: ing Java tests make: Nothing to be done for `default'. Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests Skipped list contained 'compositing/iframes/composited-iframe.html', but no file of that name could be found Testing 19072 test cases. fast/dom/Window/window-property-descriptors.html -> failed Exiting early after 1 failures. 6790 tests run. 129.73s total testing time 6789 test cases (99%) succeeded 1 test case (<1%) had incorrect layout 1 test case (<1%) had stderr output Full output: http://webkit-commit-queue.appspot.com/results/3195102

Comment on attachment 57926 [details] patch Rejecting patch 57926 from commit-queue. Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--ignore-tests', 'compositing', '--quiet']" exit_code: 1 Last 500 characters of output: ing Java tests make: Nothing to be done for `default'. Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests Skipped list contained 'compositing/iframes/composited-iframe.html', but no file of that name could be found Testing 19079 test cases. fast/dom/Window/window-property-descriptors.html -> failed Exiting early after 1 failures. 6791 tests run. 127.44s total testing time 6790 test cases (99%) succeeded 1 test case (<1%) had incorrect layout 1 test case (<1%) had stderr output Full output: http://webkit-commit-queue.appspot.com/results/3226234

One more try failed. I looked at the patch again. The only possibility is I added a assertMsg() function to LayoutTests/fast/js/resources/js-test-pre.js. Since I have no way to confirm it, I'll just refactor the test to remove the assertMsg() and give it another try. The only problem I see is that we have a few other WebGL conformance tests that we might want to add to LayoutTests, and they also use assertMsg, so they also need to be rewritten.

(In reply to comment #9) > One more try failed. I looked at the patch again. The only possibility is I added a assertMsg() function to LayoutTests/fast/js/resources/js-test-pre.js. > > Since I have no way to confirm it, I'll just refactor the test to remove the assertMsg() and give it another try. Did you run this test on your machine? If it is a real failure then it will reproduce there. run-webkit-tests [--debug] LayoutTests/fast/dom/Window/window-property-descriptors.html

(In reply to comment #10) > (In reply to comment #9) > > One more try failed. I looked at the patch again. The only possibility is I added a assertMsg() function to LayoutTests/fast/js/resources/js-test-pre.js. > > > > Since I have no way to confirm it, I'll just refactor the test to remove the assertMsg() and give it another try. > > Did you run this test on your machine? If it is a real failure then it will reproduce there. > > run-webkit-tests [--debug] LayoutTests/fast/dom/Window/window-property-descriptors.html I just applied your patch and found that this test does in fact fail with your patch applied. Run it on your own machine to see the error reported. You should consider doing full LayoutTest runs ("run-webkit-tests [--debug]") for changes that touch non-WebGL code.

(In reply to comment #11) > (In reply to comment #10) > > (In reply to comment #9) > > > One more try failed. I looked at the patch again. The only possibility is I added a assertMsg() function to LayoutTests/fast/js/resources/js-test-pre.js. > > > > > > Since I have no way to confirm it, I'll just refactor the test to remove the assertMsg() and give it another try. > > > > Did you run this test on your machine? If it is a real failure then it will reproduce there. > > > > run-webkit-tests [--debug] LayoutTests/fast/dom/Window/window-property-descriptors.html > > I just applied your patch and found that this test does in fact fail with your patch applied. Run it on your own machine to see the error reported. You should consider doing full LayoutTest runs ("run-webkit-tests [--debug]") for changes that touch non-WebGL code. OK, I could reproduce it by run-webkit-tests. Originally I tested with run-safari and open the failed test and saw no FAIL, so I thought I could not reproduce the error. Actually because I added a new function in js-test-pre.js, the expected txt will have one extra PASS line, that's why it failed.

Created attachment 58497 [details] revised patch Moved assertMsg() to webgl-test.js since it's only used by WebGL. The rest is the same.

Looks fine.

Comment on attachment 58497 [details] revised patch let's try again.

Comment on attachment 58497 [details] revised patch Clearing flags on attachment: 58497 Committed r61185: <http://trac.webkit.org/changeset/61185>

All reviewed patches have been landed. Closing bug.

http://trac.webkit.org/changeset/61185 might have broken SnowLeopard Intel Release (Tests)

(In reply to comment #18) > http://trac.webkit.org/changeset/61185 might have broken SnowLeopard Intel Release (Tests) pretty diff here: http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20%28Tests%29/r61186%20%2811683%29/fast/canvas/webgl/texture-npot-pretty-diff.html