Page/layer flashes after GPU-accelerated CSS transition
Created attachment 115080 [details] Patch
Comment on attachment 115080 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=115080&action=review This needs testing directions at least, even if they are manual. Some other comments inline > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:285 > + if (m_context) you don't need this check any more, since the only caller is drawLayersInternal() which can't be reached if context is null > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:286 > + m_context->reshape(std::max(1, viewportWidth()), std::max(1, viewportHeight())); the style was wrong here before, but this is a good time to fix it - we don't do calls like this with "std::max()" in WebKit, we put a using namespace std; declaration at the top of the .cpp and then just call 'max()'. This file already has the using declaration so just remove the std:: from these callers looking more closely if we do it this way then we shouldn't need the max() at all - the only callsite is guarded by an if (viewportSize().isEmpty()) check, so we should never get here if width or height <= 0 > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.h:194 > + void doViewportChanged(); what an ugly function name - i hate all the doXXX() functions. Do you even need a function here? there's only one caller, why not just do it there?
Created attachment 115210 [details] Patch
Comment on attachment 115080 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=115080&action=review Added a unit test to verify that reshape doesn't happen until drawLayers. >> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:285 >> + if (m_context) > > you don't need this check any more, since the only caller is drawLayersInternal() which can't be reached if context is null Done. >> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:286 >> + m_context->reshape(std::max(1, viewportWidth()), std::max(1, viewportHeight())); > > the style was wrong here before, but this is a good time to fix it - we don't do calls like this with "std::max()" in WebKit, we put a using namespace std; declaration at the top of the .cpp and then just call 'max()'. This file already has the using declaration so just remove the std:: from these callers > > looking more closely if we do it this way then we shouldn't need the max() at all - the only callsite is guarded by an if (viewportSize().isEmpty()) check, so we should never get here if width or height <= 0 Done. >> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.h:194 >> + void doViewportChanged(); > > what an ugly function name - i hate all the doXXX() functions. Do you even need a function here? there's only one caller, why not just do it there? Done.
Comment on attachment 115210 [details] Patch R=me, cool test
Comment on attachment 115210 [details] Patch Clearing flags on attachment: 115210 Committed r100340: <http://trac.webkit.org/changeset/100340>
All reviewed patches have been landed. Closing bug.
The new test here is suffering an assertion failure on the Win dbg (1) canary, see e.g.: http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Win%20%28dbg%29%281%29/builds/8094/steps/webkit_unit_tests/logs/stdio Per jbates I'll roll this out.
Created attachment 115405 [details] Patch
Comment on attachment 115405 [details] Patch CCLayerImpl::draw is a NOTREACHED, so this test needed a fake CCLayerImpl that is drawable. The debug build no longer asserts with this change. PTAL.
Comment on attachment 115405 [details] Patch Clearing flags on attachment: 115405 Committed r100500: <http://trac.webkit.org/changeset/100500>