1245722 - Replace org.mozilla.gecko.sync.Utils.getLanguageTag with Locales.getLanguageTag throughout

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Nick Alexander :nalexander [he/him]]

The code at https://dxr.mozilla.org/mozilla-central/source/mobile/android/services/src/main/java/org/mozilla/gecko/sync/Utils.java#580 was duplicated to https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/Locales.java#104 for build reasons.  Those reasons no longer apply.  This ticket tracks replacing all uses of the services code with the Locales code, and removing the existing services implementation.

Comment 1

[Nick Alexander :nalexander [he/him]]

Alex: you might like to co-mentor this with me.

Comment 2

[Priyen Patel]

Hi, I would like to attempt this as a good first bug.

University	of	Toronto,	CSC302H1S-2016	
assignment.	Patch	by	Feb	26,2016

Comment 3

[Nick Alexander :nalexander [he/him]]

(In reply to Priyen Patel from comment #2)
> Hi, I would like to attempt this as a good first bug.
> 
> University	of	Toronto,	CSC302H1S-2016	
> assignment.	Patch	by	Feb	26,2016

Welcome aboard!  First steps are to get a Firefox for Android build up and running: see https://developer.mozilla.org/en-US/docs/Simple_Firefox_for_Android_build and try to get an "artifact build" working on your machine.  After that, see if you can follow the steps in c#0 above.  https://wiki.mozilla.org/Mobile/Fennec/Android#Contributing_Code is a good general resource, and Alex and I will help you here (in this ticket) and on IRC, if needed.

Comment 4

[Priyen Patel]

Hi thank you for the starting tips. I will get right on it.

Comment 5

[Matherin]

Hey guys I spotted this bug and it looks like a pretty quick fix! 

I have a few clarifying questions as I am rather new. I already have an artifact build working and set up. I am a bit confused on how would you like these changes submitted to you? I read the above links but they might be going over my head a bit. Do I need to download the source code and make the changes there? Or can I take the links Nick provided and make the changes in notepad++ or something and email the document? I am just confused on how to make changes and how these changes should be submitted. I would love a little bit of direction and I apologize if this is trivial. Happy coding!

Comment 6

[Nick Alexander :nalexander [he/him]]

(In reply to Matherin from comment #5)
> Hey guys I spotted this bug and it looks like a pretty quick fix! 
> 
> I have a few clarifying questions as I am rather new. I already have an
> artifact build working and set up. I am a bit confused on how would you like
> these changes submitted to you? I read the above links but they might be
> going over my head a bit. Do I need to download the source code and make the
> changes there? Or can I take the links Nick provided and make the changes in
> notepad++ or something and email the document? I am just confused on how to
> make changes and how these changes should be submitted. I would love a
> little bit of direction and I apologize if this is trivial. Happy coding!

No trouble.  There's are many systems built for submitting changes; Mozilla uses a combination of Bugzilla, Mercurial (a version control system), and ReviewBoard (a code review tool).  Let me find a good getting started link... https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide and in particular https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch.

You make the changes using the editor of your choice -- for Android stuff, I highly recommend using Android Studio (or IntelliJ), although Notepad and other editors will work fine too -- and then you use Mercurial to make the patch file.  Then you attach it to Bugzilla.

Feel free to ask further questions.  Welcome aboard!

Comment 7

[Matherin]

hey thank you for all of these resources! I will get on this!

Comment 8

[Priyen Patel]

Attachment: Bug1245722patch.diff

Bug 1245722 Fixes - Replaced org.mozilla.gecko.sync.Utils.getLanguageTag with Locales.getLanguageTag throughout

Comment 9

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8719229 [details] [diff] [review]
Bug1245722patch.diff

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

Priyen -- This looks mostly good, but I have an open question.  See below.

For the next version, please format the commit message like:

Bug 1245722 - Replace org.mozilla.gecko.sync.Utils.getLanguageTag with Locales.getLanguageTag. r=nalexander

We generally want the commit message to say "what was done", and then add "why" below.  We generally ask for review with r?PERSON, and then land with r=PERSON to mark reviewers.  Finally, after posting a patch like this, use Bugzilla to flag a reviewer -- for mentored bugs that's almost always the mentor listed.  In this case, me!  (Or vivek, IIRC).

All in all, good work Priyen.  Are you interested in Bug 1231549?  It's a little one, but valuable.

::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/Utils.java
@@ -593,5 @@
> -      return language;
> -    }
> -    return language + "-" + country;
> -  }
> -}

The trailing "}" is still needed, right?

Does this compile?

Comment 10

[Priyen Patel]

Yes that trailing } is needed, I apologize, it should compile.
Should I re-upload the patch file?
And I will also take a look at the bug you linked in your comment.

Comment 11

[Nick Alexander :nalexander [he/him]]

(In reply to Priyen Patel from comment #10)
> Yes that trailing } is needed, I apologize, it should compile.
> Should I re-upload the patch file?
> And I will also take a look at the bug you linked in your comment.

Yes -- re-upload, mark the old as obselete, mark your file as a patch, and flag me for review.

Comment 12

[Priyen Patel]

Thank you for the help I will upload the revised version tonight.

Comment 13

[Priyen Patel]

Attachment: patch.diff

Comment 14

[Priyen Patel]

Hi Alexander I've uploaded the revised patch and flagged for review (hopefully correctly).

Comment 15

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8719370 [details] [diff] [review]
patch.diff

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

Looks good!  I'll land this now.

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/2390f1990c62

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2390f1990c62

Comment 18

[Nick Alexander :nalexander [he/him]]

This has landed, so clearing the NI.  Priyen, did you find another ticket to look at?  If you're interested in learning more about Sync, there's something to investigate and report on in Bug 1250391.