Closed Bug 1116525 Opened 5 years ago Closed 5 years ago
Remove unused local variables in Text
This is a very simple bug for a new contributor with basic Java skills, to learn the Mozilla development process. You should be able to build Firefox Mobile on your desktop. See https://wiki.mozilla.org/Mobile/Fennec/Android for instructions if this is new to you, and ask questions on irc channels #introduction or #mobile. Also, ideally, you should have an android device for testing your solution locally, but this isn't strictly required. One of our classes, "TextSelection" has two unused variables, which we want to remove. See http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/TextSelection.java?rev=1278a4bf957c&mark=74-75,#36 So basically you'll: 1) Remove the code, 2) rebuild the browser, 3) test the browser, 4) get approvals and move it to production.
Hi Mark i want to take this bug.
This is my first time as contributor, and this seems a good starting exercise. The constructor of the class TextSelection require 5 args, the last two of them are unused TextSelection(TextSelectionHandle anchorHandle, TextSelectionHandle caretHandle, TextSelectionHandle focusHandle, EventDispatcher eventDispatcher, GeckoApp activity) But here http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoApp.java#1588 the constructor TextSelection is called with all the arguments. So we need to take care of this too. Is it right? Or a better way can be to write two constructors one with the full argument list that call the other constructor with only the used variables?
I propose this patch
Comment on attachment 8543255 [details] [diff] [review] Proposed Patch Assigning to firstname.lastname@example.org and requesting feedback on his patch.
Attachment #8543255 - Flags: feedback?(markcapella)
This looks good at first glance. Were you able to build this locally?
Yep, I have done steps 1)-3), and it seems working. I tested it locally on my Android device, I selected some text and looking at the logs using adb logcat -v time | grep GeckoTextSelection is it enough?
Fantastic :) Can you push the patch to the try-server to see if it's successful through our integration test suite? If you can, post a link to it here. If not, I can push it for you ... let me know ... After we get that done, you'll have to repost your patch with a modified commit message. You'll be flagging r=margaret vs. r=reviewers. Also, how did you generate that patch? It looks unlike something generated by mercurial. see: https://bugzilla.mozilla.org/attachment.cgi?id=8541777&action=edit for an example, and https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F for instructions. Again, if you get lost, let me know :)
Attachment #8543255 - Flags: feedback?(markcapella) → feedback+
Updated proposed patch.
Attachment #8543255 - Attachment is obsolete: true
Hi Mark, I have redone all the steps and now the patch seems more similar to the example one in https://bugzilla.mozilla.org/attachment.cgi?id=8541777&action=edit and I flagged it r=margaret. I am trying to push on the try server using the following $ hg qref --message "try: -b do -p android,android-x86 -u all -t all" $ hg push -f ssh://hg.mozilla.org/try/ but the last command fails with: pushing to ssh://hg.mozilla.org/try/ remote: Permission denied (publickey). abort: no suitable response from remote hg! I think I need to be added to the try server, is it right? Thank you!
For now I'll push to try for you, https://tbpl.mozilla.org/?tree=Try&rev=2aed91b6e752 For this in the future you'll need L1 commit access, see here: https://www.mozilla.org/en-US/about/governance/policies/commit/ After you complete a bug or two, you get a couple people to vouch for you and fill out forms :-)
The try was successful, so I'll post the patch with the correct commit msg, and ping the proper reviewer.
Attachment #8543571 - Flags: review?(margaret.leibovic) → review+
Fantastic :-) Now we push to fx-team and your patch is complete \o/ https://hg.mozilla.org/integration/fx-team/rev/121c15407fbd Thanks for helping out, and as I replied to your email, Bug 1117226 - Use sendRequestForResult for reader mode favicon request might be a good followup bug. Otherwise, you can always check http://www.joshmatthews.net/bugsahoy/ for "Good first bugs" / mentored bugs, and/or ask around in #introduction :-D
Thank you for the support Mark! :)
Assignee: nobody → p.calligaris.code
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 37
You need to log in before you can comment on or make changes to this bug.