1116525 - Remove unused local variables in TextSelection

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Text Selection, defect)

Description

[Mark Capella [:capella]]

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.

Comment 1

[surabhi anand]

Hi Mark i want to take this bug.

Comment 2

[p.calligaris.code]

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?

Comment 3

[p.calligaris.code]

Attachment: Proposed Patch

I propose this patch

Comment 4

[Mark Capella [:capella]]

Comment on attachment 8543255 [details] [diff] [review]
Proposed Patch

Assigning to [e-mail] and requesting feedback on his patch.

Comment 5

[Mark Capella [:capella]]

This looks good at first glance. Were you able to build this locally?

Comment 6

[p.calligaris.code]

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?

Comment 7

[Mark Capella [:capella]]

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 :)

Comment 8

[p.calligaris.code]

Attachment: Updated Proposed Patch

Updated proposed patch.

Comment 9

[p.calligaris.code]

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!

Comment 10

[Mark Capella [:capella]]

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 :-)

Comment 11

[Mark Capella [:capella]]

Attachment: bug1116525.diff

The try was successful, so I'll post the patch with the correct commit msg, and ping the proper reviewer.

Comment 12

[Mark Capella [:capella]]

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

Comment 13

[p.calligaris.code]

Thank you for the support Mark! :)

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/121c15407fbd