951415 - White edge when scrolling

Status: RESOLVED DUPLICATE of bug 935219

(Core :: Graphics: Layers, defect)

Description

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Composition bounds are being update incorrectly and applied incorrectly.

Steps to reproduce:

1. Navigate to daringfireball.net
2. Scroll down
3. Zoom all the way out
4. Scroll left and right

Expected results:
Dark background

Actual results:
White bar on the right of the screen

Comment 1

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Fix GetExpandedScrollRect() mixed co-ordinate spaces;

Comment 2

[Kartikaya Gupta (email:[e-mail])]

Way to steal my patch. See bug 951320. We should coordinate more about what you're working on so this doesn't happen again.

Comment 3

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8349051 [details] [diff] [review]
Fix GetExpandedScrollRect() mixed co-ordinate spaces;

Review of attachment 8349051 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/FrameMetrics.h
@@ +122,5 @@
>    // down. i.e. we know that scrollableRect can go back as far as zero.
>    // but we don't know how much further ahead it can go.
>    CSSRect GetExpandedScrollableRect() const
>    {
> +    CSSRect cssCompositionBounds = mCompositionBounds / mZoom;

This should probably just use CalculateCompositedRectInCssPixels. Feel free to fix bug 948377 while you're at it.

::: gfx/layers/ipc/AsyncPanZoomController.cpp
@@ +1456,5 @@
>      // in some things into our local mFrameMetrics because these things are
>      // determined by Gecko and our copy in mFrameMetrics may be stale.
>      mFrameMetrics.mScrollableRect = aLayerMetrics.mScrollableRect;
> +    // We only want to use the child's composition bounds if we've no the
> +    // top level APZC.

I don't understand why you made this change. Can you explain?

Comment 4

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #2)
> Way to steal my patch. See bug 951320. We should coordinate more about what
> you're working on so this doesn't happen again.

I just stumbled across the issue so I thought I'd include the fix in my patch. It is fairly obviously wrong but perhaps it works on metro where the scale is more often 1:1.

> ::: gfx/layers/ipc/AsyncPanZoomController.cpp
> @@ +1456,5 @@
> >      // in some things into our local mFrameMetrics because these things are
> >      // determined by Gecko and our copy in mFrameMetrics may be stale.
> >      mFrameMetrics.mScrollableRect = aLayerMetrics.mScrollableRect;
> > +    // We only want to use the child's composition bounds if we've no the
> > +    // top level APZC.
>
> I don't understand why you made this change. Can you explain?

At the top level APZCTreeManager::UpdateRootCompositionBounds() sets the composition bounds based on the local tree. Right now the composition bounds are being wiped out by a old value being sent back from TabChild when a paint completes. It possibly makes more sense to move all the composition bounds logic inside APZCTreeManager rather than having it split between two classes.

Comment 5

[Kartikaya Gupta (email:[e-mail])]

(In reply to Anthony Jones (:kentuckyfriedtakahe, :k17e) from comment #4)
> At the top level APZCTreeManager::UpdateRootCompositionBounds() sets the
> composition bounds based on the local tree. Right now the composition bounds
> are being wiped out by a old value being sent back from TabChild when a
> paint completes. It possibly makes more sense to move all the composition
> bounds logic inside APZCTreeManager rather than having it split between two
> classes.

Ah. I just removed UpdateRootCompositionBounds as part of bug 950487. Now the composition bounds is only ever updated by the NotifyLayersUpdated code path.

Comment 6

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Your change seems to make the b2g bug worse. I'll have to trace the composition bounds back to see where we're getting the wrong value from.

Comment 7

[Kartikaya Gupta (email:[e-mail])]

Yeah, please do. When I removed UpdateRootCompositionBounds it was my understanding based on the code that we should always get the same composition bounds coming in on the next call to NotifyLayersUpdate so if there's a bad value sneaking in somewhere we should track it down.

Comment 8

[Kartikaya Gupta (email:[e-mail])]

Note also that there is a bug on file and being worked on to fix the composition bounds calculation in RecordFrameMetrics. bug 935219.

Comment 9

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

UpdateRootCompositionBounds was giving us the right answer and RecordFrameMetrics (in nsDisplayList.cpp) is giving us the wrong answer. Fixing RecordFrameMetrics should fix this issue.

Comment 10

[Kartikaya Gupta (email:[e-mail])]

Bug 935219 is fixed now; is there anything left here that needs to be addressed?