Make aborted commits inform the scroll delegate.

Make aborted commits inform the scroll delegate.

This way of writing it was considered in
https://codereview.chromium.org/19106007#msg30 but the direct
scroll_delta_ was felt to be slightly cleaner and with no behavior
difference unless "you had some weird stateful scroll offset delegate."
Pinch viewport mode indeed introduced state inside
LayerScrollOffsetDelegateProxy to buffer the values provided by the
inner and outer viewports before calling the delegate, so this is now
more correct.  Specifically, it's now necessary to echo back to the
delegate the values forced by its getter, or they may be clobbered with
older values later.

NOTRY=true
BUG=426891
Committed: https://crrev.com/431583b93d7893784b2d5f28e80037ab9522207e
Cr-Commit-Position: refs/heads/master@{#302711}

[aelias_OOO_until_Jul13, 2014-10-29 22:32:03]

aelias@chromium.org changed reviewers:
+ bokan@chromium.org, mkosiba@chromium.org

[aelias_OOO_until_Jul13, 2014-10-29 22:40:20]

PTAL.

[bokan, 2014-10-29 23:55:42]

https://codereview.chromium.org/665233004/diff/20001/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/665233004/diff/20001/cc/layers/layer_impl.cc#...
cc/layers/layer_impl.cc:407: ScrollDelta() - sent_scroll_delta_);
SetScrollOffsetAndDelta sets the unsent delta on the twin layer in the pending
tree, will that now remove the sent delta twice?

[aelias_OOO_until_Jul13, 2014-10-30 01:46:55]

https://codereview.chromium.org/665233004/diff/20001/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/665233004/diff/20001/cc/layers/layer_impl.cc#...
cc/layers/layer_impl.cc:407: ScrollDelta() - sent_scroll_delta_);
On 2014/10/29 23:55:42, bokan wrote:
> SetScrollOffsetAndDelta sets the unsent delta on the twin layer in the pending
> tree, will that now remove the sent delta twice?

Empirically I don't see any bugs if I try scrolling on continuously invalidating
pages.  It seems to be the case that a pending tree never exists when a commit
is aborted, otherwise strangeness would ensue already since nothing in the
commit abort flow updates the pending tree.  Enne, is that a safe assumption?

[mkosiba (inactive), 2014-10-30 08:52:48]

lgtm

[bokan, 2014-10-30 16:55:57]

Ok, perhaps we don't do impl-side painting on android webview?  lgtm.

[danakj, 2014-10-30 16:56:55]

On Thu, Oct 30, 2014 at 12:55 PM, <bokan@chromium.org> wrote:

> Ok, perhaps we don't do impl-side painting on android webview?  lgtm.
>

Huh? We do.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[aelias_OOO_until_Jul13, 2014-10-30 20:44:27]

Yes, WebView uses impl-side painting.  I'm just making the claim that in the
specific case of an aborted commit, there wouldn't be a pending tree.

Looks like enne is on vacation.  Dana, do you understand aborted commit
semantics to answer the question of whether that's always true?

[danakj, 2014-10-30 21:00:04]

On Thu, Oct 30, 2014 at 4:44 PM, <aelias@chromium.org> wrote:

> Yes, WebView uses impl-side painting.  I'm just making the claim that in
> the
> specific case of an aborted commit, there wouldn't be a pending tree.
>
> Looks like enne is on vacation.  Dana, do you understand aborted commit
> semantics to answer the question of whether that's always true?
>

+brianderson in case.

I don't think that's something we can guarantee.

There was a time when we did not send BeginFrame to the main thread while
there was a pending tree. We did this because we can't /commit/ until the
pending tree is gone.

However with scheduler work they've done a lot to try make things overlap
more. I believe there are cases where we will send BeginFrame to the main
thread while there is a pending tree. We will still not commit until the
pending tree is gone, however.

But when you say aborted commit, I think you mean aborted BeginFrame,
because a commit must always complete once BeginFrameComplete succeeds (the
main thread blocks on it).

Take a look at SchedulerStateMachine::ShouldSendBeginMainFrame. That's
where we decide if we can send a BeginFrame to the main thread.

  if (impl_latency_takes_priority_ &&
      (has_pending_tree_ || active_tree_needs_first_draw_)) {
    return false;
  }

That used to be if has_pending_tree then return false. Now we can still
return true if impl_latency_takes_priority_ is true. Ie when we're not
in SMOOTHNESS_TAKES_PRIORITY we try to BeginFrame sooner.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[danakj, 2014-10-30 21:01:13]

On Thu, Oct 30, 2014 at 4:59 PM, Dana Jansens <danakj@chromium.org> wrote:

> On Thu, Oct 30, 2014 at 4:44 PM, <aelias@chromium.org> wrote:
>
>> Yes, WebView uses impl-side painting.  I'm just making the claim that in
>> the
>> specific case of an aborted commit, there wouldn't be a pending tree.
>>
>> Looks like enne is on vacation.  Dana, do you understand aborted commit
>> semantics to answer the question of whether that's always true?
>>
>
> +brianderson in case.
>
> I don't think that's something we can guarantee.
>
> There was a time when we did not send BeginFrame to the main thread while
> there was a pending tree. We did this because we can't /commit/ until the
> pending tree is gone.
>
> However with scheduler work they've done a lot to try make things overlap
> more. I believe there are cases where we will send BeginFrame to the main
> thread while there is a pending tree. We will still not commit until the
> pending tree is gone, however.
>
> But when you say aborted commit, I think you mean aborted BeginFrame,
> because a commit must always complete once BeginFrameComplete succeeds (the
> main thread blocks on it).
>
> Take a look at SchedulerStateMachine::ShouldSendBeginMainFrame. That's
> where we decide if we can send a BeginFrame to the main thread.
>
>   if (impl_latency_takes_priority_ &&
>       (has_pending_tree_ || active_tree_needs_first_draw_)) {
>     return false;
>   }
>
> That used to be if has_pending_tree then return false. Now we can still
> return true if impl_latency_takes_priority_ is true. Ie when we're not
> in SMOOTHNESS_TAKES_PRIORITY we try to BeginFrame sooner.
>

Mmh. I mean " Now we can still return true if impl_latency_takes_priority_
is false."

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[aelias_OOO_until_Jul13, 2014-10-31 20:10:14]

I think my assumption of absence of pending tree is correct then, because
impl-scrolls push us into SMOOTHNESS_TAKES_PRIOITY.

If this were not the case, then this implies CC has a preexisting
double-scrolling glitch (which doesn't require aborted commits to repro). 
ProcessScrollDeltas is called on each BeginFrame, so if there exists a scroll
delta on the active tree, it will get sent twice in this scenario (since
sent_scroll_delta is only subtracted on activation).

It does seem that SMOOTHNESS_TAKES_PRIORITY protecting us from this bug is maybe
a lucky side effect rather than a conscious design decision.  Maybe this should
be revisited to be enforced in a more formalized way.  Either way, for the
purposes of this particular patch, I think we need to keep assuming the absence
of pending tree.  (I can't come up with anything saner to do with respect to
sent_scroll_delta application on abort, because the values would already be
busted before this even happened.)

[danakj, 2014-10-31 20:38:24]

On Fri, Oct 31, 2014 at 4:10 PM, <aelias@chromium.org> wrote:

> I think my assumption of absence of pending tree is correct then, because
> impl-scrolls push us into SMOOTHNESS_TAKES_PRIOITY.
>
> If this were not the case, then this implies CC has a preexisting
> double-scrolling glitch (which doesn't require aborted commits to repro).
> ProcessScrollDeltas is called on each BeginFrame, so if there exists a
> scroll
> delta on the active tree, it will get sent twice in this scenario (since
> sent_scroll_delta is only subtracted on activation).
>
> It does seem that SMOOTHNESS_TAKES_PRIORITY protecting us from this bug is
> maybe
> a lucky side effect rather than a conscious design decision.  Maybe this
> should
> be revisited to be enforced in a more formalized way.  Either way, for the
> purposes of this particular patch, I think we need to keep assuming the
> absence
> of pending tree.  (I can't come up with anything saner to do with respect
> to
> sent_scroll_delta application on abort, because the values would already be
> busted before this even happened.)
>

OK Cool, if it's true today, then using that assumption is fine as long as
it is well guarded by a unit test (or dchecks if that's not possible but it
should be).

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[aelias_OOO_until_Jul13, 2014-11-04 06:43:15]

OK, I added a DCHECK.  After thinking about it a bit, I think that's more
appropriate than a unit test here because I can't write a test case for every
hypothetical scheduling scenario that might create a pending tree, particularly
since they would get created by unknown future scheduler changes.

[danakj, 2014-11-04 15:55:00]

On Tue, Nov 4, 2014 at 1:43 AM, <aelias@chromium.org> wrote:

> OK, I added a DCHECK.  After thinking about it a bit, I think that's more
> appropriate than a unit test here because I can't write a test case for
> every
> hypothetical scheduling scenario that might create a pending tree,
> particularly
> since they would get created by unknown future scheduler changes.
>

I'm not sure what you mean here, a pending tree is only created by starting
a commit. Scheduler may make many decisions about when to start a commit
and when to not, but it's always a commit happening that makes a pending
tree. Or can you not isolate that in a test for this?

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[aelias_OOO_until_Jul13, 2014-11-04 20:53:33]

Right, I mean that many potential sequences of events might start a commit, then
start an aborted commit before the activation occurs.  I'm trying to prove the
negative that this sequence doesn't happen; I can't do that by enumerating
scenarios.

[danakj, 2014-11-04 21:11:35]

https://codereview.chromium.org/665233004/diff/80001/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/665233004/diff/80001/cc/layers/layer_impl.cc#...
cc/layers/layer_impl.cc:403: // The pending tree shouldn't exist during aborted
commits; we don't know how
Can you maybe say the pending tree shouldn't exist during aborted commits while
doing an impl scroll?

Should there be an early out if the sent delta is empty? Won't we try apply an
empty scroll delta here for every aborted beginmainframe?

[aelias_OOO_until_Jul13, 2014-11-04 21:19:16]

https://codereview.chromium.org/665233004/diff/80001/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/665233004/diff/80001/cc/layers/layer_impl.cc#...
cc/layers/layer_impl.cc:403: // The pending tree shouldn't exist during aborted
commits; we don't know how
On 2014/11/04 at 21:11:34, danakj wrote:
> Can you maybe say the pending tree shouldn't exist during aborted commits
while doing an impl scroll?
> 
> Should there be an early out if the sent delta is empty? Won't we try apply an
empty scroll delta here for every aborted beginmainframe?

Good idea.  I added an early return and made the comment more specific.

[enne (OOO), 2014-11-04 23:31:48]

enne@chromium.org changed reviewers:
+ enne@chromium.org

[enne (OOO), 2014-11-04 23:31:49]

lgtm

https://codereview.chromium.org/665233004/diff/80001/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/665233004/diff/80001/cc/layers/layer_impl.cc#...
cc/layers/layer_impl.cc:405:
DCHECK(!layer_tree_impl()->FindPendingTreeLayerById(id()));
Yeah, by definition there is no pending tree during an aborted commit, since a
commit is what creates a pending tree.  Even in the future when we send begin
main frame messages while there is a pending tree, the commit itself cannot
happen until the pending tree activates and a new pending tree is created during
the commit.

This DCHECK should really just check that the pending tree doesn't exist at all,
rather than the pending layer by id.

[aelias_OOO_until_Jul13, 2014-11-04 23:35:00]

https://codereview.chromium.org/665233004/diff/80001/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/665233004/diff/80001/cc/layers/layer_impl.cc#...
cc/layers/layer_impl.cc:405:
DCHECK(!layer_tree_impl()->FindPendingTreeLayerById(id()));
On 2014/11/04 23:31:49, enne wrote:
> Yeah, by definition there is no pending tree during an aborted commit, since a
> commit is what creates a pending tree.  Even in the future when we send begin
> main frame messages while there is a pending tree, the commit itself cannot
> happen until the pending tree activates and a new pending tree is created
during
> the commit.
> 
> This DCHECK should really just check that the pending tree doesn't exist at
all,
> rather than the pending layer by id.

Thanks for the clarification.  I wanted to check for the pending tree itself but
didn't find a public method for that and it didn't seem like I should add one
for a DCHECK.

[aelias_OOO_until_Jul13, 2014-11-04 23:41:50]

The CQ bit was checked by aelias@chromium.org

[commit-bot: I haz the power, 2014-11-04 23:44:10]

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

[commit-bot: I haz the power, 2014-11-04 23:45:42]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2014-11-04 23:48:06]

Patchset 6 (id:??) landed as
https://crrev.com/431583b93d7893784b2d5f28e80037ab9522207e
Cr-Commit-Position: refs/heads/master@{#302711}