930059 - Overscroll.java doesn't need to use compatibility libraries

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Toolbar, defect)

Description

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: fix_overscroll.patch

We're using the compatibility libraries from API level 4, but have a minimum requirement of API level 9.

Comment 1

[Wesley Johnston (:wesj)]

I'm confused. EdgeEffect is an API14 thing. I don't think we can use it on API9 devices without the compat libraries.

Comment 2

[Brad Lassey [:blassey] (use needinfo?)]

strange, the docs for EdgeEffectCompat say that EdgeEffect was introduced in version 4:

"Helper for accessing EdgeEffect introduced after API level 4 in a backwards compatible fashion."

It goes on to say:
"This class is used to access EdgeEffect on platform versions that support it. When running on older platforms it will result in no-ops. It should be used by views that wish to use the standard Android visual effects at the edges of scrolling containers."

If all we're getting from this is no-ops on older platforms, I'd rather test the API level and bail rather than require embedders to include the compatibility libs.

Comment 3

[Wesley Johnston (:wesj)]

EdgeEffect was definitely added in API14. If we want to avoid the no-ops, we could avoid instantiating an Overscroll object here:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/gfx/LayerView.java#113

and just protect all the calls with a null check? I'd rather do that (and then flip this to use EdgeEffect).

Comment 4

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: fix_overscroll.patch

Comment 5

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: fix_overscroll.patch

Comment 6

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: no_compat_lib.patch

Comment 7

[Wesley Johnston (:wesj)]

Comment on attachment 8334984 [details] [diff] [review]
fix_overscroll.patch

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

::: mobile/android/base/gfx/LayerView.java
@@ +112,5 @@
>          mTouchInterceptors = new ArrayList<TouchEventInterceptor>();
> +        if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.ICE_CREAM_SANDWICH) {
> +            mOverscroll = new OverscrollEdgeEffect(this);
> +        } else {
> +            mOverscroll = null;

not needed. mOverscroll should just be null by default. Also, a little comment here about the blue highlight being no-op on GB would be nice.

@@ +120,5 @@
>  
>      public void initializeView(EventDispatcher eventDispatcher) {
>          mLayerClient = new GeckoLayerClient(getContext(), this, eventDispatcher);
> +        if (mOverscroll != null)
> +            mLayerClient.setOverscrollHandler(mOverscroll);

Wrap in parenthesis.

@@ +528,5 @@
>              mListener.sizeChanged(width, height);
>          }
>  
> +        if (mOverscroll != null)
> +            mOverscroll.setSize(width, height);

wrap

@@ +540,5 @@
>          }
>  
> +
> +        if (mOverscroll != null)
> +            mOverscroll.setSize(width, height);

wrap

Comment 8

[Wesley Johnston (:wesj)]

Comment on attachment 8334987 [details] [diff] [review]
no_compat_lib.patch

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

We rely on this a lot. ProGuard should help us only ship what we need (but that's increasingly become all of it :)

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Backed out for test bustages.
https://hg.mozilla.org/integration/fx-team/rev/0b0f7dd39c38

https://tbpl.mozilla.org/php/getParsedLog.php?id=30801994&tree=Fx-Team
https://tbpl.mozilla.org/php/getParsedLog.php?id=30804032&tree=Fx-Team
https://tbpl.mozilla.org/php/getParsedLog.php?id=30801698&tree=Fx-Team
https://tbpl.mozilla.org/php/getParsedLog.php?id=30801837&tree=Fx-Team

Comment 10

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: overscroll_base.patch

overscroll for pre-ICS

Comment 11

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: JB_invalidate.patch

Comment 12

[Wesley Johnston (:wesj)]

Comment on attachment 8335060 [details] [diff] [review]
JB_invalidate.patch

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

I trust you can do this, but feel free to ask for r? again if you want. A little comment indicating where this code is from might be nice....

::: mobile/android/base/gfx/OverscrollEdgeEffect.java
@@ +57,5 @@
> +    private void invalidate() {
> +        if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.JELLY_BEAN) {
> +            mView.postInvalidateOnAnimation();
> +        } else {
> +            mView.postInvalidate();

We can steal from the ViewCompat here. They use:

view.postInvalidateDelayed(getFrameTime()); // getFrameTime hardcoded to 10?

http://androidxref.com/4.2.2_r1/xref/frameworks/support/v4/java/android/support/v4/view/ViewCompat.java#171

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c2c1b31a62f1
https://hg.mozilla.org/mozilla-central/rev/e9215c8d444c

Comment 14

[Wesley Johnston (:wesj)]

Comment on attachment 8335059 [details] [diff] [review]
overscroll_base.patch

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

I think we probably need something prettier than yellow here, but there are some code problems too...

::: mobile/android/base/gfx/OverscrollBase.java
@@ +22,5 @@
>      private static final int LEFT = 2;
>      private static final int RIGHT = 3;
>  
> +    private static final float kBaseStroke = 10;
> +    private static final float kMaxStroke = 40;

You probably need to adjust these based on dpi. Easiest way is to put them in dimensions/values/dimens.xml and then read them out with v.getContext().getResources().getDimensionPixelSize(R.id.my_dimension)

@@ +35,5 @@
> +    class Task extends TimerTask {
> +        public void run() {
> +            mShowEffect[dir] = false;
> +            setStroke(mPaint[dir], kBaseStroke);
> +            mView.postInvalidate();

This will hide this instantly after some timeout. It feels a bit strange maybe we can make this really short. 100ms?

@@ +37,5 @@
> +            mShowEffect[dir] = false;
> +            setStroke(mPaint[dir], kBaseStroke);
> +            mView.postInvalidate();
> +        }
> +        int dir;

private int mDir;

@@ +80,5 @@
> +            mTask[edge].cancel();
> +            mTask[edge] = null;
> +        }
> +        float stroke = mPaint[edge].getStrokeWidth();
> +        float abs_dist = Math.min(kMaxStroke, Math.abs(velocity));

We probably need to multiply this velocity by something. I'd start by looking at what EdgeEffect does:

http://androidxref.com/4.2.2_r1/xref/frameworks/base/core/java/android/widget/EdgeEffect.java#304

@@ +96,3 @@
>      }
>  
>      public void setDistance(final float distance, final Axis axis) {

This distance is actually a delta. We could rename it to dx if that makes it clearer to implementors.

@@ +103,3 @@
>          }
> +        float stroke = mPaint[edge].getStrokeWidth();
> +        float abs_dist = Math.min(kMaxStroke, Math.abs((int)distance));

And this cast to int is pushing distance to zero for most of these updates. But this should probably be something like:

max(0, min(maxStroke, distance + stroke));

although that causes some strange issues too. I'll leave it to you to figure out :)

@@ +123,1 @@
>          }

this whole invalidate |= bit is a bit wasted. You don't use it, and the draw() methods always return true.