Bug 72343 – [Chromium] Page/layer flashes after GPU-accelerated CSS transition

• Component: New Bugs
• Status: RESOLVED FIXED

[John Bates]

Page/layer flashes after GPU-accelerated CSS transition

[John Bates]

Created attachment 115080 [details]
Patch

[James Robinson]

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?

[John Bates]

Created attachment 115210 [details]
Patch

[John Bates]

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.

[James Robinson]

Comment on attachment 115210 [details]
Patch

R=me, cool test

[WebKit Review Bot]

Comment on attachment 115210 [details]
Patch

Clearing flags on attachment: 115210

Committed r100340: <http://trac.webkit.org/changeset/100340>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Peter Kasting]

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.

[John Bates]

Created attachment 115405 [details]
Patch

[John Bates]

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.

[WebKit Review Bot]

Comment on attachment 115405 [details]
Patch

Clearing flags on attachment: 115405

Committed r100500: <http://trac.webkit.org/changeset/100500>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.