Closed Bug 943629 Opened 6 years ago Closed 6 years ago

Remove unnecessary comment in Tabs

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 30

People

(Reporter: mcomella, Assigned: csu)

Details

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

Attachments

(1 file)

via IRC:

<mcomella> bnicholson: Just wondering why this comment makes sense (https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/Tabs.java#563) (you are the hg blame for that line)
<bnicholson> mcomella: looks like it doesn't make sense anymore. here's the patch: https://bug844407.bugzilla.mozilla.org/attachment.cgi?id=724665
<firebot> Bug https://bugzilla.mozilla.org/show_bug.cgi?id=844407 nor, --, Firefox 22, bnicholson, RESO FIXED, Make Tabs threadsafe
<bnicholson> that method used to call getActivity(), which threw if attachToActivity wasn't called first
<bnicholson> but getActivity() is gone now
<bnicholson> looking at comments 6-8, i think those were actually rnewman's changes that got committed under my name

Here's a stable version of the comment: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/Tabs.java?rev=18c49e0f4eb5#563
I'm David, a senior from the College of Charleston in a Software Engineering class and I'd like to work on this bug.
Flags: needinfo?(michael.l.comella)
Hey, David!

Sure, I've assigned you to the bug so you can get working whenever you are ready.

Have you already set up a build environment? If not, 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! ^_^
Assignee: nobody → djbrunea
Status: NEW → ASSIGNED
Flags: needinfo?(michael.l.comella)
Hey, David! Are you still working on this?
Flags: needinfo?(djbrunea)
Unassigning due to inactivity.
Assignee: djbrunea → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(djbrunea)
Hi!

Could I take this? This would be my first bug. I'm setting up the build environment now. I don't have an Android device though; I hope that's okay.

Would this just be a matter of removing the comment entirely (ie no need to edit or modify the content of the comment)?
Flags: needinfo?(michael.l.comella)
Hello, Christopher! Welcome to Bugzilla!

(In reply to Christopher Su from comment #5)
> Could I take this?

Sure, you've been assigned!

> I don't have an Android device though; I hope that's okay.

It should be fine for this bug, seeing as it's just removing a comment - if the code compiles, it's probably fine.

However, if you're looking to work on future Firefox for Android bugs, having a device might be necessary. I haven't heard of anyone using the emulator for Firefox development - even if it's possible, it'd probably be terribly slow. :\

I'd recommend trying to install a release build on the emulator [1] and seeing if you think it runs well enough for you to attempt developing on it.

> Would this just be a matter of removing the comment entirely (ie no need to
> edit or modify the content of the comment)?

Yep!

If you have any questions, please let me know! You can also find me on IRC in #mobile with the nick "mcomella". If you need setup instructions, see [2].

[1]: ftp://ftp.mozilla.org/pub/mobile/releases/latest/android/en-US/
[2]: https://wiki.mozilla.org/Irc
Assignee: nobody → csu
Status: NEW → ASSIGNED
Flags: needinfo?(michael.l.comella)
I hope I did this right. I'm still learning Mercurial. Let me know if I need to change anything. :)
Attachment #8385059 - Flags: review+
Attachment #8385059 - Flags: feedback+
(In reply to Christopher Su from comment #7)
> Created attachment 8385059 [details] [diff] [review]
> Patch to remove the unnecessary comment in Tabs.

Also hoping I'm using flags correctly here... are the review and feedback flags used to request review and feedback or to indicate they've already been given? I was using them to request.
Flags: needinfo?(michael.l.comella)
Comment on attachment 8385059 [details] [diff] [review]
Patch to remove the unnecessary comment in Tabs.

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

Looks good to me. Do you know about the check-in needed keyword?
Attachment #8385059 - Flags: review+
(In reply to Christopher Su from comment #8)
> Also hoping I'm using flags correctly here... are the review and feedback
> flags used to request review and feedback or to indicate they've already
> been given? I was using them to request.

To request a review, you should set the flag to r? or f?, and enter the email of your reviewer (like you did with the needinfo flag). You can type ":" followed by their IRC nick to get an easier auto-completion. Reviews are typically used in a situation like this while feedback is typically used on works in progress. Only one is usually used at a time (at least per person - feel free to f? someone and r? someone else). The response will change these flags to r+, r-, f+, f-, etc. to describe whether the feedback was positive or not.

In this particular case, you would want to r? me.
Flags: needinfo?(michael.l.comella)
Keywords: checkin-needed
(In reply to Michael Comella (:mcomella) from comment #9)
> Looks good to me. Do you know about the check-in needed keyword?

I didn't before, but I just looked it up and added it. Hope that's correct!
(In reply to Christopher Su from comment #11)
> I didn't before, but I just looked it up and added it. Hope that's correct!

Looks good! Thanks for your help!
https://hg.mozilla.org/integration/fx-team/rev/2d0534a0b50f
Keywords: checkin-needed
Whiteboard: [mentor=mcomella][lang=java][good first bug] → [mentor=mcomella][lang=java][good first bug][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/2d0534a0b50f
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [mentor=mcomella][lang=java][good first bug][fixed-in-fx-team] → [mentor=mcomella][lang=java][good first bug]
Target Milestone: --- → Firefox 30
You need to log in before you can comment on or make changes to this bug.