51668 2010-12-27 22:43:05 -0800 cleanupAfterGraphicsCall is never called in WebGLRenderingContext::checkFramebufferStatus 2011-01-07 09:51:54 -0800 1 1 1 Unclassified WebKit WebGL 528+ (Nightly build) All All RESOLVED FIXED P3 Trivial --- 0 toyoshim webkit-unassigned cmarrin eric kbr simon.fraser webkit.review.bot zmo oldest_to_newest 327194 0 toyoshim 2010-12-27 22:43:05 -0800 cleanupAfterGraphicsCall() is called at the end of functions for marking graphic context as dirty. In WebGLRenderingContext::checkFramebufferStatus(), cleanupAfterGraphicsCall() is called after return. It mean this cleanAfterGraphicsCall() is never called. Actually, argument of this function is 'false', and it mean this function do nothing. But I think this is a trivial oversight, and it cause compile error with some gcc option (for example, -finstrument-functions). This problem appears in a debug configuration of chromium. (http://codereview.chromium.org/5991005/) 327195 1 77538 toyoshim 2010-12-27 22:55:49 -0800 Created attachment 77538 a simple patch which doesn't change the program meaning 327196 2 webkit.review.bot 2010-12-27 22:56:42 -0800 Attachment 77538 did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'WebCore/ChangeLog', u'WebCore/html/canvas/WebGLRenderingContext.cpp']" exit_code: 1 WebCore/ChangeLog:6: 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. 327197 3 77539 toyoshim 2010-12-27 23:03:11 -0800 Created attachment 77539 revised patch (previous one contained TAB character in ChangeLog and it broke style check) 327345 4 eric 2010-12-28 13:52:38 -0800 Are you sure this is the right call order? 327351 5 77539 zmo 2010-12-28 13:57:54 -0800 Comment on attachment 77539 revised patch (previous one contained TAB character in ChangeLog and it broke style check) View in context: https://bugs.webkit.org/attachment.cgi?id=77539&action=review > WebCore/html/canvas/WebGLRenderingContext.cpp:565 > } You should call m_context->checkFramebufferStatus(target), cache the returned value, cleanupAfterGraphicsCall, then return the cached value. 327480 6 toyoshim 2010-12-28 20:46:55 -0800 Thank you for reviewing the code. I'll post revised one. 327481 7 77600 toyoshim 2010-12-28 21:02:16 -0800 Created attachment 77600 Patch 327784 8 77600 kbr 2010-12-29 18:25:41 -0800 Comment on attachment 77600 Patch Looks fine per Mo's earlier review. 330825 9 77600 zmo 2011-01-07 09:51:49 -0800 Comment on attachment 77600 Patch Clearing flags on attachment: 77600 Committed r75251: <http://trac.webkit.org/changeset/75251> 330826 10 zmo 2011-01-07 09:51:54 -0800 All reviewed patches have been landed. Closing bug. 77538 2010-12-27 22:55:49 -0800 2010-12-27 23:03:11 -0800 a simple patch which doesn't change the program meaning webgl.diff text/plain 1387 toyoshim SW5kZXg6IFdlYkNvcmUvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvQ2hhbmdlTG9n CShyZXZpc2lvbiA3NDcwOCkKKysrIFdlYkNvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBA IC0xLDMgKzEsMTYgQEAKKzIwMTAtMTItMjcgIFRha2FzaGkgVG95b3NoaW1hICA8dG95b3NoaW1A Z29vZ2xlLmNvbT4KKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAg ICAgICBjbGVhbnVwQWZ0ZXJHcmFwaGljc0NhbGwoKSBpcyBuZXZlciBjYWxsZWQgaW4KKwlXZWJH TFJlbmRlcmluZ0NvbnRleHQ6OmNoZWNrRnJhbWVidWZmZXJTdGF0dXMoKQorICAgICAgICBodHRw czovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9NTE2NjgKKworICAgICAgICBObyBu ZXcgdGVzdHMuIFRoaXMgaXMgYSB0cml2aWFsIGJ1Zy4KKworICAgICAgICAqIGh0bWwvY2FudmFz L1dlYkdMUmVuZGVyaW5nQ29udGV4dC5jcHA6CisgICAgICAgIChXZWJDb3JlOjpXZWJHTFJlbmRl cmluZ0NvbnRleHQ6OmNoZWNrRnJhbWVidWZmZXJTdGF0dXMpOgorCiAyMDEwLTEyLTI3ICBIZWxk ZXIgQ29ycmVpYSAgPGhlbGRlckBzZW5jaGEuY29tPgogCiAgICAgICAgIFJldmlld2VkIGJ5IEFy aXlhIEhpZGF5YXQuCkluZGV4OiBXZWJDb3JlL2h0bWwvY2FudmFzL1dlYkdMUmVuZGVyaW5nQ29u dGV4dC5jcHAKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PQotLS0gV2ViQ29yZS9odG1sL2NhbnZhcy9XZWJHTFJlbmRlcmlu Z0NvbnRleHQuY3BwCShyZXZpc2lvbiA3NDY4MCkKKysrIFdlYkNvcmUvaHRtbC9jYW52YXMvV2Vi R0xSZW5kZXJpbmdDb250ZXh0LmNwcAkod29ya2luZyBjb3B5KQpAQCAtNTYwLDggKzU2MCw4IEBA IHVuc2lnbmVkIGxvbmcgV2ViR0xSZW5kZXJpbmdDb250ZXh0OjpjaGUKICAgICAgICAgcmV0dXJu IEdyYXBoaWNzQ29udGV4dDNEOjpGUkFNRUJVRkZFUl9DT01QTEVURTsKICAgICBpZiAobV9mcmFt ZWJ1ZmZlckJpbmRpbmctPmlzSW5jb21wbGV0ZSh0cnVlKSkKICAgICAgICAgcmV0dXJuIEdyYXBo aWNzQ29udGV4dDNEOjpGUkFNRUJVRkZFUl9VTlNVUFBPUlRFRDsKLSAgICByZXR1cm4gbV9jb250 ZXh0LT5jaGVja0ZyYW1lYnVmZmVyU3RhdHVzKHRhcmdldCk7CiAgICAgY2xlYW51cEFmdGVyR3Jh cGhpY3NDYWxsKGZhbHNlKTsKKyAgICByZXR1cm4gbV9jb250ZXh0LT5jaGVja0ZyYW1lYnVmZmVy U3RhdHVzKHRhcmdldCk7CiB9CiAKIHZvaWQgV2ViR0xSZW5kZXJpbmdDb250ZXh0OjpjbGVhcih1 bnNpZ25lZCBsb25nIG1hc2spCg== 77539 2010-12-27 23:03:11 -0800 2010-12-28 21:02:10 -0800 revised patch (previous one contained TAB character in ChangeLog and it broke style check) webgl-2.diff text/plain 1394 toyoshim SW5kZXg6IFdlYkNvcmUvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvQ2hhbmdlTG9n CShyZXZpc2lvbiA3NDcwOCkKKysrIFdlYkNvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBA IC0xLDMgKzEsMTYgQEAKKzIwMTAtMTItMjcgIFRha2FzaGkgVG95b3NoaW1hICA8dG95b3NoaW1A Z29vZ2xlLmNvbT4KKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAg ICAgICBjbGVhbnVwQWZ0ZXJHcmFwaGljc0NhbGwoKSBpcyBuZXZlciBjYWxsZWQgaW4KKyAgICAg ICAgV2ViR0xSZW5kZXJpbmdDb250ZXh0OjpjaGVja0ZyYW1lYnVmZmVyU3RhdHVzKCkKKyAgICAg ICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTUxNjY4CisKKyAgICAg ICAgTm8gbmV3IHRlc3RzLiBUaGlzIGlzIGEgdHJpdmlhbCBidWcuCisKKyAgICAgICAgKiBodG1s L2NhbnZhcy9XZWJHTFJlbmRlcmluZ0NvbnRleHQuY3BwOgorICAgICAgICAoV2ViQ29yZTo6V2Vi R0xSZW5kZXJpbmdDb250ZXh0OjpjaGVja0ZyYW1lYnVmZmVyU3RhdHVzKToKKwogMjAxMC0xMi0y NyAgSGVsZGVyIENvcnJlaWEgIDxoZWxkZXJAc2VuY2hhLmNvbT4KIAogICAgICAgICBSZXZpZXdl ZCBieSBBcml5YSBIaWRheWF0LgpJbmRleDogV2ViQ29yZS9odG1sL2NhbnZhcy9XZWJHTFJlbmRl cmluZ0NvbnRleHQuY3BwCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvaHRtbC9jYW52YXMvV2ViR0xS ZW5kZXJpbmdDb250ZXh0LmNwcAkocmV2aXNpb24gNzQ2ODApCisrKyBXZWJDb3JlL2h0bWwvY2Fu dmFzL1dlYkdMUmVuZGVyaW5nQ29udGV4dC5jcHAJKHdvcmtpbmcgY29weSkKQEAgLTU2MCw4ICs1 NjAsOCBAQCB1bnNpZ25lZCBsb25nIFdlYkdMUmVuZGVyaW5nQ29udGV4dDo6Y2hlCiAgICAgICAg IHJldHVybiBHcmFwaGljc0NvbnRleHQzRDo6RlJBTUVCVUZGRVJfQ09NUExFVEU7CiAgICAgaWYg KG1fZnJhbWVidWZmZXJCaW5kaW5nLT5pc0luY29tcGxldGUodHJ1ZSkpCiAgICAgICAgIHJldHVy biBHcmFwaGljc0NvbnRleHQzRDo6RlJBTUVCVUZGRVJfVU5TVVBQT1JURUQ7Ci0gICAgcmV0dXJu IG1fY29udGV4dC0+Y2hlY2tGcmFtZWJ1ZmZlclN0YXR1cyh0YXJnZXQpOwogICAgIGNsZWFudXBB ZnRlckdyYXBoaWNzQ2FsbChmYWxzZSk7CisgICAgcmV0dXJuIG1fY29udGV4dC0+Y2hlY2tGcmFt ZWJ1ZmZlclN0YXR1cyh0YXJnZXQpOwogfQogCiB2b2lkIFdlYkdMUmVuZGVyaW5nQ29udGV4dDo6 Y2xlYXIodW5zaWduZWQgbG9uZyBtYXNrKQo= 77600 2010-12-28 21:02:16 -0800 2011-01-07 09:51:49 -0800 Patch bug-51668-20101229140214.patch text/plain 1422 toyoshim SW5kZXg6IFdlYkNvcmUvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvQ2hhbmdlTG9n CShyZXZpc2lvbiA3NDczMykKKysrIFdlYkNvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBA IC0xLDMgKzEsMTYgQEAKKzIwMTAtMTItMjkgIFRha2FzaGkgVG95b3NoaW1hICA8dG95b3NoaW1A Z29vZ2xlLmNvbT4KKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAg ICAgICBjbGVhbnVwQWZ0ZXJHcmFwaGljc0NhbGwoKSBpcyBuZXZlciBjYWxsZWQgaW4KKyAgICAg ICAgV2ViR0xSZW5kZXJpbmdDb250ZXh0OjpjaGVja0ZyYW1lYnVmZmVyU3RhdHVzKCkKKyAgICAg ICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTUxNjY4CisKKyAgICAg ICAgTm8gbmV3IHRlc3RzLiBUaGlzIGlzIGEgdHJpdmlhbCBidWcuCisKKyAgICAgICAgKiBodG1s L2NhbnZhcy9XZWJHTFJlbmRlcmluZ0NvbnRleHQuY3BwOgorICAgICAgICAoV2ViQ29yZTo6V2Vi R0xSZW5kZXJpbmdDb250ZXh0OjpjaGVja0ZyYW1lYnVmZmVyU3RhdHVzKToKKwogMjAxMC0xMi0y OCAgRGFuaWVsIEJhdGVzICA8ZGJhdGVzQHJpbS5jb20+CiAKICAgICAgICAgUmV2aWV3ZWQgYnkg U2FtIFdlaW5pZy4KSW5kZXg6IFdlYkNvcmUvaHRtbC9jYW52YXMvV2ViR0xSZW5kZXJpbmdDb250 ZXh0LmNwcAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09Ci0tLSBXZWJDb3JlL2h0bWwvY2FudmFzL1dlYkdMUmVuZGVyaW5n Q29udGV4dC5jcHAJKHJldmlzaW9uIDc0NzMzKQorKysgV2ViQ29yZS9odG1sL2NhbnZhcy9XZWJH TFJlbmRlcmluZ0NvbnRleHQuY3BwCSh3b3JraW5nIGNvcHkpCkBAIC01NzgsOCArNTc4LDkgQEAg dW5zaWduZWQgbG9uZyBXZWJHTFJlbmRlcmluZ0NvbnRleHQ6OmNoZQogICAgICAgICByZXR1cm4g R3JhcGhpY3NDb250ZXh0M0Q6OkZSQU1FQlVGRkVSX0NPTVBMRVRFOwogICAgIGlmIChtX2ZyYW1l YnVmZmVyQmluZGluZy0+aXNJbmNvbXBsZXRlKHRydWUpKQogICAgICAgICByZXR1cm4gR3JhcGhp Y3NDb250ZXh0M0Q6OkZSQU1FQlVGRkVSX1VOU1VQUE9SVEVEOwotICAgIHJldHVybiBtX2NvbnRl eHQtPmNoZWNrRnJhbWVidWZmZXJTdGF0dXModGFyZ2V0KTsKKyAgICB1bnNpZ25lZCBsb25nIHJl c3VsdCA9IG1fY29udGV4dC0+Y2hlY2tGcmFtZWJ1ZmZlclN0YXR1cyh0YXJnZXQpOwogICAgIGNs ZWFudXBBZnRlckdyYXBoaWNzQ2FsbChmYWxzZSk7CisgICAgcmV0dXJuIHJlc3VsdDsKIH0KIAog dm9pZCBXZWJHTFJlbmRlcmluZ0NvbnRleHQ6OmNsZWFyKHVuc2lnbmVkIGxvbmcgbWFzaykK