1126290 - Remove JavaPanZoomController.PAN_THRESHOLD and use PanZoomController.PAN_THRESHOLD

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Toolbar, defect)

Description

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

JavaPanZoomController has a PAN_THRESHOLD defined at [1]. The PanZoomController has an identical PAN_THRESHOLD defined at [2]. The second one was added later but the first one was never removed. We should remove it, and make sure all code using the first one is switched to using the second one. (I think there is only one place it's used inside JavaPanZoomController).

[1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/gfx/JavaPanZoomController.java?rev=69c75b3dc3d0#56
[2] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/gfx/PanZoomController.java?rev=9ff86c2ca064#19

Comment 1

[Rashi Karanpuria]

Hi, I wish to work on this bug. Can you please guide me.

Comment 2

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

Sure, the first step in working on this is to get a build of Firefox for Android up and running. There are instructions on how to do this at https://wiki.mozilla.org/Mobile/Fennec/Android - please follow the instructions there and let me know once you have the build and are able to successfully run it on your device. Once you have done that I can assign the bug to you and give you more details on what needs to be done. It's a pretty simple code change that needs to be made.

If you run into any problems or have any questions feel free to ask here or on IRC, in the #mobile channel. Thanks!

Comment 3

[Rashi Karanpuria]

Thanks Kartikaya, I am onto it. I will get to you as soon as I am done with the setup.

Comment 4

[Rashi Karanpuria]

As far as i have understood this code,I feel that in the JavaPanZoomController class we have implemented PanZoomController interface,so all we have to do is replace the  PAN_THRESHOLD variable used in [1] with PanZoomController.PAN_THRESHOLD and remove the PAN_THRESHOLD variable in [1] where it was declared.....Please correct me if I am wrong...?..

Comment 5

[Rashi Karanpuria]

Attachment: JavaPanZoomController.java

Hey Kartikaya,Please look into this....

Comment 6

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

Hi Rashi,

Any changes you make should be attached in the form of a patch, as it's very difficult to review changes if you just attach a modified file. Please see the instructions at [1] on how to generate patches.

That being said the changes you outlined in comment 4 are correct, you just need to remove JavaPanZoomController.PAN_THRESHOLD and replace the usage with PanZoomController.PAN_THRESHOLD.

Thanks!

[1] https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Comment 7

[acarson13]

Hi, I am a new contributor to Mozilla for Android and would like to take this bug along with my team.  We have a mentor to help us create the patch.  Please advice for my next step, thank you.

Comment 8

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

Hi acarson13, usually I prefer if only one person works on a bug at any given time, and given Rashi just started working on this I would like to give him some more time before passing it to somebody else. Thanks!

Comment 9

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

Hi Rashi,

Any progress on this bug? Let me know if you get stuck!

Comment 10

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

Rashi said he was unable to continue working on this, so it's open for others to take.

Comment 11

[Miles Rakestraw]

Hello I am new to contributing and would like to work on this bug if no one has claimed it.

Comment 12

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

Hi Miles, yes you are welcome to work on this bug. Please see comment 2 for instructions on how to get a build up and running. Let me know once you have that, and I can assign the bug to you. Thanks!

Comment 13

[Miles Rakestraw]

Excellent! I already have the environment set up so I am ready to get started right away.

Comment 14

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

Great! The changes that need to be made are pretty straightforward; just remove the PAN_THRESHOLD variable defined in mobile/android/base/gfx/JavaPanZoomController.java and replace all the use-sites of that variable with the equivalent one from PanZoomController.java.

Please see [1] on how to generate a patch to attach to the bug for submission. Thanks!

[1] https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Comment 15

[Miles Rakestraw]

Excellent I will get started.  Is there an associated IRC channel, or should I direct questions to #introduction/#developer channels?

Comment 16

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

#mobile is the channel for fennec developers. I'm in there too ("kats") even though I don't work much on Fennec any more.

Comment 17

[Miles Rakestraw]

Just an update. I have worked the bug and am reading up on submitting the patch.

Comment 18

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

Great, thanks!

Comment 19

[Miles Rakestraw]

Attachment: bug1126290.diff

Comment 20

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

Comment on attachment 8566210 [details] [diff] [review]
bug1126290.diff

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

This looks great, thanks!

The only change I would make is that when you specify the reviewer in the commit message, you should just put their handle instead of the full name and email address. In this case it would be r=kats. I can fix that up when I land the patch, but keep that in mind for future patches. I'll land this later today, once I've verified the build locally.

Comment 21

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

Landed on fx-team: https://hg.mozilla.org/integration/fx-team/rev/4d66d5d58232

It should get merged to mozilla-central in the next day or so, at which point this bug will be marked fixed. Thanks Miles!

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4d66d5d58232

Comment 23

[Miles Rakestraw]

Thank you all for the help through this process.  I look forward to contributing further.  If there is anything else I can help with I would be more than willing to give it a shot.  I am a senior in college and am always looking for more real world experience.  Thanks again.

Comment 24

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

Thank you for choosing to contribute to Mozilla! We look forward to seeing more contributions from in the future :)