Closed
Bug 824511
Opened 12 years ago
Closed 12 years ago
Remove Axis.cpp?mark=76,79,82#73 dead code and make Axis less sensitive to random move events distance
Categories
(Core :: Widget, defect)
Tracking
()
RESOLVED
FIXED
mozilla22
People
(Reporter: romaxa, Assigned: tatiana)
References
Details
Attachments
(1 file, 5 obsolete files)
(deleted),
patch
|
tatiana
:
review+
|
Details | Diff | Splinter Review |
http://mxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/Axis.cpp?mark=76,79,82#73
Line 82 has code which make 73-80 lines wasting of time...
Comment 1•12 years ago
|
||
+cjones
I think cjones was going to patch this out or at least brought it up before. Not sure what happened to that. Also not sure if it was him or someone else.
Comment 2•12 years ago
|
||
For reference though, yeah, this code is clearly not working as intended. It came from here:
https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/ui/Axis.java#192
Since we don't know how important this code was and the Java code has had a lot of work put into it, I think the best way to go is remove l82 and make sure it doesn't break anything.
I can take this.
Assignee: nobody → bugzilla
Reporter | ||
Comment 3•12 years ago
|
||
not sure how it works in Java code, but on first velocity check, we have mVelocity == 0, and new Velocity > minimal
so we endup with second condition line 78, where maxChange become 0.
Also it does not make sense to calculate velocity when aPos == mPos...
I guess this code worked in Java because apos/mpos - are float.
Attachment #695535 -
Flags: feedback?(bugzilla)
Comment 4•12 years ago
|
||
Comment on attachment 695535 [details] [diff] [review]
Remove dead code.
Review of attachment 695535 [details] [diff] [review]:
-----------------------------------------------------------------
::: gfx/layers/ipc/Axis.cpp
@@ +77,4 @@
>
> // If a direction change has happened, or the current velocity due to this new
> // touch is relatively low, then just apply it. If not, throttle it.
> + if (!mVelocity || curVelocityIsLow || (directionChange && fabs(newVelocity) - EPSILON <= 0.0f)) {
This will have unintended consequences. If the velocity is 0 and we get some garbage point that's very far away we'll accelerate very quickly to it, and the acceleration capping code won't get a chance to reduce it.
Attachment #695535 -
Flags: feedback?(bugzilla) → feedback-
Reporter | ||
Comment 6•12 years ago
|
||
This should be better, just use default 1.0 value when mVelocity == 0, also change MAX_EVENT_ACCELERATION to 12 same as fennec java code
Attachment #695535 -
Attachment is obsolete: true
Attachment #696458 -
Flags: review?(bugzilla)
Reporter | ||
Comment 7•12 years ago
|
||
Comment on attachment 696458 [details] [diff] [review]
Use cap limit functionality for asyncPanZoomController
No this does not look right either
with this cap implementation we limiting velocity in such way that when velocity is very low during fling (end of kinetic motion) and one more motion performed with normal speed (even fast), cap limit maxChange calculated as very low value, and does not allow to speedup fling in this case.
Attachment #696458 -
Flags: review?(bugzilla)
Reporter | ||
Comment 8•12 years ago
|
||
I got it working good only when velocity cap function was simple as this
mVelocity = NS_MIN(NS_MAX(newVelocity, -MAX_EVENT_ACCELERATION), MAX_EVENT_ACCELERATION);
just limit it by hard...
Reporter | ||
Updated•12 years ago
|
Summary: Axis.cpp?mark=76,79,82#73 seems contain dead/wrong code → Remove Axis.cpp?mark=76,79,82#73 dead code and make Axis less sensitive to random move events distance
Assignee | ||
Comment 9•12 years ago
|
||
Assignee: romaxa → tanya.meshkova
Attachment #696458 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #712716 -
Flags: review?(bugzilla)
Assignee | ||
Comment 10•12 years ago
|
||
CancelTouch handling is missing in previous version, fixed here.
Attachment #712716 -
Attachment is obsolete: true
Attachment #712716 -
Flags: review?(bugzilla)
Attachment #712831 -
Flags: review?(bugzilla)
Comment 11•12 years ago
|
||
Comment on attachment 712831 [details] [diff] [review]
use last five velocity records (v2)
Review of attachment 712831 [details] [diff] [review]:
-----------------------------------------------------------------
Hey, sorry about the late review. I should be able to get to them much more quickly now. Here it is.
::: gfx/layers/ipc/Axis.cpp
@@ +51,5 @@
> */
> static float gFlingStoppedThreshold = 0.01f;
>
> +/**
> + * Maximum size of velocity queue
I'd prefer a more useful comment explaining what the velocity queue is.
::: gfx/layers/ipc/Axis.h
@@ +9,5 @@
>
> #include "nsGUIEvent.h"
> #include "mozilla/TimeStamp.h"
> #include "mozilla/gfx/2D.h"
> +#include <queue>
I'd prefer to just use an nsTArray for this.
Attachment #712831 -
Flags: review?(bugzilla)
Assignee | ||
Comment 12•12 years ago
|
||
Attachment #712831 -
Attachment is obsolete: true
Attachment #716275 -
Flags: review?(bugzilla)
Comment 13•12 years ago
|
||
Comment on attachment 716275 [details] [diff] [review]
use nsTArray for velocity queue
Review of attachment 716275 [details] [diff] [review]:
-----------------------------------------------------------------
Good. Just 2 changes.
::: gfx/layers/ipc/Axis.cpp
@@ +52,5 @@
> static float gFlingStoppedThreshold = 0.01f;
>
> +/**
> + * Maximum size of velocity queue. The queue contains last N velocity records.
> + * On touch end we calculate the average velocity
Fix spacing here please. This is an awkwardly placed line break.
@@ +64,5 @@
> Preferences::AddFloatVarCache(&gFlingFriction, "gfx.axis.fling_friction", gFlingFriction);
> Preferences::AddFloatVarCache(&gVelocityThreshold, "gfx.axis.velocity_threshold", gVelocityThreshold);
> Preferences::AddFloatVarCache(&gAccelerationMultiplier, "gfx.axis.acceleration_multiplier", gAccelerationMultiplier);
> Preferences::AddFloatVarCache(&gFlingStoppedThreshold, "gfx.axis.fling_stopped_threshold", gFlingStoppedThreshold);
> + Preferences::AddIntVarCache(&gMaxVelocityQueueSize, "gfx.max_velocity_queue", gMaxVelocityQueueSize);
"gfx.axis.max_velocity_queue_size"
Attachment #716275 -
Flags: review?(bugzilla) → review+
Assignee | ||
Comment 14•12 years ago
|
||
moving r+ from the previous patch
Attachment #716275 -
Attachment is obsolete: true
Attachment #717203 -
Flags: review+
Assignee | ||
Updated•12 years ago
|
Keywords: checkin-needed
Comment 15•12 years ago
|
||
Keywords: checkin-needed
Comment 16•12 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
You need to log in
before you can comment on or make changes to this bug.
Description
•