Closed Bug 1116525 Opened 5 years ago Closed 5 years ago

Remove unused local variables in TextSelection

Categories

(Firefox for Android :: Text Selection, defect)

ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 37

People

(Reporter: capella, Assigned: p.calligaris.code, Mentored)

Details

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

Attachments

(1 file, 2 obsolete files)

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?
Attached patch Proposed Patch (obsolete) — Splinter Review
I propose this patch
Comment on attachment 8543255 [details] [diff] [review]
Proposed Patch

Assigning to p.calligaris.code@gmail.com 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+
Attached patch Updated Proposed Patch (obsolete) — Splinter Review
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 :-)
Attached patch bug1116525.diffSplinter Review
The try was successful, so I'll post the patch with the correct commit msg, and ping the proper reviewer.
Attachment #8543491 - Attachment is obsolete: true
Attachment #8543571 - Flags: review?(margaret.leibovic)
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! :)
https://hg.mozilla.org/mozilla-central/rev/121c15407fbd
Assignee: nobody → p.calligaris.code
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → Firefox 37
You need to log in before you can comment on or make changes to this bug.