Closed Bug 1124943 Opened 5 years ago Closed 5 years ago

Make the methods of TilesRecorder static

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 39
Tracking Status
firefox39 --- fixed

People

(Reporter: mcomella, Assigned: jannis, Mentored)

References

Details

(Whiteboard: [lang=java][good first bug])

Attachments

(1 file, 3 obsolete files)

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! ^_^
Hey,I am a newbie here,I wish to fix this bug.Can someone get me started?
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 !
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
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!).
Hey Michael,I am working on this bug,I am going through its details,I'll get to you quick.
Hey Michael,

My apologies I didn't realize this bug was already taken ,my bad.
Michael,Can you tell me the specific hours when you will be available so that we can discuss on this bug?
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.
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?
(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!
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.
(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?
Assignee: nobody → 2013001
Flags: needinfo?(2013001)
Thanks Michael,now how should i proceed further?I mean where do i have to upload the fixed code?
Flags: needinfo?(2013001)
Attached patch TopSitesPanel.java (obsolete) — Splinter Review
Commented out the sections wherever the class of TileRecorder was instatntiated.
Attached patch TilesRecorder.java (obsolete) — Splinter Review
Made the methods static and created a private constructor.
Hey Michael,I have uploaded the patch,Please look into them.
(Spoke on IRC - these patches are not a valid patch format)
Attachment #8561669 - Flags: review+ → review?(michael.l.comella)
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!
Attachment #8561669 - Flags: review?(michael.l.comella) → feedback+
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.
Assignee: 2013001 → andrew.xia
Status: NEW → ASSIGNED
Attachment #8555652 - Attachment is obsolete: true
Attachment #8555653 - Attachment is obsolete: true
Still working on this, Andrew?
Flags: needinfo?(andrew.xia)
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! :)
Attachment #8581286 - Flags: review?(michael.l.comella)
Assignee: andrew.xia → jannis
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
Attachment #8581286 - Flags: review?(michael.l.comella) → review+
Attachment #8561669 - Attachment is obsolete: true
Flags: needinfo?(andrew.xia)
Keywords: checkin-needed
(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.
https://hg.mozilla.org/integration/fx-team/rev/bf13341538f7
Keywords: checkin-needed
Whiteboard: [lang=java][good first bug] → [lang=java][good first bug][fixed-in-fx-team]
(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.
https://hg.mozilla.org/mozilla-central/rev/bf13341538f7
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [lang=java][good first bug][fixed-in-fx-team] → [lang=java][good first bug]
Target Milestone: --- → Firefox 39
You need to log in before you can comment on or make changes to this bug.