Fixed position layer counter-scroll with main thread scroll offset fractional part

Fixed position layer counter-scroll with main thread scroll offset fractional part

Main thread can scroll with non-integral scroll offset, but Blink currently
only handles integral part of the scroll offset. For example, if main thread
scrolls by 10.5, Blink tells CC that the scroll offset is 10 (currently truncated
inside Blink) and Blink also sets fixed-position layer's position at 10.

This CL makes the non-integral scroll offset works correctly in this case:
1. Blink can tell CC the non-integral scroll offset 10.5 (truncation inside Blink
will be removed after this CL).
2. Blink sets fixed-position layer at 10.
3. CC tracks the fractional part of the scroll offset 0.5 and applied it when
computing scroll compensation for fixed-position layer during drawing.

BUG=414283
Committed: https://crrev.com/2c78036b520cbb4ab8bf7e090208bbbe22f43cba
Cr-Commit-Position: refs/heads/master@{#313783}

[Yufeng Shen (Slow to review), 2015-01-28 01:41:13]

miletus@chromium.org changed reviewers:
+ aelias@chromium.org

[aelias_OOO_until_Jul13, 2015-01-28 02:02:15]

aelias@chromium.org changed reviewers:
+ danakj@chromium.org

[aelias_OOO_until_Jul13, 2015-01-28 02:04:00]

As part of this, would it work to remove the gfx::ToFlooredVector2d() call in
CollectScrollDeltas() in LayerTreeHostImpl?  I think this might remove the last
blocker to remove that hack (which leaves a permanent fractional part in
LayerImpl::ScrollDelta currently because Blink couldn't store fractions)?

https://codereview.chromium.org/877173002/diff/40001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/877173002/diff/40001/cc/layers/layer.cc#newco...
cc/layers/layer.cc:672: scroll_offset_fractional_part_ =
scroll_offset.DeltaFrom(
I see Dana made the comment on your doc "This seems magical in that blink
positions things at integers since it knows cc will happen to only use the
integer parts of the things it gave. It should have to be more explicit."  In
keeping with that, how about a new API "SetScrollOffsetFractionalPart" to be
called from Blink and leave SetScrollOffset unchanged?

https://codereview.chromium.org/877173002/diff/40001/cc/layers/layer_impl.h
File cc/layers/layer_impl.h (right):

https://codereview.chromium.org/877173002/diff/40001/cc/layers/layer_impl.h#n...
cc/layers/layer_impl.h:407: gfx::Vector2dF MainScrollOffsetFracitionalPart()
const {
spelling: should be "Fractional", not "Fracitional"

https://codereview.chromium.org/877173002/diff/40001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_common.cc (right):

https://codereview.chromium.org/877173002/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common.cc:50: layer->ScrollDelta() +
layer->MainScrollOffsetFracitionalPart();
Please add a comment explaining why the fractional part needs to be included
here.

[Yufeng Shen (Slow to review), 2015-01-28 04:49:23]

"
As part of this, would it work to remove the gfx::ToFlooredVector2d() call in
CollectScrollDeltas() in LayerTreeHostImpl?  I think this might remove the last
blocker to remove that hack (which leaves a permanent fractional part in
LayerImpl::ScrollDelta currently because Blink couldn't store fractions)?
"

I did a quick test and it sort of work. But:
1. there are some mechanical change to the pinch viewport scrolling code path
to support floating point scroll_offset
2. can't make pinch viewport scrolling smooth (I need more time on this)
3. allow sending small scroll_delta to main will trigger more scrolling cod
path in main/blink so it might have performance implication (while currently
only when scroll_delta is at least 1 that we send it to main)

I prefer doing this in another CL, what do you think ?

https://codereview.chromium.org/877173002/diff/40001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/877173002/diff/40001/cc/layers/layer.cc#newco...
cc/layers/layer.cc:672: scroll_offset_fractional_part_ =
scroll_offset.DeltaFrom(
On 2015/01/28 02:04:00, aelias wrote:
> I see Dana made the comment on your doc "This seems magical in that blink
> positions things at integers since it knows cc will happen to only use the
> integer parts of the things it gave. It should have to be more explicit."  In
> keeping with that, how about a new API "SetScrollOffsetFractionalPart" to be
> called from Blink and leave SetScrollOffset unchanged?

would an API that takes 2 parameters be better ?
e.g. SetScrollOffset(scroll_offset_integer_part, scroll_offset_fractional_part)

so the caller won't forget to call one without the other.

[aelias_OOO_until_Jul13, 2015-01-28 05:50:29]

OK, sounds fine to do the fractional part sendback in another patch.

https://codereview.chromium.org/877173002/diff/40001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/877173002/diff/40001/cc/layers/layer.cc#newco...
cc/layers/layer.cc:672: scroll_offset_fractional_part_ =
scroll_offset.DeltaFrom(
On 2015/01/28 04:49:23, Yufeng Shen wrote:
> On 2015/01/28 02:04:00, aelias wrote:
> > I see Dana made the comment on your doc "This seems magical in that blink
> > positions things at integers since it knows cc will happen to only use the
> > integer parts of the things it gave. It should have to be more explicit." 
In
> > keeping with that, how about a new API "SetScrollOffsetFractionalPart" to be
> > called from Blink and leave SetScrollOffset unchanged?
> 
> would an API that takes 2 parameters be better ?
> e.g. SetScrollOffset(scroll_offset_integer_part,
scroll_offset_fractional_part)
> 
> so the caller won't forget to call one without the other.
> 

I'd rather keep them separate, it makes sense not to call the fractional one if
the fixed-position problem in Blink is fixed at some point.

[Yufeng Shen (Slow to review), 2015-01-28 22:28:09]

Blink side change:

https://codereview.chromium.org/889453002/

https://codereview.chromium.org/877173002/diff/40001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/877173002/diff/40001/cc/layers/layer.cc#newco...
cc/layers/layer.cc:672: scroll_offset_fractional_part_ =
scroll_offset.DeltaFrom(
On 2015/01/28 05:50:29, aelias wrote:
> On 2015/01/28 04:49:23, Yufeng Shen wrote:
> > On 2015/01/28 02:04:00, aelias wrote:
> > > I see Dana made the comment on your doc "This seems magical in that blink
> > > positions things at integers since it knows cc will happen to only use the
> > > integer parts of the things it gave. It should have to be more explicit." 
> In
> > > keeping with that, how about a new API "SetScrollOffsetFractionalPart" to
be
> > > called from Blink and leave SetScrollOffset unchanged?
> > 
> > would an API that takes 2 parameters be better ?
> > e.g. SetScrollOffset(scroll_offset_integer_part,
> scroll_offset_fractional_part)
> > 
> > so the caller won't forget to call one without the other.
> > 
> 
> I'd rather keep them separate, it makes sense not to call the fractional one
if
> the fixed-position problem in Blink is fixed at some point.

Done.

https://codereview.chromium.org/877173002/diff/40001/cc/layers/layer_impl.h
File cc/layers/layer_impl.h (right):

https://codereview.chromium.org/877173002/diff/40001/cc/layers/layer_impl.h#n...
cc/layers/layer_impl.h:407: gfx::Vector2dF MainScrollOffsetFracitionalPart()
const {
On 2015/01/28 02:04:00, aelias wrote:
> spelling: should be "Fractional", not "Fracitional"

Done.

https://codereview.chromium.org/877173002/diff/40001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_common.cc (right):

https://codereview.chromium.org/877173002/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common.cc:50: layer->ScrollDelta() +
layer->MainScrollOffsetFracitionalPart();
On 2015/01/28 02:04:00, aelias wrote:
> Please add a comment explaining why the fractional part needs to be included
> here.

Done.

[aelias_OOO_until_Jul13, 2015-01-28 22:34:16]

https://codereview.chromium.org/877173002/diff/80001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/877173002/diff/80001/cc/layers/layer.cc#newco...
cc/layers/layer.cc:675: void Layer::SetScrollOffsetFractionalPart(
Let's not mutate scroll_offset_ in this method.  We can just do something very
simple here:

void Layer::SetScrollOffsetFractionalPart(...) {
  if (scroll_offset_fractional_part_ == scroll_offset_fractional_part)
    return;
  scroll_offset_fractional_part_ = scroll_offset_fractional_part;
  SetNeedsCommit();
}

gfx::Vector2dF Layer::MainScrollOffsetFractionalPart() const {
  return scroll_offset_fractional_part_;
}

[Yufeng Shen (Slow to review), 2015-01-28 22:49:27]

https://codereview.chromium.org/877173002/diff/80001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/877173002/diff/80001/cc/layers/layer.cc#newco...
cc/layers/layer.cc:675: void Layer::SetScrollOffsetFractionalPart(
On 2015/01/28 22:34:16, aelias wrote:
> Let's not mutate scroll_offset_ in this method.  We can just do something very
> simple here:
> 
> void Layer::SetScrollOffsetFractionalPart(...) {
>   if (scroll_offset_fractional_part_ == scroll_offset_fractional_part)
>     return;
>   scroll_offset_fractional_part_ = scroll_offset_fractional_part;
>   SetNeedsCommit();
> }
> 
> gfx::Vector2dF Layer::MainScrollOffsetFractionalPart() const {
>   return scroll_offset_fractional_part_;
> }

Done.

[aelias_OOO_until_Jul13, 2015-01-28 22:54:39]

lgtm

[Yufeng Shen (Slow to review), 2015-01-29 20:48:01]

The CQ bit was checked by miletus@chromium.org

[commit-bot: I haz the power, 2015-01-29 20:48:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/877173002/120001

[commit-bot: I haz the power, 2015-01-29 20:53:04]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-01-29 20:53:55]

Patchset 7 (id:??) landed as
https://crrev.com/2c78036b520cbb4ab8bf7e090208bbbe22f43cba
Cr-Commit-Position: refs/heads/master@{#313783}