88704 2012-06-09 00:34:40 -0700 [Qt][Texmap] Falling leaves demo missing clipping. 2012-06-12 06:33:57 -0700 1 1 1 Unclassified WebKit Platform 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 0 dongseong.hwang webkit-unassigned noam webkit.review.bot oldest_to_newest 645264 0 dongseong.hwang 2012-06-09 00:34:40 -0700 The bug originated from begining native painting mode of QPainter after enabling scissor test, because begining native painting mode disables scissor test. 645265 1 146691 dongseong.hwang 2012-06-09 00:36:22 -0700 Created attachment 146691 patch v.1 645344 2 146691 noam 2012-06-09 09:22:19 -0700 Comment on attachment 146691 patch v.1 View in context: https://bugs.webkit.org/attachment.cgi?id=146691&action=review > Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:-270 > glDisable(GL_DEPTH_TEST); > glEnable(GL_SCISSOR_TEST); > -#if PLATFORM(QT) > - if (m_context) { > - QPainter* painter = m_context->platformContext(); > - painter->save(); > - painter->beginNativePainting(); > - } > -#endif Shouldn't the new code be called before the other GL calls? 645640 3 146691 dongseong.hwang 2012-06-10 16:31:23 -0700 Comment on attachment 146691 patch v.1 View in context: https://bugs.webkit.org/attachment.cgi?id=146691&action=review >> Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:-270 >> -#endif > > Shouldn't the new code be called before the other GL calls? I think there is no problem to put initializeOpenGLShims() before PLATFORM(QT) code, because initializeOpenGLShims() does not change any GL states and QPainter does not need something like OpenGLShims. Moreover, I think it is more obvious to save gl states for QPainter before starting Texmap routines like TextureMapperGL::endPainting restores gl states for QPainter after all Texmap tasks are finished. 645641 4 dongseong.hwang 2012-06-10 16:33:40 -0700 Oops, I made typo (In reply to comment #3) > (From update of attachment 146691 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=146691&action=review > > >> Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:-270 > >> -#endif > > > > Shouldn't the new code be called before the other GL calls? > > I think there is no problem to put initializeOpenGLShims() before PLATFORM(QT) code, because initializeOpenGLShims() does not change any GL states and QPainter does not need something like OpenGLShims. before -> after > Moreover, I think it is more obvious to save gl states for QPainter before starting Texmap routines like TextureMapperGL::endPainting restores gl states for QPainter after all Texmap tasks are finished. 645907 5 dongseong.hwang 2012-06-11 04:34:29 -0700 (In reply to comment #2) > (From update of attachment 146691 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=146691&action=review > > > Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:-270 > > glDisable(GL_DEPTH_TEST); > > glEnable(GL_SCISSOR_TEST); > > -#if PLATFORM(QT) > > - if (m_context) { > > - QPainter* painter = m_context->platformContext(); > > - painter->save(); > > - painter->beginNativePainting(); > > - } > > -#endif > > Shouldn't the new code be called before the other GL calls? Would you like to explain more detail. I think I don't understand what you mean. 645933 6 noam 2012-06-11 06:08:57 -0700 (In reply to comment #5) > (In reply to comment #2) > > (From update of attachment 146691 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=146691&action=review > > > > > Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:-270 > > > glDisable(GL_DEPTH_TEST); > > > glEnable(GL_SCISSOR_TEST); > > > -#if PLATFORM(QT) > > > - if (m_context) { > > > - QPainter* painter = m_context->platformContext(); > > > - painter->save(); > > > - painter->beginNativePainting(); > > > - } > > > -#endif > > > > Shouldn't the new code be called before the other GL calls? > > Would you like to explain more detail. I think I don't understand what you mean. i'm saying it should look more like this: > > > -#if PLATFORM(QT) > > > - if (m_context) { > > > - QPainter* painter = m_context->platformContext(); > > > - painter->save(); > > > - painter->beginNativePainting(); > > > - } > > > -#endif > > > glDisable(GL_DEPTH_TEST); > > > glEnable(GL_SCISSOR_TEST); > > 646681 7 147022 dongseong.hwang 2012-06-11 23:47:22 -0700 Created attachment 147022 patch v.2 646687 8 147022 dongseong.hwang 2012-06-11 23:53:22 -0700 Comment on attachment 147022 patch v.2 View in context: https://bugs.webkit.org/attachment.cgi?id=147022&action=review > Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:270 > data().previousDepthState = glIsEnabled(GL_DEPTH_TEST); I'm afraid that it is right to save previous states before begin NativePainting, because restoring previous states is before ending NativePainting in TextureMapperGL::endPainting(). It is asymmetric between beginPainting and endPainting. 646886 9 147022 noam 2012-06-12 06:26:56 -0700 Comment on attachment 147022 patch v.2 OK, I misread the patch. LGtm. 646892 10 147022 webkit.review.bot 2012-06-12 06:33:53 -0700 Comment on attachment 147022 patch v.2 Clearing flags on attachment: 147022 Committed r120066: <http://trac.webkit.org/changeset/120066> 646893 11 webkit.review.bot 2012-06-12 06:33:57 -0700 All reviewed patches have been landed. Closing bug. 146691 2012-06-09 00:36:22 -0700 2012-06-11 23:47:22 -0700 patch v.1 0001-Qt-Texmap-Falling-leaves-demo-missing-clipping.patch text/plain 2827 dongseong.hwang RnJvbSA0YzUzNTAxZDhhZGIzM2YxMWViMTU4YTJiOGIxYzJiMTcyMDAxZmNjIE1vbiBTZXAgMTcg MDA6MDA6MDAgMjAwMQpGcm9tOiBIdWFuZyBEb25nc3VuZyA8bHV4dGVsbGFAY29tcGFueTEwMC5u ZXQ+CkRhdGU6IFNhdCwgOSBKdW4gMjAxMiAxNjoxMToxOCArMDkwMApTdWJqZWN0OiBbUEFUQ0hd IFtRdF1bVGV4bWFwXSBGYWxsaW5nIGxlYXZlcyBkZW1vIG1pc3NpbmcgY2xpcHBpbmcuCiBodHRw czovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9ODg3MDQKClRoZSBidWcgb3JpZ2lu YXRlZCBmcm9tIGJlZ2luaW5nIG5hdGl2ZSBwYWludGluZyBtb2RlIG9mIFFQYWludGVyIGFmdGVy IGVuYWJsaW5nCnNjaXNzb3IgdGVzdCwgYmVjYXVzZSBiZWdpbmluZyBuYXRpdmUgcGFpbnRpbmcg bW9kZSBkaXNhYmxlcyBzY2lzc29yIHRlc3QuCgpDb3ZlcmVkIGJ5IGV4aXN0aW5nIGNvbXBvc2l0 aW5nIHRlc3RzLgotLS0KIFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZyAgICAgICAgICAgICAgICAg ICAgICAgICAgIHwgICAxNSArKysrKysrKysrKysrKysKIC4uLi9wbGF0Zm9ybS9ncmFwaGljcy90 ZXhtYXAvVGV4dHVyZU1hcHBlckdMLmNwcCAgIHwgICAxNCArKysrKysrLS0tLS0tLQogMiBmaWxl cyBjaGFuZ2VkLCAyMiBpbnNlcnRpb25zKCspLCA3IGRlbGV0aW9ucygtKQoKZGlmZiAtLWdpdCBh L1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwppbmRl eCA5YTM0NzliLi42ODg1ZmUzIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cK KysrIGIvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCkBAIC0xLDUgKzEsMjAgQEAKIDIwMTItMDYt MDkgIEh1YW5nIERvbmdzdW5nICA8bHV4dGVsbGFAY29tcGFueTEwMC5uZXQ+CiAKKyAgICAgICAg W1F0XVtUZXhtYXBdIEZhbGxpbmcgbGVhdmVzIGRlbW8gbWlzc2luZyBjbGlwcGluZy4KKyAgICAg ICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTg4NzA0CisKKyAgICAg ICAgVGhlIGJ1ZyBvcmlnaW5hdGVkIGZyb20gYmVnaW5pbmcgbmF0aXZlIHBhaW50aW5nIG1vZGUg b2YgUVBhaW50ZXIgYWZ0ZXIgZW5hYmxpbmcKKyAgICAgICAgc2Npc3NvciB0ZXN0LCBiZWNhdXNl IGJlZ2luaW5nIG5hdGl2ZSBwYWludGluZyBtb2RlIGRpc2FibGVzIHNjaXNzb3IgdGVzdC4KKwor ICAgICAgICBDb3ZlcmVkIGJ5IGV4aXN0aW5nIGNvbXBvc2l0aW5nIHRlc3RzLgorCisgICAgICAg IFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgICogcGxhdGZvcm0vZ3JhcGhp Y3MvdGV4bWFwL1RleHR1cmVNYXBwZXJHTC5jcHA6CisgICAgICAgIChXZWJDb3JlOjpUZXh0dXJl TWFwcGVyR0w6OmJlZ2luUGFpbnRpbmcpOgorCisyMDEyLTA2LTA5ICBIdWFuZyBEb25nc3VuZyAg PGx1eHRlbGxhQGNvbXBhbnkxMDAubmV0PgorCiAgICAgICAgIFtRdF1bVGV4bWFwXSBBbGwgbGF5 ZXJzIHdpdGggYmFja2luZ1N0b3JlIGFyZSBvcGFxdWUgd2hlbiB1c2luZyBUZXh0dXJlTWFwcGVy R0wuCiAgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD04ODcw MwogCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9ncmFwaGljcy90ZXhtYXAv VGV4dHVyZU1hcHBlckdMLmNwcCBiL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL2dyYXBoaWNzL3Rl eG1hcC9UZXh0dXJlTWFwcGVyR0wuY3BwCmluZGV4IGIxNjRhMWYuLjVmZmFiMjkgMTAwNjQ0Ci0t LSBhL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL2dyYXBoaWNzL3RleG1hcC9UZXh0dXJlTWFwcGVy R0wuY3BwCisrKyBiL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL2dyYXBoaWNzL3RleG1hcC9UZXh0 dXJlTWFwcGVyR0wuY3BwCkBAIC0yNTAsNiArMjUwLDEzIEBAIFRleHR1cmVNYXBwZXJHTDo6Q2xp cFN0YWNrJiBUZXh0dXJlTWFwcGVyR0w6OmNsaXBTdGFjaygpCiAKIHZvaWQgVGV4dHVyZU1hcHBl ckdMOjpiZWdpblBhaW50aW5nKFBhaW50RmxhZ3MgZmxhZ3MpCiB7CisjaWYgUExBVEZPUk0oUVQp CisgICAgaWYgKG1fY29udGV4dCkgeworICAgICAgICBRUGFpbnRlciogcGFpbnRlciA9IG1fY29u dGV4dC0+cGxhdGZvcm1Db250ZXh0KCk7CisgICAgICAgIHBhaW50ZXItPnNhdmUoKTsKKyAgICAg ICAgcGFpbnRlci0+YmVnaW5OYXRpdmVQYWludGluZygpOworICAgIH0KKyNlbmRpZgogICAgIC8v IE1ha2Ugc3VyZSB0aGF0IG5vIEdMIGVycm9yIGNvZGUgc3RheXMgZnJvbSBwcmV2aW91cyBvcGVy YXRpb25zLgogICAgIGdsR2V0RXJyb3IoKTsKIApAQCAtMjYxLDEzICsyNjgsNiBAQCB2b2lkIFRl eHR1cmVNYXBwZXJHTDo6YmVnaW5QYWludGluZyhQYWludEZsYWdzIGZsYWdzKQogICAgIGRhdGEo KS5wcmV2aW91c0RlcHRoU3RhdGUgPSBnbElzRW5hYmxlZChHTF9ERVBUSF9URVNUKTsKICAgICBn bERpc2FibGUoR0xfREVQVEhfVEVTVCk7CiAgICAgZ2xFbmFibGUoR0xfU0NJU1NPUl9URVNUKTsK LSNpZiBQTEFURk9STShRVCkKLSAgICBpZiAobV9jb250ZXh0KSB7Ci0gICAgICAgIFFQYWludGVy KiBwYWludGVyID0gbV9jb250ZXh0LT5wbGF0Zm9ybUNvbnRleHQoKTsKLSAgICAgICAgcGFpbnRl ci0+c2F2ZSgpOwotICAgICAgICBwYWludGVyLT5iZWdpbk5hdGl2ZVBhaW50aW5nKCk7Ci0gICAg fQotI2VuZGlmCiAgICAgZGF0YSgpLmRpZE1vZGlmeVN0ZW5jaWwgPSBmYWxzZTsKICAgICBHTF9D TUQoZ2xEZXB0aE1hc2soMCkpOwogICAgIEdMX0NNRChnbEdldEludGVnZXJ2KEdMX1ZJRVdQT1JU LCBkYXRhKCkudmlld3BvcnQpKTsKLS0gCjEuNy45LjUKCg== 147022 2012-06-11 23:47:22 -0700 2012-06-12 06:33:53 -0700 patch v.2 0001-Qt-Texmap-Falling-leaves-demo-missing-clipping.patch text/plain 2597 dongseong.hwang RnJvbSA5ZGViMzYzZWZmNzJhOWQyZDU0NzYwMmNlNDA5ZWNmZDBiYjBjMDJlIE1vbiBTZXAgMTcg MDA6MDA6MDAgMjAwMQpGcm9tOiBIdWFuZyBEb25nc3VuZyA8bHV4dGVsbGFAY29tcGFueTEwMC5u ZXQ+CkRhdGU6IFNhdCwgOSBKdW4gMjAxMiAxNjoxMToxOCArMDkwMApTdWJqZWN0OiBbUEFUQ0hd IFtRdF1bVGV4bWFwXSBGYWxsaW5nIGxlYXZlcyBkZW1vIG1pc3NpbmcgY2xpcHBpbmcuCiBodHRw czovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9ODg3MDQKClRoZSBidWcgb3JpZ2lu YXRlZCBmcm9tIGJlZ2luaW5nIG5hdGl2ZSBwYWludGluZyBtb2RlIG9mIFFQYWludGVyIGFmdGVy IGVuYWJsaW5nCnNjaXNzb3IgdGVzdCwgYmVjYXVzZSBiZWdpbmluZyBuYXRpdmUgcGFpbnRpbmcg bW9kZSBkaXNhYmxlcyBzY2lzc29yIHRlc3QuCgpDb3ZlcmVkIGJ5IGV4aXN0aW5nIGNvbXBvc2l0 aW5nIHRlc3RzLgotLS0KIFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZyAgICAgICAgICAgICAgICAg ICAgICAgICAgIHwgICAxNSArKysrKysrKysrKysrKysKIC4uLi9wbGF0Zm9ybS9ncmFwaGljcy90 ZXhtYXAvVGV4dHVyZU1hcHBlckdMLmNwcCAgIHwgICAgNCArKy0tCiAyIGZpbGVzIGNoYW5nZWQs IDE3IGluc2VydGlvbnMoKyksIDIgZGVsZXRpb25zKC0pCgpkaWZmIC0tZ2l0IGEvU291cmNlL1dl YkNvcmUvQ2hhbmdlTG9nIGIvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCmluZGV4IDMzMmU1ODQu LmQ0M2JkMjAgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZworKysgYi9Tb3Vy Y2UvV2ViQ29yZS9DaGFuZ2VMb2cKQEAgLTEsNSArMSwyMCBAQAogMjAxMi0wNi0wOSAgSHVhbmcg RG9uZ3N1bmcgIDxsdXh0ZWxsYUBjb21wYW55MTAwLm5ldD4KIAorICAgICAgICBbUXRdW1RleG1h cF0gRmFsbGluZyBsZWF2ZXMgZGVtbyBtaXNzaW5nIGNsaXBwaW5nLgorICAgICAgICBodHRwczov L2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9ODg3MDQKKworICAgICAgICBUaGUgYnVn IG9yaWdpbmF0ZWQgZnJvbSBiZWdpbmluZyBuYXRpdmUgcGFpbnRpbmcgbW9kZSBvZiBRUGFpbnRl ciBhZnRlciBlbmFibGluZworICAgICAgICBzY2lzc29yIHRlc3QsIGJlY2F1c2UgYmVnaW5pbmcg bmF0aXZlIHBhaW50aW5nIG1vZGUgZGlzYWJsZXMgc2Npc3NvciB0ZXN0LgorCisgICAgICAgIENv dmVyZWQgYnkgZXhpc3RpbmcgY29tcG9zaXRpbmcgdGVzdHMuCisKKyAgICAgICAgUmV2aWV3ZWQg YnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgKiBwbGF0Zm9ybS9ncmFwaGljcy90ZXhtYXAv VGV4dHVyZU1hcHBlckdMLmNwcDoKKyAgICAgICAgKFdlYkNvcmU6OlRleHR1cmVNYXBwZXJHTDo6 YmVnaW5QYWludGluZyk6CisKKzIwMTItMDYtMDkgIEh1YW5nIERvbmdzdW5nICA8bHV4dGVsbGFA Y29tcGFueTEwMC5uZXQ+CisKICAgICAgICAgW1F0XVtUZXhtYXBdIEFsbCBsYXllcnMgd2l0aCBi YWNraW5nU3RvcmUgYXJlIG9wYXF1ZSB3aGVuIHVzaW5nIFRleHR1cmVNYXBwZXJHTC4KICAgICAg ICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTg4NzAzCiAKZGlmZiAt LWdpdCBhL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL2dyYXBoaWNzL3RleG1hcC9UZXh0dXJlTWFw cGVyR0wuY3BwIGIvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vZ3JhcGhpY3MvdGV4bWFwL1RleHR1 cmVNYXBwZXJHTC5jcHAKaW5kZXggNzRiMjM4ZS4uMTI5ODc4NiAxMDA2NDQKLS0tIGEvU291cmNl L1dlYkNvcmUvcGxhdGZvcm0vZ3JhcGhpY3MvdGV4bWFwL1RleHR1cmVNYXBwZXJHTC5jcHAKKysr IGIvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vZ3JhcGhpY3MvdGV4bWFwL1RleHR1cmVNYXBwZXJH TC5jcHAKQEAgLTI2OCw4ICsyNjgsNiBAQCB2b2lkIFRleHR1cmVNYXBwZXJHTDo6YmVnaW5QYWlu dGluZygpCiAgICAgZ2xHZXRJbnRlZ2VydihHTF9DVVJSRU5UX1BST0dSQU0sICZkYXRhKCkucHJl dmlvdXNQcm9ncmFtKTsKICAgICBkYXRhKCkucHJldmlvdXNTY2lzc29yU3RhdGUgPSBnbElzRW5h YmxlZChHTF9TQ0lTU09SX1RFU1QpOwogICAgIGRhdGEoKS5wcmV2aW91c0RlcHRoU3RhdGUgPSBn bElzRW5hYmxlZChHTF9ERVBUSF9URVNUKTsKLSAgICBnbERpc2FibGUoR0xfREVQVEhfVEVTVCk7 Ci0gICAgZ2xFbmFibGUoR0xfU0NJU1NPUl9URVNUKTsKICNpZiBQTEFURk9STShRVCkKICAgICBp ZiAobV9jb250ZXh0KSB7CiAgICAgICAgIFFQYWludGVyKiBwYWludGVyID0gbV9jb250ZXh0LT5w bGF0Zm9ybUNvbnRleHQoKTsKQEAgLTI3Nyw2ICsyNzUsOCBAQCB2b2lkIFRleHR1cmVNYXBwZXJH TDo6YmVnaW5QYWludGluZygpCiAgICAgICAgIHBhaW50ZXItPmJlZ2luTmF0aXZlUGFpbnRpbmco KTsKICAgICB9CiAjZW5kaWYKKyAgICBnbERpc2FibGUoR0xfREVQVEhfVEVTVCk7CisgICAgZ2xF bmFibGUoR0xfU0NJU1NPUl9URVNUKTsKICAgICBkYXRhKCkuZGlkTW9kaWZ5U3RlbmNpbCA9IGZh bHNlOwogICAgIGdsRGVwdGhNYXNrKDApOwogICAgIGdsR2V0SW50ZWdlcnYoR0xfVklFV1BPUlQs IGRhdGEoKS52aWV3cG9ydCk7Ci0tIAoxLjcuOS41Cgo=