[webkit-reviews] review denied: [Bug 66472] [chromium] Add backend compositor support for rescaling (zooming) textures during zoom animation. : [Attachment 106587] Patch

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has denied W. James MacLean
<wjmaclean at chromium.org>'s request for review:
Bug 66472: [chromium] Add backend compositor support for rescaling (zooming)
textures during zoom animation.
https://bugs.webkit.org/show_bug.cgi?id=66472

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

------- Additional Comments from James Robinson <jamesr at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=106587&action=review


After thinking about it, I really think that depending on the uninitialized
compositor blue is an absolute no-go for me.  We really want to change that
behavior ASAP and having a test that depends on it is just going to be a mess.

Until you get the frontend in (which will presumably fix this) could you just
remove the <1 zoom factor test?

As a general note, I wish that you would be a little more careful about adding
unnecessary getters.  If you end up adding a function and it never gets called,
that's a sign that you need to think a little bit more about whether that
getter really should be part of the object's interface or not.

> LayoutTests/platform/chromium/compositing/zoom-animator-scale-test2.html:13
> +	       document.getElementById("aBox").style.backgroundColor =
"#0000FF"; // Change to blue

we prefer green to blue for 'good' in layout tests. you can also just set the
backgroundColor to "blue" and not need the comment

> Source/WebCore/page/Settings.cpp:219
> +    , m_zoomAnimatorScale(1.0)

webkit style prefers '1' for constants like this instead of 1.0 unless the type
is necessary (which it isn't here)

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:166
> +    , m_textureZoom(1.0)

1

why is it zoomAnimatorScale in Settings and m_textureZoom here?  I think
zoomAnimatorScale makes more sense

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:299
> +    if (m_textureZoom != 1.0)
> +	   zoomMatrix.scale3d(m_textureZoom, m_textureZoom, 1.0);

i'm not sure why this scale is conditional but the other scales (like in
CCLayerTreeHost) are not. is there any particular reason?

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.h:113
> +    double getTextureZoom() { return m_textureZoom; }

nack, no 'get' on getters.  does anybody call this? it looks like the zoom
value is only used in this class directly, so i don't see any reason to add a
getter at all

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:57
> +    , m_textureZoom(1.0)

1

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:119
> +    hostImpl->setTextureZoom(textureZoom());

why doesn't this just use m_textureZoom? then you wouldn't need a getter

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:187
> +void CCLayerTreeHost::setTextureZoom(const double zoom)

the 'const' here doesn't seem to add much since this is already passed by value


> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.h:133
> +    void setTextureZoom(const double);

'const double' as a parameter type makes zero sense. the double is passed by
value (obviously) so const doesn't change the semantics

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.h:152
> +    double textureZoom() const { return m_textureZoom; }

nobody calls this, remove it

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:50
> +    , m_textureZoom(1.0)

1

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:161
> +void CCLayerTreeHostImpl::setTextureZoom(double zoom)
> +{
> +    m_textureZoom = zoom;

why are we storing this on the CCLayerTreeHostImpl at all? we never use this
value, only the LayerRendererChromium uses it.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.h:91
> +    double m_textureZoom;

why does this store the textureZoom value if the LayerRendererChromium is also
storing it?

> Source/WebKit/chromium/src/WebViewImpl.cpp:1827
> +	   m_layerTreeHost->setTextureZoom(1.0);

1.0 should be 1

> Source/WebKit/chromium/src/WebViewImpl.cpp:2573
>  {
> -#if USE(THREADED_COMPOSITING)
>      if (m_layerTreeHost)
> -	   m_layerTreeHost->setNeedsCommitAndRedraw();
> -#else
> +	  
m_layerTreeHost->setTextureZoom(m_page->settings()->zoomAnimatorScale());
> +#if !USE(THREADED_COMPOSITING)
>      m_client->scheduleComposite();
>  #endif

Is this a bad merge?  This change doesn't look right....