1063227 - Make axis locking constants preffable

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Botond Ballo [:botond]]

The axis locking code added in bug 984794 uses a few constants [1] that are hard-coded. We should make these preffable so they can be adjusted more easily.

[1] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/AsyncPanZoomController.cpp?rev=9fe5fe620802#322

Comment 1

[Botond Ballo [:botond]]

Attachment: bug1063227.patch

Comment 2

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8485094 [details] [diff] [review]
bug1063227.patch

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

r+ with comments addressed

::: gfx/layers/apz/src/AsyncPanZoomController.cpp
@@ +170,5 @@
> + * If the angle from an axis to the line drawn by a pan move is less than
> + * this value, we can assume that panning can be done in the allowed direction
> + * (horizontal or vertical).
> + * Currently used only for touch-action css property stuff and was addded to
> + * keep behaviour consistent with IE.

Units: radians

::: modules/libpref/init/all.js
@@ +442,5 @@
> +pref("apz.axis_lock.mode", 0);
> +pref("apz.axis_lock.lock_angle", 0.5235987);        // PI / 6 (30 degrees)
> +pref("apz.axis_lock.breakout_threshold", 0.03125);  // 1/32 inches
> +pref("apz.axis_lock.breakout_angle", 0.3926991);    // PI / 8 (22.5 degrees)
> +pref("apz.axis_lock.direct_pan_angle", 1.047197);   // PI / 3 (60 degrees)

All the float values need to be strings

Comment 3

[Botond Ballo [:botond]]

Attachment: Make APZ axis locking constants preffable

Comment 4

[Botond Ballo [:botond]]

Comment on attachment 8485286 [details] [diff] [review]
Make APZ axis locking constants preffable

(Sorry, experimenting with bzexport.)

This patch addresses the review comments. Carrying r+.

Comment 5

[Botond Ballo [:botond]]

Try push that includes this patch: https://tbpl.mozilla.org/?tree=Try&rev=f29f913e155b

Comment 6

[Botond Ballo [:botond]]

Attachment: Make APZ axis locking constants preffable

Had some Windows failures, updated patch to fix them. I'd like to get re-review on the update, but I'll wait until I have a clean Try push before flagging.

New Try push: https://tbpl.mozilla.org/?tree=Try&rev=fafdcf7d6c7f

Comment 7

[Botond Ballo [:botond]]

Comment on attachment 8485315 [details] [diff] [review]
Make APZ axis locking constants preffable

Re-requesting review for the added M_PI stuff in gfxPrefs.h.

Comment 8

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8485315 [details] [diff] [review]
Make APZ axis locking constants preffable

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

::: gfx/thebes/gfxPrefs.h
@@ +12,5 @@
>  
> +#include <math.h>  // for M_PI
> +
> +// Not all platforms provide M_PI in <math.h>
> +#ifndef M_PI

I'd rather use #include "mozilla/Constants.h"

Comment 9

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8485315 [details] [diff] [review]
Make APZ axis locking constants preffable

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

.. although if you make the change, r+. So I might as well say r=me with comment addressed.

Comment 10

[Botond Ballo [:botond]]

Attachment: Make APZ axis locking constants preffable

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #8)
> ::: gfx/thebes/gfxPrefs.h
> @@ +12,5 @@
> >  
> > +#include <math.h>  // for M_PI
> > +
> > +// Not all platforms provide M_PI in <math.h>
> > +#ifndef M_PI
> 
> I'd rather use #include "mozilla/Constants.h"

Oh, good find! Updated to use this, carrying r+.

Comment 11

[Botond Ballo [:botond]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d0429319c564

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d0429319c564