Bug 9976 - Fix negative width issue in Hixie's test
Summary: Fix negative width issue in Hixie's test
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 420+
Hardware: Mac OS X 10.4
: P2 Normal
Assignee: Rob Buis
URL: http://hixie.ch/tests/adhoc/svg/error...
Keywords:
Depends on:
Blocks: 6018
  Show dependency treegraph
 
Reported: 2006-07-17 13:07 PDT by Rob Buis
Modified: 2007-06-26 01:56 PDT (History)
1 user (show)

See Also:


Attachments
First attempt (4.17 KB, patch)
2006-07-19 00:29 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Now with testcase (10.85 KB, patch)
2006-07-31 01:11 PDT, Rob Buis
eric: review-
Details | Formatted Diff | Diff
Another attempt (43.38 KB, patch)
2006-09-14 05:48 PDT, Rob Buis
eric: review-
Details | Formatted Diff | Diff
Retrying this one. (28.07 KB, patch)
2007-06-14 23:43 PDT, Rob Buis
mjs: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Rob Buis 2006-07-17 13:07:50 PDT
Fix negative width issues in test error/013.xml of Hixie's svg
tests, see bug 6018.
Comment 1 Rob Buis 2006-07-19 00:29:46 PDT
Created attachment 9563 [details]
First attempt

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

Rob.
Comment 2 Rob Buis 2006-07-31 01:11:32 PDT
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.
Comment 3 Ian 'Hixie' Hickson 2006-07-31 01:27:35 PDT
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.)
Comment 4 Rob Buis 2006-07-31 02:40:48 PDT
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.
Comment 5 Maciej Stachowiak 2006-08-02 23:57:05 PDT
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.
Comment 6 Maciej Stachowiak 2006-08-02 23:58:23 PDT
Comment on attachment 9770 [details]
Now with testcase

r=me
Comment 7 Eric Seidel (no email) 2006-08-12 03:37:53 PDT
Rob, are you going to land this?
Comment 8 Rob Buis 2006-08-12 04:45:12 PDT
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.
Comment 9 Rob Buis 2006-08-12 14:57:40 PDT
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.
Comment 10 Eric Seidel (no email) 2006-08-14 23:33:04 PDT
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.
Comment 11 Eric Seidel (no email) 2006-09-12 23:22:52 PDT
/me pokes rob to see if he wants to take another go at fixing this patch.
Comment 12 Rob Buis 2006-09-13 07:29:17 PDT
(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.
Comment 13 Rob Buis 2006-09-14 05:48:51 PDT
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.
Comment 14 Eric Seidel (no email) 2006-09-14 15:27:47 PDT
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.
Comment 15 Eric Seidel (no email) 2006-09-18 21:15:51 PDT
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.
Comment 16 Rob Buis 2006-09-19 00:29:54 PDT
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. 
Comment 17 Rob Buis 2006-09-19 00:34:34 PDT
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. 
Comment 18 Rob Buis 2006-12-10 12:39:31 PST
This should probably be (finally) done after SVGLength redesign is done.
Cheers,

Rob.
Comment 19 Rob Buis 2007-06-14 23:43:55 PDT
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.
Comment 20 Maciej Stachowiak 2007-06-25 20:27:05 PDT
Comment on attachment 15043 [details]
Retrying this one.

r=me
Comment 21 Rob Buis 2007-06-26 01:56:06 PDT
Landed in 23789.