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

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

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.

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 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 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 :)

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

overscroll for pre-ICS

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

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

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.