The setting acceleratedCanvas2DEnabled will soon be turned on by default in Chromium. The effect of this run-time flag must be changed so that it enables the Skia-based GPU acceleration of the 2D canvas. Furthermore, a new runtime flag must be added in order to enable the legacy accelerated 2D canvas. The legacy flag will have priority when both flags are enabled simultaneously.
Created attachment 91924 [details] Patch
Comment on attachment 91924 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=91924&action=review > Source/WebCore/page/Page.cpp:752 > + m_sharedGraphicsContext3D = SharedGraphicsContext3D::create(chrome(), settings()->legacyAccelerated2dCanvasEnabled() ? 0 : SharedGraphicsContext3D::UseSkiaGPU); Since this now makes skia-gpu the default path for accelerated canvas, won't we also need to add all failing tests to test_expectations along with this change?
Comment on attachment 91924 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=91924&action=review > Source/WebCore/page/Settings.cpp:722 > +void Settings::setLegacyAccelerated2dCanvasEnabled(bool enabled) This is a chromium-only flag so I think it should only exist in the chromium websettings files much like it was done with the compositeToTexture flag in : https://bugs.webkit.org/show_bug.cgi?id=52311 It will actually simplify things and you won't have to touch quite that many files.
Delivery plan proposed by Stephen White: 1: Add --enable-legacy-accelerated-2d-canvas flag to both Chrome and DRT (won't do anything yet, since old path is still the default) 2: Change the --accelerated-2d-canvas flag to mean Ganesh, not the old path, and remove the double-meaning of --enable-accelerated-drawing (have it mean only Alok's compositor -> skia stuff, not canvas2D). This should be a 1-liner in the code. Along with this change, submit suppressions to test_expectations for the new failures. 3: Make canvas2D GPU (skia path) on by default in Chrome. Replace the --enable-accelerated-2d-canvas flag with --disable-accelerated-2d-canvas. Pass --disable-accelerated-2d-canvas to the CPU step on the canary and trybots, and remove --enable-accelerated-2d-canvas from the GPU step. 4. Rebaseline the fast/canvas tests that need new pixel results, and remove the suppressions from test_expectations.
(In reply to comment #4) > Delivery plan proposed by Stephen White: > > 1: Add --enable-legacy-accelerated-2d-canvas flag to both Chrome and DRT (won't do anything yet, since old path is still the default) > 2: Change the --accelerated-2d-canvas flag to mean Ganesh, not the old path, and remove the double-meaning of --enable-accelerated-drawing (have it mean only Alok's compositor -> skia stuff, not canvas2D). This should be a 1-liner in the code. Along with this change, submit suppressions to test_expectations for the new failures. > 3: Make canvas2D GPU (skia path) on by default in Chrome. Replace the --enable-accelerated-2d-canvas flag with --disable-accelerated-2d-canvas. Pass --disable-accelerated-2d-canvas to the CPU step on the canary and trybots, and remove --enable-accelerated-2d-canvas from the GPU step. > 4. Rebaseline the fast/canvas tests that need new pixel results, and remove the suppressions from test_expectations. Sounds good. We should probably flip #3 and #4 so that we have the tests producing the correct results before we turn this on by default.
(In reply to comment #3) > (From update of attachment 91924 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=91924&action=review > > > Source/WebCore/page/Settings.cpp:722 > > +void Settings::setLegacyAccelerated2dCanvasEnabled(bool enabled) > > This is a chromium-only flag so I think it should only exist in the chromium websettings files much like it was done with the compositeToTexture flag in : > > https://bugs.webkit.org/show_bug.cgi?id=52311 > > It will actually simplify things and you won't have to touch quite that many files. I tried to do what you suggest, but the problem is that I can't figure out a clean way to access WebSettings from outside of WebKit/chromium/src. I think your suggestion might not work for what we are trying to do here, unless I am missing something?
Comment on attachment 91924 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=91924&action=review >>> Source/WebCore/page/Settings.cpp:722 >>> +void Settings::setLegacyAccelerated2dCanvasEnabled(bool enabled) >> >> This is a chromium-only flag so I think it should only exist in the chromium websettings files much like it was done with the compositeToTexture flag in : >> >> https://bugs.webkit.org/show_bug.cgi?id=52311 >> >> It will actually simplify things and you won't have to touch quite that many files. > > I tried to do what you suggest, but the problem is that I can't figure out a clean way to access WebSettings from outside of WebKit/chromium/src. I think your suggestion might not work for what we are trying to do here, unless I am missing something? No, you are correct. You can't access the Chromium WebKit API from WebCore. You can access it from Chrome sources and from within WebKit/chromium/src. The way you've done things looks fine. > Source/WebCore/page/Settings.h:492 > + bool m_legacyAcceleratedCanvas2dEnabled : 1; You need to initialize this new bit to false in the Settings constructor. > Source/WebKit/mac/WebView/WebPreferenceKeysPrivate.h:100 > +#define WebKitLegacyAccelerated2dCanvasEnabledPreferenceKey @"WebKitLegacyAccelerated2dCanvasEnabled" You don't need to touch any of these Mac-specific files. The Chromium port is the only one using this legacy accelerated canvas 2D code at this point, so it's better not to add the new flag to any of the other ports' WebPreferences-like classes.
Created attachment 92099 [details] Patch
Comment on attachment 92099 [details] Patch Looks fine.
Comment on attachment 92099 [details] Patch Clearing flags on attachment: 92099 Committed r85720: <http://trac.webkit.org/changeset/85720>
All reviewed patches have been landed. Closing bug.
(In reply to comment #11) > All reviewed patches have been landed. Closing bug. The patch that was landed is only phase 1. Further work on this issue continues here: https://bugs.webkit.org/show_bug.cgi?id=60173
Add legacyAccelerated2dCanvasEnabled to Settings with no explanatory comment is a bad thing to do. I have no idea what the difference is between these two prefs.
(In reply to comment #13) > Add legacyAccelerated2dCanvasEnabled to Settings with no explanatory comment is a bad thing to do. I have no idea what the difference is between these two prefs. Thanks for spotting this. This code is now vestigial, and should be cleaned out. Just created: https://bugs.webkit.org/show_bug.cgi?id=75108