Bug 42403 – [chromium] r63450 caused some svg mask tests to fail pixel tests

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Ojan Vafai]

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

[Alex Nicolaou]

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

[Ojan Vafai]

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.

[Alex Nicolaou]

Created attachment 62705 [details]
Patch

[Ojan Vafai]

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.

[Alex Nicolaou]

Created attachment 62754 [details]
Patch

[Alex Nicolaou]

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.

[Ojan Vafai]

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.

[Ojan Vafai]

(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?

[Alex Nicolaou]

Created attachment 62770 [details]
Patch

[Alex Nicolaou]

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.

[Ojan Vafai]

(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.

[Alex Nicolaou]

Created attachment 62781 [details]
Patch

[Alex Nicolaou]

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?

[Alex Nicolaou]

Comment on attachment 62770 [details]
Patch

If you can re-r+/c+ this one, that'd be great, else I can create separate bugs.

[Alex Nicolaou]

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.

[Alex Nicolaou]

(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?

[Ojan Vafai]

(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.

[Alex Nicolaou]

Now that all the sub-bugs on this bug are resolved, this one can be too but I don't appear to have permission.