334950 - bogus assertion is killing me: "XXX. We shouldn't get here. Viewbox width/height is set to 0. Need to disable display of element as per specs."

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jonathan Watt [:jwatt]]

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: testcase

no viewBox on marker -> loads of assertions

Comment 2

[Justin Wood (:Callek)]

I do not have a debug build to test with at the moment, but if the testcase fires the assertion, something is wrong (no viewbox specified).

Though the spec does state what the code does: http://www.w3.org/TR/SVG/coords.html#ViewBoxAttribute

Comment 3

[Robert Longson [:longsonr]]

Attachment: patch

The change to nsSVGMarkerElement.cpp fixes most of the asserts and also fixes the display of markers.

A single assert is left which occurs once when refreshing the page, this is fixed by the change to nsSVGMarkerFrame.cpp. In this case getCanvasTM not called from inside a PaintMark or RegionMark call and you get the assert because only in these functions is SetParentCoordCtxProvider set to a non nsnull value, allowing the viewbox to be correctly determined. I don't really understand why GetCanvasTM is called at this point. I'm hoping this is the correct way to sort it out but it may not be.

Comment 4

[Jonathan Watt [:jwatt]]

Robert, I've only managed to glance over this. I have very little time right now, so it would help if you could comment on your justification for removing mInUse2, and if you've considered whether nsSVGMarkerElement should inherit nsSVGCoordCtxProvider (that may make no sense whatsoever)?

Comment 5

[Robert Longson [:longsonr]]

The methods PaintMark and RegionMark both call SetParentCoordCtxProvider. Thus outside these functions ctx is not set as both of the functions set this to nsnull on exit. I don't know why they set it to nsnull and perhaps the correct fix would be to remove those lines.

Given the lifetime of ctx GetCanvasTM only works properly when called from within these methods as it needs the ctx set in order to call GetViewboxToViewportTransform. Thus I changed the code so that we only have one inuse set when PaintMark or RegionMark is called and I use that to check that GetCanvasTM is called from within these methods.

I don't know where nsMarkerElement could inherit nsSVGCoordCtxProvider from as the only other implementation is in nsSVGSVGElement and these two classes are only related by their common base class nsSVGElement.

Comment 6

[Robert Longson [:longsonr]]

Comment on attachment 220639 [details] [diff] [review]
patch

Perhaps its best if we just ask tor to look at this directly.

Comment 7

[tor]

The reason that I null out the context is that it is only valid for that particular use of the marker.  If we're getting called outside of {Paint,Region}Mark that needs to be investigated.

Comment 8

[Robert Longson [:longsonr]]

Attachment: updated patch

Looks like the layout change is no longer necessary. Some other fix must have sorted it out. This is just the content change which I'm much happier about. 

The testcase displays correctly with this.

Comment 9

[tor]

Likely the bug 344904 checkin took care of the layout bit.

Comment 10

[Robert Longson [:longsonr]]

Checked in.