The default bug view has changed. See this FAQ.

Zooming is not taken into account for min pan distance

RESOLVED FIXED in mozilla17

Status

()

Core
Graphics: Layers
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: drs, Assigned: drs)

Tracking

unspecified
mozilla17
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

Comment hidden (empty)
(Assignee)

Comment 1

5 years ago
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.
Assignee: nobody → bugzilla
Attachment #648022 - Flags: review?(jones.chris.g)
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()?
Attachment #648022 - Flags: review?(jones.chris.g)
(Assignee)

Comment 3

5 years ago
Created attachment 648947 [details] [diff] [review]
Factor zoom into min pan distance before moving the viewport
Attachment #648022 - Attachment is obsolete: true
Attachment #648947 - Flags: review?(jones.chris.g)
(Assignee)

Comment 4

5 years ago
Created attachment 648971 [details] [diff] [review]
Factor zoom into min pan distance before moving the viewport

herp derp
Attachment #648947 - Attachment is obsolete: true
Attachment #648947 - Flags: review?(jones.chris.g)
Attachment #648971 - Flags: review?(jones.chris.g)
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.
Attachment #648971 - Flags: review?(jones.chris.g)
(Assignee)

Comment 6

5 years ago
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.
(Assignee)

Comment 7

5 years ago
Created attachment 649792 [details] [diff] [review]
Factor zoom into min pan distance before moving the viewport
Attachment #648971 - Attachment is obsolete: true
Attachment #649792 - Flags: review?(jones.chris.g)
(Assignee)

Comment 8

5 years ago
Created attachment 649794 [details] [diff] [review]
Properly protect state in AsyncPanZoomController with a monitor
Attachment #649794 - Flags: review?(jones.chris.g)
Attachment #649792 - Flags: review?(jones.chris.g) → review+
Attachment #649794 - Flags: review?(jones.chris.g) → review+
(Assignee)

Comment 9

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/4d52cfac7c37
https://hg.mozilla.org/integration/mozilla-inbound/rev/ce9014442e13
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/d8878c0bd67c - something in that push made Linux oddly unhappy.
(Assignee)

Comment 11

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/7f25c1478701
https://hg.mozilla.org/integration/mozilla-inbound/rev/b468eccf7261
https://hg.mozilla.org/mozilla-central/rev/7f25c1478701
https://hg.mozilla.org/mozilla-central/rev/b468eccf7261
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in before you can comment on or make changes to this bug.