100923 2012-11-01 00:07:34 -0700 Free GL resources allocated by GraphicsContext3DEfl 2012-11-01 07:17:26 -0700 1 1 1 Unclassified WebKit WebKit EFL 528+ (Nightly build) PC Linux RESOLVED FIXED P2 Normal --- 0 kalyan.kondapally webkit-unassigned dino lucas.de.marchi webkit.review.bot oldest_to_newest 755948 0 kalyan.kondapally 2012-11-01 00:07:34 -0700 GraphicsContext3DEfl creates FBO's, textures and render buffer's, but doesn't free them. 755949 1 171784 kalyan.kondapally 2012-11-01 00:10:01 -0700 Created attachment 171784 proposed-patch 756076 2 171784 kenneth 2012-11-01 04:18:21 -0700 Comment on attachment 171784 proposed-patch View in context: https://bugs.webkit.org/attachment.cgi?id=171784&action=review > Source/WebCore/platform/graphics/efl/GraphicsContext3DEfl.cpp:156 > + if (m_renderStyle == RenderToCurrentGLContext || !m_private) > + return; > + makeContextCurrent(); We usually add a newline after return statements and before if/for/while etc sentenses unless the code is very relevant to it, like declaration of iterators So newline after return here. I also think this return needs a comment ontop of the if. > Source/WebCore/platform/graphics/efl/GraphicsContext3DEfl.cpp:158 > + glDeleteTextures(1, &m_texture); > + if (m_attrs.antialias) { newline before if here > Source/WebCore/platform/graphics/efl/GraphicsContext3DEfl.cpp:161 > + if (m_attrs.stencil || m_attrs.depth) > + glDeleteRenderbuffers(1, &m_multisampleDepthStencilBuffer); Newline before and after > Source/WebCore/platform/graphics/efl/GraphicsContext3DEfl.cpp:166 > + glDeleteRenderbuffers(1, &m_depthBuffer); newline after this 756130 3 171823 kalyan.kondapally 2012-11-01 05:46:40 -0700 Created attachment 171823 patch Review Changes 756139 4 171823 kenneth 2012-11-01 05:58:53 -0700 Comment on attachment 171823 patch View in context: https://bugs.webkit.org/attachment.cgi?id=171823&action=review > Source/WebCore/platform/graphics/efl/GraphicsContext3DEfl.cpp:155 > + if (m_renderStyle == RenderToCurrentGLContext || !m_private) > + return; Couldn't we have a comment here why no freeing is needed in that case? > Source/WebCore/platform/graphics/efl/GraphicsContext3DEfl.cpp:169 > + } else if (m_attrs.stencil || m_attrs.depth) { > + > +#if USE(OPENGL_ES_2) no newline here sorry. Why does this have to be a separate "else if". You are chekcing with if's later down 756192 5 171823 kenneth 2012-11-01 07:09:06 -0700 Comment on attachment 171823 patch Let's just land this 756207 6 171823 webkit.review.bot 2012-11-01 07:17:22 -0700 Comment on attachment 171823 patch Clearing flags on attachment: 171823 Committed r133162: <http://trac.webkit.org/changeset/133162> 756208 7 webkit.review.bot 2012-11-01 07:17:26 -0700 All reviewed patches have been landed. Closing bug. 171784 2012-11-01 00:10:01 -0700 2012-11-01 05:46:40 -0700 proposed-patch deleteglobjects.patch text/plain 2188 kalyan.kondapally ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJDb3JlL0No YW5nZUxvZwppbmRleCBiODUzYWNlLi43YWVlMWUxIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29y ZS9DaGFuZ2VMb2cKKysrIGIvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTYg QEAKKzIwMTItMTAtMzEgIEtvbmRhcGFsbHkgS2FseWFuICA8a2FseWFuLmtvbmRhcGFsbHlAaW50 ZWwuY29tPgorCisgICAgICAgIFtFRkxdW0FDXUZyZWUgR0wgcmVzb3VyY2VzIGFsbG9jYXRlZCBi eSBHcmFwaGljc0NvbnRleHQzREVmbC4KKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcv c2hvd19idWcuY2dpP2lkPTEwMDkyMy4KKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9P UFMhKS4KKworICAgICAgICBHcmFwaGljc0NvbnRleHQzREVmbCBjcmVhdGVzIEZCTydzLCB0ZXh0 dXJlcyBhbmQgcmVuZGVyIGJ1ZmZlcidzLCBidXQgZG9lc24ndCBmcmVlIHRoZW0uCisgICAgICAg IFRoaXMgcGF0Y2ggZW5zdXJlcyB0aGF0IEdyYXBoaWNzQ29udGV4dDNkRUZsIGRlbGV0ZXMgYWxs IHRoZSBHTCByZXNvdXJjZXMgYWxsb2NhdGVkIGJ5IGl0LgorCisgICAgICAgICogcGxhdGZvcm0v Z3JhcGhpY3MvZWZsL0dyYXBoaWNzQ29udGV4dDNERWZsLmNwcDoKKyAgICAgICAgKFdlYkNvcmU6 OkdyYXBoaWNzQ29udGV4dDNEOjp+R3JhcGhpY3NDb250ZXh0M0QpOgorCiAyMDEyLTEwLTMxICBB bGV4YW5kcnUgQ2hpY3VsaXRhICA8YWNoaWN1QGFkb2JlLmNvbT4KCiAgICAgICAgIEN1c3RvbUZp bHRlck9wZXJhdGlvbiBwYXJhbWV0ZXJzIGxpc3QgaXMgbm90IGRlY29kZWQgY29ycmVjdGx5IGlu IENvb3JkaW5hdGVkR3JhcGhpY3NBcmd1bWVudENvZGVycwpkaWZmIC0tZ2l0IGEvU291cmNlL1dl YkNvcmUvcGxhdGZvcm0vZ3JhcGhpY3MvZWZsL0dyYXBoaWNzQ29udGV4dDNERWZsLmNwcCBiL1Nv dXJjZS9XZWJDb3JlL3BsYXRmb3JtL2dyYXBoaWNzL2VmbC9HcmFwaGljc0NvbnRleHQzREVmbC5j cHAKaW5kZXggYjRjMTM1NC4uN2Y0NmE1MiAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvcGxh dGZvcm0vZ3JhcGhpY3MvZWZsL0dyYXBoaWNzQ29udGV4dDNERWZsLmNwcAorKysgYi9Tb3VyY2Uv V2ViQ29yZS9wbGF0Zm9ybS9ncmFwaGljcy9lZmwvR3JhcGhpY3NDb250ZXh0M0RFZmwuY3BwCkBA IC0xNTEsNiArMTUxLDI1IEBAIEdyYXBoaWNzQ29udGV4dDNEOjpHcmFwaGljc0NvbnRleHQzRChH cmFwaGljc0NvbnRleHQzRDo6QXR0cmlidXRlcyBhdHRycywgSG9zdFdpCgogR3JhcGhpY3NDb250 ZXh0M0Q6On5HcmFwaGljc0NvbnRleHQzRCgpCiB7CisgICAgaWYgKG1fcmVuZGVyU3R5bGUgPT0g UmVuZGVyVG9DdXJyZW50R0xDb250ZXh0IHx8ICFtX3ByaXZhdGUpCisgICAgICAgIHJldHVybjsK KyAgICBtYWtlQ29udGV4dEN1cnJlbnQoKTsKKyAgICBnbERlbGV0ZVRleHR1cmVzKDEsICZtX3Rl eHR1cmUpOworICAgIGlmIChtX2F0dHJzLmFudGlhbGlhcykgeworICAgICAgICBnbERlbGV0ZVJl bmRlcmJ1ZmZlcnMoMSwgJm1fbXVsdGlzYW1wbGVDb2xvckJ1ZmZlcik7CisgICAgICAgIGlmICht X2F0dHJzLnN0ZW5jaWwgfHwgbV9hdHRycy5kZXB0aCkKKyAgICAgICAgICAgIGdsRGVsZXRlUmVu ZGVyYnVmZmVycygxLCAmbV9tdWx0aXNhbXBsZURlcHRoU3RlbmNpbEJ1ZmZlcik7CisgICAgICAg IGdsRGVsZXRlRnJhbWVidWZmZXJzKDEsICZtX211bHRpc2FtcGxlRkJPKTsKKyAgICB9IGVsc2Ug aWYgKG1fYXR0cnMuc3RlbmNpbCB8fCBtX2F0dHJzLmRlcHRoKSB7CisjaWYgVVNFKE9QRU5HTF9F U18yKQorICAgICAgICBpZiAobV9hdHRycy5kZXB0aCkKKyAgICAgICAgICAgIGdsRGVsZXRlUmVu ZGVyYnVmZmVycygxLCAmbV9kZXB0aEJ1ZmZlcik7CisgICAgICAgIGlmIChtX2F0dHJzLnN0ZW5j aWwpCisgICAgICAgICAgICBnbERlbGV0ZVJlbmRlcmJ1ZmZlcnMoMSwgJm1fc3RlbmNpbEJ1ZmZl cik7CisjZW5kaWYKKyAgICAgICAgZ2xEZWxldGVSZW5kZXJidWZmZXJzKDEsICZtX2RlcHRoU3Rl bmNpbEJ1ZmZlcik7CisgICAgfQorICAgIGdsRGVsZXRlRnJhbWVidWZmZXJzKDEsICZtX2Zibyk7 CiB9CgogUGxhdGZvcm1HcmFwaGljc0NvbnRleHQzRCBHcmFwaGljc0NvbnRleHQzRDo6cGxhdGZv cm1HcmFwaGljc0NvbnRleHQzRCgpCg== 171823 2012-11-01 05:46:40 -0700 2012-11-01 07:17:22 -0700 patch 100923.patch text/plain 2201 kalyan.kondapally ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJDb3JlL0No YW5nZUxvZwppbmRleCBiODUzYWNlLi5iNzQ2ZjVkIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29y ZS9DaGFuZ2VMb2cKKysrIGIvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTYg QEAKKzIwMTItMTAtMzEgIEtvbmRhcGFsbHkgS2FseWFuICA8a2FseWFuLmtvbmRhcGFsbHlAaW50 ZWwuY29tPgorCisgICAgICAgIFtFRkxdW0FDXUZyZWUgR0wgcmVzb3VyY2VzIGFsbG9jYXRlZCBi eSBHcmFwaGljc0NvbnRleHQzREVmbC4KKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcv c2hvd19idWcuY2dpP2lkPTEwMDkyMy4KKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9P UFMhKS4KKworICAgICAgICBHcmFwaGljc0NvbnRleHQzREVmbCBjcmVhdGVzIEZCTydzLCB0ZXh0 dXJlcyBhbmQgcmVuZGVyIGJ1ZmZlcidzLCBidXQgZG9lc24ndCBmcmVlIHRoZW0uCisgICAgICAg IFRoaXMgcGF0Y2ggbWFrZXMgc3VyZSB0aGF0IEdyYXBoaWNzQ29udGV4dDNkRWZsIGZyZWVzIGFs bCB0aGUgR0wgcmVzb3VyY2VzIGFsbG9jYXRlZCBieSBpdC4KKworICAgICAgICAqIHBsYXRmb3Jt L2dyYXBoaWNzL2VmbC9HcmFwaGljc0NvbnRleHQzREVmbC5jcHA6CisgICAgICAgIChXZWJDb3Jl OjpHcmFwaGljc0NvbnRleHQzRDo6fkdyYXBoaWNzQ29udGV4dDNEKToKKwogMjAxMi0xMC0zMSAg QWxleGFuZHJ1IENoaWN1bGl0YSAgPGFjaGljdUBhZG9iZS5jb20+CgogICAgICAgICBDdXN0b21G aWx0ZXJPcGVyYXRpb24gcGFyYW1ldGVycyBsaXN0IGlzIG5vdCBkZWNvZGVkIGNvcnJlY3RseSBp biBDb29yZGluYXRlZEdyYXBoaWNzQXJndW1lbnRDb2RlcnMKZGlmZiAtLWdpdCBhL1NvdXJjZS9X ZWJDb3JlL3BsYXRmb3JtL2dyYXBoaWNzL2VmbC9HcmFwaGljc0NvbnRleHQzREVmbC5jcHAgYi9T b3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9ncmFwaGljcy9lZmwvR3JhcGhpY3NDb250ZXh0M0RFZmwu Y3BwCmluZGV4IGI0YzEzNTQuLmQxMzkxMzAgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJDb3JlL3Bs YXRmb3JtL2dyYXBoaWNzL2VmbC9HcmFwaGljc0NvbnRleHQzREVmbC5jcHAKKysrIGIvU291cmNl L1dlYkNvcmUvcGxhdGZvcm0vZ3JhcGhpY3MvZWZsL0dyYXBoaWNzQ29udGV4dDNERWZsLmNwcApA QCAtMTUxLDYgKzE1MSwzMSBAQCBHcmFwaGljc0NvbnRleHQzRDo6R3JhcGhpY3NDb250ZXh0M0Qo R3JhcGhpY3NDb250ZXh0M0Q6OkF0dHJpYnV0ZXMgYXR0cnMsIEhvc3RXaQoKIEdyYXBoaWNzQ29u dGV4dDNEOjp+R3JhcGhpY3NDb250ZXh0M0QoKQogeworICAgIGlmIChtX3JlbmRlclN0eWxlID09 IFJlbmRlclRvQ3VycmVudEdMQ29udGV4dCB8fCAhbV9wcml2YXRlKQorICAgICAgICByZXR1cm47 CisKKyAgICBtYWtlQ29udGV4dEN1cnJlbnQoKTsKKyAgICBnbERlbGV0ZVRleHR1cmVzKDEsICZt X3RleHR1cmUpOworCisgICAgaWYgKG1fYXR0cnMuYW50aWFsaWFzKSB7CisgICAgICAgIGdsRGVs ZXRlUmVuZGVyYnVmZmVycygxLCAmbV9tdWx0aXNhbXBsZUNvbG9yQnVmZmVyKTsKKworICAgICAg ICBpZiAobV9hdHRycy5zdGVuY2lsIHx8IG1fYXR0cnMuZGVwdGgpCisgICAgICAgICAgICBnbERl bGV0ZVJlbmRlcmJ1ZmZlcnMoMSwgJm1fbXVsdGlzYW1wbGVEZXB0aFN0ZW5jaWxCdWZmZXIpOwor CisgICAgICAgIGdsRGVsZXRlRnJhbWVidWZmZXJzKDEsICZtX211bHRpc2FtcGxlRkJPKTsKKyAg ICB9IGVsc2UgaWYgKG1fYXR0cnMuc3RlbmNpbCB8fCBtX2F0dHJzLmRlcHRoKSB7CisKKyNpZiBV U0UoT1BFTkdMX0VTXzIpCisgICAgICAgIGlmIChtX2F0dHJzLmRlcHRoKQorICAgICAgICAgICAg Z2xEZWxldGVSZW5kZXJidWZmZXJzKDEsICZtX2RlcHRoQnVmZmVyKTsKKworICAgICAgICBpZiAo bV9hdHRycy5zdGVuY2lsKQorICAgICAgICAgICAgZ2xEZWxldGVSZW5kZXJidWZmZXJzKDEsICZt X3N0ZW5jaWxCdWZmZXIpOworI2VuZGlmCisgICAgICAgIGdsRGVsZXRlUmVuZGVyYnVmZmVycygx LCAmbV9kZXB0aFN0ZW5jaWxCdWZmZXIpOworICAgIH0KKyAgICBnbERlbGV0ZUZyYW1lYnVmZmVy cygxLCAmbV9mYm8pOwogfQoKIFBsYXRmb3JtR3JhcGhpY3NDb250ZXh0M0QgR3JhcGhpY3NDb250 ZXh0M0Q6OnBsYXRmb3JtR3JhcGhpY3NDb250ZXh0M0QoKQo=