Closed
Bug 1116525
Opened 11 years ago
Closed 11 years ago
Remove unused local variables in TextSelection
Categories
(Firefox for Android Graveyard :: Text Selection, defect)
Tracking
(Not tracked)
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)
3.12 KB,
patch
|
Margaret
:
review+
|
Details | Diff | Splinter Review |
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•11 years ago
|
||
Hi Mark i want to take this bug.
Assignee | ||
Comment 2•11 years ago
|
||
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?
Assignee | ||
Comment 3•11 years ago
|
||
I propose this patch
Reporter | ||
Comment 4•11 years ago
|
||
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)
Reporter | ||
Comment 5•11 years ago
|
||
This looks good at first glance. Were you able to build this locally?
Assignee | ||
Comment 6•11 years ago
|
||
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?
Reporter | ||
Comment 7•11 years ago
|
||
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 :)
Reporter | ||
Updated•11 years ago
|
Attachment #8543255 -
Flags: feedback?(markcapella) → feedback+
Assignee | ||
Comment 8•11 years ago
|
||
Updated proposed patch.
Attachment #8543255 -
Attachment is obsolete: true
Assignee | ||
Comment 9•11 years ago
|
||
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!
Reporter | ||
Comment 10•11 years ago
|
||
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 :-)
Reporter | ||
Comment 11•11 years ago
|
||
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)
Updated•11 years ago
|
Attachment #8543571 -
Flags: review?(margaret.leibovic) → review+
Reporter | ||
Comment 12•11 years ago
|
||
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
Assignee | ||
Comment 13•11 years ago
|
||
Thank you for the support Mark! :)
Comment 14•11 years ago
|
||
Assignee: nobody → p.calligaris.code
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → Firefox 37
Updated•5 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•