Bug 31363

Created attachment 42982 [details] Null Checks

I wonder why this is failing. Brian, do you know if it ALWAYS fails? We need to find out kCGColorSpaceSRGB is supposed to be available on Windows. If it is not available and it always fails, that is a bummer because it means this feature won't work on Windows. If this feature really doesn't exist on Windows, then the best solution is probably to edit the #ifdef in sRGBColorSpace() and make it return deviceColorSpace for Tiger and Windows. Then we will not need the null checks, and we will still draw something when the property is specified.

Comment on attachment 42982 [details] Null Checks I don’t understand why this is appropriate. Is it really OK to just do the wrong thing and use the wrong color? Doesn't this break the feature on Windows and worse, make it use the wrong color in various places? > - CGContextSetStrokeColorSpace(context, sRGBColorSpaceRef()); > + if (CGColorSpaceRef sRGBColorSpace = sRGBColorSpaceRef()) > + CGContextSetFillColorSpace(context, sRGBColorSpace); You changed stroke to fill here. If Windows CG is getting 0 for some of these things we need to figure out why. Just doing the wrong thing to avoid a crash is not good. In the mean time, skipping the tests might be the best short term measure. review- because of the stroke to fill change and also my general disagreement that this is a fix.

Understood, this patch was to get feedback on the best way to handle it, which I now have plenty of. I like Beth's solution, and think that would be the right way to handle it for now.

kCGColorSpaceSRGB should be available according to the header. Looks like we need to file a CG bug.

Until then I will skip these tests on Windows.

Created attachment 42996 [details] Add tests to Skipped List Should we also add a temporary check in static CGColorSpaceRef sRGBColorSpaceRef() to return deviceRGBColorSpaceRef() if we are on Windows until the bug is fixed? If so, I can do that in another patch.

(In reply to comment #7) > Should we also add a temporary check in > > static CGColorSpaceRef sRGBColorSpaceRef() to return deviceRGBColorSpaceRef() > if we are on Windows until the bug is fixed? If so, I can do that in another > patch. You should talk to Beth and Adele about that.

Comment on attachment 42996 [details] Add tests to Skipped List Skipped list changes landed in r50841.

Created attachment 43006 [details] WebCore Workaround + Skipped List Removal

Comment on attachment 43006 [details] WebCore Workaround + Skipped List Removal > +#if PLATFORM(WIN) > + return deviceRGBColorSpaceRef(); > +#else > #ifdef BUILDING_ON_TIGER > return deviceRGBColorSpaceRef(); > #else > static CGColorSpaceRef sRGBSpace = CGColorSpaceCreateWithName(kCGColorSpaceSRGB); > return sRGBSpace; > #endif > +#endif > } I think you can re-write this as: #if PLATORM(WIN) || DEFINED(BUILDING_ON_TIGER) return deviceRGBColorSpaceRef(); #else static CGColorSpaceRef sRGBSpace = CGColorSpaceCreateWithName(kCGColorSpaceSRGB); return sRGBSpace; #endif which might be a little neater, but it's up to you.

defined, not DEFINED

Happy I re-compiled after making that fix. That is a lot cleaner, I am making that change and landing. Keeping the bug open though until this workaround is fixed. I'll add a FIXME as well referencing this bug.

Comment on attachment 43006 [details] WebCore Workaround + Skipped List Removal Landed in r50843. Keeping the bug open until we can use correct sCGColorSpaceSRGB without crashing.

(In reply to comment #14) > Keeping the bug open until we can use correct sCGColorSpaceSRGB without crashing. I'm not sure that's right. Lets make a new bug talking about what's really wrong.

(In reply to comment #15) > (In reply to comment #14) > > Keeping the bug open until we can use correct sCGColorSpaceSRGB without crashing. > > I'm not sure that's right. Lets make a new bug talking about what's really > wrong. Sure, that's fair. I'll close this, since the tests aren't crashing anymore.

<https://bugs.webkit.org/show_bug.cgi?id=31316> and <rdar://problem/7387791> Make -webkit-color-correction correct color on Windows To track re-enabling this feature. I also filed a CG bug linked to from the Radar.

Committed r54502: <http://trac.webkit.org/changeset/54502>

Hmm, sorry about that. Seems like "webkit-patch land" picked up the wrong bug ID; ignore the previous comment.

The NULL color space issue is now corrected.