Summary: | HitTestResult::boundingBox can ASSERT in SVGRenderBase::mapLocalToContainer | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Holger Freyther <zecke> | ||||
Component: | WebCore Misc. | Assignee: | Holger Freyther <zecke> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | eric, simon.fraser | ||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | PC | ||||||
OS: | OS X 10.5 | ||||||
Attachments: |
|
Description
Holger Freyther
2009-07-16 05:16:38 PDT
Created attachment 32862 [details] 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(-) Simon is really your best bet for this patch. Comment on attachment 32862 [details]
Make using HitTestResult on SVG not run into assertions
Could you also add a LayoutTest for this?
Assigning to zecke so no over-eager committers grab it before he can add his test and land it. Comment on attachment 32862 [details]
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.
(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? 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. :) Mostly this bug hadn't been touched in a month, and I was cleaning out the queue. :) (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? (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. :} |