Bug 27347 - HitTestResult::boundingBox can ASSERT in SVGRenderBase::mapLocalToContainer
Summary: HitTestResult::boundingBox can ASSERT in SVGRenderBase::mapLocalToContainer
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Holger Freyther
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-07-16 05:16 PDT by Holger Freyther
Modified: 2009-10-28 04:47 PDT (History)
2 users (show)

See Also:


Attachments
Make using HitTestResult on SVG not run into assertions (5.31 KB, patch)
2009-07-16 05:28 PDT, Holger Freyther
eric: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Holger Freyther 2009-07-16 05:16:38 PDT
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.
Comment 1 Holger Freyther 2009-07-16 05:28:16 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(-)
Comment 2 Eric Seidel (no email) 2009-07-23 23:24:58 PDT
Simon is really your best bet for this patch.
Comment 3 Simon Fraser (smfr) 2009-07-24 09:17:19 PDT
Comment on attachment 32862 [details]
Make using HitTestResult on SVG not run into assertions

Could you also add a LayoutTest for this?
Comment 4 Eric Seidel (no email) 2009-07-24 10:44:07 PDT
Assigning to zecke so no over-eager committers grab it before he can add his test and land it.
Comment 5 Eric Seidel (no email) 2009-08-21 14:52:16 PDT
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.
Comment 6 Holger Freyther 2009-08-21 17:30:42 PDT
(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?
Comment 7 Eric Seidel (no email) 2009-08-21 17:40:30 PDT
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. :)
Comment 8 Eric Seidel (no email) 2009-08-21 17:41:01 PDT
Mostly this bug hadn't been touched in a month, and I was cleaning out the queue. :)
Comment 9 Holger Freyther 2009-08-21 17:44:21 PDT
(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?
Comment 10 Holger Freyther 2009-08-21 17:45:06 PDT
(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. :}
Comment 11 Holger Freyther 2009-10-28 04:47:22 PDT
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.