Bug 9976 – Fix negative width issue in Hixie's test

• Component: SVG
• Status: RESOLVED FIXED

[Rob Buis]

Fix negative width issues in test error/013.xml of Hixie's svg
tests, see bug 6018.

[Rob Buis]

Created attachment 9563 [details]
First attempt

As the ChangeLog says, handle negative widths/heights properly.
Cheers,

Rob.

[Rob Buis]

Created attachment 9770 [details]
Now with testcase

I didnt realize the testcase was needed for review! But OTOH I could have known :) So
in future I'll try to include it.
Cheers,

Rob.

[Ian 'Hixie' Hickson]

Note: As far as I can tell from reading the patch, this fix doesn't actually completely fix the original test case. (In particular, the last part of the pass condition isn't addressed.)

[Rob Buis]

Hi,

(In reply to comment #3)
> Note: As far as I can tell from reading the patch, this fix doesn't actually
> completely fix the original test case. (In particular, the last part of the
> pass condition isn't addressed.)

There is no code for the svg error reporting at all, besides the fprintf error
reporting which is mostly useful only to developers. Do you have suggestions
on how to signal errors visually? I heard w3c recommends a checkerboard pattern,
but I could be wrong...
Cheers,

Rob.

[Maciej Stachowiak]

Despite this not being a complete fix for all issues in the test, I think it is still worth landing. Perhaps with a FIXME in the appropriate place.

[Maciej Stachowiak]

Comment on attachment 9770 [details]
Now with testcase

r=me

[Eric Seidel (no email)]

Rob, are you going to land this?

[Rob Buis]

Hi Eric,

(In reply to comment #7)
> Rob, are you going to land this?
> 

Yes, I am in the process of making my mbp
a triple-boot machine, but when I find some time
I'll land it.
Cheers,

Rob.

[Rob Buis]

Hi Eric,

(In reply to comment #7)
> Rob, are you going to land this?
> 

Update, there is actually a different rendering for missing-xlink.svg that shows a
bug, the error handling code creates a xhtml doc, the svg element should calculate
now against the parent (body), not the viewport anymore. Because we have no code for
this it defaults to w/h zero, and the patch doesnt render in this case. I'll see if I can come
up with code to calculate against parent, but until that time I think it is impossible to land this patch.
Cheers,

Rob.

[Eric Seidel (no email)]

Comment on attachment 9770 [details]
Now with testcase

Since rob has since found issues with his own patch, I'm removing the r+ for now.

[Eric Seidel (no email)]

/me pokes rob to see if he wants to take another go at fixing this patch.

[Rob Buis]

(In reply to comment #11)
> /me pokes rob to see if he wants to take another go at fixing this patch.

Looking at it tonight.
Cheers,

Rob.

[Rob Buis]

Created attachment 10555 [details]
Another attempt

This patch now should also fix calculating width/height against non-svg viewports.
Also it seems to me viewportElement member var is not needed anymore, so I removed it.
Let me know what you think about this, and how we can improve the Lengths further.
Cheers,

Rob.

[Eric Seidel (no email)]

Comment on attachment 10555 [details]
Another attempt

Not safe:
+    if (viewportElement->isSVGElement()) {
         const SVGSVGElement* svg = static_cast<const SVGSVGElement*>(viewportElement);

Having SVGLength objects have a context is still the wrong model.  They shouldn't need to be "smart tearoffs", they should just be containers of length data, other classes should have the smarts.

[Eric Seidel (no email)]

Comment on attachment 10555 [details]
Another attempt

I'm not sure why I didn't r- this before.  The unsafe cast would prevent this from landing.

However, I think it's also time to finally fix the SVGLength issue, and maybe that part of this patch should be broken off into a separate patch.  To fix SVGLength would require all uses of x()->doSomethingToTheLength() to be removed.  and setX(new SVGLength()) to be used instead.  Better yet would be to stop using Length* and instead store the values on the objects themselves.

Let's talk over IRC, maybe we can find a smaller piece to chop off for this bug.

[Rob Buis]

Hi Eric,

(In reply to comment #31)
> (From update of attachment 10574 [details] [edit])
> I'm still not a big fan of needing a separate class "CursorList" to hold all
> the nodes, when the individual CursorData nodes could just form a singly linked
> list and know how to delete their "next" pointer when they are themselves
> deleted.
> 
> This would also remove the need for RenderStyle::addCursor...

I think I agree with ap, seems a bit silly to redo linked lists multiple times. (See earlier entries)

> +using namespace SVGNames;
> 
> needs to be inside a SVG_SUPPORT block in FrameView.cpp

Fixed locally.
 
> Is there any need to check the type here?
> +            value = valueList->next(); // comma

Will check.
 
> We'll need some more tests.

For testing css3 cursor syntax?
 
> It's not clear to me how the CursorList is ever destroyed.

It is referenced by a RefPtr, so it should destroy itself whenever RenderStyle
gets deleted(but taking into account refcounting ofcourse).
 
> It's also not clear to me that two divs, one inside the other, both which use
> independent cursor fallback will properly function (i.e. when is the cursors()
> list cleared?)

Can you give an example? I tried but could not get it right.

> I don't think we can land this without at least answering the leak question.

Sure, np. I did some --leaks runs, didnt see a problem, but will do some bigger
tests now.
Cheers,

Rob. 

[Rob Buis]

Hi Eric,

(In reply to comment #31)
> (From update of attachment 10574 [details] [edit])
> I'm still not a big fan of needing a separate class "CursorList" to hold all
> the nodes, when the individual CursorData nodes could just form a singly linked
> list and know how to delete their "next" pointer when they are themselves
> deleted.
> 
> This would also remove the need for RenderStyle::addCursor...

I think I agree with ap, seems a bit silly to redo linked lists multiple times. (See earlier entries)

> +using namespace SVGNames;
> 
> needs to be inside a SVG_SUPPORT block in FrameView.cpp

Fixed locally.
 
> Is there any need to check the type here?
> +            value = valueList->next(); // comma

Will check.
 
> We'll need some more tests.

For testing css3 cursor syntax?
 
> It's not clear to me how the CursorList is ever destroyed.

It is referenced by a RefPtr, so it should destroy itself whenever RenderStyle
gets deleted(but taking into account refcounting ofcourse).
 
> It's also not clear to me that two divs, one inside the other, both which use
> independent cursor fallback will properly function (i.e. when is the cursors()
> list cleared?)

Can you give an example? I tried but could not get it right.

> I don't think we can land this without at least answering the leak question.

Sure, np. I did some --leaks runs, didnt see a problem, but will do some bigger
tests now.
Cheers,

Rob. 

[Rob Buis]

This should probably be (finally) done after SVGLength redesign is done.
Cheers,

Rob.

[Rob Buis]

Created attachment 15043 [details]
Retrying this one.

I think because of a lot of other fixes have gone in this one should work now.
Cheers,

Rob.

[Maciej Stachowiak]

Comment on attachment 15043 [details]
Retrying this one.

r=me

[Rob Buis]

Landed in 23789.