Closed Bug 1280656 Opened 3 years ago Closed 3 years ago

Use Android ViewConfiguration to get max fling velocity.

Categories

(Firefox for Android :: Toolbar, defect, P3)

defect

Tracking

()

VERIFIED FIXED
Firefox 50
Tracking Status
firefox48 --- verified
firefox49 --- verified
firefox50 --- verified

People

(Reporter: rbarker, Assigned: rbarker)

References

Details

Attachments

(1 file, 1 obsolete file)

The native Android fling animation should use ViewConfiguration to get the max fling speed.
Assignee: nobody → rbarker
Comment on attachment 8763259 [details] [diff] [review]
0001-Bug-1280656-Use-Android-ViewConfiguration-to-get-max-fling-velocity.-r-16061717-f328aee.patch

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

::: gfx/layers/apz/src/AndroidAPZ.cpp
@@ +106,5 @@
>    if (length > 0.0f) {
>      mFlingDirection = velocity / length;
> +
> +    if (length > sMaxFlingSpeed) {
> +       velocity = mFlingDirection * sMaxFlingSpeed;

You should skip the clamping if sMaxFlingSpeed is 0, like if it couldn't be read properly.
Attachment #8763261 - Flags: review?(botond) → review?(bugmail.mozilla)
(Sorry for the slow response time on reviews. I'm at a C++ standards meeting this week, and while I've been trying to squeeze some time to do some reviews, amidst the evening sessions and impromptu discussions I haven't managed to yet.)
Depends on: 1276463
Comment on attachment 8763261 [details] [diff] [review]
0001-Bug-1280656-Use-Android-ViewConfiguration-to-get-max-fling-velocity.-r-16061717-33eacc9.patch

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

LGTM, seems pretty straightforward.
Attachment #8763261 - Flags: review?(bugmail.mozilla) → review+
Pushed by rbarker@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b9f69e5a6565
Use Android ViewConfiguration to get max fling velocity. r=kats
https://hg.mozilla.org/mozilla-central/rev/b9f69e5a6565
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
Flag for uplift?
Flags: needinfo?(rbarker)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #8)
> Flag for uplift?

I'm not sure. On my (mid to low range) devices it doesn't seem to make much of a difference but I believe on devices with more powerful CPUs, where it is more able to keep up with paint requests, this patch appears to reduce checker boarding a little.
Flags: needinfo?(rbarker)
Bug 1279433 at least seems to be better, probably because of this. I vote we uplift it. Can you request approval?
Flags: needinfo?(rbarker)
Comment on attachment 8763261 [details] [diff] [review]
0001-Bug-1280656-Use-Android-ViewConfiguration-to-get-max-fling-velocity.-r-16061717-33eacc9.patch

Approval Request Comment
[Feature/regressing bug #]: Fling velocity exceeds the maximum speed which allows fennec to checker board more easily.
[User impact if declined]: Appears to increase checker boarding
[Describe test coverage new/current, TreeHerder]: just manual testing
[Risks and why]: Minimal. Code prevents the starting velocity of a fling from exceeding a maximum speed.
[String/UUID change made/needed]: none.
Flags: needinfo?(rbarker)
Attachment #8763261 - Flags: approval-mozilla-beta?
Attachment #8763261 - Flags: approval-mozilla-aurora?
Comment on attachment 8763261 [details] [diff] [review]
0001-Bug-1280656-Use-Android-ViewConfiguration-to-get-max-fling-velocity.-r-16061717-33eacc9.patch

OK, improve the situation, taking it.

Should be in 48 beta 6
Attachment #8763261 - Flags: approval-mozilla-beta?
Attachment #8763261 - Flags: approval-mozilla-beta+
Attachment #8763261 - Flags: approval-mozilla-aurora?
Attachment #8763261 - Flags: approval-mozilla-aurora+
QE, could you verify this one (bug 1279433 has some str)
Flags: qe-verify+
I was able to reproduce this issue consistently on Firefox 48 Beta 4 on Samsung Galaxy Tab S2 (Android 5.0.2). 
I am not seeing this issue on Firefox 48 Beta 6, latest Nightly and Aurora, on the same device.
Marking this as verified fixed; we might want to check if this issue still persist on another device, to open up another bug.
Based on comment 16 I will remove the qe-verify flag, thanks.
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.