Bug 49842

Summary: [chromium] Accumulated scroll damage rect should be in the content space
Product: WebKit Reporter: Grace Kloba <klobag>
Component: Layout and RenderingAssignee: Grace Kloba <klobag>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, enne, kbr, nduca, thakis, vangelis
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch
none
new patch none

Description Grace Kloba 2010-11-19 16:49:49 PST
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.
Comment 1 Grace Kloba 2010-11-19 16:54:53 PST
Created attachment 74443 [details]
Patch
Comment 2 Kenneth Russell 2010-11-19 17:31:48 PST
Vangelis should look at this. It basically looks fine to me but I don't really know the compositor's math.
Comment 3 Vangelis Kokkevis 2010-11-19 17:56:48 PST
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.
Comment 4 Nat Duca 2010-11-22 10:59:51 PST
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?
Comment 5 Grace Kloba 2010-11-22 14:53:19 PST
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.
Comment 6 Nat Duca 2010-11-22 14:58:34 PST
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.
Comment 7 Grace Kloba 2010-11-22 15:06:00 PST
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.
Comment 8 Nat Duca 2010-11-22 15:23:52 PST
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.
Comment 9 Adrienne Walker 2010-11-22 15:52:09 PST
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.
Comment 10 Grace Kloba 2010-11-22 16:00:28 PST
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.
Comment 11 Nat Duca 2010-11-22 16:04:20 PST
I suggest using the reduced patch. It feels lower risk to me.
Comment 12 Grace Kloba 2010-11-22 16:11:55 PST
Sure. I will update the new patch.

(In reply to comment #11)
> I suggest using the reduced patch. It feels lower risk to me.
Comment 13 Grace Kloba 2010-11-22 16:51:20 PST
Created attachment 74615 [details]
new patch

Per discussion, use the alternative fix.
Comment 14 Nat Duca 2010-11-23 08:34:13 PST
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. :)
Comment 15 Grace Kloba 2010-11-23 09:07:43 PST
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. :)
Comment 16 Kenneth Russell 2010-11-23 10:43:20 PST
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?)?
Comment 17 Grace Kloba 2010-11-23 10:49:57 PST
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?)?
Comment 18 Kenneth Russell 2010-11-23 11:21:04 PST
Comment on attachment 74615 [details]
new patch

r=me
Comment 19 WebKit Commit Bot 2010-11-23 12:33:18 PST
Comment on attachment 74615 [details]
new patch

Clearing flags on attachment: 74615

Committed r72620: <http://trac.webkit.org/changeset/72620>
Comment 20 WebKit Commit Bot 2010-11-23 12:33:23 PST
All reviewed patches have been landed.  Closing bug.