Bug 49842 – [chromium] Accumulated scroll damage rect should be in the content space

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Grace Kloba]

The accumulated scroll damage rect in WebViewImpl::scrollRootLayerRect() should be in the content space. Otherwise when scrollRootLayerRect() is called more frequent than the scroll damage rect is consumed in doComposite(), we will have garbage in the final result. e.g. if scrollRootLayerRect() is called twice with (10, 0) before doComposite() is called, the accumulated screen rect will have width of 10 while the expected width should be 20.

[Grace Kloba]

Created attachment 74443 [details]
Patch

[Kenneth Russell]

Vangelis should look at this. It basically looks fine to me but I don't really know the compositor's math.

[Vangelis Kokkevis]

I think I agree with you but I'm cc-ing nduca as well who I know spent a bunch of time getting this working just in case he feels otherwise.

[Nat Duca]

I'm a bit confused, actually. This is basically the undo of 46864... can you take a peek at that bug and let me know what you think?

[Grace Kloba]

I don't think this is a fully undo of 46864 (cute bug number :)).

My understanding of 46864 is to fix the scrollbar redraw issue. Scroll bar likes fixed position object. Its invalidation rect is set through WebViewImpl::invalidateRootLayerRect() and it is in the screen coordinate. And we want to keep it that way. That is what the CL for 46864 needs to do.

But the scroll damage rect should be in the content space. e.g. if we scroll (0, 10) twice, their screen rect will be same while the content rect will be different. So I think scroll damage rect should stay in the content space to get the correct invalidation rect.

[Nat Duca]

What about a simpler fix:
    m_rootLayerScrollDamage.move(scrollDelta.width(), scrollDelta.height());
    m_rootLayerScrollDamage.unite(damagedContentsRect);

Doesn't this do the same thing?

(In reply to comment #5)
> I don't think this is a fully undo of 46864 (cute bug number :)).
> 
> My understanding of 46864 is to fix the scrollbar redraw issue. Scroll bar likes fixed position object. Its invalidation rect is set through WebViewImpl::invalidateRootLayerRect() and it is in the screen coordinate. And we want to keep it that way. That is what the CL for 46864 needs to do.
> 
> But the scroll damage rect should be in the content space. e.g. if we scroll (0, 10) twice, their screen rect will be same while the content rect will be different. So I think scroll damage rect should stay in the content space to get the correct invalidation rect.

[Grace Kloba]

I think your proposal will work. But it seems not as clean as the other way.

One proposal, m_rootLayerScrollDamage accumulated in the content space.

Another proposal, m_rootLayerScrollDamage is mapped to the screen space. When it needs to be accumulated, the previous one will be first shifted to compensate the new scroll offset, and then do union.

I prefer one, but I am ok to do two if it is what preferred.

(In reply to comment #6)
> What about a simpler fix:
>     m_rootLayerScrollDamage.move(scrollDelta.width(), scrollDelta.height());
>     m_rootLayerScrollDamage.unite(damagedContentsRect);
> 
> Doesn't this do the same thing?
> 
> (In reply to comment #5)
> > I don't think this is a fully undo of 46864 (cute bug number :)).
> > 
> > My understanding of 46864 is to fix the scrollbar redraw issue. Scroll bar likes fixed position object. Its invalidation rect is set through WebViewImpl::invalidateRootLayerRect() and it is in the screen coordinate. And we want to keep it that way. That is what the CL for 46864 needs to do.
> > 
> > But the scroll damage rect should be in the content space. e.g. if we scroll (0, 10) twice, their screen rect will be same while the content rect will be different. So I think scroll damage rect should stay in the content space to get the correct invalidation rect.

[Nat Duca]

I totally agree that the two are equivalent. :)

From my perspective, its up to you to make the call, I don't feel very strongly.

A few thoughts to help with your decision making...
1) I think we should be consistent about trakcing all types of damage --- etiher all in screenspace or all in content space.

2) The tiling work is going to rip this code to shreds since it needs to track in content space. I'm not sure how far along Enne is on that work, but if you do make the switch, it would be good to coordinate with them.


- N

(In reply to comment #7)
> I think your proposal will work. But it seems not as clean as the other way.
> 
> One proposal, m_rootLayerScrollDamage accumulated in the content space.
> 
> Another proposal, m_rootLayerScrollDamage is mapped to the screen space. When it needs to be accumulated, the previous one will be first shifted to compensate the new scroll offset, and then do union.
> 
> I prefer one, but I am ok to do two if it is what preferred.
> 
> (In reply to comment #6)
> > What about a simpler fix:
> >     m_rootLayerScrollDamage.move(scrollDelta.width(), scrollDelta.height());
> >     m_rootLayerScrollDamage.unite(damagedContentsRect);
> > 
> > Doesn't this do the same thing?
> > 
> > (In reply to comment #5)
> > > I don't think this is a fully undo of 46864 (cute bug number :)).
> > > 
> > > My understanding of 46864 is to fix the scrollbar redraw issue. Scroll bar likes fixed position object. Its invalidation rect is set through WebViewImpl::invalidateRootLayerRect() and it is in the screen coordinate. And we want to keep it that way. That is what the CL for 46864 needs to do.
> > > 
> > > But the scroll damage rect should be in the content space. e.g. if we scroll (0, 10) twice, their screen rect will be same while the content rect will be different. So I think scroll damage rect should stay in the content space to get the correct invalidation rect.

[Adrienne Walker]

To address Nat's comments, I don't know if we can be consistent.  The content is in screen space, but the scrollbars are always in content space.  I suspect that higher-level API refactoring might be needed to have this be more sane.

Also, my tiling patch should refactor this function quite a bit and no longer does invalidations based on scrolling.

[Grace Kloba]

It sounds like this will be changed when tiling is landed.

Can I check this is as it is?

(In reply to comment #9)
> To address Nat's comments, I don't know if we can be consistent.  The content is in screen space, but the scrollbars are always in content space.  I suspect that higher-level API refactoring might be needed to have this be more sane.
> 
> Also, my tiling patch should refactor this function quite a bit and no longer does invalidations based on scrolling.

[Nat Duca]

I suggest using the reduced patch. It feels lower risk to me.

[Grace Kloba]

Sure. I will update the new patch.

(In reply to comment #11)
> I suggest using the reduced patch. It feels lower risk to me.

[Grace Kloba]

Created attachment 74615 [details]
new patch

Per discussion, use the alternative fix.

[Nat Duca]

Does this work as exptected?

=> Rect r(0,0,0,0)
=> r.move(0,-10)
=> rNew = r.union(Rect(10,10,10,10))
=> Assert(rNew == Rect(10,10,10,10))

Otherwise, lgtm. :)

[Grace Kloba]

It should. In IntRect.unite(), it has the special code to handle empty case.

    // Handle empty special cases first.
    if (other.isEmpty())
        return;
    if (isEmpty()) {
        *this = other;
        return;
    }

(In reply to comment #14)
> Does this work as exptected?
> 
> => Rect r(0,0,0,0)
> => r.move(0,-10)
> => rNew = r.union(Rect(10,10,10,10))
> => Assert(rNew == Rect(10,10,10,10))
> 
> Otherwise, lgtm. :)

[Kenneth Russell]

I'm assuming that the new patch above is complete. Is that correct? If so, I'm happy to r+ it; should I also submit it to the commit queue (you should usually request cq?)?

[Grace Kloba]

Yes. It is ready to do now. Thanks for submitting it.

(In reply to comment #16)
> I'm assuming that the new patch above is complete. Is that correct? If so, I'm happy to r+ it; should I also submit it to the commit queue (you should usually request cq?)?

[Kenneth Russell]

Comment on attachment 74615 [details]
new patch

r=me

[WebKit Commit Bot]

Comment on attachment 74615 [details]
new patch

Clearing flags on attachment: 74615

Committed r72620: <http://trac.webkit.org/changeset/72620>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.