49842 2010-11-19 16:49:49 -0800 [chromium] Accumulated scroll damage rect should be in the content space 2010-11-23 12:33:23 -0800 1 1 1 Unclassified WebKit Layout and Rendering 528+ (Nightly build) All All RESOLVED FIXED P2 Normal --- 1 klobag klobag commit-queue enne kbr nduca thakis vangelis oldest_to_newest 311755 0 klobag 2010-11-19 16:49:49 -0800 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. 311758 1 74443 klobag 2010-11-19 16:54:53 -0800 Created attachment 74443 Patch 311771 2 kbr 2010-11-19 17:31:48 -0800 Vangelis should look at this. It basically looks fine to me but I don't really know the compositor's math. 311789 3 vangelis 2010-11-19 17:56:48 -0800 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. 312415 4 nduca 2010-11-22 10:59:51 -0800 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? 312540 5 klobag 2010-11-22 14:53:19 -0800 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. 312543 6 nduca 2010-11-22 14:58:34 -0800 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. 312548 7 klobag 2010-11-22 15:06:00 -0800 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. 312557 8 nduca 2010-11-22 15:23:52 -0800 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. 312568 9 enne 2010-11-22 15:52:09 -0800 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. 312575 10 klobag 2010-11-22 16:00:28 -0800 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. 312579 11 nduca 2010-11-22 16:04:20 -0800 I suggest using the reduced patch. It feels lower risk to me. 312583 12 klobag 2010-11-22 16:11:55 -0800 Sure. I will update the new patch. (In reply to comment #11) > I suggest using the reduced patch. It feels lower risk to me. 312608 13 74615 klobag 2010-11-22 16:51:20 -0800 Created attachment 74615 new patch Per discussion, use the alternative fix. 312864 14 nduca 2010-11-23 08:34:13 -0800 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. :) 312884 15 klobag 2010-11-23 09:07:43 -0800 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. :) 312918 16 kbr 2010-11-23 10:43:20 -0800 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?)? 312924 17 klobag 2010-11-23 10:49:57 -0800 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?)? 312939 18 74615 kbr 2010-11-23 11:21:04 -0800 Comment on attachment 74615 new patch r=me 312967 19 74615 commit-queue 2010-11-23 12:33:18 -0800 Comment on attachment 74615 new patch Clearing flags on attachment: 74615 Committed r72620: <http://trac.webkit.org/changeset/72620> 312968 20 commit-queue 2010-11-23 12:33:23 -0800 All reviewed patches have been landed. Closing bug. 74443 2010-11-19 16:54:53 -0800 2010-11-23 10:42:14 -0800 Patch scroll.txt text/plain 3451 klobag SW5kZXg6IFdlYktpdC9jaHJvbWl1bS9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gV2ViS2l0L2No cm9taXVtL0NoYW5nZUxvZwkocmV2aXNpb24gNzI0NTMpCisrKyBXZWJLaXQvY2hyb21pdW0vQ2hh bmdlTG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMgKzEsMTQgQEAKKzIwMTAtMTEtMTkgIEdyYWNl IEtsb2JhICA8a2xvYmFnQGNocm9taXVtLm9yZz4KKworICAgICAgICBSZXZpZXdlZCBieSBOT0JP RFkgKE9PUFMhKS4KKworICAgICAgICBbY2hyb21pdW1dIEFjY3VtdWxhdGVkIHNjcm9sbCBkYW1h Z2UgcmVjdCBzaG91bGQgYmUgaW4gdGhlIGNvbnRlbnQgc3BhY2UKKyAgICAgICAgaHR0cHM6Ly9i dWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTQ5ODQyCisKKyAgICAgICAgKiBzcmMvV2Vi Vmlld0ltcGwuY3BwOgorICAgICAgICAoV2ViS2l0OjpXZWJWaWV3SW1wbDo6c2Nyb2xsUm9vdExh eWVyUmVjdCk6VXNlIGNvbnRlbnQgcmVjdCB0byBjYWxjdWxhdGUgbV9yb290TGF5ZXJTY3JvbGxE YW1hZ2UKKyAgICAgICAgKFdlYktpdDo6V2ViVmlld0ltcGw6OmRvQ29tcG9zaXRlKTogQ29udmVy dCBtX3Jvb3RMYXllclNjcm9sbERhbWFnZSB0byBzY3JlZW4gc3BhY2UuCisKIDIwMTAtMTEtMTkg IElseWEgU2hlcm1hbiAgPGlzaGVybWFuQGNocm9taXVtLm9yZz4KIAogICAgICAgICBSZXZpZXdl ZCBieSBEYXJpbiBGaXNoZXIuCkluZGV4OiBXZWJLaXQvY2hyb21pdW0vc3JjL1dlYlZpZXdJbXBs LmNwcAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09Ci0tLSBXZWJLaXQvY2hyb21pdW0vc3JjL1dlYlZpZXdJbXBsLmNwcAko cmV2aXNpb24gNzI0NDIpCisrKyBXZWJLaXQvY2hyb21pdW0vc3JjL1dlYlZpZXdJbXBsLmNwcAko d29ya2luZyBjb3B5KQpAQCAtMjI4MiwxMCArMjI4MiwxMCBAQCB2b2lkIFdlYlZpZXdJbXBsOjpz Y3JvbGxSb290TGF5ZXJSZWN0KGNvCiAgICAgICAgIHJldHVybjsKIAogICAgIEludFJlY3QgY29u dGVudFJlY3QgPSB2aWV3LT52aXNpYmxlQ29udGVudFJlY3QoZmFsc2UpOwotICAgIEludFJlY3Qg c2NyZWVuUmVjdCA9IHZpZXctPmNvbnRlbnRzVG9XaW5kb3coY29udGVudFJlY3QpOwogCiAgICAg Ly8gV2Ugc3VwcG9ydCBmYXN0IHNjcm9sbGluZyBpbiBvbmUgZGlyZWN0aW9uIGF0IGEgdGltZS4K ICAgICBpZiAoc2Nyb2xsRGVsdGEud2lkdGgoKSAmJiBzY3JvbGxEZWx0YS5oZWlnaHQoKSkgewor ICAgICAgICBJbnRSZWN0IHNjcmVlblJlY3QgPSB2aWV3LT5jb250ZW50c1RvV2luZG93KGNvbnRl bnRSZWN0KTsKICAgICAgICAgaW52YWxpZGF0ZVJvb3RMYXllclJlY3QoV2ViUmVjdChzY3JlZW5S ZWN0KSk7CiAgICAgICAgIHJldHVybjsKICAgICB9CkBAIC0yMjk2LDI0ICsyMjk2LDI0IEBAIHZv aWQgV2ViVmlld0ltcGw6OnNjcm9sbFJvb3RMYXllclJlY3QoY28KICAgICBJbnRSZWN0IGRhbWFn ZWRDb250ZW50c1JlY3Q7CiAgICAgaWYgKHNjcm9sbERlbHRhLndpZHRoKCkpIHsKICAgICAgICAg aW50IGR4ID0gc2Nyb2xsRGVsdGEud2lkdGgoKTsKLSAgICAgICAgZGFtYWdlZENvbnRlbnRzUmVj dC5zZXRZKHNjcmVlblJlY3QueSgpKTsKLSAgICAgICAgZGFtYWdlZENvbnRlbnRzUmVjdC5zZXRI ZWlnaHQoc2NyZWVuUmVjdC5oZWlnaHQoKSk7CisgICAgICAgIGRhbWFnZWRDb250ZW50c1JlY3Qu c2V0WShjb250ZW50UmVjdC55KCkpOworICAgICAgICBkYW1hZ2VkQ29udGVudHNSZWN0LnNldEhl aWdodChjb250ZW50UmVjdC5oZWlnaHQoKSk7CiAgICAgICAgIGlmIChkeCA+IDApIHsKLSAgICAg ICAgICAgIGRhbWFnZWRDb250ZW50c1JlY3Quc2V0WChzY3JlZW5SZWN0LngoKSk7CisgICAgICAg ICAgICBkYW1hZ2VkQ29udGVudHNSZWN0LnNldFgoY29udGVudFJlY3QueCgpKTsKICAgICAgICAg ICAgIGRhbWFnZWRDb250ZW50c1JlY3Quc2V0V2lkdGgoZHgpOwogICAgICAgICB9IGVsc2Ugewot ICAgICAgICAgICAgZGFtYWdlZENvbnRlbnRzUmVjdC5zZXRYKHNjcmVlblJlY3QucmlnaHQoKSAr IGR4KTsKKyAgICAgICAgICAgIGRhbWFnZWRDb250ZW50c1JlY3Quc2V0WChjb250ZW50UmVjdC5y aWdodCgpICsgZHgpOwogICAgICAgICAgICAgZGFtYWdlZENvbnRlbnRzUmVjdC5zZXRXaWR0aCgt ZHgpOwogICAgICAgICB9CiAgICAgfSBlbHNlIHsKICAgICAgICAgaW50IGR5ID0gc2Nyb2xsRGVs dGEuaGVpZ2h0KCk7Ci0gICAgICAgIGRhbWFnZWRDb250ZW50c1JlY3Quc2V0WChzY3JlZW5SZWN0 LngoKSk7Ci0gICAgICAgIGRhbWFnZWRDb250ZW50c1JlY3Quc2V0V2lkdGgoc2NyZWVuUmVjdC53 aWR0aCgpKTsKKyAgICAgICAgZGFtYWdlZENvbnRlbnRzUmVjdC5zZXRYKGNvbnRlbnRSZWN0Lngo KSk7CisgICAgICAgIGRhbWFnZWRDb250ZW50c1JlY3Quc2V0V2lkdGgoY29udGVudFJlY3Qud2lk dGgoKSk7CiAgICAgICAgIGlmIChkeSA+IDApIHsKLSAgICAgICAgICAgIGRhbWFnZWRDb250ZW50 c1JlY3Quc2V0WShzY3JlZW5SZWN0LnkoKSk7CisgICAgICAgICAgICBkYW1hZ2VkQ29udGVudHNS ZWN0LnNldFkoY29udGVudFJlY3QueSgpKTsKICAgICAgICAgICAgIGRhbWFnZWRDb250ZW50c1Jl Y3Quc2V0SGVpZ2h0KGR5KTsKICAgICAgICAgfSBlbHNlIHsKLSAgICAgICAgICAgIGRhbWFnZWRD b250ZW50c1JlY3Quc2V0WShzY3JlZW5SZWN0LmJvdHRvbSgpICsgZHkpOworICAgICAgICAgICAg ZGFtYWdlZENvbnRlbnRzUmVjdC5zZXRZKGNvbnRlbnRSZWN0LmJvdHRvbSgpICsgZHkpOwogICAg ICAgICAgICAgZGFtYWdlZENvbnRlbnRzUmVjdC5zZXRIZWlnaHQoLWR5KTsKICAgICAgICAgfQog ICAgIH0KQEAgLTI0NjAsNyArMjQ2MCw5IEBAIHZvaWQgV2ViVmlld0ltcGw6OmRvQ29tcG9zaXRl KCkKIAogICAgIC8vIERyYXcgdGhlIGNvbnRlbnRzIG9mIHRoZSByb290IGxheWVyLgogICAgIFZl Y3RvcjxJbnRSZWN0PiBkYW1hZ2VSZWN0czsKLSAgICBkYW1hZ2VSZWN0cy5hcHBlbmQobV9yb290 TGF5ZXJTY3JvbGxEYW1hZ2UpOworICAgIC8vIGRhbWFnZVJlY3RzIHNob3VsZCBiZSBpbiB0aGUg c2NyZWVuIHNwYWNlLCB3aGlsZSB0aGUgYWNjdW11bGF0ZWQgbV9yb290TGF5ZXJTY3JvbGxEYW1h Z2UKKyAgICAvLyBpcyBpbiB0aGUgY29udGVudCBzcGFjZS4KKyAgICBkYW1hZ2VSZWN0cy5hcHBl bmQodmlldy0+Y29udGVudHNUb1dpbmRvdyhtX3Jvb3RMYXllclNjcm9sbERhbWFnZSkpOwogICAg IGRhbWFnZVJlY3RzLmFwcGVuZChtX3Jvb3RMYXllckRpcnR5UmVjdCk7CiAgICAgZm9yIChzaXpl X3QgaSA9IDA7IGkgPCBkYW1hZ2VSZWN0cy5zaXplKCk7ICsraSkgewogICAgICAgICBJbnRSZWN0 IGRhbWFnZWRSZWN0ID0gZGFtYWdlUmVjdHNbaV07Cg== 74615 2010-11-22 16:51:20 -0800 2010-11-23 12:33:17 -0800 new patch scroll2.txt text/plain 1303 klobag SW5kZXg6IFdlYktpdC9jaHJvbWl1bS9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gV2ViS2l0L2No cm9taXVtL0NoYW5nZUxvZwkocmV2aXNpb24gNzI1NzMpCisrKyBXZWJLaXQvY2hyb21pdW0vQ2hh bmdlTG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMgKzEsMTMgQEAKKzIwMTAtMTEtMjIgIEdyYWNl IEtsb2JhICA8a2xvYmFnQGNocm9taXVtLm9yZz4KKworICAgICAgICBSZXZpZXdlZCBieSBOT0JP RFkgKE9PUFMhKS4KKworICAgICAgICBbY2hyb21pdW1dIEFjY3VtdWxhdGVkIHNjcm9sbCBkYW1h Z2UgcmVjdCBzaG91bGQgYmUgaW4gdGhlIGNvbnRlbnQgc3BhY2UKKyAgICAgICAgaHR0cHM6Ly9i dWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTQ5ODQyCisKKyAgICAgICAgKiBzcmMvV2Vi Vmlld0ltcGwuY3BwOgorICAgICAgICAoV2ViS2l0OjpXZWJWaWV3SW1wbDo6c2Nyb2xsUm9vdExh eWVyUmVjdCk6IEFkanVzdCB0aGUgcHJldmlvdXMgc2Nyb2xsIGRhbWFnZSB3aXRoIHRoZSBzY3Jv bGwgZGVsdGEgYmVmb3JlIHVuaW9uIGl0IHdpdGggdGhlIG5ldyBkYW1hZ2UgcmVjdC4KKwogMjAx MC0xMS0yMiAgVG9ueSBDaGFuZyAgPHRvbnlAY2hyb21pdW0ub3JnPgogCiAgICAgICAgIFVucmV2 aWV3ZWQsIHJvbGwgQ2hyb21pdW0gREVQUyB0byByNjcwMDQuCkluZGV4OiBXZWJLaXQvY2hyb21p dW0vc3JjL1dlYlZpZXdJbXBsLmNwcAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBXZWJLaXQvY2hyb21pdW0vc3Jj L1dlYlZpZXdJbXBsLmNwcAkocmV2aXNpb24gNzI1NzEpCisrKyBXZWJLaXQvY2hyb21pdW0vc3Jj L1dlYlZpZXdJbXBsLmNwcAkod29ya2luZyBjb3B5KQpAQCAtMjMxOCw2ICsyMzE4LDkgQEAgdm9p ZCBXZWJWaWV3SW1wbDo6c2Nyb2xsUm9vdExheWVyUmVjdChjbwogICAgICAgICB9CiAgICAgfQog CisgICAgLy8gTW92ZSB0aGUgcHJldmlvdXMgZGFtYWdlCisgICAgbV9yb290TGF5ZXJTY3JvbGxE YW1hZ2UubW92ZShzY3JvbGxEZWx0YS53aWR0aCgpLCBzY3JvbGxEZWx0YS5oZWlnaHQoKSk7Cisg ICAgLy8gVW5pb24gd2l0aCB0aGUgbmV3IGRhbWFnZSByZWN0LgogICAgIG1fcm9vdExheWVyU2Ny b2xsRGFtYWdlLnVuaXRlKGRhbWFnZWRDb250ZW50c1JlY3QpOwogCiAgICAgLy8gU2Nyb2xsIGFu eSBleGlzdGluZyBkYW1hZ2UgdGhhdCBpbnRlcnNlY3RzIHdpdGggY2xpcCByZWN0Cg==