Closed Bug 930059 Opened 6 years ago Closed 6 years ago

Overscroll.java doesn't need to use compatibility libraries

Categories

(Firefox for Android :: Toolbar, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 28

People

(Reporter: blassey, Assigned: blassey)

References

Details

Attachments

(3 files, 3 obsolete files)

Attached patch fix_overscroll.patch (obsolete) — Splinter Review
We're using the compatibility libraries from API level 4, but have a minimum requirement of API level 9.
Attachment #821069 - Flags: review?(wjohnston)
Blocks: 930072
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).
Attached patch fix_overscroll.patch (obsolete) — Splinter Review
Attachment #821069 - Attachment is obsolete: true
Attachment #821069 - Flags: review?(wjohnston)
Attachment #8334962 - Flags: review?(wjohnston)
Attachment #8334962 - Attachment is obsolete: true
Attachment #8334962 - Flags: review?(wjohnston)
Attachment #8334984 - Flags: review?(wjohnston)
Attached patch no_compat_lib.patch (obsolete) — Splinter Review
Assignee: nobody → blassey.bugs
Attachment #8334987 - Flags: review?(wjohnston)
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
Attachment #8334984 - Flags: review?(wjohnston) → review+
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 :)
Attachment #8334987 - Flags: review?(wjohnston) → review-
overscroll for pre-ICS
Attachment #8334987 - Attachment is obsolete: true
Attachment #8335059 - Flags: review?(wjohnston)
Attachment #8335060 - Flags: review?(wjohnston)
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
Attachment #8335060 - Flags: review?(wjohnston) → review+
https://hg.mozilla.org/mozilla-central/rev/c2c1b31a62f1
https://hg.mozilla.org/mozilla-central/rev/e9215c8d444c
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 28
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.
Attachment #8335059 - Flags: review?(wjohnston) → review-
You need to log in before you can comment on or make changes to this bug.