Bug 42403

Summary: [chromium] r63450 caused some svg mask tests to fail pixel tests
Product: WebKit Reporter: Ojan Vafai <ojan>
Component: SVGAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: agl, anicolao, evan, krit, zimmermann
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Bug Depends on: 43100, 43102, 43104, 43106, 43107    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Description Ojan Vafai 2010-07-15 13:58:07 PDT
I don't know SVG well enough to evaluate whether these changes are correct and it's hard to tell from the original patch since it only added a new test.

LINUX WIN : svg/W3C-SVG-1.1/masking-intro-01-f.svg = IMAGE
LINUX WIN : svg/css/circle-in-mask-with-shadow.svg = IMAGE
LINUX WIN : svg/custom/grayscale-gradient-mask.svg = IMAGE
MAC WIN : svg/custom/mask-colorspace.svg = IMAGE
Comment 1 Alex Nicolaou 2010-07-15 14:12:21 PDT
These probably need rebaselining, I will investigate both what the right fix is for this and _why_ I didn't catch this when writing the original patch. Sorry
Comment 2 Ojan Vafai 2010-07-16 10:41:32 PDT
It looks like the expected png/checksum for svg/custom/mask-colorspace.svg checked in with r63450 matched chromium-linux, not webkit mac. The correct webkit mac checksum was committed at http://trac.webkit.org/changeset/63534/ and it now fails on all chromium platforms.
Comment 3 Alex Nicolaou 2010-07-27 10:00:13 PDT
Created attachment 62705 [details]
Patch
Comment 4 Ojan Vafai 2010-07-27 11:23:41 PDT
Comment on attachment 62705 [details]
Patch

> +++ b/LayoutTests/platform/chromium-linux/svg/custom/mask-colorspace-expected.txt
This test is only failing the pixel results. You shouldn't need to add a linux-specific text result.

> diff --git a/LayoutTests/platform/chromium/test_expectations.txt b/LayoutTests/platform/chromium/test_expectations.txt
> +BUGWK42403 WIN WIN-XP WIN-VISTA MAC : svg/custom/mask-colorspace.svg = IMAGE

I know this is probably the rebaseline tools doing, but you only need to include "WIN MAC" XP and VISTA aren't needed if you have WIN.
Comment 5 Alex Nicolaou 2010-07-27 15:20:56 PDT
Created attachment 62754 [details]
Patch
Comment 6 Alex Nicolaou 2010-07-27 15:27:58 PDT
Um ... no actually the extra platforms were my fault, fixed now.

Also dropped the .txt file.

On Mac, this test is failing due to a difference in how CG converts from sRGB to linear colour space for the mask. The colours are close enough, I was going to submit a baseline that's technically incorrect to alert us to other changes, along with a patch to make sure colour conversion happens on chromium mac, which it currently does not. Does that sound OK?

On Windows, it's broken because the colour conversion isn't being done for windows at all. I was going to leave it failing, the colours are way off.

I should have the mac patch up tonight if I don't get distracted.
Comment 7 Ojan Vafai 2010-07-27 15:53:44 PDT
Comment on attachment 62754 [details]
Patch

> +++ b/LayoutTests/ChangeLog
> +        * platform/chromium-linux/svg/custom/mask-colorspace-expected.txt: Copied from LayoutTests/platform/mac/svg/custom/mask-colorspace-expected.txt.

ChangeLog needs this line removed.
Comment 8 Ojan Vafai 2010-07-27 15:56:11 PDT
(In reply to comment #6)
> On Mac, this test is failing due to a difference in how CG converts from sRGB to linear colour space for the mask. The colours are close enough, I was going to submit a baseline that's technically incorrect to alert us to other changes, along with a patch to make sure colour conversion happens on chromium mac, which it currently does not. Does that sound OK?

That sounds fine as long as there is a bug on file to fix the underlying bug.

> On Windows, it's broken because the colour conversion isn't being done for windows at all. I was going to leave it failing, the colours are way off.

Makes sense, except the test was passing before. Was the old result also incorrect?
Comment 9 Alex Nicolaou 2010-07-27 16:43:27 PDT
Created attachment 62770 [details]
Patch
Comment 10 Alex Nicolaou 2010-07-27 16:46:47 PDT
New patch has the ChangeLog fixed - if you can r+ c+ that'd be great.

The checked in expected outputs for this test seem to be incorrect, I think someone tried to fix them up for me without really looking at the output. The two squares/circles are supposed to be identical.
Comment 11 Ojan Vafai 2010-07-27 16:51:48 PDT
(In reply to comment #10)
> New patch has the ChangeLog fixed - if you can r+ c+ that'd be great.

For future reference, if you want someone to set cq+, you just need to set cq? on the patch. That communicates to the reviewer that you want it put in the commit queue once it's approved.

> The checked in expected outputs for this test seem to be incorrect, I think someone tried to fix them up for me without really looking at the output. The two squares/circles are supposed to be identical.

Can you file a new bug, update the BUG*** entry in test_expectations and put this explanation along with the revision it started failing at again (i.e., r63450)? That way, when someone tries to fix this in the future, they'll have all the context from this bug.
Comment 12 Alex Nicolaou 2010-07-27 18:19:57 PDT
Created attachment 62781 [details]
Patch
Comment 13 Alex Nicolaou 2010-07-27 18:21:15 PDT
Duh ... my mac patch obsoleted my linux patch. Is it best to break this out into a ton of little bugs and use this bug as a superbug that is blocked by each test/platform, or is it best to put all the patches here?
Comment 14 Alex Nicolaou 2010-07-27 18:22:44 PDT
Comment on attachment 62770 [details]
Patch

If you can re-r+/c+ this one, that'd be great, else I can create separate bugs.
Comment 15 Alex Nicolaou 2010-07-27 19:11:44 PDT
I've just read "Only do one patch per bug. Multiple iterations of the same patch is fine but use a new bug for a new patch. Otherwise, it gets confusing to read through all of the comments and understand which ones apply to each patch." from http://trac.webkit.org/wiki/CodeReview while trying to figure out what the right answer to my question about multiple bugs is. I will use this bug as the master bug and move my patches into bugs that block this bug.
Comment 16 Alex Nicolaou 2010-07-28 07:00:54 PDT
(In reply to comment #8)
> > On Windows, it's broken because the colour conversion isn't being done for windows at all. I was going to leave it failing, the colours are way off.
> 
> Makes sense, except the test was passing before. Was the old result also incorrect?

Um, stupid question: where do I go to easily see the windows pass you're talking about?
Comment 17 Ojan Vafai 2010-07-28 16:19:51 PDT
(In reply to comment #16)
> (In reply to comment #8)
> > > On Windows, it's broken because the colour conversion isn't being done for windows at all. I was going to leave it failing, the colours are way off.
> > 
> > Makes sense, except the test was passing before. Was the old result also incorrect?
> 
> Um, stupid question: where do I go to easily see the windows pass you're talking about?

Not a stupid question, I'm just wrong. It never passed.
Comment 18 Alex Nicolaou 2010-08-08 10:58:55 PDT
Now that all the sub-bugs on this bug are resolved, this one can be too but I don't appear to have permission.