Make LayerScrollOffsetDelegate updates consistent.

Make LayerScrollOffsetDelegate updates consistent.

Looks like the inner+outer viewport changes had made the scroll
size/range depend on the pageScale.

BUG=340646
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=270686

[mkosiba (inactive), 2014-04-29 12:24:16]

enne@ for cc/*
boliu@ for android_webview/* and synchronous compositor (I'll ask Marcus to
rubber-stamp later).

[mkosiba (inactive), 2014-04-29 13:33:43]

some (hopefully useful) context. This CL is trying to address 2 issues:
 1) The code in LayerTreeHostImpl::PinchGestureUpdate causes the webview to
observe an intermediate state where the scroll offset is outside of the maximum
scroll range (this gets passed on to the embedding app causing weird behavior).

 2) The delegate has a bunch of methods that were only called on activation
(scroll range updates, size updates) and now need to be called on pagescale
changes. I was tempted to mush everything into one update method but that seemed
a bit inefficient so we're left with two.

[boliu, 2014-04-30 05:48:41]

https://codereview.chromium.org/256303006/diff/40001/android_webview/browser/...
File android_webview/browser/browser_view_renderer.cc (right):

https://codereview.chromium.org/256303006/diff/40001/android_webview/browser/...
android_webview/browser/browser_view_renderer.cc:460:
scoped_ptr<base::DictionaryValue> state(new base::DictionaryValue);
Isn't this gonna be slow in production?

I think if you factor out this into a function, then TRACE_EVENT won't call it
if trace is not on, and safe some cpu cycles.

Also there are other kind of traces like counters, which might be a better fit,
though some of these values might already be traced in cc.

https://codereview.chromium.org/256303006/diff/40001/android_webview/java/src...
File android_webview/java/src/org/chromium/android_webview/AwContents.java
(right):

https://codereview.chromium.org/256303006/diff/40001/android_webview/java/src...
android_webview/java/src/org/chromium/android_webview/AwContents.java:2010:
float oldPageScaleFactor = mPageScaleFactor;
indent

https://codereview.chromium.org/256303006/diff/40001/android_webview/javatest...
File
android_webview/javatests/src/org/chromium/android_webview/test/AndroidScrollIntegrationTest.java
(right):

https://codereview.chromium.org/256303006/diff/40001/android_webview/javatest...
android_webview/javatests/src/org/chromium/android_webview/test/AndroidScrollIntegrationTest.java:831:
overScrollByCallbackHelper.waitForCallback(callCount);
If this is synchronous, should we check this is called immediately after zoomIn
on UI thread instead?

[mkosiba (inactive), 2014-05-01 11:13:24]

https://codereview.chromium.org/256303006/diff/40001/android_webview/browser/...
File android_webview/browser/browser_view_renderer.cc (right):

https://codereview.chromium.org/256303006/diff/40001/android_webview/browser/...
android_webview/browser/browser_view_renderer.cc:460:
scoped_ptr<base::DictionaryValue> state(new base::DictionaryValue);
On 2014/04/30 05:48:42, boliu wrote:
> Isn't this gonna be slow in production?
> 
> I think if you factor out this into a function, then TRACE_EVENT won't call it
> if trace is not on, and safe some cpu cycles.
> 
> Also there are other kind of traces like counters, which might be a better
fit,
> though some of these values might already be traced in cc.

Done. The main reason for adding these is that we're getting scrolling-related
bugs again so having a bit more tracing here should make figuring out the issue
from traces folks upload a bit easier.

https://codereview.chromium.org/256303006/diff/40001/android_webview/java/src...
File android_webview/java/src/org/chromium/android_webview/AwContents.java
(right):

https://codereview.chromium.org/256303006/diff/40001/android_webview/java/src...
android_webview/java/src/org/chromium/android_webview/AwContents.java:2010:
float oldPageScaleFactor = mPageScaleFactor;
On 2014/04/30 05:48:42, boliu wrote:
> indent

Done.

https://codereview.chromium.org/256303006/diff/40001/android_webview/javatest...
File
android_webview/javatests/src/org/chromium/android_webview/test/AndroidScrollIntegrationTest.java
(right):

https://codereview.chromium.org/256303006/diff/40001/android_webview/javatest...
android_webview/javatests/src/org/chromium/android_webview/test/AndroidScrollIntegrationTest.java:831:
overScrollByCallbackHelper.waitForCallback(callCount);
On 2014/04/30 05:48:42, boliu wrote:
> If this is synchronous, should we check this is called immediately after
zoomIn
> on UI thread instead?

Done.

[boliu, 2014-05-01 16:31:49]

android_webview and sync compositor lgtm

https://codereview.chromium.org/256303006/diff/100001/android_webview/browser...
File android_webview/browser/browser_view_renderer.cc (right):

https://codereview.chromium.org/256303006/diff/100001/android_webview/browser...
android_webview/browser/browser_view_renderer.cc:356:
TRACE_EVENT2("android_webview",
I'm nit picking TRACE_EVENTs...

Can these new traces be INSTANTs? It doesn't look like it's wrapping anything
particularly expensive.

[mkosiba (inactive), 2014-05-01 16:42:22]

https://codereview.chromium.org/256303006/diff/100001/android_webview/browser...
File android_webview/browser/browser_view_renderer.cc (right):

https://codereview.chromium.org/256303006/diff/100001/android_webview/browser...
android_webview/browser/browser_view_renderer.cc:356:
TRACE_EVENT2("android_webview",
On 2014/05/01 16:31:50, boliu wrote:
> I'm nit picking TRACE_EVENTs...
> 
> Can these new traces be INSTANTs? It doesn't look like it's wrapping anything
> particularly expensive.

good call. This is probably my 2nd time adding traces so I didn't give any
thought to whether these should be instant or not. done!

[mkosiba (inactive), 2014-05-02 09:55:39]

I guess this is more of a gestures/scrolling-related CL. Re-routing to aelias@

[aelias_OOO_until_Jul13, 2014-05-03 01:19:29]

https://codereview.chromium.org/256303006/diff/130001/cc/input/layer_scroll_o...
File cc/input/layer_scroll_offset_delegate.h (right):

https://codereview.chromium.org/256303006/diff/130001/cc/input/layer_scroll_o...
cc/input/layer_scroll_offset_delegate.h:47: // SetTotalScrollOffset method for
efficiency.
I don't really believe "for efficiency" in cases like this unless that's backed
by profile data.  I'd suggest deleting SetTotalScrollOffset to avoid the
redundancy.

[mkosiba (inactive), 2014-05-06 14:04:48]

https://codereview.chromium.org/256303006/diff/130001/cc/input/layer_scroll_o...
File cc/input/layer_scroll_offset_delegate.h (right):

https://codereview.chromium.org/256303006/diff/130001/cc/input/layer_scroll_o...
cc/input/layer_scroll_offset_delegate.h:47: // SetTotalScrollOffset method for
efficiency.
On 2014/05/03 01:19:30, aelias wrote:
> I don't really believe "for efficiency" in cases like this unless that's
backed
> by profile data.  I'd suggest deleting SetTotalScrollOffset to avoid the
> redundancy.

Done.

[chromium-reviews, 2014-05-08 23:04:11]

Ping?
On May 6, 2014 3:04 PM, <mkosiba@chromium.org> wrote:

>
> https://codereview.chromium.org/256303006/diff/130001/cc/
> input/layer_scroll_offset_delegate.h
> File cc/input/layer_scroll_offset_delegate.h (right):
>
> https://codereview.chromium.org/256303006/diff/130001/cc/
> input/layer_scroll_offset_delegate.h#newcode47
> cc/input/layer_scroll_offset_delegate.h:47: // SetTotalScrollOffset
> method for efficiency.
> On 2014/05/03 01:19:30, aelias wrote:
>
>> I don't really believe "for efficiency" in cases like this unless
>>
> that's backed
>
>> by profile data.  I'd suggest deleting SetTotalScrollOffset to avoid
>>
> the
>
>> redundancy.
>>
>
> Done.
>
> https://codereview.chromium.org/256303006/
>

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-05-09 01:00:01]

Sorry, I missed your last update.  One more comment (which I should've noticed
before, sorry about that):

https://codereview.chromium.org/256303006/diff/190001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/256303006/diff/190001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:2691:
active_tree_->SetRootLayerScrollOffsetDelegate(NULL);
Is there any way to avoid this workaround by changing the order that things
happen in the scrolling guts?  I don't understand quite what the problem is from
code inspection, I guess either a clamp or a max scroll offset update happens
too late?

[mkosiba (inactive), 2014-05-09 17:05:33]

https://codereview.chromium.org/256303006/diff/190001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/256303006/diff/190001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:2691:
active_tree_->SetRootLayerScrollOffsetDelegate(NULL);
On 2014/05/09 01:00:01, aelias wrote:
> Is there any way to avoid this workaround by changing the order that things
> happen in the scrolling guts?  I don't understand quite what the problem is
from
> code inspection, I guess either a clamp or a max scroll offset update happens
> too late?

AFAICT the problem is caused by 2 things:
a) the LayerImpl::MaxScrollOffset seems to now depend on pagescale,
b) updating the scroll offset on one of the viewport causes an update on the
delegate.

I think the above makes it quite hard to change the scroll offset in a way that
doesn't result in the delegate seeing an intermediate value (even though it
might be possible to change this in a way that makes the delegate not see a
value that's outside of the max range).

[aelias_OOO_until_Jul13, 2014-05-09 19:05:28]

OK, I still don't really understand the exact sequence of events, particularly
as the outer viewport is currently NULL on Android.  But I'll lgtm this since
it's not too bad and will likely need to be revisited in any case when outer
viewport becomes non-NULL.

As a heads-up, Android is still running in a legacy mode that doesn't turn on
most of the changes in the new pinch zoom model (i.e. outer viewport is NULL). 
ChromeOS is shipping the new pinch zoom model (controlled by
--enable-pinch-virtual-viewport) in M36, and we're planning to fix the remaining
Android-specific bugs and enable on Android in M37.  This will likely cause a
few WebView-specific problems as well.  The master bug is
http://crbug.com/132607 and the design doc is
https://docs.google.com/a/chromium.org/document/d/1KewSqexq4Pd99RUFB-yVoUIJoc...

[mkosiba (inactive), 2014-05-14 10:43:17]

On 2014/05/09 19:05:28, aelias wrote:
> OK, I still don't really understand the exact sequence of events, particularly
> as the outer viewport is currently NULL on Android.  But I'll lgtm this since
> it's not too bad and will likely need to be revisited in any case when outer
> viewport becomes non-NULL.

Sorry, it looks like I got a bit confused when explaining this.

Before the CL that introduced inner/outer viewports the
layer_impl->MaxScrollOffset
did not depend on page scale and so the LayerTreeImpl set it on the delegate
only
on activation.
After that CL layer_impl->MaxScrollOffset depends on page_scale_factor but the
delegate's SetMaxTotalScrollOffset method is still only called on activation.
The smallest fix for this would be to add the extra call.

However I think the current LayerScrollOffsetDelegate API sucks for 2 reasons:
 - too many methods: LayerTreeImpl was calling all of the
non-scroll-offset-related
   methods almost all of the time anyway,
 - call order dependencies between cc/aw: if cc code calls say
setPageScaleFactor
   before setmaxscrolloffset some of our internal calculations may be off, etc..

So I changed the LayerScrollOffsetDelegate API while I was at it.

The downside is that the pinch zoom operation causes the new API to report an
inconsistent state to the webview because SetPageScaleDelta and ScrollBy
(and ClampScrollToMaxScrollOffset) cause a call to the update method.
The effect was that the unittests were failing on:
 DCHECK(total_scroll_offset.y() <= max_scroll_offset.y());

We could probably change SetPageScaleDelta to do the equivalent of
ClampScrollToMaxScrollOffset so that the scroll never goes outside of the bounds
but it seems to me that not exposing the delegate to the intermediate state
is simpler.

> As a heads-up, Android is still running in a legacy mode that doesn't turn on
> most of the changes in the new pinch zoom model (i.e. outer viewport is NULL).


aah. I didn't know that.

> ChromeOS is shipping the new pinch zoom model (controlled by
> --enable-pinch-virtual-viewport) in M36, and we're planning to fix the
remaining
> Android-specific bugs and enable on Android in M37.  This will likely cause a
> few WebView-specific problems as well.  The master bug is
> http://crbug.com/132607 and the design doc is
>
https://docs.google.com/a/chromium.org/document/d/1KewSqexq4Pd99RUFB-yVoUIJoc...

[mkosiba (inactive), 2014-05-14 10:50:55]

The CQ bit was checked by mkosiba@chromium.org

[commit-bot: I haz the power, 2014-05-14 10:51:30]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mkosiba@chromium.org/256303006/210001

[commit-bot: I haz the power, 2014-05-14 13:48:43]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)

[commit-bot: I haz the power, 2014-05-14 14:06:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-14 14:06:34]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)

[mkosiba (inactive), 2014-05-14 14:11:20]

The CQ bit was checked by mkosiba@chromium.org

[commit-bot: I haz the power, 2014-05-14 14:11:30]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mkosiba@chromium.org/256303006/210001

[commit-bot: I haz the power, 2014-05-14 14:32:19]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)

[commit-bot: I haz the power, 2014-05-14 14:51:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-14 14:51:44]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)

[mkosiba (inactive), 2014-05-15 08:55:40]

The CQ bit was checked by mkosiba@chromium.org

[commit-bot: I haz the power, 2014-05-15 08:56:21]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mkosiba@chromium.org/256303006/210001

[commit-bot: I haz the power, 2014-05-15 16:32:08]

Change committed as 270686