Bug 15343 - Many SVG tests "fail" due to changed text color dumping (after feature brach rebase)
Summary: Many SVG tests "fail" due to changed text color dumping (after feature brach ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 523.x (Safari 3)
Hardware: Macintosh OS X 10.4
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2007-10-02 08:33 PDT by Eric Seidel (no email)
Modified: 2007-10-06 17:25 PDT (History)
2 users (show)

See Also:


Attachments
attempted (sorta) fix (2.49 KB, patch)
2007-10-02 08:33 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2007-10-02 08:33:05 PDT
Many SVG tests "fail" due to changed text color dumping (after feature brach rebase)

I believe the previous dumping method was not really determinant.  Or at least it wasn't dumping what it was trying to dump.  First of all, color() is not really the normal way to change text color in SVG.  Second, it seems that sometimes that was the fill color, and sometimes the stroke.  I believe color() was previously being modified by the painting code, but is no longer.  I'm not sure why that would change with the rebase (perhaps all the tests got updated?)

I have attached one attempted fix, but it doesn't really make the situation any better or worse.  It also seems to still miss some colors that it should be dumping (I'm unsure why).
Comment 1 Eric Seidel (no email) 2007-10-02 08:33:32 PDT
Created attachment 16504 [details]
attempted (sorta) fix
Comment 2 Nikolas Zimmermann 2007-10-02 10:07:12 PDT
Yes, I updated all tests to not dump text colors at all in feature-branch. It's definately better than dumping invalid colors (as previously, as you said, the paint servers mutated color() when painting text - I changed that).

I haven't checked the attached patch for correctness, as I'm really busy learning atm. Next week I can comment :-)

Greetings,
Niko
Comment 3 Oliver Hunt 2007-10-02 11:24:46 PDT
How many failures are left are this change?
Comment 4 Eric Seidel (no email) 2007-10-03 09:24:46 PDT
I suggest we just remove the "color()" call, or apply my patch, and rebase.  My patch should probably be changed to label the colors its dumping.  Such as "fillColor=" instead of "color="

How many tests still failed after this?  Lots.  A different subset of the SVG tests.
Comment 5 Eric Seidel (no email) 2007-10-03 09:33:19 PDT
I should note, that if we're going to apply this patch, the "else if"s should just turn into ifs.  And the color dumps should possibly be re-ordered.  I'm not sure that dumping solid colors for text is very useful though (especially since this patch doesn't seem to catch all cases anyway.
Comment 6 Nikolas Zimmermann 2007-10-06 17:25:07 PDT
Fixed in fb svn.