1124943 - Make the methods of TilesRecorder static

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

As discussed with bnicholson on IRC, the methods in TilesRecorder keep no state and thus can be static. Then we no longer need the constructor so make it private to prevent construction from other classes and remove the member instance in mobile/android/base/home/TopSitesPanel.java.

Note that this bug requires the patches in bug 1096958 to land first.

To start, set up a build environment - you can see the instructions here: https://wiki.mozilla.org/Mobile/Fennec/Android

If you need any help, you can reply to this bug, or feel free to message me on IRC - my nick is "mcomella" and you can find me in #mobile. If you need IRC setup instructions, see https://wiki.mozilla.org/IRC

Thanks and happy coding! ^_^

Comment 1

[Aanand shekhar roy]

Hey,I am a newbie here,I wish to fix this bug.Can someone get me started?

Comment 2

[ronak khandelwal]

Hey Michael,
I am new here can you be any more specific I couldn't find the TilesRecorder class .I apologise if I ask any stupid question .
Thanks !

Comment 3

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Hi, Aanand.

The instructions on what to do can be found in comment 0. You can find the TilesRecorder class in <mozilla-central>/mobile/android/base/tiles [1].

[1]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tiles/TilesRecorder.java

Comment 4

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Hey, Ronak.

I was actually going to recommend to you another bug to work on as Aanand got to this one first but it seems you're already working on the bug I was going to recommend (bug 1122767)! :P

Let me know if you want more bugs to work on (though you should probably land your first patch before you take on a bigger load!).

Comment 5

[Aanand shekhar roy]

Hey Michael,I am working on this bug,I am going through its details,I'll get to you quick.

Comment 6

[ronak khandelwal]

Hey Michael,

My apologies I didn't realize this bug was already taken ,my bad.

Comment 7

[Aanand shekhar roy]

Michael,Can you tell me the specific hours when you will be available so that we can discuss on this bug?

Comment 8

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Hi, Aanand.

I am typically available on IRC PST working hours (9-6pm pst, Monday - Friday). Otherwise, feel free to leave comments on the bug and I'll get back to you that way.

Comment 9

[Aanand shekhar roy]

Sorry if it seems a naive question,but in this we have two methods namely "recordAction" and "jsonForTile",Do we just have to add "static" keyword on both the methods?

Comment 10

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Aanand shekhar roy from comment #9)
> Sorry if it seems a naive question,but in this we have two methods namely
> "recordAction" and "jsonForTile",Do we just have to add "static" keyword on
> both the methods?

No worries – don't be afraid to ask questions! Yes, you should add the static keyword and you should prevent the class from being instantiated, as per comment 0. This makes the class only accessible from a static context.

By the way, you can add a "Need more information from" flag (at the bottom of the page) to give me a notification and make sure your questions are seen!

Comment 11

[Aanand shekhar roy]

Hey Michael,As of now,I have done three things.
1.Made the methods (ie.recordAction and jsonforTile ) of TilesRecorder static.
2.Added an empty private constructor to the TilesRecorder.
3.Instead of creating object of TileRecorder in TopSitePanel.java,I have directly used methods as they are static in nature.
I am sending you the java files on your mail id as im stucked up in creating and adding them as patch.
Kindly look into them.

Comment 12

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Aanand shekhar roy from comment #11)
> I am sending you the java files on your mail id as im stucked up in creating
> and adding them as patch.

So let's work on getting your patch working - what are you having trouble with?

Comment 13

[Aanand shekhar roy]

Thanks Michael,now how should i proceed further?I mean where do i have to upload the fixed code?

Comment 14

[Aanand shekhar roy]

Attachment: TopSitesPanel.java

Commented out the sections wherever the class of TileRecorder was instatntiated.

Comment 15

[Aanand shekhar roy]

Attachment: TilesRecorder.java

Made the methods static and created a private constructor.

Comment 16

[Aanand shekhar roy]

Hey Michael,I have uploaded the patch,Please look into them.

Comment 17

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(Spoke on IRC - these patches are not a valid patch format)

Comment 18

[Andrew Xia]

Attachment: staticmethods.patch, made the methods static. Could not test on foxinabox, need help testing.

Comment 19

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8561669 [details] [diff] [review]
staticmethods.patch, made the methods static. Could not test on foxinabox, need help testing.

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

This is a right first step but it is incomplete. Calling static methods through an instance (e.g. `mTilesRecorder.staticMethod()` as opposed to `TilesRecorder.staticMethod()`) is looked down upon in Java so you should update these calls accordingly. This is especially important given my comment 0:

(In reply to Michael Comella (:mcomella) from comment #0)
> As discussed with bnicholson on IRC, the methods in TilesRecorder keep no
> state and thus can be static. Then we no longer need the constructor so make
> it private to prevent construction from other classes and remove the member
> instance in mobile/android/base/home/TopSitesPanel.java.

When you implement this, there will be no instances from which to call these static methods.

(In reply to Andrew Xia from comment #18)
> Created attachment 8561669 [details] [diff] [review]
> staticmethods.patch, made the methods static. Could not test on foxinabox,
> need help testing.

Are you able to successfully build Firefox for Android?

For testing, we can run it through our try server as no user facing features have changed but this will not likely be the case for future patches - 
please make sure you have a way of testing the code!

By the way, when you want to get your patch reviewed, you should select "r?" with your reviewers name - in this case, ":mcomella" should auto-complete my email address (you must give your reviewers email address in the field).

Also, to prevent yourself from doing duplicate work, you should ask before taking over bugs! In this case (due to the length of inactivity of the previous assignee), I think it's fine but it's good to be careful!

Comment 20

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Aanand, I assumed you were no longer working on this bug from your inactivity - please let me know if there is another bug you may want to work on.

Comment 21

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Still working on this, Andrew?

Comment 22

[pinjiz]

Attachment: bug_1124943.patch

Hi, since there was no activity in this bug report for over a month, I decided to attempt a bug fix. 

I changed the methods in TilesRecorder to static, added a private constructor to prevent instantiation and removed the member instances of TilesRecorder in TopSitesPanel. 

Please let me know if there is something wrong, thanks! :)

Comment 23

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8581286 [details] [diff] [review]
bug_1124943.patch

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

Looks good to me - thanks Jannis!

Here is a push to our try test servers: https://treeherder.mozilla.org/#/jobs?repo=try&revision=37b248e106aa

Once it goes green, feel free to add the checkin-needed keyword [1]. Let me know if you need help reading the results. Note that all patches that use "checkin-needed" must also have an associated green try run.

[1]: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch#Getting_the_patch_checked_into_the_tree

Comment 24

[pinjiz]

(In reply to Michael Comella (:mcomella) from comment #23)
> Looks good to me - thanks Jannis!
> 
> Here is a push to our try test servers:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=37b248e106aa
> 
> Once it goes green, feel free to add the checkin-needed keyword [1]. Let me
> know if you need help reading the results. Note that all patches that use
> "checkin-needed" must also have an associated green try run.

Thanks Michael! Everything went green, except "Android 4.0 API11+ opt (rc3/rc4)" but I assume it's unrelated to my patch.

Comment 25

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/integration/fx-team/rev/bf13341538f7

Comment 26

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to pinjiz from comment #24)
> Thanks Michael! Everything went green, except "Android 4.0 API11+ opt
> (rc3/rc4)" but I assume it's unrelated to my patch.

I agree. Note that blue test suites are infrastructure errors and are re-run automatically, and orange are test failures. It looks like the test failure in rc3 was an intermittent issue (i.e. it matched one of the filed bugs at the bottom of the screen).

If you'd like some followup bugs, perhaps you'd like to work on bug 1145252 or bug 1146730? Let me know if you would prefer something else too.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

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