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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Sep 6 15:31:16 PDT 2011

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.

Attachment 106470: Patch

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

The settings stuff isn't right.  I'm not sure you need to edit WebSettings or
WebSettingsImpl at all, I can't find any code in this patch that uses that
logic and it seems wrong (there's nothing to propagate sets on WebSettings to
Settings that I can see).

> +layer at (0,0) size 800x600
> +  RenderView at (0,0) size 800x600
> +layer at (0,0) size 800x600
> +  RenderBlock {HTML} at (0,0) size 800x600
> +    RenderBody {BODY} at (8,8) size 784x584
> +layer at (100,100) size 100x100
> +  RenderBlock (positioned) {DIV} at (100,100) size 100x100 [bgcolor=#0000FF]

there shouldn't be a rendertree dump for this test

> LayoutTests/platform/chromium/compositing/zoom-animator-scale-test.html:9
> +	  if (window.layoutTestController)
> +	    window.layoutTestController.waitUntilDone();

this should be dumpAsText(true)

there also should be some sort of comment somewhere in this .html explaining
what we should see in a pass

> LayoutTests/platform/chromium/test_expectations.txt:2590
> +// This is a temporary test that relies on the debug-clear colour, so don't
> +// in release mode.
platform/chromium/compositing/zoom-animator-scale-test.html = IMAGE

we clear to blue in release mode too, so this should pass in release mode -
does it not?

we should _not_ skip this test in release. at most, mark it as = IMAGE
(although i doubt that is necessary)

> Source/WebCore/testing/Internals.cpp:252
> +    document->settings()->setZoomAnimatorScale(scale);

this is setting it on the WebCore::Settings object, right? Do you need to
modify WebKit::WebSettings at all?

> Source/WebKit/chromium/public/WebSettings.h:108
> +    virtual double getZoomAnimatorScale() = 0;

this is incorrectly named, but it also seems uncalled so maybe just delete it

> Source/WebKit/chromium/src/WebSettingsImpl.cpp:325
> +    if (scale > 0.0)
> +	   m_zoomAnimatorScale = scale;

why doesn't this just set the value on m_settings ?

> Source/WebKit/chromium/src/WebSettingsImpl.h:100
> +    virtual double getZoomAnimatorScale() { return m_zoomAnimatorScale; }

we don't prefix getters with "get" in WebKit, this should be just
zoomAnimatorScale() if it's here at all

why is this needed? who queries a WebSettingsImpl for this value (as opposed to
a Settings)?

> Source/WebKit/chromium/src/WebSettingsImpl.h:132
> +
> +    double m_zoomAnimatorScale;

i don't understand why this is here

More information about the webkit-reviews mailing list