775469 - Implement support for "overscroll" indicators in gecko

Status: RESOLVED DUPLICATE of bug 998025

(Core :: Graphics: Layers, defect)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

When asyncronously panning, most modern browsers indicate to the user when they've tried to pan past the pannable bounds.  The current fennec frontend does this through a layers widget callback, but that approach doesn't scale to multiple async pan/zoomed frames.

The right way to do this is in layers itself, possible through some sort of render callback per layer, but lots of details to work out there.

We should make the support completely effect-neutral, so that we can plug 'n play different effects as desired.

Comment 1

[Dietrich Ayala (:dietrich)]

Nice to have per cjones, blocking-.

Comment 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Not a b2g v1 blocker, but could block k9o.

Comment 4

[Cody Brocious [:Daeken]]

Attachment: Initial patch for overscrolling

This patch adds the initial effect to be used for overscrolling.  Architecturally, I believe it's sound; we may want to expose the overscroll data from the APZC differently, but that's about it.

There are a couple of things that still need to be done, though:
1) The overscroll tracking in APZC is broken (that's easy to fix) and we need a way to come back once an overscrolled surface is released.
2) The actual overscroll shader needs to be moved from the WebGL prototype into the layer effect.
3) There's a bug in the composition effect where the layer is being composited to the wrong position on screen; since this is 99.9% identical to the container layer code, I'm at a bit of a loss.

#1 and #2 I can take care of no problem, but I'm at a loss for #3; any input would be appreciated.

Comment 5

[Faramarz Rashed]

Cody: ETA on #1 and #2 please?

Comment 6

[Cody Brocious [:Daeken]]

Quick update: The rendering bug in #3 is actually an issue in bug 783919.  I've taken over that now and I'm attempting to get it all fixed.  Will update when I've finished that up and gotten this side working.

Comment 7

[Cody Brocious [:Daeken]]

Attachment: Makes the overscrolling functional

With this patch, overscrolling is mostly functional.  It doesn't return to zero properly and doesn't apply a resistance curve, but otherwise it's there.  Rather than using the shader approach and adding a bunch of additional pieces, I'm reusing the existing shaders and doing a simple translation on the draw.

Comment 8

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

If the effects only require applying an extra translation, we can do that entirely within the existing shadow-transform and apzc machinery and don't need the work here specifically for that.

(We still need it for scroll indicators, though.)

Comment 9

[Cody Brocious [:Daeken]]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #8)
> If the effects only require applying an extra translation, we can do that
> entirely within the existing shadow-transform and apzc machinery and don't
> need the work here specifically for that.
> 
> (We still need it for scroll indicators, though.)

Agreed, with the current overscroll effect it's a simple matrix transformation (translate right now, and a small rotate possibly) -- it's something that we could easily pull into the APZC core.  However, I wanted to keep it separate so 1) we could change the effect as desired without a lot of work (e.g. we could switch back to using a dedicated shader like in the prototype), and 2) it'd provide a base on which we could test the effect framework and figure out what needs to change (if anything) for the scroll indicators.

I wouldn't be opposed to moving the overscrolling code into APZC in the future -- it'd make the code a bit simpler and increase performance -- but I think it's good to keep it out for now.

Comment 10

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

How we organize the overscroll-effect code is a separate issue from adding layers API.  New layers API are a pretty big maintenance burden, so if we can implement this without doing that, I'm very much in favor.

Comment 11

[Cody Brocious [:Daeken]]

Attachment: APZC overscroll implementation

This patch implements the APZC side of overscrolling.  There are a couple things that are not yet done (noted with XXX) but I wanted to get a review on the structure while I do those last few little things.  Big things I don't know about:

1) I'm now exposing GetFrameMetrics() since it's needed by the overscroll effect to get the width/height of the viewport.  Should this be exposed differently?
2) The overscroll values in APZC are directly exposed.  Should accessors be used for this instead?

Comment 12

[Cody Brocious [:Daeken]]

Attachment: Overscroll layer effect

This implements the overscroll layer effect.  The curve calculation is off, but it's functional.  The code is fairly straightforward and is mostly shared with the container layer OGL implementation, but I'm seeing some odd flickering at times; not sure why that is.

Note: the reason this is done here rather than in the core APZC scrolling code is that it makes the scroll indicators much simpler and cleaner, as well as exercising the layer effect interface.

Comment 13

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 664369 [details] [diff] [review]
Overscroll layer effect

I would prefer that we had these effects compartmentalized into their
own file, but that's going to require creating at least one more
header and rearranging more stuff.  So this is OK for now.  Let's file
a followup bug on reorganizing.

>diff --git a/gfx/layers/ipc/CompositorParent.cpp b/gfx/layers/ipc/CompositorParent.cpp

>+#define SIGNF(x) ((x) / fabs(x))
>+

Does signbit() do what you want here?  This macro doesn't check for 0
which means other copy 'n pasters would hit FPEs.  (You do this
manually below.)

>+class OverscrollEffectRenderer : public EffectRendererOGL {

Let's give this class a name that more specifically denotes the effect
it's applying.  (I'm not sure what that effect is, yet ;) .)

We need to add a summary comment to this class describing what effect
it's trying to apply.

>+public:
>+  OverscrollEffectRenderer(AsyncPanZoomController* aApzc)
>+  : mApzc(aApzc) {
>+  }

It seems cleaner to me to have APZC hold a reference to this class, so
that we don't have need to recreate it on every update.  But that
would require the big code reorganization I referred to above.

>+  void ApplyCurve(float& aX, float& aY) {
>+    FrameMetrics frame = mApzc->GetFrameMetrics();
>+    float width = frame.mViewport.width;
>+    float height = frame.mViewport.height;
>+
>+    aX = SIGNF(aX) * pow(fabs(aX) / width  * 4.0f, 5.0f) * width  / 4.0f;
>+    aY = SIGNF(aY) * pow(fabs(aY) / height * 4.0f, 5.0f) * height / 4.0f;
>+

I have no idea what this code is doing :).  Needs docs.

>+  void ApplyCompositionEffect(const TimeStamp& aNow,
>+                              ContainerLayer* aLayer,
>+                              int aTargetFrameBuffer,
>+                              const nsIntPoint& aOffset,
>+                              int aContentTexture,
>+                              LayerManagerOGL* aManager) {

>+    ShaderProgramOGL *rgb = aManager->GetFBOLayerProgram(maskType);
>+

Nit: |ShaderProgramOGL* rgb|.

>+private:
>+  AsyncPanZoomController* mApzc; // XXX: Should this be a raw pointer?

As this patch is currently written, it needs to be a ref pointer.  But
see below.

>@@ -1145,6 +1232,10 @@ UpdateIndirectTree(uint64_t aId, Layer* aRoot, bool isFirstPaint)
>   if (ContainerLayer* root = aRoot->AsContainerLayer()) {
>     if (AsyncPanZoomController* apzc = sIndirectLayerTrees[aId].mController) {
>       apzc->NotifyLayersUpdated(root->GetFrameMetrics(), isFirstPaint);
>+
>+      // XXX: This is happening twice on construction; why?  The layer tree shouldn't change that much... right?
>+      // Also happening on scrolling... think this needs to be changed a bit.
>+      root->SetEffectRenderer(new OverscrollEffectRenderer(apzc));

I'm not worried about the overhead of allocating this class; it's
going to happen after a paint and a bunch of IPC.

However, there are some big problems here
 - we always create an OGL effect renderer, regardless of the layer
   manager's actual type

 - it's possible for the ContainerLayer* here to hold on to the effect
   renderer after the APZC dies.

I think instead, what we should do (until the reorg) is cache the
effect renderer in LayerTreeState.  Create it when the APZC is
created.  Then we should set the effect renderer on the ContainerLayer
during AutoResolveRefLayers, and unset it using that mechanism.

We can only create this renderer if the LayerManager is of type
LAYERS_OPENGL.

The approach here is OK, but r- for the potential security bug with
memory management.

Comment 14

[Doug Sherk (:drs) (inactive)]

Comment on attachment 664368 [details] [diff] [review]
APZC overscroll implementation

Review of attachment 664368 [details] [diff] [review]:
-----------------------------------------------------------------

General approach looks ok, other than ownership, but that's more due to how the layer renderer works.

::: gfx/layers/ipc/AsyncPanZoomController.cpp
@@ +85,5 @@
>       mTouchListenerTimeoutTask(nullptr),
>       mX(this),
>       mY(this),
> +     mXOver(0.0f),
> +     mYOver(0.0f),

Perhaps we should store this on the Axis class instead, or as a gfx::Size. I would also prefer a more useful name.

@@ +662,5 @@
> +  mYOver += mY.GetOverscrollAmount(inverseScale, aDelta);
> +  float maxX = mFrameMetrics.mViewport.width / 4.0f;
> +  float maxY = mFrameMetrics.mViewport.height / 4.0f;
> +  mXOver = PR_MAX(PR_MIN(mXOver, maxX), -maxX);
> +  mYOver = PR_MAX(PR_MIN(mYOver, maxY), -maxY);

Use clamped().

@@ +674,4 @@
>    return true;
>  }
>  
> +#define SIGNF(x) ((x) / fabs(x))

I'm not comfortable adding this. In general I've been just using a conditional to determine sign elsewhere in this code. Also this doesn't account for x == 0.0.

@@ +680,5 @@
> +  if (mState != OVERSCROLL_RETURN) {
> +    return false;
> +  }
> +
> +  if (mXOver < 1.0f && mXOver > -1.0) {

Document these.

@@ +686,5 @@
> +  }
> +  if (mYOver < 1.0f && mYOver > -1.0) {
> +    mYOver = 0.0f;
> +  }
> +  if (mXOver == 0.0f && mYOver == 0.0f) {

This needs to account for float precision. There should be an EPSILON floating around in this file that you can use.

@@ +692,5 @@
> +    mState = NOTHING;
> +    return false;
> +  }
> +
> +  // XXX: Curve should be calculated here.

Please elaborate and file followup bugs.

@@ +697,5 @@
> +  float width = mFrameMetrics.mViewport.width;
> +  float height = mFrameMetrics.mViewport.height;
> +  // Should return a third of the buffer per second.
> +  float xOff = SIGNF(mXOver) * (width / 3.0) * aDelta.ToSeconds();
> +  float yOff = SIGNF(mYOver) * (height / 3.0) * aDelta.ToSeconds();

Document these. I don't really understand what's happening here.

@@ +708,5 @@
> +  if (SIGNF(mYOver) == SIGNF(mYOver - yOff)) {
> +    mYOver -= yOff;
> +  } else {
> +    mYOver = 0.0;
> +  }

Document these. I don't get this either.

::: gfx/layers/ipc/AsyncPanZoomController.h
@@ +196,5 @@
> +  /**
> +   * Gets the current frame metrics. This is *not* the Gecko copy stored in the
> +   * layers code.
> +   */
> +  const FrameMetrics& GetFrameMetrics();

I'd rather not expose this. I think this indicates a much bigger problem that we need this. cjones has already gone over the ownership model but I don't understand the layer renderer stuff enough to talk about it.

In this case, we can avoid needing to expose this because we can just get the FrameMetrics from the ContainerLayer passed in to ApplyCompositionEffect().

@@ +201,5 @@
> +
> +  /**
> +   * XXX: Write this
> +   */
> +  float mXOver, mYOver;

Document, make this private, and add a public getter.

@@ +417,5 @@
>      WAITING_LISTENERS, /* a state halfway between NOTHING and TOUCHING - the user has
>                      put a finger down, but we don't yet know if a touch listener has
>                      prevented the default actions yet. we still need to abort animations. */
> +    OVERSCROLL_RETURN, /* a gesture caused an overscroll and completed, so we're returning
> +                    to the nearest edge */

It isn't necessarily a gesture that caused this. In fact, right now, a gesture can't cause it. Only panning can.

::: gfx/layers/ipc/Axis.cpp
@@ +118,5 @@
> +    return 0.0f;
> +  }
> +  return DisplacementWillOverscrollAmount(displacement);
> +}
> +

This is mostly copied and pasted from GetDisplacementForDuration(). It could use a refactor.

Comment 15

[Josh Carpenter [:jcarpenter]]

Is there any indication when we can see this on device? I'm worried that I don't know what this patch is going to look like. Once we see initial implementation Patryk and I will provide feedback and work with Cody to iterate.

Comment 16

[Josh Carpenter [:jcarpenter]]

I connected w/ Cody on IRC. We are not going to be using elastic or bouncing scrolling behavior in v1, so it is unclear how much of the implementation above that we can use. I will email the participants in this issues (dev + UX alike) to establish next steps.

Also being tracked here: https://github.com/mozilla-b2g/gaia/issues/2559

Comment 17

[Joe Drew (not getting mail)]

The graphics team is going to take this on, but first we need firm UX mockups. Josh, can you (or your designate) attach those here?

Comment 18

[Josh Carpenter [:jcarpenter]]

Hi Joe, 

* I've asked the visual design team to update this post with the latest and greatest mockups. Steve from TEF is the primary UX contact on this. In the interim, a one page summary of their concept (which everyone quite likes) is here: https://www.dropbox.com/sh/qxcj0om6ifv54gf/j-D65n0MfE
* This was discussed in a v1 Outstanding Features audit today (10/23) in London and tentatively agreed as being doable for v1. Andreas will be reaching out to the relevant devs to confirm. 
* There is an existing email thread to that effect that I will also direct here, so we can keep the commentary on Bugzilla.

Comment 19

[steve]

All design work (done very quickly :-) can be found in the folder. 
https://www.dropbox.com/sh/qxcj0om6ifv54gf/j-D65n0MfE

Including summary wireframe, visual mockup, video showing a very quick prototype we created and links to prototype (note this was created to just show what it would roughly look like, it does not include the automatic colour box growth that we also require after a speed scroll).

Comment 20

[Benoit Jacob [:bjacob] (mostly away)]

Hi, just confirming I've started working on this. Mostly catching up with above discussion so far. I guess I'll start by unbitrotting Cody's patches and applying above review comments, and we'll see from there.

My understanding is that the video linked in comment 19 determines what we want this to look like.

Comment 21

[Andrew Overholt [:overholt]]

Should we block on this?

Comment 22

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Nope, but it's at the top of the nice-to-have list from UX, and the implementation is small and not risky.  a+ would approve again

Comment 23

[Benoit Jacob [:bjacob] (mostly away)]

Update on non-progress: I am completely taken by bug 806369 (an emergency for Fennec 17). I plan to return to this bug later this week. The furthest I went on this bug was I made a build with all the patches, and it didn't draw any overscroll effect, so I tried to debug it, but all the overscroll code was being hit, so the next step was to run this in APItrace.

Comment 24

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: crash of main process in debug build

OK, I'm back to this now --- don't think I'll be able to do much more on bug 806369 for the time being.

In a debug (but optimized) build of B2G, doing some overscrolling at the top of lemonde.fr, I get a main-process crash (so it reboots the phone) with attached stack. Looking into it. Hopefully this will give us a clue into the previously reported problem that nothing gets drawn...

Comment 25

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: fix the monitor assertion failure. not clean for landing, but removes crash.

Doug, what would be the clean version of this patch?

The CompositorParent is trying to GetFrameMetrics here. GetFrameMetrics requires the monitor to be locked. Should APZC allow users to lock its monitor (as this patch does) or not? If not, then how are users supposed to call GetFrameMetrics?

Comment 26

[Doug Sherk (:drs) (inactive)]

(In reply to Benoit Jacob [:bjacob] from comment #25)
> Created attachment 677842 [details] [diff] [review]
> fix the monitor assertion failure. not clean for landing, but removes crash.
> 
> Doug, what would be the clean version of this patch?
> 
> The CompositorParent is trying to GetFrameMetrics here. GetFrameMetrics
> requires the monitor to be locked. Should APZC allow users to lock its
> monitor (as this patch does) or not? If not, then how are users supposed to
> call GetFrameMetrics?

Looking at attachment 664369 [details] [diff] [review] some more, it looks like the only place GetFrameMetrics() is used now is here:

> +  void ApplyCurve(float& aX, float& aY) {
> +    FrameMetrics frame = mApzc->GetFrameMetrics();
> +    float width = frame.mViewport.width;
> +    float height = frame.mViewport.height;

I don't think we actually want to be using mViewport here. What we really want is the area being composited to, i.e. mCompositionBounds. You might want to just use this instead since I think it should be functionally the same for you:

> nsIntRect visibleRect = aLayer->GetEffectiveVisibleRegion().GetBounds();

If we stick with getting this data from the APZC, I'd rather just expose the mCompositionBounds field, protected by the monitor, than the entire FrameMetrics.

Comment 27

[Benoit Jacob [:bjacob] (mostly away)]

Yoohoo, I finally found something that explains why it doesn't draw anything. Would have been easier to find this out if I had been able to get APItrace to work on B2G, but I didn't manage to (see bug 808135 for one issue I ran into).

So I added lots of gl get-calls to see the current GL state at the time we issue the glDrawArrays call there. Here's the relevant part:

I/Gecko   (  106): [gl:0x4b7e5800] > GLint mozilla::gl::GLContext::fGetUniformLocation(GLint, const GLchar*)
I/Gecko   (  106): [gl:0x4b7e5800] < GLint mozilla::gl::GLContext::fGetUniformLocation(GLint, const GLchar*) [0x0000]
I/Gecko   (  106): [gl:0x4b7e5800] > void mozilla::gl::GLContext::fGetUniformfv(GLuint, GLint, GLfloat*)
I/Gecko   (  106): [gl:0x4b7e5800] < void mozilla::gl::GLContext::fGetUniformfv(GLuint, GLint, GLfloat*) [0x0000]
I/Gecko   (  106): YYY uMatrixProj:
I/Gecko   (  106):  0.00625 0 0 0 0 -0.00416667 0 0 0 0 0 0 -1 1 0 1
I/Gecko   (  106): [gl:0x4b7e5800] > GLint mozilla::gl::GLContext::fGetUniformLocation(GLint, const GLchar*)
I/Gecko   (  106): [gl:0x4b7e5800] < GLint mozilla::gl::GLContext::fGetUniformLocation(GLint, const GLchar*) [0x0000]
I/Gecko   (  106): [gl:0x4b7e5800] > void mozilla::gl::GLContext::fGetUniformfv(GLuint, GLint, GLfloat*)
I/Gecko   (  106): [gl:0x4b7e5800] < void mozilla::gl::GLContext::fGetUniformfv(GLuint, GLint, GLfloat*) [0x0000]
I/Gecko   (  106): YYY uLayerQuadTransform:
I/Gecko   (  106):  320 0 0 0 0 710 0 0 0 0 1 0 0 -71 0 1
I/Gecko   (  106): [gl:0x4b7e5800] > GLint mozilla::gl::GLContext::fGetUniformLocation(GLint, const GLchar*)
I/Gecko   (  106): [gl:0x4b7e5800] < GLint mozilla::gl::GLContext::fGetUniformLocation(GLint, const GLchar*) [0x0000]
I/Gecko   (  106): [gl:0x4b7e5800] > void mozilla::gl::GLContext::fGetUniformfv(GLuint, GLint, GLfloat*)
I/Gecko   (  106): [gl:0x4b7e5800] < void mozilla::gl::GLContext::fGetUniformfv(GLuint, GLint, GLfloat*) [0x0000]
I/Gecko   (  106): YYY uLayerTransform:
I/Gecko   (  106):  1 0 0 0 0 1 0 0 0 0 1 0 NaN NaN NaN NaN

--> the uLayerTransform uniform containts NaN's !

Enough for today; but getting more optimistic about a fix now.

Comment 28

[Benoit Jacob [:bjacob] (mostly away)]

The NaN's were produced by the SIGNF function applied to 0: SIGNF(0)=0/fabs(0)=NaN.

I did see comment 13 warning against it but I didn't realize that that would be exactly the issue I was hitting. The reason why I hit it was I was testing exactly vertical scrolling, so my X displacement was exactly 0.

Also, note that the only thing that generates SIGFPE's by default is _integer_ divide by zero.

With this fixed I no longer have NaN's but I still don't see overscrolling indicators.

The first thing that is going wrong here is that overscroll indicator drawing code is only hit when flinging, not when progressively panning. Trying to understand that.

Comment 29

[Benoit Jacob [:bjacob] (mostly away)]

Currently hitting bug 781000 in debug builds. I guess I'll revert to non-debug.

Comment 30

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: NaN bug fix

Comment 31

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to Benoit Jacob [:bjacob] from comment #29)
> Currently hitting bug 781000 in debug builds. I guess I'll revert to
> non-debug.

Sorry, meant bug 810514.

Comment 32

[Benoit Jacob [:bjacob] (mostly away)]

Here is another reason for this not to work -- this time in preexisting code, apparently from Bug 750974 / commit 44c0830: in Axis::UpdateWithTouchAtDevicePoint, we have:

+  if (curVelocityIsLow || (directionChange && fabs(newVelocity) - EPSILON <= 0.0f)) {
+    mVelocity = newVelocity;
+  } else {
+    float maxChange = fabsf(mVelocity * aTimeDelta * MAX_EVENT_ACCELERATION);
+    mVelocity = NS_MIN(mVelocity + maxChange, NS_MAX(mVelocity - maxChange, newVelocity));
+  }
+
+  mVelocity = newVelocity;
+  mPos = aPos;

So the whole if() is useless, as we overwrite mVelocity anyways.

Comment 33

[Benoit Jacob [:bjacob] (mostly away)]

This function also seems unsafe wrt zero TimeDuration's: it unconditionally divides by aTimeDelta.ToMilliseconds(); but that could conceivably be zero.

Comment 34

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: call GetOverscrollAmount before exiting early in no-scroll case

OK, I think I can actually finish the work quickly now. Here are the 2 remaining issues that were preventing me from making progress:

1) we were only ever passing nonzero mXOver and mYOver when flinging, not during regular continuous scrolling (TrackTouch), because of a bug: we were returning early in the no-scrolling case before we updated mXOver and mYOver. As a result, when hitting the end of document, as scrolling stopped, we left mXOver and mYOver with their default zero value. Fixed by attached patch

2) I thought, wrongly, that the patches here were supposed to implement the blue overscrolling indicators and I had to figure why they didn't. In fact, they implement a different effect (bounce-back), which is another good reason why I couldn't get them to draw blue overscrolling indicators. Now that I understand that, I expect to be able to finish this very shortly :-)

Comment 35

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: get overscroll amount before exiting early in no-scroll case

Had to move the clamping code as well.

Comment 36

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: finally draw overscrolling effect

This works, but there are a few unknown lefts around the design.

What color do designers want? I color-picked this one from the PDF spec.

What color blending mode do designers want? That didn't seem to be described in the documents linked above. It's hard to know from the just the video, because the video only shows black text on a white background, and it's not clear what effect would be wanted in a different color situation (e.g. light text on dark background).

The present patch use standard alpha-blending (alpha*src + (1-alpha)*dst), which is clearly not what is shown in the video (The video's effect can be done e.g. by multiplying the dst color by a constant blue color, but that would look terrible with light text on dark background). It's easy to do anything you guys want, you just have to describe it precisely.

Also, I kept the various numeric parameters in the patch unchanged, but some don't match what is described in the video. For example, the overscroll effect is clamped to 1/4 the dimensions of the screen, which is far more than what the video suggests, as far as I can see. I have also kept the various acceleration formulas (ApplyCurve...) unchanged as I have no idea how much work went into it and whether it is already known to be what the designers want.

Another issue is that the black bar at the bottom of the browser, with the prev/next page arrow icons and the bookmarking star icon, is apparently hiding the overscrolling effect at the bottom unless you fling really fast. We seem to be using the wrong rectangle to draw this effect to. We get the layer's rect, so maybe we are using the wrong layer?

Comment 37

[Benoit Jacob [:bjacob] (mostly away)]

> Another issue is that the black bar at the bottom of the browser, with the
> prev/next page arrow icons and the bookmarking star icon, is apparently
> hiding the overscrolling effect at the bottom unless you fling really fast.
> We seem to be using the wrong rectangle to draw this effect to. We get the
> layer's rect, so maybe we are using the wrong layer?

In fact, the layer dimentions that we are using here (visibleRect) are 320x726 pixels. That seems wrong. Doesn't the Otoro screen have 320x640 pixels? That would explain very well why the overscrolling indicator looks too big at the top, and is very hard to ever see at the bottom (because it's drawn too far below).

We are getting this visibleRect in the usual way that layers painting works:

     aLayer->GetEffectiveVisibleRegion().GetBounds();

So why 726 pixels high?

Comment 38

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: finally draw overscrolling effect

This version gets much closer to what the UX spec video/pdf seem to want.

Works:
 - Overscrolling effect now really looks like what the UX spec wants; achieved by using just multiply-by-constant-color as our blend operation. Warning: this will not look good for light-text-on-dark-background. Fixing that issue, while retaining the effect wanted in the UX spec, would require us knowing what is the overall foreground and background color in the page, which AFAIK we don't know at the moment.
 - Overscrolling effect keeps working during scrolling in perpendicular direction.
 - In ApplyCurve, replaced x^5 by x^2 to get more progressive growth. With x^5 it looked all-or-nothing.

Does not work:
 - Top and bottom overscrolling effect still mostly hidden by chrome. We must be applying the effect to the wrong container layer (?)
 - In fact, it seems that we are applying overscrolling effects also to the container layers of the chrome elements, such as the bar at the bottom with the prev/next icons. How do we get this to be applied only to the right containerlayer of the content?
 - While scrolling in vertical direction in a newspaper page that has screen width (like http://lemonde.fr) we draw overscrolling along the left/right edge as it's hard to scroll _exactly_ in vertical direction. We need some thresholding there, but it's not clear to me how to implement it with the current design, as the code for that seems to be around GetOverscrollAmount in Axis.cpp and there we only know information about one axis at a time, so we can't easily implement logic like "don't report overscrolling in one direction if the scrolling is much faster in the perpendicular direction".

Comment 39

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: finally draw overscrolling effect

Finally got the effect applied at the right spot!

The main issue was I was effectively applying the layer transform twice (once manually on the drawing rect and once by setting it on the layer).

This patch also temporarily has ApplyCurve be a no-op, which already seems like an improvement over the current ApplyCurve; I guess I'll have to grab a designer and have him tell me which ApplyCurve function looks best to him.

More fixes coming: will reduce the effects size a bit, and most importantly, will fix the above-discussed issue where left or right edge overscrolling is triggered during vertical scrolling.

Should be all downhill now.

Comment 40

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: overscrolling effect

Alright, this is the overscolling effect implementation, working here on my Otoro.

A few questions remain in flux:

 - I didn't solve the problem of why SetEffectRenderer gets called more than once and how to avoid that. I looked a bit, but this is an issue that requires input from someone who knows the design around this IPC stuff. (Problem mentioned in Cody's original patch).

 - I also didn't look at whether it is safe that AsyncPanZoomController* mApzc; is a raw pointer (problem also mentioned in Cody's original patch).

 - there is a visual glitch visible sometimes when flinging and overscrolling at the same time: the navigation bar at the bottom will then flicker. Need some investigation.

The other question is how do we make builds and put them in the hands of Josh?

Comment 41

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: overscrolling effect

This new patch implements Chris' idea in comment 13:

(In reply to Chris Jones [:cjones] [:warhammer] from comment #13)
> >+public:
> >+  OverscrollEffectRenderer(AsyncPanZoomController* aApzc)
> >+  : mApzc(aApzc) {
> >+  }
> 
> It seems cleaner to me to have APZC hold a reference to this class, so
> that we don't have need to recreate it on every update.  But that
> would require the big code reorganization I referred to above.

and this patch also does some reorganization, moving stuff to separate OverscrollEffectRenderer.* files.

So we are no longer recreating the OverscrollEffectRenderer on every update.

The ownership problem is clarified in the following way, which is not great, but is at least safe:

* APZC holds a reference to OverscrollEffectRenderer
* ContainerLayer also holds a reference to OverscrollEffectRenderer
* OverscrollEffectRenderer has a raw pointer to APZC
* To prevent this raw pointer from going dangling, APZC clears it in its destructor.

There are two possible ways to improve this:
* Either clarify the ownership around APZC and ContainerLayer to prove that the APZC won't die too soon.
* Or just use cycle collection. I wanted to do it, but it's a bit nontrivial as the code around here uses MFBT RefPtr which doesn't support cycle collection.

Comment 42

[Benoit Jacob [:bjacob] (mostly away)]

Filed bug 820257 about adding CC support to MFBT RefPtr. I don't think this actually blocks this bug, but it's related and we use the 'Blocks' field in a loose way.

Comment 43

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Implement overscroll indicators

Finally done, 100% working from all I can tell, and ready for review!

Changes since last patch include:
 - Fixed the positioning of the effect again. Turned out that my previous approach failed to account for clip rects. Using the scissor rect seems like exactly what I had been looking for all along.
 - Fixed the size of the effect: it was depending on the page being browsed, and on the user zoom level. Now it doesn't.
 - Fixed the user experience a lot from feedback from Patryk Adamczyk in the Toronto office. Among other things, the color is now a legit color from the Firefox OS UX palette, and it's the lightest blue color in that palette to be least intrusive. Other scalar parameters tweaked as well; they are all documented in OverscrollEffectRenderer.h.
 - Fixed the glich mentioned in comment 40. The problem was OpenGL ES 2 glGetIntegerv doesn't allow querying the old BLEND_SRC/DST, only the separate RGB/ALPHA values.

Comment 44

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 691562 [details] [diff] [review]
Implement overscroll indicators

Feel free to re-r? me on the apzc parts if desired.  We need a higher bus factor on this code ;).

Comment 45

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Don't we need an additional patch defining EffectRendererOGL? The patches in bug 783919 are presumably rotten.

Anthony can probably review the APZC code, he's been working on it lately. So can I.

Comment 46

[Benoit Jacob [:bjacob] (mostly away)]

The present patch applies on top of the patches in bug 783919. Chris just told me on IRC, if I understand correctly, that in order for them to obtain r+ they will need to drop support for overriding the whole compositing and instead just allow drawing an overlay. So the present patch will need to be updated. Still, 90% of the code will be unchanged.

Comment 47

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 691562 [details] [diff] [review]
Implement overscroll indicators

Updating the patches on bug 783919; the present patch will actually be significantly simpler, as I didn't realize that we were getting a timestamp passed to the overlay drawing function, and that actually leads to a better design than having this DoOverscrollReturn function.

Comment 48

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Implement overscroll indicators

This updated patch applies on top of the new patch on bug 783919. It does an OVERLAY effect instead of overriding the whole composition. Also, while doing that I realized that the overlay effect function gets a timestamp passed to it, which makes a lot of sense, and led to some improvements in the design and a smoother correct decay effect.

Comment 49

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 692068 [details] [diff] [review]
Implement overscroll indicators

Review of attachment 692068 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/ipc/OverscrollEffectRenderer.cpp
@@ +64,5 @@
> +  GLContext *gl = aManager->gl();
> +
> +  // The scissorRect is the right way to know exactly what rect on
> +  // screen our layer gets rendered to.
> +  nsIntRect scissorRect = aLayer->CalculateScissorRect(gl->ScissorRect(), nullptr);

I don't think it necessarily is. The scissor rect corresponds to the clip rect set on the layer. There might not be a clip rect, or the layer might be transformed (e.g. rotated); the clip rect, if any, applies after the transform.

What guarantees that there is a meaningful clip rect on the layer?

::: gfx/layers/ipc/OverscrollEffectRenderer.h
@@ +54,5 @@
> + * blue color from the Firefox OS UX palette.
> + */
> +const float OVERSCROLL_EFFECT_COLOR[3] = { 178.0f/255, 242.0f/255, 255.0f/255 };
> +
> +inline float floatSign(float x)

FloatSign

Comment 50

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Implement overscroll indicators

This new version reworks the ownership so that we don't rely anymore on the EffectRenderer being refcounted.

Now it's very simple: the OverscrollEffectRenderer holds a ref to the the AsyncPanZoomController ; the AsyncPanZoomController doesn't know at all about the OverscrollEffectRenderer. Feels much cleaner, and the patch also shed 1k.

Also tweaked the effect parameters a bit further.

Comment 51

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #49)
> Comment on attachment 692068 [details] [diff] [review]
> Implement overscroll indicators
> 
> Review of attachment 692068 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/ipc/OverscrollEffectRenderer.cpp
> @@ +64,5 @@
> > +  GLContext *gl = aManager->gl();
> > +
> > +  // The scissorRect is the right way to know exactly what rect on
> > +  // screen our layer gets rendered to.
> > +  nsIntRect scissorRect = aLayer->CalculateScissorRect(gl->ScissorRect(), nullptr);
> 
> I don't think it necessarily is. The scissor rect corresponds to the clip
> rect set on the layer. There might not be a clip rect, or the layer might be
> transformed (e.g. rotated); the clip rect, if any, applies after the
> transform.
> 
> What guarantees that there is a meaningful clip rect on the layer?

I have no idea, to be honest. I just bugged my colleagues in the Toronto office for a couple of weeks asking them how to determine where on screen a given layer is painted; tried a lot of half-working solutions involving visible regions and clip rects (see above comments and obsolete patches); so when we found out that the scissor rect seemed to be what I needed, and that seemed to work perfectly on the sites I tested, I didn't question it.

If the scissor rect is not right, then please do teach me/us the right way to determine where on screen a layer ends up being painted. I'd really like to know!

Comment 52

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Implement overscroll indicators

Minor update: fixes a off-by-one-pixel glitch that was due to inconsistent rounding.

Comment 53

[Benoit Jacob [:bjacob] (mostly away)]

In fact, it seems that you are right, the scissor rect is not always the right rect. Sometimes (I don't know exactly when/why) the bottom of the rect is too far down.

Comment 54

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The layer system itself does not have the information you need. I think you want something like the transformed bounds of the scrollable element associated with the layer(s) for the scrolled content. The layer tree itself does not have that information.

The APZC does though, from the FrameMetrics.

Comment 55

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Comment on attachment 692288 [details] [diff] [review]
Implement overscroll indicators

Review of attachment 692288 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/ipc/AsyncPanZoomController.cpp
@@ +88,5 @@
> +
> +/**
> + * Speed of the overscroll decay. This is an exponential decay.
> + */
> +const float OVERSCROLL_AMPLITUDE_DECAY_SPEED = 18.0f;

Suggestion: This constant could be in more well defined units such as the number of pixels that it would overscroll in 1 second. I think this is e^18 but I'm not sure.

@@ +703,1 @@
>  

I don't think it's safe to remove the call to ScheduleComposite() here. It won't composite until the async paint has completed. This will affect panning on slow sites like nytimes.com.

@@ +796,5 @@
> +    mOverscrollAmplitudeX = 0.0f;
> +  }
> +  if (mOverscrollAmplitudeY < 1.0f && mOverscrollAmplitudeY > -1.0f) {
> +    mOverscrollAmplitudeY = 0.0f;
> +  }

Suggestion: Using abs(mOverscrollAmplitudeX) < 1.0f would show intent more clearly

::: gfx/layers/ipc/AsyncPanZoomController.h
@@ +525,5 @@
>    AxisX mX;
>    AxisY mY;
>  
> +  // Current horizontal and vertical overscroll effect amplitudes.
> +  float mOverscrollAmplitudeX, mOverscrollAmplitudeY;

Suggestion: I'd rather see a Point used instead of X and Y co-ordinates here and elsewhere.

Comment 56

[Benoit Jacob [:bjacob] (mostly away)]

Roc, Anthony: thanks for the review comments, currently trying to figure the right rect from the FrameMetrics. FYI, you can use the "r-" button without hurting my feelings, and it could be helpful in cleaning up your review queues ;-)

Comment 57

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #54)
> The layer system itself does not have the information you need. I think you
> want something like the transformed bounds of the scrollable element
> associated with the layer(s) for the scrolled content. The layer tree itself
> does not have that information.
> 
> The APZC does though, from the FrameMetrics.

I tried this:

  const FrameMetrics& frameMetrics = mApzc->GetFrameMetrics();
  gfx3DMatrix transform = aLayer->GetEffectiveTransform();
  gfx::Rect scrollableRect = frameMetrics.mScrollableRect;
  gfxRect transformedScrollableRect
    = transform.TransformBounds(gfxRect(scrollableRect.x,
                                        scrollableRect.y,
                                        scrollableRect.width,
                                        scrollableRect.height));
  transformedScrollableRect.RoundOut();
  nsIntRect visibleRect(transformedScrollableRect.x,
                        transformedScrollableRect.y,
                        transformedScrollableRect.width,
                        transformedScrollableRect.height);

This has two issues:
 1) as I scroll, the transform takes some time to catch up with scrolling, due I suppose to async scrolling.
 2) I can't see overscrolling indicators at the bottom of the page, which means that the |height| I'm computing here is too large.

Any specific idea? Is this even what you meant?

Comment 58

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

>  1) as I scroll, the transform takes some time to catch up with scrolling,
> due I suppose to async scrolling.

Try putting the ScheduleComposite() back into APZC::TrackTouch(). If you don't schedule a composite then it will not update until the async paint has finished.

Comment 59

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to Anthony Jones (:kentuckyfriedtakahe) from comment #58)
> >  1) as I scroll, the transform takes some time to catch up with scrolling,
> > due I suppose to async scrolling.
> 
> Try putting the ScheduleComposite() back into APZC::TrackTouch(). If you
> don't schedule a composite then it will not update until the async paint has
> finished.

Thanks for the tip, but that doesn't make a difference here: as I scroll, the overscrolling indicators are still off, and only catch up once I've stopped scrolling.

The "solution" with the scissorRect somehow doesn't have this problem.

Comment 60

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: address Anthony's comments + don't use the scissor Rect. Problems described in comment 57.

Comment 61

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Benoit Jacob [:bjacob] from comment #57)
>   const FrameMetrics& frameMetrics = mApzc->GetFrameMetrics();
>   gfx3DMatrix transform = aLayer->GetEffectiveTransform();
>   gfx::Rect scrollableRect = frameMetrics.mScrollableRect;

mScrollableRect isn't what you want. See FrameMetrics.h:
  // The scrollable bounds of a frame. This is determined by reflow.
  // For the top-level |window|,
  // { x = window.scrollX, y = window.scrollY, // could be 0, 0
  //   width = window.innerWidth, height = window.innerHeight }

You probably want mViewport here.

>   gfxRect transformedScrollableRect
>     = transform.TransformBounds(gfxRect(scrollableRect.x,
>                                         scrollableRect.y,
>                                         scrollableRect.width,
>                                         scrollableRect.height));
>   transformedScrollableRect.RoundOut();

Using TransformBounds isn't going to give you a good result when the transform does some rotation or skew. I guess you can ignore it for now, but really you want to be able to handle situations where the viewport does not map to an axis-aligned rectangle.

Comment 62

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Implement overscroll indicators

So indeed, the viewportRect is the right rect. Thanks!

Regarding non-translation transforms, I believe that this new patch handles them correctly: it no longer applies the transform to the rect and gets the resulting rect bounds; instead, it applies the transform only for the actual drawing:

  program->SetLayerTransform(aLayer->GetEffectiveTransform());

So that if the transform is e.g. a rotation, you'll get rotated overscrolling indicators but the logic will otherwise be unchanged.

Assuming that this sounds right, the one unresolved issue is that it is still "lagging" while scrolling, and takes a certain amount of time to catch up, that seems to depend on how long the page takes to paint. Any ideas about that? It seems that mScrollOffset tells the actuall scroll offset without lagging, but I wouldn't know the right way to make use of that information given that I need the transform and viewport anyway (and either of them must be lagging --- not sure which one yet).

Comment 63

[Benoit Jacob [:bjacob] (mostly away)]

Note: I had to multiply the viewport by (resolution / zoom) to get something that works on different sites at different zoom levels.

Comment 64

[Benoit Jacob [:bjacob] (mostly away)]

Added some logging; it is definitely the layer Transform that is lagging (see how it returns the same translation coords across multiple frames), while mScrollOffset is not lagging; but it doesn't look obvious to me how to use mScrollOffset to fix it.

I/Gecko   ( 1566): ###############################
I/Gecko   ( 1566): FrameMetrics data:
I/Gecko   ( 1566):  mZoom 1 1
I/Gecko   ( 1566):  mResolution 0.326531 0.326531
I/Gecko   ( 1566):  mDevPixelsPerCSSPixel 1
I/Gecko   ( 1566):  mScrollOffset 0 1060.11
I/Gecko   ( 1566):  Transform 0 -151
I/Gecko   ( 1566):  Viewport 0 0
I/Gecko   ( 1566): ###############################
I/Gecko   ( 1566): FrameMetrics data:
I/Gecko   ( 1566):  mZoom 1 1
I/Gecko   ( 1566):  mResolution 0.326531 0.326531
I/Gecko   ( 1566):  mDevPixelsPerCSSPixel 1
I/Gecko   ( 1566):  mScrollOffset 0 1062.26
I/Gecko   ( 1566):  Transform 0 -151
I/Gecko   ( 1566):  Viewport 0 0
I/Gecko   ( 1566): ###############################
I/Gecko   ( 1566): FrameMetrics data:
I/Gecko   ( 1566):  mZoom 1 1
I/Gecko   ( 1566):  mResolution 0.326531 0.326531
I/Gecko   ( 1566):  mDevPixelsPerCSSPixel 1
I/Gecko   ( 1566):  mScrollOffset 0 1064.15
I/Gecko   ( 1566):  Transform 0 -151
I/Gecko   ( 1566):  Viewport 0 0

Comment 65

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Benoit Jacob [:bjacob] from comment #64)
> Added some logging; it is definitely the layer Transform that is lagging
> (see how it returns the same translation coords across multiple frames),
> while mScrollOffset is not lagging; but it doesn't look obvious to me how to
> use mScrollOffset to fix it.

That is strange. You're calling GetEffectiveTransform, which calls GetLocalTransform, which should be calling GetShadowTransform, which should have the transform that CompositorParent (controlled by APZC) set for scrolling. Try dumping the base, shadow and local transforms as well as the effective transform.

Comment 66

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Comment on attachment 694023 [details] [diff] [review]
Implement overscroll indicators

Review of attachment 694023 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/ipc/AsyncPanZoomController.cpp
@@ +96,5 @@
> + * OVERSCROLL_AMPLITUDE_DECAY_SPEED = 18.0, every 0.1 seconds,
> + * the amplitudes are multiplied by
> + *     exp (-18.0 * 0.1) == exp (-1.8) == 0.17.
> + */
> +const float OVERSCROLL_AMPLITUDE_DECAY_SPEED = 18.0f;

This is just repeating the code below which doesn't add any value so you're better off without it.

The constant should be in some stated units or you could at least say something like "setting this to 18 will scroll 100 pixels in 500ms and setting it to twice the value will make it take twice as long". You want people to understand the meaning of the constant and possibly how to adjust it without having to think too hard.

@@ +103,5 @@
> +{
> +  return x > 0.f ? 1.f
> +       : x < 0.f ? -1.f
> +       : 0.f;
> +}

Use an enum rather than sentinels which in this case are just magic numbers. Perhaps something like:

enum Direction { NONE, HORIZONTAL, VERTICAL };

Direction DominantDirection(gfxPoint aPoint) { ... }

@@ +717,5 @@
> +  // if you are scrolling down a page whose width is equal to screen width, you shouldn't
> +  // see overscroll effects everytime your scrolling is not exactly vertical.
> +  if (overscrollGeneralDirection == velocityGeneralDirection) {
> +    mOverscrollAmplitudes.x += xOverscroll;
> +    mOverscrollAmplitudes.y += yOverscroll;

Suggestion: Using a point for overscroll would allow you to use mOverscrollAmplitudes += overscroll

@@ +809,5 @@
> +    mLastOverscrollDecayTime = aNow;
> +    // We decay exponentially, because that's simplest to implement and will feel natural.
> +    float decayFactor = std::exp(-OVERSCROLL_AMPLITUDE_DECAY_SPEED * decayTime);
> +    mOverscrollAmplitudes.x *= decayFactor;
> +    mOverscrollAmplitudes.y *= decayFactor;

Suggestion: mOverscrollAmplitude *= decayFactor

Comment 67

[Benoit Jacob [:bjacob] (mostly away)]

@ Anthony: thanks for the review comments, will try to make the explanation more clear/useful.

@ Roc: thanks for the tips; it seems that GetEffectiveTransform is just returning a precomputed value; I added a call to ComputeEffectiveTransforms on the root layer, but that doesn't resolve the lagging problem; I printed a lot of things around ComputeEffectiveTransforms, DefaultComputeEffectiveTransforms, etc; indeed it is calling GetLocalTransform which is calling GetShadowTransform, and the return value of GetShadowTransform itself has the lagging problem.

Comment 68

[Robert O'Callahan (:roc) (email my personal email if necessary)]

ComputeEffectiveTransforms should already be called on every layer in the tree.

I don't know why GetShadowTransform is lagging. CompositorParent::ApplyAsyncContentTransformToTree should have set it appropriately.

Comment 69

[Benoit Jacob [:bjacob] (mostly away)]

Josh: the patch is ready for your testing, except for one known issue discussed in the above comments --- that the indicators lag behind during scrolling. UX feedback welcome on all other aspects.

Comment 70

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

bjacob, (if you don't have other blockers and have cycles), do you think we can get this in before EOW?

Comment 71

[Benoit Jacob [:bjacob] (mostly away)]

I can't tell: text layout is not my area, I'm just profiling in the dark, not really knowing what to do myself with the profiles. Today (if this is still a blocker) I plan to try a different approach: minimize the page.

Comment 72

[Benoit Jacob [:bjacob] (mostly away)]

Oops ---- wrong bug, sorry!

Yes, if I get back to this bug, there is a solid chance to get it in by the end of this week, given that there is only the lag-behing-scrolling issue remaining.

Comment 73

[Benoit Jacob [:bjacob] (mostly away)]

Looks like i didn't make it. For the record, I applied the patches from bug 811950 and bug 825808, but that didn't make a visible difference.

Comment 74

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Needed for competitive parity, and fix really bad UX.

Comment 75

[Benoit Jacob [:bjacob] (mostly away)]

@Anthony: can you take a look and tell why the indicators are lagging behind during scrolling, i.e. the scroll position we're getting is retarded, and only gets synced after ~1 second?

Comment 76

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

The overscroll indicators don't seem to work for me.

Comment 77

[Benoit Jacob [:bjacob] (mostly away)]

Do you mean they're bitrotted now? Could you please go into some more detail?

Comment 78

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Implement overscroll indicators, fixed a race condition

This fixes a race condition that was causing the overscrolling indicators to fail to work depending on luck --- definitely what Anthony was seeing.

The overscrolling indicators seem to work as before now.

The issue, that their metrics are 'lagging behind' during scrolling (only updated a few times per second during smooth scrolling), is still present.

Also, I'm not sure if that issue was already present, so let's assume it was: the metrics seem just wrong at times. Especially scrolling to the bottom of a page, reveals that the bottom indicator is often rendered to high or too low on the page, making it sometimes completely unvisible.

In short, we just have a metrics problem here before this is good to go.

Comment 79

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Video of my test

I'm not sure what it is supposed to look like but this is what I'm seeing.

Comment 80

[Benoit Jacob [:bjacob] (mostly away)]

Yes, that's exactly what it's supposed to look like.

Here you tried the top of the page, which is already working correctly. The problems start when you scroll downwards while you have a _sideways_ overscroll indicator painted. It will scroll a bit with the page, only catching up about once per second depending on the page, whereas it should never appear to scroll with the page. (Sideways overscroll indicators should occupy the entire height of the web view area at all times, just like the top-overscroll indicator in your video occupies the entire width of the web view area).

The other issue, possibly linked to the first, is that if you scroll to the bottom of the page, the overscroll indicator there may be painted at an incorrect Y coordinate, indicating that there is something wrong in how we query some Y or Height parameter.

Comment 81

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

OverscrollEffectRenderer::ApplyOverlayEffect() is calling GetFrameMetrics on the layer which returns the frame metrics that were sent from the content process. These are from the last paint and aren't updated during scroll.

There are two ways this could be fixed:

1. Updating mViewport in the frame metrics when scrolling; don't know what this would affect
2. Use the transform that matches the frame metrics which is unadjusted by scrolling

Comment 82

[Benoit Jacob [:bjacob] (mostly away)]

Thanks for the help; I would need more detailed instructions, not knowing much around frame metrics:
 - I know much less than you what 1. would affect, and it's not clear to me how to implement that.
 - I don't know how to do 2.

Comment 83

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Bug 775469 - Use content transform for overscroll indicators

Comment 84

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

I've attached a patch which appears to solve the problem. I haven't tested it extensively but it does prove my understanding of what is causing the problem.

Comment 85

[Benoit Jacob [:bjacob] (mostly away)]

Sorry about the delay, was assigned several other B2G emergencies (most recently bug 862324).

Tested Anthony's patch.

It does solve the lag-behind-scrolling issue. It works perfectly on lemonde.fr.

But on nytimes.com, it introduces a new problem: the overscroll indicators are now drawn in a tiny rectange at the upper-left corner of the screen. The aspect ration of that rectangle does not match that of the sceen or browser view.

At this point I want to ask:
 - Anthony: what might be the reason for the above-described regression with your patch on nytimes.com?
 - Should we still work on this for b2g18 or rebase this work to mozilla-central?
 - Should I still be the sole assignee for this bug? Anthony is far more knowledgeable about AsyncPanZoomController and I have enough high-priority B2G gfx bugs to keep me busy at the moment.
 - For example, if we want to switch to mozilla-central, I could provide the mozilla-central equivalent of the b2g18 patch on bug 783919 to enable overlay effects, and let AsyncPanZoomController specialists handle the rest?

Comment 86

[Benoit Jacob [:bjacob] (mostly away)]

Flagging a bunch of people :needinfo. See previous comment.
 - Anthony: please comment on the nytimes.com regression
 - Josh/Dietrich: please comment from a b2g perspective, is this still wanted for b2g18, should we move to mozilla-central
 - Roc, Milan: who should be working on this? Does my suggestion at the bottom of comment 85 sound reasonable?

Comment 87

[Milan Sreckovic [:milan] (needinfo for best results)]

I would rather this gets handed off to one of the people looking at ASPZ controller work, even though the over-scroll indicators are not likely to be in the first batch of functionality implemented.  I'll re-assign to ajones as a place holder.

Comment 88

[Josh Carpenter [:jcarpenter]]

(In reply to Benoit Jacob [:bjacob] from comment #86)
> Flagging a bunch of people :needinfo. See previous comment.
>  - Anthony: please comment on the nytimes.com regression
>  - Josh/Dietrich: please comment from a b2g perspective, is this still
> wanted for b2g18, should we move to mozilla-central

We're not concerned with landing this for 1.1, although it would be a nice to have. We (UX) is more focused on working with devs to land something for 1.2 timeline (which should pick up Gecko 23 or 24, AFAIK). We'd also like to use the longer timelines there to revisit the visual treatment we're using. The blue glow was always meant to be a stop gap. With the benefit of more time we may be able to explore alternative, more attractive approaches.

Comment 89

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

My change clearly fixes the offset but not the scale. The transform calculations are clearly wrong.