Last Comment Bug 779572 - Zooming is not taken into account for min pan distance
: Zooming is not taken into account for min pan distance
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics: Layers (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla17
Assigned To: Doug Sherk (:drs) (inactive)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-08-01 11:30 PDT by Doug Sherk (:drs) (inactive)
Modified: 2012-08-09 04:52 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Factor zoom into min pan distance before moving the viewport (1.13 KB, patch)
2012-08-01 11:31 PDT, Doug Sherk (:drs) (inactive)
no flags Details | Diff | Review
Factor zoom into min pan distance before moving the viewport (1.26 KB, patch)
2012-08-03 22:05 PDT, Doug Sherk (:drs) (inactive)
no flags Details | Diff | Review
Factor zoom into min pan distance before moving the viewport (1.76 KB, patch)
2012-08-04 01:00 PDT, Doug Sherk (:drs) (inactive)
no flags Details | Diff | Review
Factor zoom into min pan distance before moving the viewport (1.62 KB, patch)
2012-08-07 13:54 PDT, Doug Sherk (:drs) (inactive)
cjones.bugs: review+
Details | Diff | Review
Properly protect state in AsyncPanZoomController with a monitor (8.33 KB, patch)
2012-08-07 13:54 PDT, Doug Sherk (:drs) (inactive)
cjones.bugs: review+
Details | Diff | Review

Description Doug Sherk (:drs) (inactive) 2012-08-01 11:30:11 PDT

    
Comment 1 Doug Sherk (:drs) (inactive) 2012-08-01 11:31:54 PDT
Created attachment 648022 [details] [diff] [review]
Factor zoom into min pan distance before moving the viewport

When we're zoomed out really far, we have to move our finger across almost the entire screen just to start moving it. This fixes that.
Comment 2 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-08-01 18:38:34 PDT
Comment on attachment 648022 [details] [diff] [review]
Factor zoom into min pan distance before moving the viewport

>diff --git a/gfx/layers/ipc/AsyncPanZoomController.cpp b/gfx/layers/ipc/AsyncPanZoomController.cpp

>+      MonitorAutoLock monitor(mMonitor);
>+      if (PanDistance() / mFrameMetrics.mResolution.width < panThreshold) {
>         return nsEventStatus_eIgnore;
>       }
>+

Let's keep the locking scoped to this computation so that we don't
hold it in StartPanning().

And hm, do we need to be locking in StartPanning()?
Comment 3 Doug Sherk (:drs) (inactive) 2012-08-03 22:05:26 PDT
Created attachment 648947 [details] [diff] [review]
Factor zoom into min pan distance before moving the viewport
Comment 4 Doug Sherk (:drs) (inactive) 2012-08-04 01:00:08 PDT
Created attachment 648971 [details] [diff] [review]
Factor zoom into min pan distance before moving the viewport

herp derp
Comment 5 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-08-06 12:10:27 PDT
Comment on attachment 648971 [details] [diff] [review]
Factor zoom into min pan distance before moving the viewport

Document that StartPanning() needs the state mutex and assert that it's held in the function.
Comment 6 Doug Sherk (:drs) (inactive) 2012-08-07 13:53:31 PDT
Ok, so it turns out that StartPanning should not be protected except for its change to |mState|, which was not being properly protected anywhere else. I have removed the monitor holding from that and will be re-adding it back in a followup patch which will protect all changes to |mState| properly.
Comment 7 Doug Sherk (:drs) (inactive) 2012-08-07 13:54:01 PDT
Created attachment 649792 [details] [diff] [review]
Factor zoom into min pan distance before moving the viewport
Comment 8 Doug Sherk (:drs) (inactive) 2012-08-07 13:54:38 PDT
Created attachment 649794 [details] [diff] [review]
Properly protect state in AsyncPanZoomController with a monitor
Comment 10 Phil Ringnalda (:philor) 2012-08-07 21:07:44 PDT
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/d8878c0bd67c - something in that push made Linux oddly unhappy.

Note You need to log in before you can comment on or make changes to this bug.