Bug 72208 – [chromium] Fix scaleDelta zoom-out visibility rect bug

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Alexandre Elias]

[chromium] Fix scaleDelta zoom-out visibility rect bug

[Alexandre Elias]

Created attachment 114825 [details]
Patch

[Alexandre Elias]

Created attachment 114826 [details]
Patch

[W. James MacLean]

setDeltaScale() just seems to support a scale change, but zoomAnimatorTransform needs to support translation as well. Perhaps I'm missing something, but I don't see how that will work after this patch lands.

Also, we're currently trying to test double-tap for zoom, so it's a problem if this is being disabled in the upstream repo.

[Alexandre Elias]

OK, I didn't know it required translation.  From the point of view of what I'm doing, it makes the code less opaque not to have a generalized transform.

By the way, zoomAnimatorTransform should already be similarly glitchy upstream due to the scrolling layer being moved down several layers (so it cannot).  It does need a similar fix to what I did for pageScaleDelta.

[Alexandre Elias]

Created attachment 115006 [details]
Patch

[Alexandre Elias]

OK, I added a similar fix for zoomAnimatorTransform.  I kept them on different codepaths to make it easier to work separately, but zoomAnimatorTransform is now applied the same way.

(Minor: I also removed the option to have a gray background when zoomAnimatorTransform is active.  Now that the clip-rect is no longer buggy, we should never see the glClear color anyway, since it will draw tiles -- checkerboard if necessary -- all over the screen.)

[James Robinson]

(In reply to comment #3)
> setDeltaScale() just seems to support a scale change, but zoomAnimatorTransform needs to support translation as well.

Can you expand a bit on why?  That seems wrong, translations should be an orthogonal concept to zoom - you typically want to change the scroll offsets when zooming but that's handled by separate code.

[James Robinson]

Comment on attachment 115006 [details]
Patch

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

> Source/WebCore/ChangeLog:30
> +>2011-11-14  Pavel Feldman  <[e-mail]>

something went awry here, there's a leading > on this line

> Source/WebCore/platform/graphics/chromium/LayerChromium.h:125
> +    TransformationMatrix zoomAnimatorTransform() const { return TransformationMatrix(); }

hmmm, so main thread layers never have a non-identify zoomAnimatorTransform? How does the test hookup work then - that used to manipulate the zoomAnimatorTransform on CCLayerTreeHost directly.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.h:123
> +    void setZoomAnimatorTransform(const TransformationMatrix&);
> +    const TransformationMatrix& zoomAnimatorTransform() const { return m_zoomAnimatorTransform; }

can you update CCLayerImplTest for this property?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.h:164
> +    float scaleDelta() const { return m_scaleDelta; }
> +    void setScaleDelta(float);

same here - please update CCLayerImplTest

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:199
> +    // LT = Tr[origin] * M[zoomAnimator] * S[scaleDelta] * Tr[origin2anchor]

the old comments grew the expression in the comment as the subsequent components were added, but this one seems to have the first three components in the comments before any modifications have taken place. I think this first comment should just be "LT = Tr[origin] * M[zoomAnimator]"

[Alexandre Elias]

Comment on attachment 115006 [details]
Patch

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

>> Source/WebCore/ChangeLog:30
>> +>2011-11-14  Pavel Feldman  <[e-mail]>
> 
> something went awry here, there's a leading > on this line

Fixed

>> Source/WebCore/platform/graphics/chromium/LayerChromium.h:125
>> +    TransformationMatrix zoomAnimatorTransform() const { return TransformationMatrix(); }
> 
> hmmm, so main thread layers never have a non-identify zoomAnimatorTransform? How does the test hookup work then - that used to manipulate the zoomAnimatorTransform on CCLayerTreeHost directly.

It still manipulates CCLayerTreeHost directly, and then CCLayerTreeHostImpl pushes it to the CCLayerImpl.  This call is just to make the template happy.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.h:123
>> +    const TransformationMatrix& zoomAnimatorTransform() const { return m_zoomAnimatorTransform; }
> 
> can you update CCLayerImplTest for this property?

Done

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.h:164
>> +    void setScaleDelta(float);
> 
> same here - please update CCLayerImplTest

Done

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:199
>> +    // LT = Tr[origin] * M[zoomAnimator] * S[scaleDelta] * Tr[origin2anchor]
> 
> the old comments grew the expression in the comment as the subsequent components were added, but this one seems to have the first three components in the comments before any modifications have taken place. I think this first comment should just be "LT = Tr[origin] * M[zoomAnimator]"

Fixed

[Alexandre Elias]

Created attachment 115037 [details]
Patch

[Alexandre Elias]

Created attachment 115060 [details]
Patch

[Alexandre Elias]

The zoom animator layout tests were causing problems, as they're covering a non-impl-thread side codepath that is never really used in practice, and the scroll layer isn't available from non-impl side.  After talking it over with James, I removed the tests -- sorry about that.  For testing impl-side changes, we'll have to focus more on unit tests.

[James Robinson]

Comment on attachment 115060 [details]
Patch

R=me.

[WebKit Review Bot]

Comment on attachment 115060 [details]
Patch

Clearing flags on attachment: 115060

Committed r100235: <http://trac.webkit.org/changeset/100235>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[W. James MacLean]

(In reply to comment #6)
> OK, I added a similar fix for zoomAnimatorTransform.  I kept them on different codepaths to make it easier to work separately, but zoomAnimatorTransform is now applied the same way.
> 
> (Minor: I also removed the option to have a gray background when zoomAnimatorTransform is active.  Now that the clip-rect is no longer buggy, we should never see the glClear color anyway, since it will draw tiles -- checkerboard if necessary -- all over the screen.)

Correct me if I'm wrong, but doesn't this only work if the set of tiles currently being zoomed is enough to fill the screen? For example, if we zoom out (shrink the page), there may be regions that don't have tiles, and those won't get checkerboards?

[W. James MacLean]

(In reply to comment #7)
> (In reply to comment #3)
> > setDeltaScale() just seems to support a scale change, but zoomAnimatorTransform needs to support translation as well.
> 
> Can you expand a bit on why?  That seems wrong, translations should be an orthogonal concept to zoom - you typically want to change the scroll offsets when zooming but that's handled by separate code.

The zoom animator only redraws existing textures, so pageScale doesn't change during the animation. If I remember correctly, given the mis-match between page scale and the zoom animation, scrolling does not work correctly *during* the animation.

When the final page scale is set, we use an animation-less scroll to align the content around the zoom point.

[James Robinson]

(In reply to comment #16)
> (In reply to comment #6)
> > OK, I added a similar fix for zoomAnimatorTransform.  I kept them on different codepaths to make it easier to work separately, but zoomAnimatorTransform is now applied the same way.
> > 
> > (Minor: I also removed the option to have a gray background when zoomAnimatorTransform is active.  Now that the clip-rect is no longer buggy, we should never see the glClear color anyway, since it will draw tiles -- checkerboard if necessary -- all over the screen.)
> 
> Correct me if I'm wrong, but doesn't this only work if the set of tiles currently being zoomed is enough to fill the screen? For example, if we zoom out (shrink the page), there may be regions that don't have tiles, and those won't get checkerboards?

The entire viewport has tiles, so unless you allow the user to zoom out such that more than the viewport is visible (which you won't) this isn't an issue.

[James Robinson]

(In reply to comment #17)
> (In reply to comment #7)
> > (In reply to comment #3)
> > > setDeltaScale() just seems to support a scale change, but zoomAnimatorTransform needs to support translation as well.
> > 
> > Can you expand a bit on why?  That seems wrong, translations should be an orthogonal concept to zoom - you typically want to change the scroll offsets when zooming but that's handled by separate code.
> 
> The zoom animator only redraws existing textures, so pageScale doesn't change during the animation. If I remember correctly, given the mis-match between page scale and the zoom animation, scrolling does not work correctly *during* the animation.

Right, that's why you need to adjust the scroll offsets when adjusting the scale during the animation.'

> 
> When the final page scale is set, we use an animation-less scroll to align the content around the zoom point.

Correct.

[Alexandre Elias]

(In reply to comment #16)
> Correct me if I'm wrong, but doesn't this only work if the set of tiles currently being zoomed is enough to fill the screen? For example, if we zoom out (shrink the page), there may be regions that don't have tiles, and those won't get checkerboards?

We aren't implementing checkerboards by first applying them to tiles, then painting them -- that would be wasteful of RAM as well as having the problem you mention.  Checkerboarding is done by fallback "virtual" tiles that are drawn by a shader whenever no tile is available.  So as long as request to draw a tile is issued for every visible area on the screen, we will fill the screen with either valid content or checkerboard.

[W. James MacLean]

(In reply to comment #20)
> (In reply to comment #16)
> > Correct me if I'm wrong, but doesn't this only work if the set of tiles currently being zoomed is enough to fill the screen? For example, if we zoom out (shrink the page), there may be regions that don't have tiles, and those won't get checkerboards?
> 
> We aren't implementing checkerboards by first applying them to tiles, then painting them -- that would be wasteful of RAM as well as having the problem you mention.  Checkerboarding is done by fallback "virtual" tiles that are drawn by a shader whenever no tile is available.  So as long as request to draw a tile is issued for every visible area on the screen, we will fill the screen with either valid content or checkerboard.

Ahh, OK. I knew it was virtual tiles, but I wasn't sure they would propagate everywhere during the zoom animation.