37194 2010-04-07 01:49:53 -0700 Prevent wrong use of PrintContext 2010-04-09 16:43:00 -0700 1 1 1 Unclassified WebKit WebCore Misc. 528+ (Nightly build) All All RESOLVED FIXED P2 Normal --- 1 hamaji webkit-unassigned hayato yuzo oldest_to_newest 209534 0 hamaji 2010-04-07 01:49:53 -0700 We can misuse PrintContext in several ways. We can call PrintContext::begin() twice, we can call PrintContext::end() without PrintContext::begin(), and we can forget PrintContext::end() after PrintContext::begin(). Bug 36049 is an example of such mistakes. Let's add a boolean flag to prevent such misuses. 209535 1 52714 hamaji 2010-04-07 01:51:31 -0700 Created attachment 52714 Patch v1 209538 2 52714 hamaji 2010-04-07 01:59:45 -0700 Comment on attachment 52714 Patch v1 > PrintContext::~PrintContext() > { > + if (m_isPrinting) > + end(); > m_pageRects.clear(); > } Or, we may want to add ASSERT(!m_isPrinting) here. I personally like this kind of automatic release, but I'm not sure if this is suitable for this case. If a user of PrintContext relies on this, her/his code may have begin() without end(), which might look weird a bit. Hmm... I changed my mind while I'm writing this comment. I'll revise the patch. 209539 3 52716 hamaji 2010-04-07 02:01:56 -0700 Created attachment 52716 Patch v2 210911 4 52716 eric 2010-04-09 14:03:13 -0700 Comment on attachment 52716 Patch v2 if (m_isPrinting) 43 end(); isn't needed. the ASSERT is enough. 210912 5 52716 eric 2010-04-09 14:04:01 -0700 Comment on attachment 52716 Patch v2 You should consider documenting what m_isPrinting is for in teh header, otherwise this looks OK (- the release-only auto-end stuff). r=me with the two above comments addressed. 210923 6 yuzo 2010-04-09 14:13:12 -0700 Why is m_isPrinting protected rather than private? 211023 7 hamaji 2010-04-09 16:41:04 -0700 Committed r57384: <http://trac.webkit.org/changeset/57384> 211024 8 hamaji 2010-04-09 16:43:00 -0700 Thanks for your suggestions! I addressed the three (2 from eric + 1 from yuzo) comments and landed the patch. 52714 2010-04-07 01:51:31 -0700 2010-04-07 02:01:51 -0700 Patch v1 bug-37194-20100407015129.patch text/plain 2228 hamaji ZGlmZiAtLWdpdCBhL1dlYkNvcmUvQ2hhbmdlTG9nIGIvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXgg MzMxMTM1MS4uNjU4OWFiZCAxMDA2NDQKLS0tIGEvV2ViQ29yZS9DaGFuZ2VMb2cKKysrIGIvV2Vi Q29yZS9DaGFuZ2VMb2cKQEAgLTEsMyArMSwxOSBAQAorMjAxMC0wNC0wNyAgU2hpbmljaGlybyBI YW1hamkgIDxoYW1hamlAY2hyb21pdW0ub3JnPgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9E WSAoT09QUyEpLgorCisgICAgICAgIFByZXZlbnQgd3JvbmcgdXNlIG9mIFByaW50Q29udGV4dAor ICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MzcxOTQKKwor ICAgICAgICBObyBuZXcgdGVzdHMgYmVjYXVzZSB0aGlzIGRvZXNuJ3QgY2hhbmdlIHRoZSBiZWhh dmlvci4KKworICAgICAgICAqIHBhZ2UvUHJpbnRDb250ZXh0LmNwcDoKKyAgICAgICAgKFdlYkNv cmU6OlByaW50Q29udGV4dDo6UHJpbnRDb250ZXh0KToKKyAgICAgICAgKFdlYkNvcmU6OlByaW50 Q29udGV4dDo6flByaW50Q29udGV4dCk6CisgICAgICAgIChXZWJDb3JlOjpQcmludENvbnRleHQ6 OmJlZ2luKToKKyAgICAgICAgKFdlYkNvcmU6OlByaW50Q29udGV4dDo6ZW5kKToKKyAgICAgICAg KiBwYWdlL1ByaW50Q29udGV4dC5oOgorCiAyMDEwLTA0LTA3ICBKYWltZSBZYXAgIDxqYWltZXlh cEBnb29nbGUuY29tPgogCiAgICAgICAgIFJldmlld2VkIGJ5IFl1cnkgU2VtaWtoYXRza3kuCmRp ZmYgLS1naXQgYS9XZWJDb3JlL3BhZ2UvUHJpbnRDb250ZXh0LmNwcCBiL1dlYkNvcmUvcGFnZS9Q cmludENvbnRleHQuY3BwCmluZGV4IDBkOWM0NTYuLjg2OGY4ZTEgMTAwNjQ0Ci0tLSBhL1dlYkNv cmUvcGFnZS9QcmludENvbnRleHQuY3BwCisrKyBiL1dlYkNvcmUvcGFnZS9QcmludENvbnRleHQu Y3BwCkBAIC0zMiwxMSArMzIsMTQgQEAgbmFtZXNwYWNlIFdlYkNvcmUgewogCiBQcmludENvbnRl eHQ6OlByaW50Q29udGV4dChGcmFtZSogZnJhbWUpCiAgICAgOiBtX2ZyYW1lKGZyYW1lKQorICAg ICwgbV9pc1ByaW50aW5nKGZhbHNlKQogewogfQogCiBQcmludENvbnRleHQ6On5QcmludENvbnRl eHQoKQogeworICAgIGlmIChtX2lzUHJpbnRpbmcpCisgICAgICAgIGVuZCgpOwogICAgIG1fcGFn ZVJlY3RzLmNsZWFyKCk7CiB9CiAKQEAgLTExNCw2ICsxMTcsOSBAQCB2b2lkIFByaW50Q29udGV4 dDo6Y29tcHV0ZVBhZ2VSZWN0c1dpdGhQYWdlU2l6ZUludGVybmFsKGNvbnN0IEZsb2F0U2l6ZSYg cGFnZVNpegogCiB2b2lkIFByaW50Q29udGV4dDo6YmVnaW4oZmxvYXQgd2lkdGgpCiB7CisgICAg QVNTRVJUKCFtX2lzUHJpbnRpbmcpOworICAgIG1faXNQcmludGluZyA9IHRydWU7CisKICAgICAv LyBCeSBpbWFnaW5nIHRvIGEgd2lkdGggYSBsaXR0bGUgd2lkZXIgdGhhbiB0aGUgYXZhaWxhYmxl IHBpeGVscywKICAgICAvLyB0aGluIHBhZ2VzIHdpbGwgYmUgc2NhbGVkIGRvd24gYSBsaXR0bGUs IG1hdGNoaW5nIHRoZSB3YXkgdGhleQogICAgIC8vIHByaW50IGluIElFIGFuZCBDYW1pbm8uIFRo aXMgbGV0cyB0aGVtIHVzZSBmZXdlciBzaGVldHMgdGhhbiB0aGV5CkBAIC0xNTAsNiArMTU2LDgg QEAgdm9pZCBQcmludENvbnRleHQ6OnNwb29sUGFnZShHcmFwaGljc0NvbnRleHQmIGN0eCwgaW50 IHBhZ2VOdW1iZXIsIGZsb2F0IHdpZHRoKQogCiB2b2lkIFByaW50Q29udGV4dDo6ZW5kKCkKIHsK KyAgICBBU1NFUlQobV9pc1ByaW50aW5nKTsKKyAgICBtX2lzUHJpbnRpbmcgPSBmYWxzZTsKICAg ICBtX2ZyYW1lLT5zZXRQcmludGluZyhmYWxzZSwgMCwgMCwgdHJ1ZSk7CiB9CiAKZGlmZiAtLWdp dCBhL1dlYkNvcmUvcGFnZS9QcmludENvbnRleHQuaCBiL1dlYkNvcmUvcGFnZS9QcmludENvbnRl eHQuaAppbmRleCA4YTFhNzdkLi44MGExYWVlIDEwMDY0NAotLS0gYS9XZWJDb3JlL3BhZ2UvUHJp bnRDb250ZXh0LmgKKysrIGIvV2ViQ29yZS9wYWdlL1ByaW50Q29udGV4dC5oCkBAIC01OSw2ICs1 OSw3IEBAIHB1YmxpYzoKIHByb3RlY3RlZDoKICAgICBGcmFtZSogbV9mcmFtZTsKICAgICBWZWN0 b3I8SW50UmVjdD4gbV9wYWdlUmVjdHM7CisgICAgYm9vbCBtX2lzUHJpbnRpbmc7CiAKIHByaXZh dGU6CiAgICAgdm9pZCBjb21wdXRlUGFnZVJlY3RzV2l0aFBhZ2VTaXplSW50ZXJuYWwoY29uc3Qg RmxvYXRTaXplJiBwYWdlU2l6ZUluUGl4ZWxzLCBib29sIGFsbG93SG9yaXpvbnRhbE11bHRpUGFn ZXMpOwo= 52716 2010-04-07 02:01:56 -0700 2010-04-09 14:04:01 -0700 Patch v2 bug-37194-20100407020154.patch text/plain 2256 hamaji ZGlmZiAtLWdpdCBhL1dlYkNvcmUvQ2hhbmdlTG9nIGIvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXgg MzMxMTM1MS4uNjU4OWFiZCAxMDA2NDQKLS0tIGEvV2ViQ29yZS9DaGFuZ2VMb2cKKysrIGIvV2Vi Q29yZS9DaGFuZ2VMb2cKQEAgLTEsMyArMSwxOSBAQAorMjAxMC0wNC0wNyAgU2hpbmljaGlybyBI YW1hamkgIDxoYW1hamlAY2hyb21pdW0ub3JnPgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9E WSAoT09QUyEpLgorCisgICAgICAgIFByZXZlbnQgd3JvbmcgdXNlIG9mIFByaW50Q29udGV4dAor ICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MzcxOTQKKwor ICAgICAgICBObyBuZXcgdGVzdHMgYmVjYXVzZSB0aGlzIGRvZXNuJ3QgY2hhbmdlIHRoZSBiZWhh dmlvci4KKworICAgICAgICAqIHBhZ2UvUHJpbnRDb250ZXh0LmNwcDoKKyAgICAgICAgKFdlYkNv cmU6OlByaW50Q29udGV4dDo6UHJpbnRDb250ZXh0KToKKyAgICAgICAgKFdlYkNvcmU6OlByaW50 Q29udGV4dDo6flByaW50Q29udGV4dCk6CisgICAgICAgIChXZWJDb3JlOjpQcmludENvbnRleHQ6 OmJlZ2luKToKKyAgICAgICAgKFdlYkNvcmU6OlByaW50Q29udGV4dDo6ZW5kKToKKyAgICAgICAg KiBwYWdlL1ByaW50Q29udGV4dC5oOgorCiAyMDEwLTA0LTA3ICBKYWltZSBZYXAgIDxqYWltZXlh cEBnb29nbGUuY29tPgogCiAgICAgICAgIFJldmlld2VkIGJ5IFl1cnkgU2VtaWtoYXRza3kuCmRp ZmYgLS1naXQgYS9XZWJDb3JlL3BhZ2UvUHJpbnRDb250ZXh0LmNwcCBiL1dlYkNvcmUvcGFnZS9Q cmludENvbnRleHQuY3BwCmluZGV4IDBkOWM0NTYuLjFkODUxMWEgMTAwNjQ0Ci0tLSBhL1dlYkNv cmUvcGFnZS9QcmludENvbnRleHQuY3BwCisrKyBiL1dlYkNvcmUvcGFnZS9QcmludENvbnRleHQu Y3BwCkBAIC0zMiwxMSArMzIsMTUgQEAgbmFtZXNwYWNlIFdlYkNvcmUgewogCiBQcmludENvbnRl eHQ6OlByaW50Q29udGV4dChGcmFtZSogZnJhbWUpCiAgICAgOiBtX2ZyYW1lKGZyYW1lKQorICAg ICwgbV9pc1ByaW50aW5nKGZhbHNlKQogewogfQogCiBQcmludENvbnRleHQ6On5QcmludENvbnRl eHQoKQogeworICAgIEFTU0VSVCghbV9pc1ByaW50aW5nKTsKKyAgICBpZiAobV9pc1ByaW50aW5n KQorICAgICAgICBlbmQoKTsKICAgICBtX3BhZ2VSZWN0cy5jbGVhcigpOwogfQogCkBAIC0xMTQs NiArMTE4LDkgQEAgdm9pZCBQcmludENvbnRleHQ6OmNvbXB1dGVQYWdlUmVjdHNXaXRoUGFnZVNp emVJbnRlcm5hbChjb25zdCBGbG9hdFNpemUmIHBhZ2VTaXoKIAogdm9pZCBQcmludENvbnRleHQ6 OmJlZ2luKGZsb2F0IHdpZHRoKQogeworICAgIEFTU0VSVCghbV9pc1ByaW50aW5nKTsKKyAgICBt X2lzUHJpbnRpbmcgPSB0cnVlOworCiAgICAgLy8gQnkgaW1hZ2luZyB0byBhIHdpZHRoIGEgbGl0 dGxlIHdpZGVyIHRoYW4gdGhlIGF2YWlsYWJsZSBwaXhlbHMsCiAgICAgLy8gdGhpbiBwYWdlcyB3 aWxsIGJlIHNjYWxlZCBkb3duIGEgbGl0dGxlLCBtYXRjaGluZyB0aGUgd2F5IHRoZXkKICAgICAv LyBwcmludCBpbiBJRSBhbmQgQ2FtaW5vLiBUaGlzIGxldHMgdGhlbSB1c2UgZmV3ZXIgc2hlZXRz IHRoYW4gdGhleQpAQCAtMTUwLDYgKzE1Nyw4IEBAIHZvaWQgUHJpbnRDb250ZXh0OjpzcG9vbFBh Z2UoR3JhcGhpY3NDb250ZXh0JiBjdHgsIGludCBwYWdlTnVtYmVyLCBmbG9hdCB3aWR0aCkKIAog dm9pZCBQcmludENvbnRleHQ6OmVuZCgpCiB7CisgICAgQVNTRVJUKG1faXNQcmludGluZyk7Cisg ICAgbV9pc1ByaW50aW5nID0gZmFsc2U7CiAgICAgbV9mcmFtZS0+c2V0UHJpbnRpbmcoZmFsc2Us IDAsIDAsIHRydWUpOwogfQogCmRpZmYgLS1naXQgYS9XZWJDb3JlL3BhZ2UvUHJpbnRDb250ZXh0 LmggYi9XZWJDb3JlL3BhZ2UvUHJpbnRDb250ZXh0LmgKaW5kZXggOGExYTc3ZC4uODBhMWFlZSAx MDA2NDQKLS0tIGEvV2ViQ29yZS9wYWdlL1ByaW50Q29udGV4dC5oCisrKyBiL1dlYkNvcmUvcGFn ZS9QcmludENvbnRleHQuaApAQCAtNTksNiArNTksNyBAQCBwdWJsaWM6CiBwcm90ZWN0ZWQ6CiAg ICAgRnJhbWUqIG1fZnJhbWU7CiAgICAgVmVjdG9yPEludFJlY3Q+IG1fcGFnZVJlY3RzOworICAg IGJvb2wgbV9pc1ByaW50aW5nOwogCiBwcml2YXRlOgogICAgIHZvaWQgY29tcHV0ZVBhZ2VSZWN0 c1dpdGhQYWdlU2l6ZUludGVybmFsKGNvbnN0IEZsb2F0U2l6ZSYgcGFnZVNpemVJblBpeGVscywg Ym9vbCBhbGxvd0hvcml6b250YWxNdWx0aVBhZ2VzKTsK