1125529 - Remove unused string in StringHelper class

Status: RESOLVED FIXED

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

Description

[Mark Capella [:capella]]

We added a string to StringHelper [1][2] to support testSelection tests, which was later obviated by followup work [3] but not completely removed ... let's do that now.

This is a pretty simple code change. You'll need basic knowledge of Java, and optimally own an Android device to be able to build and test the Android version of Firefox for Mobile devices. (This is easier if you already have a source repo and can build Firefox for desktop).

A skilled first-time contributor should be able to finish this in under a week from top to bottom. Others may take longer, as creating your local build environment the first time is a challenge of itself. See [4] for overall details.

If you don't own a device for testing, we can cheat and use try-server pushs.

Leave comments / questions here, or look for me in IRC as nick:capella, in #introduction and #mobile channels.

[1] https://hg.mozilla.org/mozilla-central/rev/76f2454c7866
[2] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/StringHelper.java?rev=b8bf3fc77437&mark=151-151#135
[3] https://reviewboard.mozilla.org/r/1481/

[4] https://wiki.mozilla.org/Mobile/Fennec/Android

Comment 1

[Andy Pusch [:AndyP]]

I'd like to work on this as my first bug. Could you assign me please?

Comment 2

[Andy Pusch [:AndyP]]

I've got a quick question. I'm not sure if I've understood you correctly. Should I only remove the String highlighted in [2] or should I revert all of the changes made in [1]? Also, may other files be affected as well?

Comment 3

[Mark Capella [:capella]]

We only need the string removed.

Comment 4

[Andy Pusch [:AndyP]]

Attachment: rev1 - bug1125529_testSelectionTestStringRemoval.diff

Comment 5

[Mark Capella [:capella]]

Comment on attachment 8554498 [details] [diff] [review]
rev1 - bug1125529_testSelectionTestStringRemoval.diff

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

Great! I'd attached some of my cookie-cutter comments to this bug re: testing and such, but this one is so easy that basically if it builds locally for you (compiles) it'll work. I assume you've done that much :-)

Can you modify your commit message to include margaret as the reviewer:
Bug 1125529 - Removed obsolete string for testSelection tests in StringHelper.java, r=margaret

Then do a try-server push, and repost with r? to :margaret, and the try-server push link in the comments.

If you don't have access to the try server yet (L1 access) and plan to continue contributing patches, you might like to follow [1] so you can do this and future pushes yourself.

If you'd rather, you can re-post the patch with your updated commit message, and I'll push it for you, then we can get margarets approval.

Let me know what you want to do.

[1] https://www.mozilla.org/en-US/about/governance/policies/commit/

Comment 6

[Mark Capella [:capella]]

Comment on attachment 8554498 [details] [diff] [review]
rev1 - bug1125529_testSelectionTestStringRemoval.diff

oops

Comment 7

[Andy Pusch [:AndyP]]

Attachment: rev2 - bug1125529_testSelectionTestStringRemoval.diff

Built and tested on my HTC One M8 with Android 4.4.4.

Comment 8

[:Margaret Leibovic]

Comment on attachment 8554597 [details] [diff] [review]
rev2 - bug1125529_testSelectionTestStringRemoval.diff

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

Looks good, thanks for the patch!

Comment 9

[Andy Pusch [:AndyP]]

https://tbpl.mozilla.org/?tree=Try&rev=c2b065f52ecb

Comment 10

[Mark Capella [:capella]]

Ok, you've got a r+, and a good TRY server run. Normally, you can mark the bug "checkin-needed" and wait for a sherriff to check your patch in to fx-team, but I'm just going to do it for you.

After a good run through fx-team, it'll get migrated to mozilla-central. Basically you're done :-)

https://hg.mozilla.org/integration/fx-team/rev/71b3ff44cca

Comment 11

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/71b3ff44cca6