[chromium] Add backend compositor support for rescaling (zooming) textures during zoom animation.

• Component: New Bugs
• Status: RESOLVED FIXED

[W. James MacLean]

[chromium] Add backend compositor support for rescaling (zooming) textures during zoom animation.

[W. James MacLean]

Created attachment 104349 [details]
Patch

[W. James MacLean]

Some background: this patch provides back end support for an experimental zoom animation feature. For now this patch is relatively inactive, but I'd like to get it in place now so it's easier to maintain w.r.t. chromium's compositor changes. The front-end patch will follow in a couple of days.

[James Robinson]

Figure out a way for this to be tested before trying to check it in. we will not be able to maintain a feature like this without tests.

[W. James MacLean]

(In reply to comment #3)
> Figure out a way for this to be tested before trying to check it in. we will not be able to maintain a feature like this without tests.

Hmmm. The only way I can think of to do this without including the front-end zoom animator stuff is a compositor unit test. Do such things exist? My initial perusal of the chromium port directories didn't turn up any examples ...

[James Robinson]

We do, see Source/WebKit/chromium/tests/.  There are not many tests now, but it would be great to have more :)

Another option is to expose the zoom level through window.internals and construct normal layout tests with different zoom levels.  See http://trac.webkit.org/changeset/92697 for one example of how this can be done.

[W. James MacLean]

(In reply to comment #5)
> 
> Another option is to expose the zoom level through window.internals and construct normal layout tests with different zoom levels.  See http://trac.webkit.org/changeset/92697 for one example of how this can be done.

I like this option, except that unlike forcing the compositor on, plumbing through the zoom scale creates all sorts of code in settings that exists purely for the sake of the test. Is there some way to mitigate this excess baggage?

Having pixel comparisons in a layout test seems kinda necessary.

[James Robinson]

(In reply to comment #6)
> (In reply to comment #5)
> > 
> > Another option is to expose the zoom level through window.internals and construct normal layout tests with different zoom levels.  See http://trac.webkit.org/changeset/92697 for one example of how this can be done.
> 
> I like this option, except that unlike forcing the compositor on, plumbing through the zoom scale creates all sorts of code in settings that exists purely for the sake of the test. Is there some way to mitigate this excess baggage?

How much of this baggage is really redundant with the frontend code?

> 
> Having pixel comparisons in a layout test seems kinda necessary.

Well that depends on what you want to test.  If you want to test that the rendered output is correct, then you probably do need the pixels.

[W. James MacLean]

(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #5)
> > > 
> > > Another option is to expose the zoom level through window.internals and construct normal layout tests with different zoom levels.  See http://trac.webkit.org/changeset/92697 for one example of how this can be done.
> > 
> > I like this option, except that unlike forcing the compositor on, plumbing through the zoom scale creates all sorts of code in settings that exists purely for the sake of the test. Is there some way to mitigate this excess baggage?
> 
> How much of this baggage is really redundant with the frontend code?

When the feature is finished, there is no way to affect it through settings, so all the code in WebSettings, WebSettingsImpl is kind of wasted. I suppose it could go behind a define, but then it wouldn't get tested.

It's really just 4 accessors, one member var (in WebSettingsImpl), and one extra line of code in WebViewImpl, but still it has no purpose beyond facilitating the test.

I suppose we could yank the extra baggage out in a separate CL after the front-end code lands?

> > 
> > Having pixel comparisons in a layout test seems kinda necessary.
> 
> Well that depends on what you want to test.  If you want to test that the rendered output is correct, then you probably do need the pixels.

The scale factor, which is used to shrink/enlarge a texture during a zoom animation, is only used to quickly display the effects of a zoom during the animation, without having to re-layout (i.e. we quickly re-composite the existing layer textures without redrawing). The only meaningful test (I can think of) is either (1) the gl matrix (but there's a separate one for each layer and they'd be hard to expose), and (2) the pixels themselves.

So I think I'd need the pixels.

[James Robinson]

(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #6)
> > > (In reply to comment #5)
> > > > 
> > > > Another option is to expose the zoom level through window.internals and construct normal layout tests with different zoom levels.  See http://trac.webkit.org/changeset/92697 for one example of how this can be done.
> > > 
> > > I like this option, except that unlike forcing the compositor on, plumbing through the zoom scale creates all sorts of code in settings that exists purely for the sake of the test. Is there some way to mitigate this excess baggage?
> > 
> > How much of this baggage is really redundant with the frontend code?
> 
> When the feature is finished, there is no way to affect it through settings, so all the code in WebSettings, WebSettingsImpl is kind of wasted. I suppose it could go behind a define, but then it wouldn't get tested.
> 
> It's really just 4 accessors, one member var (in WebSettingsImpl), and one extra line of code in WebViewImpl, but still it has no purpose beyond facilitating the test.

I agree that it sucks to have to go through WebSettings, but testing is important.
> 
> I suppose we could yank the extra baggage out in a separate CL after the front-end code lands?

And test it some other way?

> 
> > > 
> > > Having pixel comparisons in a layout test seems kinda necessary.
> > 
> > Well that depends on what you want to test.  If you want to test that the rendered output is correct, then you probably do need the pixels.
> 
> The scale factor, which is used to shrink/enlarge a texture during a zoom animation, is only used to quickly display the effects of a zoom during the animation, without having to re-layout (i.e. we quickly re-composite the existing layer textures without redrawing). The only meaningful test (I can think of) is either (1) the gl matrix (but there's a separate one for each layer and they'd be hard to expose), and (2) the pixels themselves.
> 
> So I think I'd need the pixels.

Right.

[W. James MacLean]

(In reply to comment #9)
> 
> And test it some other way?

Yes, once the front-end is in place, then it can be tested through a zoom gesture test.

I nearly have a revised patch figured out, will post shortly ...

[James Robinson]

Note that you will probably also have some merge conflicts to sort out with http://trac.webkit.org/changeset/93329

[W. James MacLean]

(In reply to comment #11)
> Note that you will probably also have some merge conflicts to sort out with http://trac.webkit.org/changeset/93329

OK, so the new patch may not happen as soon as expected :-(

[W. James MacLean]

Created attachment 105804 [details]
Patch

[W. James MacLean]

(In reply to comment #12)
> (In reply to comment #11)
> > Note that you will probably also have some merge conflicts to sort out with http://trac.webkit.org/changeset/93329
> 
> OK, so the new patch may not happen as soon as expected :-(

It's here, updated to account for recent changes to LayerRendererChromium, and with a layout test.

[James Robinson]

Comment on attachment 105804 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=105804&action=review

Thanks for the test.  The code looks pretty sane (although you've got a lot of merge conflicts now. pardon our dust!)

I'm not really sure what is going on with the test, though.  What would the test display if the test failed?  Normally we try to construct tests to that you see red if it fails and green if it passes, if that's feasible, and otherwise we try to describe the output you should see in the test (in an HTML comment or the like).  I can't tell if that test is passing or failing just from looking at it.  It would also be good to have tests for >1 zooms as well as <1.  I'm also a little confused about whether the non-initialized compositor blue is showing up in the test or if that's part of the test page.  We probably shouldn't be showing the uninitialized blue ever, right?

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

please also call layoutTestController.dumpAsText(true) to indicate that the pixel output is the only thing needed

[W. James MacLean]

(In reply to comment #15)
> (From update of attachment 105804 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=105804&action=review
> 
> Thanks for the test.  The code looks pretty sane (although you've got a lot of merge conflicts now. pardon our dust!)
> 
> I'm not really sure what is going on with the test, though.  What would the test display if the test failed?  Normally we try to construct tests to that you see red if it fails and green if it passes, if that's feasible, and otherwise we try to describe the output you should see in the test (in an HTML comment or the like).  I can't tell if that test is passing or failing just from looking at it.  It would also be good to have tests for >1 zooms as well as <1.  I'm also a little confused about whether the non-initialized compositor blue is showing up in the test or if that's part of the test page.  We probably shouldn't be showing the uninitialized blue ever, right?
> 
> > LayoutTests/platform/chromium/compositing/zoom-animator-scale-test.html:9
> > +       if (window.layoutTestController)
> > +         window.layoutTestController.waitUntilDone();
> 
> please also call layoutTestController.dumpAsText(true) to indicate that the pixel output is the only thing needed

For the short term, the uninitialized blue needs to e part of the test (ask me about this offline).

I can add the description of the expected result, plus a >1 test, plus the dumpAsText call (once I get out of merge hell ;-) ).

[W. James MacLean]

(In reply to comment #15)

Base on our chat on Friday, you suggested I follow the example of setViewport as to how I can plumb the call to setTextureZoom. I did this, placing the call to hostImpl->setTextureZoom (my code) inside CCLayerTreeHost::preCommit(). And this worked.

But now preCommit() is gone, and moving the plumbing to commitTo() doesn't seem to work the same (although this is where setViewport has moved to).

So I'm not sure now where to put this, and what the difference is between preCommit and commitTo.

The ultimate problem is this: how to get the textureZoom value from WebViewImpl to LayerRendererChromium.

[W. James MacLean]

(In reply to comment #17)
> (In reply to comment #15)
> 
> Base on our chat on Friday, you suggested I follow the example of setViewport as to how I can plumb the call to setTextureZoom. I did this, placing the call to hostImpl->setTextureZoom (my code) inside CCLayerTreeHost::preCommit(). And this worked.
> 
> But now preCommit() is gone, and moving the plumbing to commitTo() doesn't seem to work the same (although this is where setViewport has moved to).
> 
> So I'm not sure now where to put this, and what the difference is between preCommit and commitTo.
> 
> The ultimate problem is this: how to get the textureZoom value from WebViewImpl to LayerRendererChromium.

OK, after putting this under the debugger it seems commitTo() should work, but isn't getting called. CCSingleThreadProxy seems to be trying to call it, but when I go to step in in gdb it seems to just continue the program (as if it had been optimised out?).

[W. James MacLean]

(In reply to comment #18)
> (In reply to comment #17)
> > (In reply to comment #15)
> > 
> > Base on our chat on Friday, you suggested I follow the example of setViewport as to how I can plumb the call to setTextureZoom. I did this, placing the call to hostImpl->setTextureZoom (my code) inside CCLayerTreeHost::preCommit(). And this worked.
> > 
> > But now preCommit() is gone, and moving the plumbing to commitTo() doesn't seem to work the same (although this is where setViewport has moved to).
> > 
> > So I'm not sure now where to put this, and what the difference is between preCommit and commitTo.
> > 
> > The ultimate problem is this: how to get the textureZoom value from WebViewImpl to LayerRendererChromium.
> 
> OK, after putting this under the debugger it seems commitTo() should work, but isn't getting called. CCSingleThreadProxy seems to be trying to call it, but when I go to step in in gdb it seems to just continue the program (as if it had been optimised out?).

So the plumbing looks OK, so I'm guessing something else has changed in LRC ... please ignore until I've had a chance to look at it further ...

[W. James MacLean]

Created attachment 106470 [details]
Patch

[W. James MacLean]

(In reply to comment #20)
> Created an attachment (id=106470) [details]
> Patch

I believe this patch addresses most of your concerns.

The one outstanding item is the debug blue clear colour in the one layout test. At some point we will draw the un-composited region with some texture, but temporarily (until we get the front end stuff in) I'd like to leave this test as is.

I've set a test expectation to skip this one test in GPU RELEASE mode ... please let me know if I've done this correctly.

[James Robinson]

Comment on attachment 106470 [details]
Patch

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).

> LayoutTests/platform/chromium-gpu-linux/platform/chromium/compositing/zoom-animator-scale-test-expected.txt:7
> +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 bother
> +// in release mode.
> +WONTFIX SKIP GPU RELEASE : 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

[W. James MacLean]

(In reply to comment #22)
> (From update of attachment 106470 [details])
> 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).
> 
> > LayoutTests/platform/chromium-gpu-linux/platform/chromium/compositing/zoom-animator-scale-test-expected.txt:7
> > +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

Fixed.

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

Fixed.

> 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 bother
> > +// in release mode.
> > +WONTFIX SKIP GPU RELEASE : 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)

It passes, updated test_expectations to remove this.

> > 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?

At some point in time I apparently did, but you're right that it's not necessary. I've removed all the WebSettings stuff (this should address all remaining comments).

> > 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

[W. James MacLean]

Created attachment 106587 [details]
Patch

[W. James MacLean]

(In reply to comment #24)
> Created an attachment (id=106587) [details]
> Patch

webbkit-patch didn't create an entry for LayoutTests/Changelog ... is that right?

[James Robinson]

Comment on attachment 106587 [details]
Patch

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....

[W. James MacLean]

(In reply to comment #26)
> (From update of attachment 106587 [details])
> 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?

Removed.

> 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.

I don't do this on purpose. In most cases these result from evolution of the patch, where the getter is used, and then obsoleted. It's a mistake when I don't spot them before submitting, but I do appreciate your catching them. I will try to do a better job of finding/removing these in future.

> > 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

Done and done.

> > 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)

Fixed (This is something about WebKit style I've yet to get used to.)

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

Fixed.

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

Changes made at different times. I've made everything zoomAnimatorScale.

> > 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?

Originally, I put this in as a guard in case scale3d was expensive, although this is probably overly cautious on my part. When half the scale computations jumped ship the conditional got lost in the merge.

> > 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

Removed. The getter was originally necessary when debug border transforms were handled differently and in a different file. Now they've been reconciled, it's no longer needed.

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

Fixed.

> > 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

I was following the same pattern as the line below this code (where viewportSize() is used instead of m_viewportSize). Fixed.

> > 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

Fixed.

> > 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

Fixed.

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

Fixed.

> > 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.

Fixed.

> > 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?

As above.

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

Fixed.

> > 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....

No, just a last-minute mistake. Fixed.

Thanks for all your patience on this ... it's been a challenging patch to keep alive.

[W. James MacLean]

Created attachment 106732 [details]
Patch

[W. James MacLean]

(In reply to comment #28)
> Created an attachment (id=106732) [details]
> Patch

I've re-included an (empty) text expected output file, since dumpAsText(true) seems to require one (at least, running the test through new-run-webkit-tests seems to crete it, whether I'm re-baselining or not).

If this is not OK, let me know what can be done about it.

[James Robinson]

Comment on attachment 106732 [details]
Patch

R=me.

the -expected.txt is correct and expected

[W. James MacLean]

(In reply to comment #30)
> (From update of attachment 106732 [details])
> R=me.
> 
> the -expected.txt is correct and expected

Thanks!

[WebKit Review Bot]

Comment on attachment 106732 [details]
Patch

Clearing flags on attachment: 106732

Committed r94789: <http://trac.webkit.org/changeset/94789>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.