1243821 - Remove deprecated Sync Strings.

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Vivek Balakrishnan[:vivek]]

This is a follow up to 1220906. As noted in https://bugzilla.mozilla.org/show_bug.cgi?id=1241846#c17 , this bug tracks removing those unused strings

Comment 1

[Vishnu (:malayaleecoder)]

Attachment: Bug1243821_v1.diff

Do review and let me know about further improvements (This patch is prone to simple errors!).

Comment 2

[Vivek Balakrishnan[:vivek]]

Comment on attachment 8713708 [details] [diff] [review]
Bug1243821_v1.diff

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

Hi malayaleecoder,

Sorry for the delay in reviewing the patch.
Your patch looks good except for the few changes that I have asked for.
Can you please address them and submit a new patch.

::: mobile/android/services/strings.xml.in
@@ -54,2 @@
>    <!-- Configure Engines -->
>    <string name="sync_configure_engines_title">&sync.configure.engines.title.label;</string>

sync_configure_engines_title & sync_configure_engines_sync_my_title are nit used anywhere. Please remove them

@@ +8,4 @@
>    <!-- Configure Engines -->
>    <string name="sync_configure_engines_title">&sync.configure.engines.title.label;</string>
>    <string name="sync_configure_engines_sync_my_title">&sync.configure.engines.sync.my.title.label;</string>
> +  

Avoid introducing white space nits

@@ -54,5 @@
>    <!-- Configure Engines -->
>    <string name="sync_configure_engines_title">&sync.configure.engines.title.label;</string>
>    <string name="sync_configure_engines_sync_my_title">&sync.configure.engines.sync.my.title.label;</string>
> -  <string name="sync_configure_engines_title_bookmarks">&sync.configure.engines.title.bookmarks;</string>
> -  <string name="sync_configure_engines_title_passwords">&sync.configure.engines.title.passwords2;</string>

sync_configure_engines_title_passwords, sync_configure_engines_title_history,
sync_configure_engines_title_tabs are used in https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/AndroidManifest.xml.in#424

@@ -96,5 @@
> -  <string name="sync_title_redirect_to_set_up_sync">&sync.title.redirect.to.set.up.sync.label;</string>
> -  <string name="sync_text_redirect_to_set_up_sync">&sync.text.redirect.to.set.up.sync.label;</string>
> -  <string name="sync_text_tab_sent">&sync.text.tab.sent.label;</string>
> -  <string name="sync_text_tab_not_sent">&sync.text.tab.not.sent.label;</string>
> -  <string name="sync_default_client_name">&sync.default.client.name;</string>

This is used in https://dxr.mozilla.org/mozilla-central/source/mobile/android/services/src/main/java/org/mozilla/gecko/background/fxa/FxAccountUtils.java#215

Comment 3

[Vishnu (:malayaleecoder)]

Vivek, I am confused! 
In Bug 1241846 Comment #20 you have asked me not to remove "sync.configure.engines.title.label"

Comment 4

[Vivek Balakrishnan[:vivek]]

Malayaleecoder,

If you check the AndroidManifest.xml.in only sync_configure_engines_title_passwords, sync_configure_engines_title_history &
sync_configure_engines_title_tabs are used [1][2][3].

So, we can safely remove sync_configure_engines_title, sync_configure_engines_sync_my_title & sync_configure_engines_title_bookmarks

Sorry for the confusion. Hope it is clear now.

[1]https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/AndroidManifest.xml.in#424
[2]https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/AndroidManifest.xml.in#430 
[3]https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/AndroidManifest.xml.in#439

Comment 5

[Nick Alexander :nalexander [he/him]]

(In reply to Vivek Balakrishnan[:vivek] from comment #4)
> Malayaleecoder,
> 
> If you check the AndroidManifest.xml.in only
> sync_configure_engines_title_passwords, sync_configure_engines_title_history
> &
> sync_configure_engines_title_tabs are used [1][2][3].
> 
> So, we can safely remove sync_configure_engines_title,
> sync_configure_engines_sync_my_title & sync_configure_engines_title_bookmarks
> 
> Sorry for the confusion. Hope it is clear now.
> 
> [1]https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/
> AndroidManifest.xml.in#424
> [2]https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/
> AndroidManifest.xml.in#430 
> [3]https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/
> AndroidManifest.xml.in#439

Malayaleecoder, are you running a build with |mach build mobile/android| after changes?  (Or with Gradle/IntelliJ/Android Studio.)  Any missing strings should break the build...

Comment 6

[Vishnu (:malayaleecoder)]

Attachment: Bug1243821_v2.diff

I have implemented the said changes and have also cleared the whitespace nits. Do check and let me know for any further improvements!

Comment 7

[Vishnu (:malayaleecoder)]

(In reply to Nick Alexander :nalexander from comment #5)
> (In reply to Vivek Balakrishnan[:vivek] from comment #4)
> > Malayaleecoder,
> > 
> > If you check the AndroidManifest.xml.in only
> > sync_configure_engines_title_passwords, sync_configure_engines_title_history
> > &
> > sync_configure_engines_title_tabs are used [1][2][3].
> > 
> > So, we can safely remove sync_configure_engines_title,
> > sync_configure_engines_sync_my_title & sync_configure_engines_title_bookmarks
> > 
> > Sorry for the confusion. Hope it is clear now.
> > 
> > [1]https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/
> > AndroidManifest.xml.in#424
> > [2]https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/
> > AndroidManifest.xml.in#430 
> > [3]https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/
> > AndroidManifest.xml.in#439
> 
> Malayaleecoder, are you running a build with |mach build mobile/android|
> after changes?  (Or with Gradle/IntelliJ/Android Studio.)  Any missing
> strings should break the build...

Actually Nick , I did not use ./mach build till I saw your comment now. Before submitting v2 I did run ./mach build and it got build successfully!
Thanks for letting me know about this.

Comment 8

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8714976 [details] [diff] [review]
Bug1243821_v2.diff

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

This looks good, but I'd like to see a try build.

Malayaleecoder, do you have try privileges?  That will let you test your changes in automation and show that nothing is broken.  You'll need to file a ticket (like Bug 1039210) and CC me and Vivek to vouch for you.  See https://wiki.mozilla.org/Build:TryServer for instructions.

I'm going to mark r+, pending a try build before checkin.  Good work!

Comment 9

[Vivek Balakrishnan[:vivek]]

Nick,
Thanks for reviewing this.

Comment 10

[Vishnu (:malayaleecoder)]

@Nick, @Vivek : I have filed a ticket, Bug 1245611, Do check that out. Thanks for the r+ :)

Comment 11

[Vivek Balakrishnan[:vivek]]

Hi malayaleecoder, 

I have vouched for you.
Hopefully, you should get try server access.

Please feel free to NI me if you need help with running the automation tests in try server.

Comment 12

[Vishnu (:malayaleecoder)]

Thanks for the vouch, Vivek!
After I get the try server access, I would be working to get a try build ASAP.

Comment 13

[Vishnu (:malayaleecoder)]

I got Level 1 access, yay!

@nick: Thanks for the vouch :)

When I am trying to push to the try server by running "hg push -f ssh://hg.mozilla.org/try/", I am getting an error, "The authenticity of host 'hg.mozilla.org (63.245.215.25)' can't be established.". Any help to fix this ?

Comment 14

[Nick Alexander :nalexander [he/him]]

(In reply to malayaleecoder from comment #13)
> I got Level 1 access, yay!
> 
> @nick: Thanks for the vouch :)
> 
> When I am trying to push to the try server by running "hg push -f
> ssh://hg.mozilla.org/try/", I am getting an error, "The authenticity of host
> 'hg.mozilla.org (63.245.215.25)' can't be established.". Any help to fix
> this ?

Hmm.  There are two things.  One is the fingerprint of the host; I think http://mozilla-version-control-tools.readthedocs.org/en/latest/hgmozilla/auth.html will help you get that setup.

After that, I suggest you use the "push-to-try" extension rather than |hg push| directly.

Comment 15

[Vishnu (:malayaleecoder)]

Hello Nick, Vivek

I am having a bad time trying to push to try server. In the process I lost my commit and am not able to get my ssh passphrase nor my RSA password :P. Please give me some more time for the fix(approx. 72hrs), as I am currently having my exams going on. Hope it won't be a problem.

Cheers!

Comment 16

[Vivek Balakrishnan[:vivek]]

Hi malayaleecoder,

Take your time. 
And all the best for your exams :)

Comment 17

[Nick Alexander :nalexander [he/him]]

(In reply to malayaleecoder from comment #15)
> Hello Nick, Vivek
> 
> I am having a bad time trying to push to try server. In the process I lost
> my commit and am not able to get my ssh passphrase nor my RSA password :P.
> Please give me some more time for the fix(approx. 72hrs), as I am currently
> having my exams going on. Hope it won't be a problem.
> 
> Cheers!

Yeah, no trouble.  I will probably land this patch -- it's ready -- and we can hammer out try access as you have more time.

Thanks!

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/18809f87ae68

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/18809f87ae68

Comment 20

[Vishnu (:malayaleecoder)]

Thank You, Nick and Vivek, for your help to land this patch.
Do suggest me some next bugs which needs to be solved :-)

Comment 21

[Nick Alexander :nalexander [he/him]]

(In reply to malayaleecoder from comment #20)
> Thank You, Nick and Vivek, for your help to land this patch.
> Do suggest me some next bugs which needs to be solved :-)

Hey!  I would love somebody to jump on Bug 1231549.  Also, I'd love Vivek to get some help finishing Bug 1232447 -- I'm not sure why that hasn't landed.

In a rather different direction, Bug 1208757 could use some attention.  It's not 100% obvious to me how to solve everything there, but we could try to make progress.