Bug 175858 – _WKThumbnailView should use the screen color space instead of sRGB

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Tim Horton]

_WKThumbnailView should use the screen color space instead of sRGB

[Tim Horton]

<rdar://problem/33925559>

[Tim Horton]

Created attachment 318820 [details]
Patch

[Tim Horton]

Created attachment 318824 [details]
Patch

[Megan Gardner]

Comment on attachment 318824 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=318824&action=review

I don't really like switching supportsAlpha to isOpaque, but I see how it makes sense, as most things support alpha, thus allowing the default to more easily be default. I think that something like "noAlpha" might be more clear, though, as Alpha is the channel that is present or not.

Other than extraneous log, it looks good to me.

> Source/WebKit/Shared/cg/ShareableBitmapCG.cpp:77
> +

Leftover logging? Please remove if so.

[Megan Gardner]

Well, looks good to what I am familiar with, obviously will need a real reviewer, and to fix the SharableBitmap build breaker.

[Tim Horton]

(In reply to Megan Gardner from comment #4)
> Comment on attachment 318824 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=318824&action=review
> 
> I don't really like switching supportsAlpha to isOpaque, but I see how it
> makes sense, as most things support alpha, thus allowing the default to more
> easily be default. I think that something like "noAlpha" might be more
> clear, though, as Alpha is the channel that is present or not.

IsOpaque is the name used on e.g. CALayer/NSView/UIView (and our own RemoteLayerBackingStore, among others). Thoughts? :)

> Other than extraneous log, it looks good to me.

That and the fact that it doesn't build basically anywhere.

> > Source/WebKit/Shared/cg/ShareableBitmapCG.cpp:77
> > +
> 
> Leftover logging? Please remove if so.

Good call!

[Tim Horton]

Created attachment 318836 [details]
Patch

[Megan Gardner]

(In reply to Tim Horton from comment #6)
> (In reply to Megan Gardner from comment #4)
> > Comment on attachment 318824 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=318824&action=review
> > 
> > I don't really like switching supportsAlpha to isOpaque, but I see how it
> > makes sense, as most things support alpha, thus allowing the default to more
> > easily be default. I think that something like "noAlpha" might be more
> > clear, though, as Alpha is the channel that is present or not.
> 
> IsOpaque is the name used on e.g. CALayer/NSView/UIView (and our own
> RemoteLayerBackingStore, among others). Thoughts? :)

Ok looking at those flags, at least in CALayer and UIView, seems to be more about optimization of drawing, in that it is guarantees that all the pixels have relevant data rather than the presence or absence of an alpha channel, which I guess after looking at the code more closely is also sorta what ours is about. So I guess that makes sense. It's a shame that the SharableBitmap flag is SupportsAlpha though.

> 
> > Other than extraneous log, it looks good to me.
> 
> That and the fact that it doesn't build basically anywhere.

Yeah, also that

> 
> > > Source/WebKit/Shared/cg/ShareableBitmapCG.cpp:77
> > > +
> > 
> > Leftover logging? Please remove if so.
> 
> Good call!

[Tim Horton]

(In reply to Megan Gardner from comment #8)
> (In reply to Tim Horton from comment #6)
> > (In reply to Megan Gardner from comment #4)
> Ok looking at those flags, at least in CALayer and UIView, seems to be more
> about optimization of drawing, in that it is guarantees that all the pixels
> have relevant data rather than the presence or absence of an alpha channel,
> which I guess after looking at the code more closely is also sorta what ours
> is about. So I guess that makes sense. It's a shame that the SharableBitmap
> flag is SupportsAlpha though.

Hmm, what? The ShareableBitmap flag is the one I changed. Is there another that I should change? I don't see any more SupportsAlpha in the tree after my change

[Tim Horton]

Created attachment 318838 [details]
Patch

[Megan Gardner]

Sry, I was looking at the unchanged code to compare flags and I got my streams crossed.

(In reply to Tim Horton from comment #9)
> (In reply to Megan Gardner from comment #8)
> > (In reply to Tim Horton from comment #6)
> > > (In reply to Megan Gardner from comment #4)
> > Ok looking at those flags, at least in CALayer and UIView, seems to be more
> > about optimization of drawing, in that it is guarantees that all the pixels
> > have relevant data rather than the presence or absence of an alpha channel,
> > which I guess after looking at the code more closely is also sorta what ours
> > is about. So I guess that makes sense. It's a shame that the SharableBitmap
> > flag is SupportsAlpha though.
> 
> Hmm, what? The ShareableBitmap flag is the one I changed. Is there another
> that I should change? I don't see any more SupportsAlpha in the tree after
> my change

[Tim Horton]

Created attachment 318840 [details]
Patch

[Dean Jackson]

Comment on attachment 318840 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=318840&action=review

> Source/WebKit/ChangeLog:13
> +        if the snapshot was instead originally painted in display space.

s/painted/taken/ ?

> Source/WebCore/platform/PlatformScreen.h:45
> +typedef struct CGColorSpace *CGColorSpaceRef;

We are inconsistent through WebCore as to whether the * should be on the left or right :(

> Source/WebKit/Shared/cg/ShareableBitmapCG.cpp:42
> +    return configuration.colorSpace.cgColorSpace.get() ?: sRGBColorSpaceRef();

What does this do? :)

[Tim Horton]

(In reply to Dean Jackson from comment #13)
> Comment on attachment 318840 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=318840&action=review
> 
> > Source/WebKit/ChangeLog:13
> > +        if the snapshot was instead originally painted in display space.
> 
> s/painted/taken/ ?

Sure!

> > Source/WebCore/platform/PlatformScreen.h:45
> > +typedef struct CGColorSpace *CGColorSpaceRef;
> 
> We are inconsistent through WebCore as to whether the * should be on the
> left or right :(

Nah, this one isn't an ObjC type so it should be on the left. I think it just keeps ending up on the right because people copy it out of CGColorSpace.h and that's how it is there.

> > Source/WebKit/Shared/cg/ShareableBitmapCG.cpp:42
> > +    return configuration.colorSpace.cgColorSpace.get() ?: sRGBColorSpaceRef();
> 
> What does this do? :)

Haha :|

Thank you!

[Tim Horton]

https://trac.webkit.org/changeset/221068/webkit

[Don Olmstead]

This ended up breaking WinCairo builds since its including GraphicsContextCG.h

https://build.webkit.org/builders/WinCairo%2064-Bit%20Release/builds/5459/steps/compile-webkit/logs/stdio

[Tim Horton]

(In reply to Don Olmstead from comment #16)
> This ended up breaking WinCairo builds since its including
> GraphicsContextCG.h
> 
> https://build.webkit.org/builders/WinCairo%2064-Bit%20Release/builds/5459/
> steps/compile-webkit/logs/stdio

Err, heh, yeah, I may have forgotten about WinCairo and thought I was safe. Easy to fix, anyway.

[Don Olmstead]

(In reply to Tim Horton from comment #17)
> (In reply to Don Olmstead from comment #16)
> > This ended up breaking WinCairo builds since its including
> > GraphicsContextCG.h
> > 
> > https://build.webkit.org/builders/WinCairo%2064-Bit%20Release/builds/5459/
> > steps/compile-webkit/logs/stdio
> 
> Err, heh, yeah, I may have forgotten about WinCairo and thought I was safe.
> Easy to fix, anyway.

No worries. We really really need an EWS bot.

[Tim Horton]

(In reply to Don Olmstead from comment #18)
> (In reply to Tim Horton from comment #17)
> > (In reply to Don Olmstead from comment #16)
> > > This ended up breaking WinCairo builds since its including
> > > GraphicsContextCG.h
> > > 
> > > https://build.webkit.org/builders/WinCairo%2064-Bit%20Release/builds/5459/
> > > steps/compile-webkit/logs/stdio
> > 
> > Err, heh, yeah, I may have forgotten about WinCairo and thought I was safe.
> > Easy to fix, anyway.
> 
> No worries. We really really need an EWS bot.

Should be fixed in https://trac.webkit.org/changeset/221088/webkit