27347 2009-07-16 05:16:38 -0700 HitTestResult::boundingBox can ASSERT in SVGRenderBase::mapLocalToContainer 2009-10-28 04:47:22 -0700 1 1 1 Unclassified WebKit WebCore Misc. 528+ (Nightly build) PC OS X 10.5 RESOLVED FIXED P2 Normal --- 1 zecke zecke eric simon.fraser oldest_to_newest 132243 0 zecke 2009-07-16 05:16:38 -0700 The implementation of HitTestResult::boundingBox is not using transforms. This means when the innerNonSharedNode is an SVG Element we will run into an assert on debug builds. The same problems seems to exist for HitTestResult::imageRect. 132245 1 32862 zecke 2009-07-16 05:28:16 -0700 Created attachment 32862 Make using HitTestResult on SVG not run into assertions WebCore 2009-07-03 Holger Hans Peter Freyther <zecke@selfish.org> Reviewed by NOBODY (OOPS!). Change HitTest to use transform for SVGRenderBase Kill HitTestResult::boundingBox as it doesn't belong into a HitTest as it is operating on points. Change HitTestResult::imageRect to use RenderBox::absoluteContentQuad to useTransform. This is required to not run into an assert with SVG Images in SVGRenderBase. * rendering/HitTestResult.cpp: (WebCore::HitTestResult::imageRect): * rendering/HitTestResult.h: WebKit/win 2009-07-03 Holger Hans Peter Freyther <zecke@selfish.org> Reviewed by NOBODY (OOPS!). HitTestResult::boundingBox is gone, use the RenderObject directly and be transformation aware to not run into asserts in SVGRenderBase. * WebElementPropertyBag.cpp: (WebElementPropertyBag::Read): WebKit/qt 2009-07-03 Holger Hans Peter Freyther <zecke@selfish.org> Reviewed by NOBODY (OOPS!). HitTestResult::boundingBox is gone, use the RenderObject directly and be transformation aware to not run into asserts in SVGRenderBase. * Api/qwebframe.cpp: (QWebHitTestResultPrivate::QWebHitTestResultPrivate): --- 7 files changed, 44 insertions(+), 15 deletions(-) 134388 2 eric 2009-07-23 23:24:58 -0700 Simon is really your best bet for this patch. 134582 3 32862 simon.fraser 2009-07-24 09:17:19 -0700 Comment on attachment 32862 Make using HitTestResult on SVG not run into assertions Could you also add a LayoutTest for this? 134596 4 eric 2009-07-24 10:44:07 -0700 Assigning to zecke so no over-eager committers grab it before he can add his test and land it. 141991 5 32862 eric 2009-08-21 14:52:16 -0700 Comment on attachment 32862 Make using HitTestResult on SVG not run into assertions We commonly r- other patches for lack of tests, so marking this one r- too. No need to sit in the list of patches to be committed w/o actually being committable. 142069 6 zecke 2009-08-21 17:30:42 -0700 (In reply to comment #5) > (From update of attachment 32862 [details]) > We commonly r- other patches for lack of tests, so marking this one r- too. No > need to sit in the list of patches to be committed w/o actually being > committable. This would mean that Apple is not allowed to add any API to WebKit Win and Mac due the lack of unit testing of their API. Are you trying to say that? The thing is I can and should test the Qt API, but I will not be able to introduce unit testing for Mac and Win. How to proceed? 142074 7 eric 2009-08-21 17:40:30 -0700 Is this not testable from a web page? It looks like imageRect() is only used from the dragging code. So the answer is probably "not easily". One could fake a drag, but it would only validate that the fake drag does not assert. You're welcome to land it with the previous review, but please update the changelog to explain why it's not testable. Thanks. :) 142076 8 eric 2009-08-21 17:41:01 -0700 Mostly this bug hadn't been touched in a month, and I was cleaning out the queue. :) 142077 9 zecke 2009-08-21 17:44:21 -0700 (In reply to comment #7) > Is this not testable from a web page? > > It looks like imageRect() is only used from the dragging code. So the answer > is probably "not easily". One could fake a drag, but it would only validate > that the fake drag does not assert. > > You're welcome to land it with the previous review, but please update the > changelog to explain why it's not testable. Thanks. :) Let me summarize again and illustrate why unit testing will not bring anything to the table. At least the Qt folks do API unit testing with release builds, Apple according to my understanding is not doing API unit testing at all. The HitTestResult methods I deleted/changed are soleley used from the WebKit layer, using these methods on SVG will result in an ASSERT, it will not crash and the results will be right for almost every case. There is no way I can call the HitTestResult methods from JavaScript as they are not bound to anything. I'm not able to come up with an idea how to test in a release build that the assert was not hit. The ports with 3d transformation don't have API unit testing, the ones without 3d transformation have API unit testing but I will not be able to have content that would give a different result due 3d transformation. Would this be a good start to say why it is not testable? 142078 10 zecke 2009-08-21 17:45:06 -0700 (In reply to comment #8) > Mostly this bug hadn't been touched in a month, and I was cleaning out the > queue. :) Yeah, I know... I plan to boot to OSX to run-webkit-tests there... I just didn't have the time to do that. :} 158516 11 zecke 2009-10-28 04:47:22 -0700 Landed in r50207 and windows buildfix in r50210. The ChangeLog was changed to refer to the assert and state why a test case in WebKit is not possible as it would require a unit test on the mac. 32862 2009-07-16 05:28:16 -0700 2009-08-21 14:52:16 -0700 Make using HitTestResult on SVG not run into assertions bug-27347-20090716142741.patch text/plain 5433 zecke ZGlmZiAtLWdpdCBhL1dlYkNvcmUvQ2hhbmdlTG9nIGIvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXgg NjQ1OWM4OS4uZjBiNjY5NyAxMDA2NDQKLS0tIGEvV2ViQ29yZS9DaGFuZ2VMb2cKKysrIGIvV2Vi Q29yZS9DaGFuZ2VMb2cKQEAgLTEsMyArMSwyMCBAQAorMjAwOS0wNy0wMyAgSG9sZ2VyIEhhbnMg UGV0ZXIgRnJleXRoZXIgIDx6ZWNrZUBzZWxmaXNoLm9yZz4KKworICAgICAgICBSZXZpZXdlZCBi eSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBDaGFuZ2UgSGl0VGVzdCB0byB1c2UgdHJhbnNm b3JtIGZvciBTVkdSZW5kZXJCYXNlCisKKyAgICAgICAgS2lsbCBIaXRUZXN0UmVzdWx0Ojpib3Vu ZGluZ0JveCBhcyBpdCBkb2Vzbid0IGJlbG9uZyBpbnRvIGEKKyAgICAgICAgSGl0VGVzdCBhcyBp dCBpcyBvcGVyYXRpbmcgb24gcG9pbnRzLgorCisgICAgICAgIENoYW5nZSBIaXRUZXN0UmVzdWx0 OjppbWFnZVJlY3QgdG8gdXNlIFJlbmRlckJveDo6YWJzb2x1dGVDb250ZW50UXVhZAorICAgICAg ICB0byB1c2VUcmFuc2Zvcm0uIFRoaXMgaXMgcmVxdWlyZWQgdG8gbm90IHJ1biBpbnRvIGFuIGFz c2VydAorICAgICAgICB3aXRoIFNWRyBJbWFnZXMgaW4gU1ZHUmVuZGVyQmFzZS4KKworICAgICAg ICAqIHJlbmRlcmluZy9IaXRUZXN0UmVzdWx0LmNwcDoKKyAgICAgICAgKFdlYkNvcmU6OkhpdFRl c3RSZXN1bHQ6OmltYWdlUmVjdCk6CisgICAgICAgICogcmVuZGVyaW5nL0hpdFRlc3RSZXN1bHQu aDoKKwogMjAwOS0wNy0xNSAgRGF2aWQgTGV2aW4gIDxsZXZpbkBjaHJvbWl1bS5vcmc+CiAKICAg ICAgICAgTGF5b3V0IHRlc3QgZml4LCByZXZlcnRpbmcgcHJldmlvdXMgY2hhbmdlLgpkaWZmIC0t Z2l0IGEvV2ViQ29yZS9yZW5kZXJpbmcvSGl0VGVzdFJlc3VsdC5jcHAgYi9XZWJDb3JlL3JlbmRl cmluZy9IaXRUZXN0UmVzdWx0LmNwcAppbmRleCA1MmVjNjY0Li5iODExMzJjIDEwMDY0NAotLS0g YS9XZWJDb3JlL3JlbmRlcmluZy9IaXRUZXN0UmVzdWx0LmNwcAorKysgYi9XZWJDb3JlL3JlbmRl cmluZy9IaXRUZXN0UmVzdWx0LmNwcApAQCAtMTIzLDE3ICsxMjMsNiBAQCBGcmFtZSogSGl0VGVz dFJlc3VsdDo6dGFyZ2V0RnJhbWUoKSBjb25zdAogICAgIHJldHVybiBmcmFtZS0+dHJlZSgpLT5m aW5kKG1faW5uZXJVUkxFbGVtZW50LT50YXJnZXQoKSk7CiB9CiAKLUludFJlY3QgSGl0VGVzdFJl c3VsdDo6Ym91bmRpbmdCb3goKSBjb25zdAotewotICAgIGlmIChtX2lubmVyTm9uU2hhcmVkTm9k ZSkgewotICAgICAgICBSZW5kZXJPYmplY3QqIHJlbmRlcmVyID0gbV9pbm5lck5vblNoYXJlZE5v ZGUtPnJlbmRlcmVyKCk7Ci0gICAgICAgIGlmIChyZW5kZXJlcikKLSAgICAgICAgICAgIHJldHVy biByZW5kZXJlci0+YWJzb2x1dGVCb3VuZGluZ0JveFJlY3QoKTsKLSAgICB9Ci0gICAgCi0gICAg cmV0dXJuIEludFJlY3QoKTsKLX0KLQogYm9vbCBIaXRUZXN0UmVzdWx0Ojppc1NlbGVjdGVkKCkg Y29uc3QKIHsKICAgICBpZiAoIW1faW5uZXJOb25TaGFyZWROb2RlKQpAQCAtMjM5LDcgKzIyOCw3 IEBAIEludFJlY3QgSGl0VGVzdFJlc3VsdDo6aW1hZ2VSZWN0KCkgY29uc3QKIHsKICAgICBpZiAo IWltYWdlKCkpCiAgICAgICAgIHJldHVybiBJbnRSZWN0KCk7Ci0gICAgcmV0dXJuIG1faW5uZXJO b25TaGFyZWROb2RlLT5yZW5kZXJCb3goKS0+YWJzb2x1dGVDb250ZW50Qm94KCk7CisgICAgcmV0 dXJuIG1faW5uZXJOb25TaGFyZWROb2RlLT5yZW5kZXJCb3goKS0+YWJzb2x1dGVDb250ZW50UXVh ZCgpLmVuY2xvc2luZ0JvdW5kaW5nQm94KCk7CiB9CiAKIEtVUkwgSGl0VGVzdFJlc3VsdDo6YWJz b2x1dGVJbWFnZVVSTCgpIGNvbnN0CmRpZmYgLS1naXQgYS9XZWJDb3JlL3JlbmRlcmluZy9IaXRU ZXN0UmVzdWx0LmggYi9XZWJDb3JlL3JlbmRlcmluZy9IaXRUZXN0UmVzdWx0LmgKaW5kZXggZGJm NDcyZC4uNjExYzYzYSAxMDA2NDQKLS0tIGEvV2ViQ29yZS9yZW5kZXJpbmcvSGl0VGVzdFJlc3Vs dC5oCisrKyBiL1dlYkNvcmUvcmVuZGVyaW5nL0hpdFRlc3RSZXN1bHQuaApAQCAtNjIsNyArNjIs NiBAQCBwdWJsaWM6CiAgICAgdm9pZCBzZXRJc092ZXJXaWRnZXQoYm9vbCBiKSB7IG1faXNPdmVy V2lkZ2V0ID0gYjsgfQogCiAgICAgRnJhbWUqIHRhcmdldEZyYW1lKCkgY29uc3Q7Ci0gICAgSW50 UmVjdCBib3VuZGluZ0JveCgpIGNvbnN0OwogICAgIGJvb2wgaXNTZWxlY3RlZCgpIGNvbnN0Owog ICAgIFN0cmluZyBzcGVsbGluZ1Rvb2xUaXAoKSBjb25zdDsKICAgICBTdHJpbmcgcmVwbGFjZWRT dHJpbmcoKSBjb25zdDsKZGlmZiAtLWdpdCBhL1dlYktpdC9xdC9BcGkvcXdlYmZyYW1lLmNwcCBi L1dlYktpdC9xdC9BcGkvcXdlYmZyYW1lLmNwcAppbmRleCAyOWQzODBkLi5mYzU5ZTk0IDEwMDY0 NAotLS0gYS9XZWJLaXQvcXQvQXBpL3F3ZWJmcmFtZS5jcHAKKysrIGIvV2ViS2l0L3F0L0FwaS9x d2ViZnJhbWUuY3BwCkBAIC0xMzM5LDcgKzEzMzksNiBAQCBRV2ViSGl0VGVzdFJlc3VsdFByaXZh dGU6OlFXZWJIaXRUZXN0UmVzdWx0UHJpdmF0ZShjb25zdCBXZWJDb3JlOjpIaXRUZXN0UmVzdWx0 CiAgICAgaWYgKCFoaXRUZXN0LmlubmVyTm9kZSgpKQogICAgICAgICByZXR1cm47CiAgICAgcG9z ID0gaGl0VGVzdC5wb2ludCgpOwotICAgIGJvdW5kaW5nUmVjdCA9IGhpdFRlc3QuYm91bmRpbmdC b3goKTsKICAgICB0aXRsZSA9IGhpdFRlc3QudGl0bGUoKTsKICAgICBsaW5rVGV4dCA9IGhpdFRl c3QudGV4dENvbnRlbnQoKTsKICAgICBsaW5rVXJsID0gaGl0VGVzdC5hYnNvbHV0ZUxpbmtVUkwo KTsKQEAgLTEzNDgsNiArMTM0Nyw3IEBAIFFXZWJIaXRUZXN0UmVzdWx0UHJpdmF0ZTo6UVdlYkhp dFRlc3RSZXN1bHRQcml2YXRlKGNvbnN0IFdlYkNvcmU6OkhpdFRlc3RSZXN1bHQKICAgICBpbWFn ZVVybCA9IGhpdFRlc3QuYWJzb2x1dGVJbWFnZVVSTCgpOwogICAgIGlubmVyTm9kZSA9IGhpdFRl c3QuaW5uZXJOb2RlKCk7CiAgICAgaW5uZXJOb25TaGFyZWROb2RlID0gaGl0VGVzdC5pbm5lck5v blNoYXJlZE5vZGUoKTsKKyAgICBib3VuZGluZ1JlY3QgPSBpbm5lck5vblNoYXJlZE5vZGUgPyBp bm5lck5vblNoYXJlZE5vZGUtPnJlbmRlcmVyKCktPmFic29sdXRlQm91bmRpbmdCb3hSZWN0KHRy dWUpIDogSW50UmVjdCgpOwogICAgIFdlYkNvcmU6OkltYWdlICppbWcgPSBoaXRUZXN0LmltYWdl KCk7CiAgICAgaWYgKGltZykgewogICAgICAgICBRUGl4bWFwICpwaXggPSBpbWctPm5hdGl2ZUlt YWdlRm9yQ3VycmVudEZyYW1lKCk7CmRpZmYgLS1naXQgYS9XZWJLaXQvcXQvQ2hhbmdlTG9nIGIv V2ViS2l0L3F0L0NoYW5nZUxvZwppbmRleCA4MDU0YzQ3Li44OWJiMmYyIDEwMDY0NAotLS0gYS9X ZWJLaXQvcXQvQ2hhbmdlTG9nCisrKyBiL1dlYktpdC9xdC9DaGFuZ2VMb2cKQEAgLTEsMyArMSwx NCBAQAorMjAwOS0wNy0wMyAgSG9sZ2VyIEhhbnMgUGV0ZXIgRnJleXRoZXIgIDx6ZWNrZUBzZWxm aXNoLm9yZz4KKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAg ICBIaXRUZXN0UmVzdWx0Ojpib3VuZGluZ0JveCBpcyBnb25lLCB1c2UgdGhlIFJlbmRlck9iamVj dAorICAgICAgICBkaXJlY3RseSBhbmQgYmUgdHJhbnNmb3JtYXRpb24gYXdhcmUgdG8gbm90IHJ1 biBpbnRvIGFzc2VydHMKKyAgICAgICAgaW4gU1ZHUmVuZGVyQmFzZS4KKworICAgICAgICAqIEFw aS9xd2ViZnJhbWUuY3BwOgorICAgICAgICAoUVdlYkhpdFRlc3RSZXN1bHRQcml2YXRlOjpRV2Vi SGl0VGVzdFJlc3VsdFByaXZhdGUpOgorCiAyMDA5LTA3LTE1ICBZYWVsIEFoYXJvbiAgPHlhZWwu YWhhcm9uQG5va2lhLmNvbT4KIAogICAgICAgICBSZXZpZXdlZCBieSBTaW1vbiBIYXVzbWFubi4K ZGlmZiAtLWdpdCBhL1dlYktpdC93aW4vQ2hhbmdlTG9nIGIvV2ViS2l0L3dpbi9DaGFuZ2VMb2cK aW5kZXggYWZmYmNjMC4uNzA0MDQwMCAxMDA2NDQKLS0tIGEvV2ViS2l0L3dpbi9DaGFuZ2VMb2cK KysrIGIvV2ViS2l0L3dpbi9DaGFuZ2VMb2cKQEAgLTEsMyArMSwxNCBAQAorMjAwOS0wNy0wMyAg SG9sZ2VyIEhhbnMgUGV0ZXIgRnJleXRoZXIgIDx6ZWNrZUBzZWxmaXNoLm9yZz4KKworICAgICAg ICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBIaXRUZXN0UmVzdWx0Ojpi b3VuZGluZ0JveCBpcyBnb25lLCB1c2UgdGhlIFJlbmRlck9iamVjdAorICAgICAgICBkaXJlY3Rs eSBhbmQgYmUgdHJhbnNmb3JtYXRpb24gYXdhcmUgdG8gbm90IHJ1biBpbnRvIGFzc2VydHMKKyAg ICAgICAgaW4gU1ZHUmVuZGVyQmFzZS4KKworICAgICAgICAqIFdlYkVsZW1lbnRQcm9wZXJ0eUJh Zy5jcHA6CisgICAgICAgIChXZWJFbGVtZW50UHJvcGVydHlCYWc6OlJlYWQpOgorCiAyMDA5LTA3 LTE1ICBEYXJpbiBBZGxlciAgPGRhcmluQGFwcGxlLmNvbT4KIAogICAgICAgICBSZXZpZXdlZCBi eSBTYW0gV2VpbmlnLgpkaWZmIC0tZ2l0IGEvV2ViS2l0L3dpbi9XZWJFbGVtZW50UHJvcGVydHlC YWcuY3BwIGIvV2ViS2l0L3dpbi9XZWJFbGVtZW50UHJvcGVydHlCYWcuY3BwCmluZGV4IDkxMTNm NGQuLmMyZmZlZTMgMTAwNjQ0Ci0tLSBhL1dlYktpdC93aW4vV2ViRWxlbWVudFByb3BlcnR5QmFn LmNwcAorKysgYi9XZWJLaXQvd2luL1dlYkVsZW1lbnRQcm9wZXJ0eUJhZy5jcHAKQEAgLTE0NSw3 ICsxNDUsOSBAQCBIUkVTVUxUIFNURE1FVEhPRENBTExUWVBFIFdlYkVsZW1lbnRQcm9wZXJ0eUJh Zzo6UmVhZChMUENPTEVTVFIgcHN6UHJvcE5hbWUsIFZBUgogICAgICAgICByZXR1cm4gU19PSzsK ICAgICB9IGVsc2UgaWYgKGlzRXF1YWwoV2ViRWxlbWVudEltYWdlUmVjdEtleSwga2V5KSkgewog ICAgICAgICBWX1ZUKHBWYXIpID0gVlRfQVJSQVk7Ci0gICAgICAgIFZfQVJSQVkocFZhcikgPSBN YXJzaGFsbGluZ0hlbHBlcnM6OmludFJlY3RUb1NhZmVBcnJheShtX3Jlc3VsdC0+Ym91bmRpbmdC b3goKSk7CisgICAgICAgIEludFJlY3QgYm91bmRpbmdCb3ggPSBtX3Jlc3VsdC0+aW5uZXJOb25T aGFyZWROb2RlKCkgJiYgbV9yZXN1bHQtPmlubmVyTm9uU2hhcmVkTm9kZSgpLT5yZW5kZXJlcigp ID8KKyAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgbV9yZXN1bHQtPmlubmVyTm9uU2hh cmVkTm9kZSgpLT5yZW5kZXJlcigpLT5hYnNvbHV0ZUJvdW5kaW5nQm94UmVjdCh0cnVlKSA6IElu dFJlY3QoKTsKKyAgICAgICAgVl9BUlJBWShwVmFyKSA9IE1hcnNoYWxsaW5nSGVscGVyczo6aW50 UmVjdFRvU2FmZUFycmF5KGJvdW5kaW5nQm94KTsKICAgICAgICAgcmV0dXJuIFNfT0s7CiAgICAg fSBlbHNlIGlmIChpc0VxdWFsKFdlYkVsZW1lbnRJbWFnZVVSTEtleSwga2V5KSkKICAgICAgICAg cmV0dXJuIGNvbnZlcnRTdHJpbmdUb1ZhcmlhbnQocFZhciwgbV9yZXN1bHQtPmFic29sdXRlSW1h Z2VVUkwoKS5zdHJpbmcoKSk7