36487 2010-03-23 04:39:04 -0700 Refactoring: HTMLFormControlElement should not have redundant null check 2010-03-24 15:45:14 -0700 1 1 1 Unclassified WebKit DOM 528+ (Nightly build) PC OS X 10.5 RESOLVED FIXED P3 Normal --- 0 morrita webkit-unassigned ap commit-queue oldest_to_newest 202996 0 morrita 2010-03-23 04:39:04 -0700 HTMLFormControlElement::dispatchBlurEvent() and HTMLFormControlElement::dispatchFocusEvent() has redundant NULL-check. We can remove it to simplify the code. 203001 1 51413 morrita 2010-03-23 04:55:10 -0700 Created attachment 51413 a small change for cleanup 203117 2 ap 2010-03-23 11:14:47 -0700 It looks like this patch introduces a redundant null check. Or am I just confused? Page* Document::page() const { return m_frame ? m_frame->page() : 0; } 203194 3 51413 darin 2010-03-23 13:57:02 -0700 Comment on attachment 51413 a small change for cleanup The changes to HTMLFormControlElement::dispatchFocusEvent and HTMLFormControlElement::dispatchBlurEvent are OK. Not a great improvement, but nice to use the Document::page helper function to save a little code. The changes to HTMLCanvasElement::convertLogicalToDevice and HTMLCanvasElement::convertLogicalToDevice are different though. Those are adding a null check of Page*. Presumably, this is a bug fix. Before, there would be a crash if the page was zero. That means we need to construct a test case to demonstrate that crash, and that the crash is fixed. review- 203322 4 51477 morrita 2010-03-23 22:18:27 -0700 Created attachment 51477 v1; removed the chagne on HTMLCanvasElemet 203323 5 morrita 2010-03-23 22:21:11 -0700 Thank you for reviewing! > The changes to HTMLCanvasElement::convertLogicalToDevice and > HTMLCanvasElement::convertLogicalToDevice are different though. Those are > adding a null check of Page*. Presumably, this is a bug fix. Before, there > would be a crash if the page was zero. That means we need to construct a test > case to demonstrate that crash, and that the crash is fixed. Agreed. But I could not figure out the test case for crash, so I remove the change on HTMLCanvasElement. @ap Thank you reviewing too. You are right. I did attempt to simplify the null-check code, but It actually introduced a redundant null check at runtime. Regards. 203662 6 51477 commit-queue 2010-03-24 15:45:09 -0700 Comment on attachment 51477 v1; removed the chagne on HTMLCanvasElemet Clearing flags on attachment: 51477 Committed r56463: <http://trac.webkit.org/changeset/56463> 203663 7 commit-queue 2010-03-24 15:45:14 -0700 All reviewed patches have been landed. Closing bug. 51413 2010-03-23 04:55:10 -0700 2010-03-23 22:18:21 -0700 a small change for cleanup bug-36487-20100323205508.patch text/plain 3083 morrita ZGlmZiAtLWdpdCBhL1dlYkNvcmUvQ2hhbmdlTG9nIGIvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXgg ZjhhN2Y3Yi4uYzA0ZDkxNiAxMDA2NDQKLS0tIGEvV2ViQ29yZS9DaGFuZ2VMb2cKKysrIGIvV2Vi Q29yZS9DaGFuZ2VMb2cKQEAgLTEsMyArMSwxOCBAQAorMjAxMC0wMy0yMyAgTU9SSVRBIEhhamlt ZSAgPG1vcnJpdGFAZ29vZ2xlLmNvbT4KKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9P UFMhKS4KKworICAgICAgICBSZWZhY3RvcmluZzogSFRNTEZvcm1Db250cm9sRWxlbWVudCBhbmQg SFRNTENhbnZhc0VsZW1lbnQgc2hvdWxkIG5vdCBoYXZlIHJlZHVuZGFudCBudWxsIGNoZWNrCisg ICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0zNjQ4NworICAg ICAgICAKKyAgICAgICAgTm8gbmV3IHRlc3RzIGFzIHRoZXJlIGlzIG5vIG5ldyBmdW5jdGlvbmFs aXR5LiAgICAgICAgCisKKyAgICAgICAgKiBodG1sL0hUTUxGb3JtQ29udHJvbEVsZW1lbnQuY3Bw OgorICAgICAgICAoV2ViQ29yZTo6SFRNTEZvcm1Db250cm9sRWxlbWVudDo6ZGlzcGF0Y2hGb2N1 c0V2ZW50KToKKyAgICAgICAgKFdlYkNvcmU6OkhUTUxGb3JtQ29udHJvbEVsZW1lbnQ6OmRpc3Bh dGNoQmx1ckV2ZW50KToKKyAgICAgICAgKiBodG1sL0hUTUxDYW52YXNFbGVtZW50LmNwcDoKKyAg ICAgICAgKFdlYkNvcmU6OkhUTUxDYW52YXNFbGVtZW50Ojpjb252ZXJ0TG9naWNhbFRvRGV2aWNl KToKKwogMjAxMC0wMy0yMiAgRnVtaXRvc2hpIFVrYWkgIDx1a2FpQGNocm9taXVtLm9yZz4KIAog ICAgICAgICBVbnJldmlld2VkIGJ1aWxkIGZpeCBmb3IgQ2hyb21pbSBNYWMgYXQgcjU2Mzc2CmRp ZmYgLS1naXQgYS9XZWJDb3JlL2h0bWwvSFRNTENhbnZhc0VsZW1lbnQuY3BwIGIvV2ViQ29yZS9o dG1sL0hUTUxDYW52YXNFbGVtZW50LmNwcAppbmRleCAzMGE2MjBjLi40MzZjODM5IDEwMDY0NAot LS0gYS9XZWJDb3JlL2h0bWwvSFRNTENhbnZhc0VsZW1lbnQuY3BwCisrKyBiL1dlYkNvcmUvaHRt bC9IVE1MQ2FudmFzRWxlbWVudC5jcHAKQEAgLTI4Niw3ICsyODYsNyBAQCBJbnRSZWN0IEhUTUxD YW52YXNFbGVtZW50Ojpjb252ZXJ0TG9naWNhbFRvRGV2aWNlKGNvbnN0IEZsb2F0UmVjdCYgbG9n aWNhbFJlY3QpCiAKIEludFNpemUgSFRNTENhbnZhc0VsZW1lbnQ6OmNvbnZlcnRMb2dpY2FsVG9E ZXZpY2UoY29uc3QgRmxvYXRTaXplJiBsb2dpY2FsU2l6ZSkgY29uc3QKIHsKLSAgICBmbG9hdCBw YWdlU2NhbGVGYWN0b3IgPSBkb2N1bWVudCgpLT5mcmFtZSgpID8gZG9jdW1lbnQoKS0+ZnJhbWUo KS0+cGFnZSgpLT5jaHJvbWUoKS0+c2NhbGVGYWN0b3IoKSA6IDEuMGY7CisgICAgZmxvYXQgcGFn ZVNjYWxlRmFjdG9yID0gZG9jdW1lbnQoKS0+cGFnZSgpID8gZG9jdW1lbnQoKS0+cGFnZSgpLT5j aHJvbWUoKS0+c2NhbGVGYWN0b3IoKSA6IDEuMGY7CiAgICAgZmxvYXQgd2YgPSBjZWlsZihsb2dp Y2FsU2l6ZS53aWR0aCgpICogcGFnZVNjYWxlRmFjdG9yKTsKICAgICBmbG9hdCBoZiA9IGNlaWxm KGxvZ2ljYWxTaXplLmhlaWdodCgpICogcGFnZVNjYWxlRmFjdG9yKTsKICAgICAKQEAgLTI5OCw3 ICsyOTgsNyBAQCBJbnRTaXplIEhUTUxDYW52YXNFbGVtZW50Ojpjb252ZXJ0TG9naWNhbFRvRGV2 aWNlKGNvbnN0IEZsb2F0U2l6ZSYgbG9naWNhbFNpemUpCiAKIEludFBvaW50IEhUTUxDYW52YXNF bGVtZW50Ojpjb252ZXJ0TG9naWNhbFRvRGV2aWNlKGNvbnN0IEZsb2F0UG9pbnQmIGxvZ2ljYWxQ b3MpIGNvbnN0CiB7Ci0gICAgZmxvYXQgcGFnZVNjYWxlRmFjdG9yID0gZG9jdW1lbnQoKS0+ZnJh bWUoKSA/IGRvY3VtZW50KCktPmZyYW1lKCktPnBhZ2UoKS0+Y2hyb21lKCktPnNjYWxlRmFjdG9y KCkgOiAxLjBmOworICAgIGZsb2F0IHBhZ2VTY2FsZUZhY3RvciA9IGRvY3VtZW50KCktPnBhZ2Uo KSA/IGRvY3VtZW50KCktPnBhZ2UoKS0+Y2hyb21lKCktPnNjYWxlRmFjdG9yKCkgOiAxLjBmOwog ICAgIGZsb2F0IHhmID0gbG9naWNhbFBvcy54KCkgKiBwYWdlU2NhbGVGYWN0b3I7CiAgICAgZmxv YXQgeWYgPSBsb2dpY2FsUG9zLnkoKSAqIHBhZ2VTY2FsZUZhY3RvcjsKICAgICAKZGlmZiAtLWdp dCBhL1dlYkNvcmUvaHRtbC9IVE1MRm9ybUNvbnRyb2xFbGVtZW50LmNwcCBiL1dlYkNvcmUvaHRt bC9IVE1MRm9ybUNvbnRyb2xFbGVtZW50LmNwcAppbmRleCBiNmExM2VjLi5iYTYyY2FiIDEwMDY0 NAotLS0gYS9XZWJDb3JlL2h0bWwvSFRNTEZvcm1Db250cm9sRWxlbWVudC5jcHAKKysrIGIvV2Vi Q29yZS9odG1sL0hUTUxGb3JtQ29udHJvbEVsZW1lbnQuY3BwCkBAIC0zNDUsMTYgKzM0NSwxNiBA QCB2b2lkIEhUTUxGb3JtQ29udHJvbEVsZW1lbnQ6OnNldEN1c3RvbVZhbGlkaXR5KGNvbnN0IFN0 cmluZyYgZXJyb3IpCiAgICAgCiB2b2lkIEhUTUxGb3JtQ29udHJvbEVsZW1lbnQ6OmRpc3BhdGNo Rm9jdXNFdmVudCgpCiB7Ci0gICAgaWYgKGRvY3VtZW50KCktPmZyYW1lKCkgJiYgZG9jdW1lbnQo KS0+ZnJhbWUoKS0+cGFnZSgpKQotICAgICAgICBkb2N1bWVudCgpLT5mcmFtZSgpLT5wYWdlKCkt PmNocm9tZSgpLT5jbGllbnQoKS0+Zm9ybURpZEZvY3VzKHRoaXMpOworICAgIGlmIChkb2N1bWVu dCgpLT5wYWdlKCkpCisgICAgICAgIGRvY3VtZW50KCktPnBhZ2UoKS0+Y2hyb21lKCktPmNsaWVu dCgpLT5mb3JtRGlkRm9jdXModGhpcyk7CiAKICAgICBIVE1MRWxlbWVudDo6ZGlzcGF0Y2hGb2N1 c0V2ZW50KCk7CiB9CiAKIHZvaWQgSFRNTEZvcm1Db250cm9sRWxlbWVudDo6ZGlzcGF0Y2hCbHVy RXZlbnQoKQogewotICAgIGlmIChkb2N1bWVudCgpLT5mcmFtZSgpICYmIGRvY3VtZW50KCktPmZy YW1lKCktPnBhZ2UoKSkKLSAgICAgICAgZG9jdW1lbnQoKS0+ZnJhbWUoKS0+cGFnZSgpLT5jaHJv bWUoKS0+Y2xpZW50KCktPmZvcm1EaWRCbHVyKHRoaXMpOworICAgIGlmIChkb2N1bWVudCgpLT5w YWdlKCkpCisgICAgICAgIGRvY3VtZW50KCktPnBhZ2UoKS0+Y2hyb21lKCktPmNsaWVudCgpLT5m b3JtRGlkQmx1cih0aGlzKTsKIAogICAgIEhUTUxFbGVtZW50OjpkaXNwYXRjaEJsdXJFdmVudCgp OwogfQo= 51477 2010-03-23 22:18:27 -0700 2010-03-24 15:45:09 -0700 v1; removed the chagne on HTMLCanvasElemet bug-36487-20100324141825.patch text/plain 1712 morrita ZGlmZiAtLWdpdCBhL1dlYkNvcmUvQ2hhbmdlTG9nIGIvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXgg ZjhhN2Y3Yi4uM2MyZmM1ZCAxMDA2NDQKLS0tIGEvV2ViQ29yZS9DaGFuZ2VMb2cKKysrIGIvV2Vi Q29yZS9DaGFuZ2VMb2cKQEAgLTEsMyArMSwxNiBAQAorMjAxMC0wMy0yMyAgTU9SSVRBIEhhamlt ZSAgPG1vcnJpdGFAZ29vZ2xlLmNvbT4KKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9P UFMhKS4KKworICAgICAgICBSZWZhY3RvcmluZzogSFRNTEZvcm1Db250cm9sRWxlbWVudCBzaG91 bGQgbm90IGhhdmUgcmVkdW5kYW50IG51bGwgY2hlY2sKKyAgICAgICAgaHR0cHM6Ly9idWdzLndl YmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTM2NDg3CisgICAgICAgIAorICAgICAgICBObyBuZXcg dGVzdHMgYXMgdGhlcmUgaXMgbm8gbmV3IGZ1bmN0aW9uYWxpdHkuICAgICAgICAKKworICAgICAg ICAqIGh0bWwvSFRNTEZvcm1Db250cm9sRWxlbWVudC5jcHA6CisgICAgICAgIChXZWJDb3JlOjpI VE1MRm9ybUNvbnRyb2xFbGVtZW50OjpkaXNwYXRjaEZvY3VzRXZlbnQpOgorICAgICAgICAoV2Vi Q29yZTo6SFRNTEZvcm1Db250cm9sRWxlbWVudDo6ZGlzcGF0Y2hCbHVyRXZlbnQpOgorCiAyMDEw LTAzLTIyICBGdW1pdG9zaGkgVWthaSAgPHVrYWlAY2hyb21pdW0ub3JnPgogCiAgICAgICAgIFVu cmV2aWV3ZWQgYnVpbGQgZml4IGZvciBDaHJvbWltIE1hYyBhdCByNTYzNzYKZGlmZiAtLWdpdCBh L1dlYkNvcmUvaHRtbC9IVE1MRm9ybUNvbnRyb2xFbGVtZW50LmNwcCBiL1dlYkNvcmUvaHRtbC9I VE1MRm9ybUNvbnRyb2xFbGVtZW50LmNwcAppbmRleCBiNmExM2VjLi5iYTYyY2FiIDEwMDY0NAot LS0gYS9XZWJDb3JlL2h0bWwvSFRNTEZvcm1Db250cm9sRWxlbWVudC5jcHAKKysrIGIvV2ViQ29y ZS9odG1sL0hUTUxGb3JtQ29udHJvbEVsZW1lbnQuY3BwCkBAIC0zNDUsMTYgKzM0NSwxNiBAQCB2 b2lkIEhUTUxGb3JtQ29udHJvbEVsZW1lbnQ6OnNldEN1c3RvbVZhbGlkaXR5KGNvbnN0IFN0cmlu ZyYgZXJyb3IpCiAgICAgCiB2b2lkIEhUTUxGb3JtQ29udHJvbEVsZW1lbnQ6OmRpc3BhdGNoRm9j dXNFdmVudCgpCiB7Ci0gICAgaWYgKGRvY3VtZW50KCktPmZyYW1lKCkgJiYgZG9jdW1lbnQoKS0+ ZnJhbWUoKS0+cGFnZSgpKQotICAgICAgICBkb2N1bWVudCgpLT5mcmFtZSgpLT5wYWdlKCktPmNo cm9tZSgpLT5jbGllbnQoKS0+Zm9ybURpZEZvY3VzKHRoaXMpOworICAgIGlmIChkb2N1bWVudCgp LT5wYWdlKCkpCisgICAgICAgIGRvY3VtZW50KCktPnBhZ2UoKS0+Y2hyb21lKCktPmNsaWVudCgp LT5mb3JtRGlkRm9jdXModGhpcyk7CiAKICAgICBIVE1MRWxlbWVudDo6ZGlzcGF0Y2hGb2N1c0V2 ZW50KCk7CiB9CiAKIHZvaWQgSFRNTEZvcm1Db250cm9sRWxlbWVudDo6ZGlzcGF0Y2hCbHVyRXZl bnQoKQogewotICAgIGlmIChkb2N1bWVudCgpLT5mcmFtZSgpICYmIGRvY3VtZW50KCktPmZyYW1l KCktPnBhZ2UoKSkKLSAgICAgICAgZG9jdW1lbnQoKS0+ZnJhbWUoKS0+cGFnZSgpLT5jaHJvbWUo KS0+Y2xpZW50KCktPmZvcm1EaWRCbHVyKHRoaXMpOworICAgIGlmIChkb2N1bWVudCgpLT5wYWdl KCkpCisgICAgICAgIGRvY3VtZW50KCktPnBhZ2UoKS0+Y2hyb21lKCktPmNsaWVudCgpLT5mb3Jt RGlkQmx1cih0aGlzKTsKIAogICAgIEhUTUxFbGVtZW50OjpkaXNwYXRjaEJsdXJFdmVudCgpOwog fQo=