Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(336)

Issue 747903005: Pass integer scroll position to CC if frameView has non-layered viewport constrained objects (Closed)

Created:
6 years ago by Yufeng Shen (Slow to review)
Modified:
6 years ago
CC:
blink-layers+watch_chromium.org, blink-reviews, blink-reviews-rendering, eae+blinkwatch, eae, jchaffraix+rendering, kenneth.christiansen, leviw+renderwatch, pdr+renderingwatchlist_chromium.org, zoltan1
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Project:
blink
Visibility:
Public.

Description

Pass integer scroll position to CC if frameView has non-layered viewport constrained objects Non-layered Viewport constrained objects, e.g. fixed position elements, are not composited and are positioned in Blink using integer coordinates. In that case, we don't want to set the WebLayer's scroll position at fractional precision otherwise the WebLayer's position after snapping to device pixel can be off with regard to fixed position elements. BUG=414283 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=187073

Patch Set 1 #

Patch Set 2 : better formmating #

Patch Set 3 : ignore invisible fixed position elements #

Patch Set 4 : remove debugging #

Patch Set 5 : call updateDescendantDependentFlags() before calling hasVisibleContent() otherwise it crashes #

Total comments: 1

Patch Set 6 : use HasNonLayerViewportConstrainedObjects #

Patch Set 7 : use preferCompositingToLCDTextEnabled() to gauge sending integer/floating point scroll offset to cc #

Patch Set 8 : restore to using ScrollingCoordinator::HasNonLayer #

Unified diffs Side-by-side diffs Delta from patch set Stats (+47 lines, -3 lines) Patch
M Source/core/page/scrolling/ScrollingCoordinator.h View 1 2 3 4 5 7 1 chunk +0 lines, -1 line 0 comments Download
M Source/core/page/scrolling/ScrollingCoordinator.cpp View 1 2 3 4 5 7 2 chunks +10 lines, -2 lines 0 comments Download
M Source/web/tests/ScrollingCoordinatorChromiumTest.cpp View 1 2 3 4 5 1 chunk +21 lines, -0 lines 0 comments Download
A Source/web/tests/data/fractional-scroll-fixed-position.html View 1 2 3 4 5 1 chunk +16 lines, -0 lines 0 comments Download

Messages

Total messages: 36 (5 generated)
Yufeng Shen (Slow to review)
6 years ago (2014-12-03 01:05:17 UTC) #2
Yufeng Shen (Slow to review)
+ skobes@.
6 years ago (2014-12-03 18:46:03 UTC) #4
Rick Byers
+vollick who may be able to provide some better guidance here. As it is now, ...
6 years ago (2014-12-03 18:47:34 UTC) #6
Yufeng Shen (Slow to review)
On 2014/12/03 18:47:34, Rick Byers wrote: > +vollick who may be able to provide some ...
6 years ago (2014-12-03 19:13:37 UTC) #7
Rick Byers
On 2014/12/03 19:13:37, Yufeng Shen wrote: > On 2014/12/03 18:47:34, Rick Byers wrote: > > ...
6 years ago (2014-12-03 22:37:17 UTC) #8
Rick Byers
Here's one idea for unifying the logic with ScrollingCoordinator::hasVisibleSlowRepaintViewportConstrainedObjects. WDYT? https://codereview.chromium.org/747903005/diff/80001/Source/core/page/scrolling/ScrollingCoordinator.cpp File Source/core/page/scrolling/ScrollingCoordinator.cpp (right): https://codereview.chromium.org/747903005/diff/80001/Source/core/page/scrolling/ScrollingCoordinator.cpp#newcode380 ...
6 years ago (2014-12-05 19:01:41 UTC) #9
Yufeng Shen (Slow to review)
On 2014/12/05 19:01:41, Rick Byers wrote: > Here's one idea for unifying the logic with ...
6 years ago (2014-12-09 04:10:53 UTC) #10
Rick Byers
On 2014/12/09 04:10:53, Yufeng Shen wrote: > On 2014/12/05 19:01:41, Rick Byers wrote: > > ...
6 years ago (2014-12-10 02:12:51 UTC) #11
Rick Byers
On 2014/12/10 02:12:51, Rick Byers wrote: > On 2014/12/09 04:10:53, Yufeng Shen wrote: > > ...
6 years ago (2014-12-10 02:21:35 UTC) #12
Yufeng Shen (Slow to review)
On 2014/12/10 02:21:35, Rick Byers wrote: > On 2014/12/10 02:12:51, Rick Byers wrote: > > ...
6 years ago (2014-12-10 03:07:14 UTC) #13
Rick Byers
On 2014/12/10 03:07:14, Yufeng Shen wrote: > On 2014/12/10 02:21:35, Rick Byers wrote: > > ...
6 years ago (2014-12-10 03:11:28 UTC) #14
Ian Vollick
On 2014/12/10 03:11:28, Rick Byers wrote: > On 2014/12/10 03:07:14, Yufeng Shen wrote: > > ...
6 years ago (2014-12-10 14:03:04 UTC) #15
Yufeng Shen (Slow to review)
Alex, mind taking a look at Source/web/ ?
6 years ago (2014-12-10 18:57:45 UTC) #17
aelias_OOO_until_Jul13
Are we planning on exposing fractional scrolling to web developers with this hack in place? ...
6 years ago (2014-12-10 20:33:45 UTC) #18
aelias_OOO_until_Jul13
I don't really understand how this change helps with "staging" your work on the feature ...
6 years ago (2014-12-10 20:43:47 UTC) #19
aelias_OOO_until_Jul13
OK, rereading the discussion, I realize you're actually planning to maintain this codepath for the ...
6 years ago (2014-12-10 21:03:22 UTC) #20
Yufeng Shen (Slow to review)
On 2014/12/10 20:33:45, aelias wrote: > Are we planning on exposing fractional scrolling to web ...
6 years ago (2014-12-10 21:09:24 UTC) #21
Yufeng Shen (Slow to review)
On 2014/12/10 21:03:22, aelias wrote: > OK, rereading the discussion, I realize you're actually planning ...
6 years ago (2014-12-10 21:14:35 UTC) #22
aelias_OOO_until_Jul13
Makes sense. Our last messages crossed. How about my alternative proposal in #20 to instead ...
6 years ago (2014-12-10 21:15:47 UTC) #23
aelias_OOO_until_Jul13
On 2014/12/10 21:14:35, Yufeng Shen wrote: > On 2014/12/10 21:03:22, aelias wrote: > > OK, ...
6 years ago (2014-12-10 21:18:02 UTC) #24
aelias_OOO_until_Jul13
On 2014/12/10 21:18:02, aelias wrote: > On 2014/12/10 21:14:35, Yufeng Shen wrote: > > On ...
6 years ago (2014-12-10 21:26:22 UTC) #25
Yufeng Shen (Slow to review)
6 years ago (2014-12-10 21:29:19 UTC) #26
Yufeng Shen (Slow to review)
On 2014/12/10 21:18:02, aelias wrote: > On 2014/12/10 21:14:35, Yufeng Shen wrote: > > On ...
6 years ago (2014-12-10 21:29:40 UTC) #27
Yufeng Shen (Slow to review)
On 2014/12/10 21:29:40, Yufeng Shen wrote: > On 2014/12/10 21:18:02, aelias wrote: > > On ...
6 years ago (2014-12-10 21:42:47 UTC) #28
aelias_OOO_until_Jul13
I think it's better to be consistent and predictable and draw a hard-line about the ...
6 years ago (2014-12-10 21:44:04 UTC) #29
Rick Byers
On 2014/12/10 21:44:04, aelias wrote: > I think it's better to be consistent and predictable ...
6 years ago (2014-12-10 22:01:06 UTC) #30
Ian Vollick
On 2014/12/10 22:01:06, Rick Byers wrote: > On 2014/12/10 21:44:04, aelias wrote: > > I ...
6 years ago (2014-12-11 16:44:31 UTC) #31
Yufeng Shen (Slow to review)
Alex, I changed to checking preferCompositingToLCDTextEnabled() for if we want to send integer or floating ...
6 years ago (2014-12-11 22:44:54 UTC) #32
aelias_OOO_until_Jul13
OK, you convinced me the situation with respect to fractional scroll is complicated enough already ...
6 years ago (2014-12-12 00:43:06 UTC) #33
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/747903005/140001
6 years ago (2014-12-12 23:12:29 UTC) #35
commit-bot: I haz the power
6 years ago (2014-12-13 01:48:11 UTC) #36
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=187073

Powered by Google App Engine
This is Rietveld 408576698