[android_webview] Use a fraction to calculate scroll offset.

[android_webview] Use a fraction to calculate scroll offset.

android_webview/ has to expose scroll offset operations in physical
pixels while cc/ stores scroll offsets in dip pixels.
Unfortunately the physical pixel values android_webview/ exposes are
rounded so there is no such combination of rounding operations that
allows for scrolls to 'bottom' in one corrdinate space to always be
'flush' against the bottom edge in the other. The most natural
solution is to convert the scroll offset to the fraction of the
scroll range.
By using the max_scroll_offset value obtained directly from the compositor
we're also removing the need to delay scrolls since the content size
is always as recent as the scroll offset update.

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

[mkosiba (inactive), 2013-09-24 18:43:04]

PTAL

https://codereview.chromium.org/23533051/diff/6001/android_webview/browser/in...
File android_webview/browser/in_process_view_renderer.cc (right):

https://codereview.chromium.org/23533051/diff/6001/android_webview/browser/in...
android_webview/browser/in_process_view_renderer.cc:733: max_offset.y());
I could alternatively write these as

  gfx::Vector2dF scroll_offset_dip(
      (scroll_offset.x() * max_scroll_offset_dip_.x()) / max_offset.x(),
      (scroll_offset.y() * max_scroll_offset_dip_.y()) / max_offset.y());
  if (!max_offset.y()) scroll_offset_dip.set_y(0);
  if (!max_offset.x()) scroll_offset_dip.set_x(0);

[aelias_OOO_until_Jul13, 2013-09-24 22:03:19]

lgtm modulo comments below

https://codereview.chromium.org/23533051/diff/6001/android_webview/browser/in...
File android_webview/browser/in_process_view_renderer.cc (right):

https://codereview.chromium.org/23533051/diff/6001/android_webview/browser/in...
android_webview/browser/in_process_view_renderer.cc:720: return
gfx::ToRoundedVector2d(gfx::ScaleVector2d(
Is there a reason why you're using Rounded and not Ceil or Floor?  In general I
prefer the latter because they behave more predictably.

https://codereview.chromium.org/23533051/diff/6001/android_webview/browser/in...
android_webview/browser/in_process_view_renderer.cc:733: max_offset.y());
The current way is fine.  Could you add a comment explaining what you're doing
though?

https://codereview.chromium.org/23533051/diff/6001/android_webview/browser/in...
android_webview/browser/in_process_view_renderer.cc:736: DCHECK(0 <=
scroll_offset_dip.x());
Use DCHECK_LE instead.  That gives more informative output on failure.

https://codereview.chromium.org/23533051/diff/6001/android_webview/javatests/...
File
android_webview/javatests/src/org/chromium/android_webview/test/AwScrollOffsetManagerTest.java
(left):

https://codereview.chromium.org/23533051/diff/6001/android_webview/javatests/...
android_webview/javatests/src/org/chromium/android_webview/test/AwScrollOffsetManagerTest.java:220:
public void testScrollDeferredOnInvalidContentSize() {
If you're improving the timing such that this is no longer needed, please say so
in your patch description.

https://codereview.chromium.org/23533051/diff/6001/android_webview/native/aw_...
File android_webview/native/aw_contents.h (right):

https://codereview.chromium.org/23533051/diff/6001/android_webview/native/aw_...
android_webview/native/aw_contents.h:169: void SetDipScale(JNIEnv* env, jobject
obj, jfloat dipScale);
nit: Chromium style is for the underscore, also xPix/yPix is more specific. 
Accidental change?

https://codereview.chromium.org/23533051/diff/6001/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/23533051/diff/6001/cc/layers/layer_impl.cc#ne...
cc/layers/layer_impl.cc:522: layer->SetMaxScrollOffset(max_scroll_offset_);
nit: moving this line for no reason?

https://codereview.chromium.org/23533051/diff/6001/content/public/android/jav...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/23533051/diff/6001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2398:
mUpdateFrameInfoListener.onFrameInfoUpdated(pageScaleFactor);
Maybe for a separate patch, but it seems to me that if content size came in with
the wrong timing and caused problems, pageScaleFactor would as well.  I suggest
adding a SetPageScaleFactor call to LayerScrollOffsetDelegate and eliminating
this.

[mkosiba (inactive), 2013-09-24 23:18:52]

Thanks Alex! Really good feedback. I'll update the CL tomorrow but I had just
one question below.

https://codereview.chromium.org/23533051/diff/6001/android_webview/browser/in...
File android_webview/browser/in_process_view_renderer.cc (right):

https://codereview.chromium.org/23533051/diff/6001/android_webview/browser/in...
android_webview/browser/in_process_view_renderer.cc:720: return
gfx::ToRoundedVector2d(gfx::ScaleVector2d(
On 2013/09/24 22:03:19, aelias wrote:
> Is there a reason why you're using Rounded and not Ceil or Floor?  In general
I
> prefer the latter because they behave more predictably.

no, there was no particular reason. I assumed the rounding wouldn't matter with
this approach.

Now that you point this out it seems like I should have used Ceil. That way if
we have a max_scroll_offset of 0.1 we can still represent 0 and 0.1 as 0 and 1
physical pixels and maintain the invariant that if the cc:: root layer thinks
it's scrollable so does the Java side (not sure it's strictly neccesary for
anything but feels like the sane thing to do).

https://codereview.chromium.org/23533051/diff/6001/content/public/android/jav...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/23533051/diff/6001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2398:
mUpdateFrameInfoListener.onFrameInfoUpdated(pageScaleFactor);
On 2013/09/24 22:03:19, aelias wrote:
> Maybe for a separate patch, but it seems to me that if content size came in
with
> the wrong timing and caused problems, pageScaleFactor would as well.  I
suggest
> adding a SetPageScaleFactor call to LayerScrollOffsetDelegate and eliminating
> this.

good spot.

Actually, after thinking about that a bit more: we might want to update
contentSize
before we scroll (so that if the embedder tries to calculate something like
webview.getContentHeight() * webView.getScale() from the onScroll callback they
get current results and not a value lagging 1 frame behind).

Do you think pushing this->children()[0]->bounds(); (taken from
LayerTreeImpl::ScrollableSize)
via the LayerScrollOffsetDelegate be OK? I'm assuming I'd need to do that only
when the
LayerScrollOffsetDelegate is set on a tree (since the layer's sizes change only
on commits).

[aelias_OOO_until_Jul13, 2013-09-24 23:24:26]

https://codereview.chromium.org/23533051/diff/6001/content/public/android/jav...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/23533051/diff/6001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2398:
mUpdateFrameInfoListener.onFrameInfoUpdated(pageScaleFactor);
On 2013/09/24 23:18:53, mkosiba wrote:
> Do you think pushing this->children()[0]->bounds(); (taken from
> LayerTreeImpl::ScrollableSize)
> via the LayerScrollOffsetDelegate be OK? I'm assuming I'd need to do that only
> when the
> LayerScrollOffsetDelegate is set on a tree (since the layer's sizes change
only
> on commits).

In principle it seems fine, but I think you'll get codereview pushback about
examining the child there.

It's been "the plan" for a long time to just set the scroll layer's bounds to
the scrollable size in Blink as well, so we can avoid walking through the tree
structure.  jamesr@ even tried to do that last year but it got reverted because
there was some scenario where it failed to be set correctly.  I'd recommend
fixing that in Blink and using the scroll layer's bounds when you do this.

[mkosiba (inactive), 2013-09-24 23:57:57]

On 2013/09/24 23:24:26, aelias wrote:
>
https://codereview.chromium.org/23533051/diff/6001/content/public/android/jav...
> File
>
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
> (right):
> 
>
https://codereview.chromium.org/23533051/diff/6001/content/public/android/jav...
>
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2398:
> mUpdateFrameInfoListener.onFrameInfoUpdated(pageScaleFactor);
> On 2013/09/24 23:18:53, mkosiba wrote:
> > Do you think pushing this->children()[0]->bounds(); (taken from
> > LayerTreeImpl::ScrollableSize)
> > via the LayerScrollOffsetDelegate be OK? I'm assuming I'd need to do that
only
> > when the
> > LayerScrollOffsetDelegate is set on a tree (since the layer's sizes change
> only
> > on commits).
> 
> In principle it seems fine, but I think you'll get codereview pushback about
> examining the child there.
> 
> It's been "the plan" for a long time to just set the scroll layer's bounds to
> the scrollable size in Blink as well, so we can avoid walking through the tree
> structure.  jamesr@ even tried to do that last year but it got reverted
because
> there was some scenario where it failed to be set correctly.  I'd recommend
> fixing that in Blink and using the scroll layer's bounds when you do this.

I think it would have to be a series of separate CLs.

I mostly wanted to confirm whether the root layer's size doesn't change between
activations. If so I could just pass LayerTreeImpl::ScrollableSize directly
to the delegate when it's set in LayerTreeImpl::SetRootLayerScrollOffsetDelegate
(thus avoiding the 'examining the child' problem you mentioned).

[aelias_OOO_until_Jul13, 2013-09-25 00:06:08]

The root layer size doesn't change between activations, no.  Using
ScrollableSize() should work (as long as SetRootLayerScrollOffsetDelegate is
called after the child's properties are pushed...)

[mkosiba (inactive), 2013-09-25 14:23:48]

https://codereview.chromium.org/23533051/diff/6001/android_webview/browser/in...
File android_webview/browser/in_process_view_renderer.cc (right):

https://codereview.chromium.org/23533051/diff/6001/android_webview/browser/in...
android_webview/browser/in_process_view_renderer.cc:736: DCHECK(0 <=
scroll_offset_dip.x());
On 2013/09/24 22:03:19, aelias wrote:
> Use DCHECK_LE instead.  That gives more informative output on failure.

Done.

https://codereview.chromium.org/23533051/diff/6001/android_webview/javatests/...
File
android_webview/javatests/src/org/chromium/android_webview/test/AwScrollOffsetManagerTest.java
(left):

https://codereview.chromium.org/23533051/diff/6001/android_webview/javatests/...
android_webview/javatests/src/org/chromium/android_webview/test/AwScrollOffsetManagerTest.java:220:
public void testScrollDeferredOnInvalidContentSize() {
On 2013/09/24 22:03:19, aelias wrote:
> If you're improving the timing such that this is no longer needed, please say
so
> in your patch description.

Done.

https://codereview.chromium.org/23533051/diff/6001/android_webview/native/aw_...
File android_webview/native/aw_contents.h (right):

https://codereview.chromium.org/23533051/diff/6001/android_webview/native/aw_...
android_webview/native/aw_contents.h:169: void SetDipScale(JNIEnv* env, jobject
obj, jfloat dipScale);
On 2013/09/24 22:03:19, aelias wrote:
> nit: Chromium style is for the underscore, also xPix/yPix is more specific. 
> Accidental change?

yes, accidental change, thanks for pointing it out.

https://codereview.chromium.org/23533051/diff/6001/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/23533051/diff/6001/cc/layers/layer_impl.cc#ne...
cc/layers/layer_impl.cc:522: layer->SetMaxScrollOffset(max_scroll_offset_);
On 2013/09/24 22:03:19, aelias wrote:
> nit: moving this line for no reason?

Done.

[mkosiba (inactive), 2013-09-25 15:02:28]

+joth for android_webview and content/ OWNER
+bulach for content/public/browser/android OWNER review

[joth, 2013-09-25 15:12:06]

lgtm with a couple nits

https://codereview.chromium.org/23533051/diff/18001/android_webview/browser/i...
File android_webview/browser/in_process_view_renderer.cc (right):

https://codereview.chromium.org/23533051/diff/18001/android_webview/browser/i...
android_webview/browser/in_process_view_renderer.cc:803:
CHECK(page_scale_factor_ > 0);
DCHECk

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

https://codereview.chromium.org/23533051/diff/18001/android_webview/java/src/...
android_webview/java/src/org/chromium/android_webview/AwContents.java:189: //
ContentViewCore, which are updated on every frame).
nit: "on" -> "after" or "at end of"

https://codereview.chromium.org/23533051/diff/18001/content/browser/android/i...
File content/browser/android/in_process/synchronous_compositor_impl.cc (right):

https://codereview.chromium.org/23533051/diff/18001/content/browser/android/i...
content/browser/android/in_process/synchronous_compositor_impl.cc:429: }
nit: why you add these here?

[bulach, 2013-09-25 15:26:04]

lgtm, just nits:

https://codereview.chromium.org/23533051/diff/18001/android_webview/browser/i...
File android_webview/browser/in_process_view_renderer.cc (right):

https://codereview.chromium.org/23533051/diff/18001/android_webview/browser/i...
android_webview/browser/in_process_view_renderer.cc:714: DCHECK(dip_scale_ > 0);
nit: DCHECK_GT

https://codereview.chromium.org/23533051/diff/18001/android_webview/browser/i...
android_webview/browser/in_process_view_renderer.cc:756: DCHECK(dip_scale_ > 0);
nit: DCHECK_GT

https://codereview.chromium.org/23533051/diff/18001/android_webview/browser/i...
android_webview/browser/in_process_view_renderer.cc:789: DCHECK(0 <=
scroll_offset.y());
nit: here and adbove, DCHECK_LE

https://codereview.chromium.org/23533051/diff/18001/android_webview/browser/i...
android_webview/browser/in_process_view_renderer.cc:803:
CHECK(page_scale_factor_ > 0);
On 2013/09/25 15:12:07, joth wrote:
> DCHECk

DCHECK_GT :)

[mkosiba (inactive), 2013-09-25 16:14:14]

thanks!

https://codereview.chromium.org/23533051/diff/18001/android_webview/browser/i...
File android_webview/browser/in_process_view_renderer.cc (right):

https://codereview.chromium.org/23533051/diff/18001/android_webview/browser/i...
android_webview/browser/in_process_view_renderer.cc:714: DCHECK(dip_scale_ > 0);
On 2013/09/25 15:26:05, bulach wrote:
> nit: DCHECK_GT

Done.

https://codereview.chromium.org/23533051/diff/18001/android_webview/browser/i...
android_webview/browser/in_process_view_renderer.cc:756: DCHECK(dip_scale_ > 0);
On 2013/09/25 15:26:05, bulach wrote:
> nit: DCHECK_GT

Done.

https://codereview.chromium.org/23533051/diff/18001/android_webview/browser/i...
android_webview/browser/in_process_view_renderer.cc:789: DCHECK(0 <=
scroll_offset.y());
On 2013/09/25 15:26:05, bulach wrote:
> nit: here and adbove, DCHECK_LE

Done.

https://codereview.chromium.org/23533051/diff/18001/android_webview/browser/i...
android_webview/browser/in_process_view_renderer.cc:803:
CHECK(page_scale_factor_ > 0);
On 2013/09/25 15:26:05, bulach wrote:
> On 2013/09/25 15:12:07, joth wrote:
> > DCHECk
> 
> DCHECK_GT :)

Done.

https://codereview.chromium.org/23533051/diff/18001/content/browser/android/i...
File content/browser/android/in_process/synchronous_compositor_impl.cc (right):

https://codereview.chromium.org/23533051/diff/18001/content/browser/android/i...
content/browser/android/in_process/synchronous_compositor_impl.cc:429: }
On 2013/09/25 15:12:07, joth wrote:
> nit: why you add these here?

Done.

[mkosiba (inactive), 2013-09-25 19:07:10]

+mnaganov FYI for the onScaleChanged changes.

[commit-bot: I haz the power, 2013-09-25 19:14:55]

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

[commit-bot: I haz the power, 2013-09-25 21:26:20]

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

[commit-bot: I haz the power, 2013-09-26 05:41:36]

Change committed as 225348