Remove deprecated Sync Strings.

RESOLVED FIXED in Firefox 47

Status

()

Firefox for Android
General
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: vivek, Assigned: malayaleecoder)

Tracking

unspecified
Firefox 47
Points:
---

Firefox Tracking Flags

(firefox47 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

2 years ago
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
(Assignee)

Comment 1

2 years ago
Created attachment 8713708 [details] [diff] [review]
Bug1243821_v1.diff

Do review and let me know about further improvements (This patch is prone to simple errors!).
Flags: needinfo?(vivekb.balakrishnan)
Attachment #8713708 - Flags: review?(vivekb.balakrishnan)
(Assignee)

Updated

2 years ago
Status: NEW → ASSIGNED
(Reporter)

Comment 2

2 years ago
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
Attachment #8713708 - Flags: review?(vivekb.balakrishnan) → feedback+
(Assignee)

Comment 3

2 years ago
Vivek, I am confused! 
In Bug 1241846 Comment #20 you have asked me not to remove "sync.configure.engines.title.label"
Flags: needinfo?(vivekb.balakrishnan)
(Assignee)

Updated

2 years ago
Flags: needinfo?(vivekb.balakrishnan)
(Reporter)

Comment 4

2 years ago
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
Flags: needinfo?(vivekb.balakrishnan)
(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...
(Assignee)

Comment 6

2 years ago
Created attachment 8714976 [details] [diff] [review]
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!
Attachment #8713708 - Attachment is obsolete: true
Flags: needinfo?(vivekb.balakrishnan)
Attachment #8714976 - Flags: review?(vivekb.balakrishnan)
(Assignee)

Comment 7

2 years ago
(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.
Flags: needinfo?(nalexander)
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!
Attachment #8714976 - Flags: review?(vivekb.balakrishnan) → review+
Flags: needinfo?(vivekb.balakrishnan)
Flags: needinfo?(nalexander)
(Reporter)

Comment 9

2 years ago
Nick,
Thanks for reviewing this.
(Assignee)

Comment 10

2 years ago
@Nick, @Vivek : I have filed a ticket, Bug 1245611, Do check that out. Thanks for the r+ :)
Flags: needinfo?(vivekb.balakrishnan)
Flags: needinfo?(nalexander)
(Reporter)

Comment 11

2 years ago
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.
Flags: needinfo?(vivekb.balakrishnan)
Flags: needinfo?(nalexander)
(Assignee)

Comment 12

2 years ago
Thanks for the vouch, Vivek!
After I get the try server access, I would be working to get a try build ASAP.
(Assignee)

Comment 13

2 years ago
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 ?
Flags: needinfo?(vivekb.balakrishnan)
Flags: needinfo?(nalexander)
(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.
Flags: needinfo?(vivekb.balakrishnan)
Flags: needinfo?(nalexander)
(Assignee)

Comment 15

2 years ago
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!
Flags: needinfo?(vivekb.balakrishnan)
Flags: needinfo?(nalexander)
(Reporter)

Comment 16

2 years ago
Hi malayaleecoder,

Take your time. 
And all the best for your exams :)
Flags: needinfo?(vivekb.balakrishnan)
Flags: needinfo?(nalexander)
(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

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

Comment 19

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/18809f87ae68
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
(Assignee)

Comment 20

2 years ago
Thank You, Nick and Vivek, for your help to land this patch.
Do suggest me some next bugs which needs to be solved :-)
Flags: needinfo?(vivekb.balakrishnan)
Flags: needinfo?(nalexander)
(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.
Flags: needinfo?(nalexander)
(Reporter)

Updated

2 years ago
Flags: needinfo?(vivekb.balakrishnan)
You need to log in before you can comment on or make changes to this bug.