Remove ColorSpace argument to all the drawing calls
Created attachment 264917 [details] Patch
Created attachment 264919 [details] Patch
I thought you could switch SVG filters into linearized sRGB? Is this ok because we don't ever paint with GraphicsContext into those buffers, or because we only paint regular sRGB colors into them? (I guess it's probably that)
I don't want to change any ImageBuffer or filter stuff at this point. This is just removing input params that are always ColorSpaceSRGB.
...always ColorSpaceSRGB (hard-coded in style) or ColorSpaceDeviceRGB (hardcoded at many call sites).
Comment on attachment 264919 [details] Patch One hard part here is getting all the platforms to build. GTK, EFL, and iOS all failed on EWS.
Created attachment 264939 [details] Patch
Comment on attachment 264939 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=264939&action=review Looking forward to merging this with my mega-patch after you land it ;) > Source/WebCore/platform/graphics/ImageBuffer.h:72 > + static std::unique_ptr<ImageBuffer> create(const FloatSize& size, RenderingMode renderingMode, float resolutionScale = 1, ColorSpace colorSpace = ColorSpaceDeviceRGB /* FIXME sRGB */) I don’t think you should land this change as part of this patch. The comment isn’t all that valuable. We’re going to remove the ColorSpaceDeviceRGB constant entirely, soon enough. > Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:989 > if (m_uncorrectedContentsImage && m_uncorrectedContentsImage.get() == newImage) Someone should come back here and figure out: 1) Why we need to null check m_uncorrectedContentsImage. Doesn’t seem we need to. 2) Why we have to say .get() here; that should not be required. > Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:85 > static CGColorSpaceRef sRGBSpace = CGColorSpaceCreateWithName(kCGColorSpaceSRGB); Are you sure this works on iOS? Did you test it? I seem to recall having a problem with this. Should make sure we also run tests on iOS, not just Mac. > Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:188 > + RetainPtr<CGContextRef> context = adoptCF(CGBitmapContextCreate(0, logicalSize().width(), logicalSize().height(), 8, 4 * logicalSize().width(), deviceRGBColorSpaceRef() /* FIXME */, kCGImageAlphaPremultipliedLast)); I don’t think this FIXME is valuable. > Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:233 > UNUSED_PARAM(useLowQualityScale); We should leave out the argument name rather than using UNUSED_PARAM. > Source/WebCore/platform/graphics/win/GraphicsContextCGWin.cpp:147 > + CGColorRef colorRef = color.isValid() ? cachedCGColor(color) : 0; Surprised you didn’t do the nullptr thing here.
(In reply to comment #8) > Comment on attachment 264939 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=264939&action=review > > Looking forward to merging this with my mega-patch after you land it ;) > > > Source/WebCore/platform/graphics/ImageBuffer.h:72 > > + static std::unique_ptr<ImageBuffer> create(const FloatSize& size, RenderingMode renderingMode, float resolutionScale = 1, ColorSpace colorSpace = ColorSpaceDeviceRGB /* FIXME sRGB */) > > I don’t think you should land this change as part of this patch. The comment > isn’t all that valuable. We’re going to remove the ColorSpaceDeviceRGB > constant entirely, soon enough. Agreed; I did't mean to include the ImageBuffer creation changes as part of the patch. I'll do them separately (possibly landing first). > > Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:989 > > if (m_uncorrectedContentsImage && m_uncorrectedContentsImage.get() == newImage) > > Someone should come back here and figure out: Yes! > 1) Why we need to null check m_uncorrectedContentsImage. Doesn’t seem we > need to. > 2) Why we have to say .get() here; that should not be required. > > > Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:85 > > static CGColorSpaceRef sRGBSpace = CGColorSpaceCreateWithName(kCGColorSpaceSRGB); > > Are you sure this works on iOS? Did you test it? I seem to recall having a > problem with this. Should make sure we also run tests on iOS, not just Mac. I will test. > > Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:188 > > + RetainPtr<CGContextRef> context = adoptCF(CGBitmapContextCreate(0, logicalSize().width(), logicalSize().height(), 8, 4 * logicalSize().width(), deviceRGBColorSpaceRef() /* FIXME */, kCGImageAlphaPremultipliedLast)); > > I don’t think this FIXME is valuable. I'll revert this change. > > Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:233 > > UNUSED_PARAM(useLowQualityScale); > > We should leave out the argument name rather than using UNUSED_PARAM. > > > Source/WebCore/platform/graphics/win/GraphicsContextCGWin.cpp:147 > > + CGColorRef colorRef = color.isValid() ? cachedCGColor(color) : 0; > > Surprised you didn’t do the nullptr thing here. Will do.
Created attachment 264991 [details] Patch
Created attachment 264992 [details] Patch
Created attachment 264994 [details] Patch
Created attachment 264995 [details] Patch
Comment on attachment 264995 [details] Patch Darin reviewed this.
It broke the Windows build.
(In reply to comment #15) > It broke the Windows build. Simon, was there a reason why you couldn't let the commit queue land this? It would have prevented breaking the Windows build.
Via <https://build.webkit.org/builders/Apple%20Win%20Debug%20%28Build%29/builds/91957/steps/compile-webkit/logs/stdio>: c:\cygwin\home\buildbot\slave\win-debug\build\source\webcore\rendering\RenderThemeWin.cpp(895): error C2039: 'colorSpace': is not a member of 'WebCore::RenderStyle' (compiling source file C:\cygwin\home\buildbot\slave\win-debug\build\Source\WebCore\rendering\RenderingAllInOne.cpp) [C:\cygwin\home\buildbot\slave\win-debug\build\WebKitBuild\Debug\Source\WebCore\WebCore.vcxproj] c:\cygwin\home\buildbot\slave\win-debug\build\source\webcore\rendering\RenderMediaControls.h(42): note: see declaration of 'WebCore::RenderStyle' (compiling source file C:\cygwin\home\buildbot\slave\win-debug\build\Source\WebCore\rendering\RenderingAllInOne.cpp) c:\cygwin\home\buildbot\slave\win-debug\build\source\webcore\rendering\RenderThemeWin.cpp(943): error C2039: 'colorSpace': is not a member of 'WebCore::RenderStyle' (compiling source file C:\cygwin\home\buildbot\slave\win-debug\build\Source\WebCore\rendering\RenderingAllInOne.cpp) [C:\cygwin\home\buildbot\slave\win-debug\build\WebKitBuild\Debug\Source\WebCore\WebCore.vcxproj] c:\cygwin\home\buildbot\slave\win-debug\build\source\webcore\rendering\RenderMediaControls.h(42): note: see declaration of 'WebCore::RenderStyle' (compiling source file C:\cygwin\home\buildbot\slave\win-debug\build\Source\WebCore\rendering\RenderingAllInOne.cpp) c:\cygwin\home\buildbot\slave\win-debug\build\source\webcore\rendering\RenderThemeWin.cpp(978): error C2039: 'colorSpace': is not a member of 'WebCore::RenderStyle' (compiling source file C:\cygwin\home\buildbot\slave\win-debug\build\Source\WebCore\rendering\RenderingAllInOne.cpp) [C:\cygwin\home\buildbot\slave\win-debug\build\WebKitBuild\Debug\Source\WebCore\WebCore.vcxproj] c:\cygwin\home\buildbot\slave\win-debug\build\source\webcore\rendering\RenderMediaControls.h(42): note: see declaration of 'WebCore::RenderStyle' (compiling source file C:\cygwin\home\buildbot\slave\win-debug\build\Source\WebCore\rendering\RenderingAllInOne.cpp) C:\cygwin\home\buildbot\slave\win-debug\build\Source\WebCore\platform\graphics\ca\win\PlatformCALayerWin.cpp(930): error C2660: 'WebCore::GraphicsContext::setFillColor': function does not take 2 arguments [C:\cygwin\home\buildbot\slave\win-debug\build\WebKitBuild\Debug\Source\WebCore\WebCore.vcxproj] JSWebKitPlaybackTargetAvailabilityEvent.cpp C:\cygwin\home\buildbot\slave\win-debug\build\Source\WebCore\platform\graphics\win\ImageCGWin.cpp(97): error C2065: 'styleColorSpace': undeclared identifier [C:\cygwin\home\buildbot\slave\win-debug\build\WebKitBuild\Debug\Source\WebCore\WebCore.vcxproj] C:\cygwin\home\buildbot\slave\win-debug\build\Source\WebCore\platform\graphics\win\ImageCGWin.cpp(105): error C2065: 'styleColorSpace': undeclared identifier [C:\cygwin\home\buildbot\slave\win-debug\build\WebKitBuild\Debug\Source\WebCore\WebCore.vcxproj]
Attempted build fix in r192143: <http://trac.webkit.org/r192143>
And another in https://trac.webkit.org/changeset/192144 This is what happens when Windows EWS is broken.
In the future, please do tell when you see EWS not processing patches. Is it right that this bug is currently open?
Last build fix in r192145: <http://trac.webkit.org/r192145>
(In reply to comment #16) > Simon, was there a reason why you couldn't let the commit queue land this? > It would have prevented breaking the Windows build. Commit queue guarantees only the patch builds and tests pass on Apple Mac platform. But if you land your patch manually, nobody prevents the author to check https://build.webkit.org/waterfall if everything works fine. ( just to document, the original patch landed in http://trac.webkit.org/changeset/192140 )