[webkit-reviews] review denied: [Bug 72343] Page/layer flashes after GPU-accelerated CSS transition : [Attachment 115080] Patch

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has denied John Bates
<jbates at google.com>'s request for review:
Bug 72343: Page/layer flashes after GPU-accelerated CSS transition
https://bugs.webkit.org/show_bug.cgi?id=72343

Attachment 115080: Patch
https://bugs.webkit.org/attachment.cgi?id=115080&action=review

------- Additional Comments from James Robinson <jamesr at chromium.org>
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?