962826 - Multiply by DPI inside AsyncPanZoomController::GetTouchStartTolerance

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

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

At [1], we call GetTouchStartTolerance and multiply it by the DPI (from the tree manager). At [2] we use gTouchStartTolerance multiplied by the DPI (which is the exact same thing). It would be cleaner to move the DPI multiplication inside GetTouchStartTolerance, and to use the helper function at [2] instead of using gTouchStartTolerance directly. This is purely refactoring, should be a straightforward change for a first-time contributor.

[1] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/GestureEventListener.cpp?rev=d1d7d42059c8#108
[2] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/AsyncPanZoomController.cpp?rev=d1d7d42059c8#594

Comment 1

[Botond Ballo [:botond]]

Stealing mentorship.

Comment 2

[Snigdha Agarwal]

Attachment: GetTouchStartTolerance() returns the product now

Comment 3

[Botond Ballo [:botond]]

(In reply to snigdhaagarwal93 from comment #2)
> Created attachment 8364981 [details] [diff] [review]
> GetTouchStartTolerance() returns the product now

Thanks for working on this! It looks like the attached patch is empty though - could you reupload it please?

Comment 4

[Snigdha Agarwal]

Attachment: GetTouchStartTolerance() returns the product now

Sorry for the empty patch earlier. 
This one has all the changes.

Comment 5

[Botond Ballo [:botond]]

Comment on attachment 8365506 [details] [diff] [review]
GetTouchStartTolerance() returns the product now

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

Thanks for the patch! I have one small suggestion:

::: gfx/layers/ipc/GestureEventListener.cpp
@@ +103,5 @@
>    case MultiTouchInput::MULTITOUCH_MOVE: {
>      // If we move too much, bail out of the tap.
>      ScreenIntPoint delta = event.mTouches[0].mScreenPoint - mTouchStartPosition;
>      if (mTouches.Length() == 1 &&
> +        NS_hypot(delta.x, delta.y) > mAsyncPanZoomController->GetTouchStartTolerance())

Since we're cleaning up this code anyways, let's clean up one more thing: GetTouchStartTolerance() is a static method of AsyncPanZoomController, so there is no need to call it through an instance. Let's call it as "AsyncPanZoomController::GetTouchStartTolerance()" instead.

Comment 6

[Snigdha Agarwal]

Attachment: Object call to GetTouchStartTolerance() removed

Updated Patch.

Comment 7

[Botond Ballo [:botond]]

Comment on attachment 8367905 [details] [diff] [review]
Object call to GetTouchStartTolerance() removed

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

Great, thanks!

You can have the patch be committed by adding "checkin-needed" to the Keywords field of the bug.

Comment 8

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #7)
> You can have the patch be committed by adding "checkin-needed" to the
> Keywords field of the bug.

Just realized you might not have the permission to do that, so I added the flag for you.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

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

Please use your full name in the Mercurial commit information :)

Comment 10

[Carsten Book [:Tomcat]]

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