_WKThumbnailView should use the screen color space instead of sRGB
<rdar://problem/33925559>
Created attachment 318820 [details] Patch
Created attachment 318824 [details] Patch
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.
Well, looks good to what I am familiar with, obviously will need a real reviewer, and to fix the SharableBitmap build breaker.
(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!
Created attachment 318836 [details] Patch
(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!
(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
Created attachment 318838 [details] Patch
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
Created attachment 318840 [details] Patch
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? :)
(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!
https://trac.webkit.org/changeset/221068/webkit
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
(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.
(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.
(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