Closed Bug 1241846 Opened 6 years ago Closed 6 years ago

Remove unused sync xml resources

Categories

(Firefox for Android Graveyard :: General, defect)

defect
Not set
normal

Tracking

(firefox47 fixed)

RESOLVED FIXED
Firefox 47
Tracking Status
firefox47 --- fixed

People

(Reporter: vivek, Assigned: malayaleecoder, Mentored)

References

Details

(Whiteboard: [lang=java][good next bug])

Attachments

(3 files)

Excellent find!  I forgot about those in my purging blitz.
Mentor: nalexander, vivekb.balakrishnan
Depends on: 1220906
Vivek, I would like to work on this bug. According to what I have inferred, all that I have to do is delete those above said nine .xml files from the repository. Please confirm, and guide me through.
Hi malayaleecoder,

Thanks for your interest in this bug. 
You are right, for this bug you need to remove the xml files that are listed in description.

Please follow the instructions at https://wiki.mozilla.org/Mobile/Fennec/Android#Building_and_deploying_the_Fennec_Android_package for building fennec.


You don't need an IDE for this bug, but if you are planning to contribute more towards fennec code then it may be worthwhile to configure for Android Studio. Follow the instructions at https://wiki.mozilla.org/Mobile/Fennec/Android/IDEs#IntelliJ_or_Android_Studio_with_Gradle

Please feel free to ping me if you need any help.
Thanks Vivek for your reply. I have the build and I have removed those xml files. Now, how am I supposed to submit the patch? "hg diff" command throws no output for me.
Hi,

What does hg status report for you [1]?
Did you remove the file from version control using 'hg remove'? 
Refer [2] if you want to remove the file from version control after deleting them.

Hope the references help you.

[1] https://selenic.com/hg/help/status
[2] https://selenic.com/hg/help/remove
Initial patch submitted.
Attachment #8711451 - Flags: review?(vivekb.balakrishnan)
Thanks Vivek for the reply. It was the problem caused by 'hg remove'. I have submitted an initial patch. Do guide me for further improvements or any problems regarding my submission.
Hi malayaleecoder,

Your patch looks good to me. 
I noticed that the following xmls not used anymore, so can you please update your patch to remove them. 

1. https://dxr.mozilla.org/mozilla-central/source/mobile/android/services/src/main/res/layout/sync_account.xml?case=true&from=sync_account.xml

2. https://dxr.mozilla.org/mozilla-central/source/mobile/android/services/src/main/res/layout/sync_list_item.xml

We can do even better and remove associated styles.xml at

https://dxr.mozilla.org/mozilla-central/source/mobile/android/services/src/main/res/values-v11/sync_styles.xml

https://dxr.mozilla.org/mozilla-central/source/mobile/android/services/src/main/res/values/sync_styles.xml

and the folder 
https://dxr.mozilla.org/mozilla-central/source/mobile/android/services/src/main/res/values-large-v11

can you please modify your patch to include the above changes? 
Sorry, I missed them in the bug description.

@Nick: there are some string constants defined for sync. should we remove them?
Flags: needinfo?(nalexander)
Comment on attachment 8711451 [details] [diff] [review]
Bug #1241846 Version-1 patch

@malayaleecoder,
Good work. 
can you please submitted an updated patch with changes I asked for.
Attachment #8711451 - Flags: review?(vivekb.balakrishnan) → feedback+
(In reply to Vivek Balakrishnan[:vivek] from comment #8)
> Hi malayaleecoder,
> 
> Your patch looks good to me. 
> I noticed that the following xmls not used anymore, so can you please update
> your patch to remove them. 
> 
> 1.
> https://dxr.mozilla.org/mozilla-central/source/mobile/android/services/src/
> main/res/layout/sync_account.xml?case=true&from=sync_account.xml
> 
> 2.
> https://dxr.mozilla.org/mozilla-central/source/mobile/android/services/src/
> main/res/layout/sync_list_item.xml
> 
> We can do even better and remove associated styles.xml at
> 
> https://dxr.mozilla.org/mozilla-central/source/mobile/android/services/src/
> main/res/values-v11/sync_styles.xml
> 
> https://dxr.mozilla.org/mozilla-central/source/mobile/android/services/src/
> main/res/values/sync_styles.xml
> 
> and the folder 
> https://dxr.mozilla.org/mozilla-central/source/mobile/android/services/src/
> main/res/values-large-v11
> 
> can you please modify your patch to include the above changes? 
> Sorry, I missed them in the bug description.
> 
> @Nick: there are some string constants defined for sync. should we remove
> them?

In general, yes.

malayaleecoder: Can you take a look for these unused strings that Vivek is referring to and remove them in a second patch, on top of the one you are working on?  Vivek can help you use Mercurial (see http://mozilla-version-control-tools.readthedocs.org/en/latest/hgmozilla/index.html) to organize these patches.

Vivek, consider getting f? from me on this patch.  Thanks!
Flags: needinfo?(nalexander)
(In reply to Vivek Balakrishnan[:vivek] from comment #9)
> Comment on attachment 8711451 [details] [diff] [review]
> Bug #1241846 Version-1 patch
> 
> @malayaleecoder,
> Good work. 
> can you please submitted an updated patch with changes I asked for.

Vivek, I have removed the extra files you have mentioned. I did not get the unused strings that Nick is talking about. Should I submit a patch for the above said?
Flags: needinfo?(vivekb.balakrishnan)
Malayaleecoder,

As Nick mentioned this will part1 of the patches. Please submit your new patch for review.

After 1220906, there are some unused string references.The second part is a separate patch to remove these string reference from https://dxr.mozilla.org/mozilla-central/source/mobile/android/services/strings.xml.in and the corresponding strings from https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/locales/en-US/sync_strings.dtd.
 
You can find the reference from Android studio/ IntelliJ by searching for strings that start with sync_ prefix. The search result should point usage only in strings.xml file and no where else.
Please let me know if you need help with this part.

Note: The Strings.xml.in I pointed out is compiled at build time to android strings.xml file. strings.dtd file is used for localization support.
Flags: needinfo?(vivekb.balakrishnan)
Attached patch Part-1_v1Splinter Review
Added the part-1 patch
Attachment #8712329 - Flags: review?(vivekb.balakrishnan)
Assignee: nobody → malayaleecoder
Status: NEW → ASSIGNED
Vivek, I searched strings.xml.in and found 55 such strings starting with "sync_". Are those the ones I should remove from strings.xml.in and sync_strings.td ?
Flags: needinfo?(vivekb.balakrishnan)
Comment on attachment 8712329 [details] [diff] [review]
Part-1_v1

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

LGTM
Attachment #8712329 - Flags: review?(vivekb.balakrishnan)
Attachment #8712329 - Flags: review+
Attachment #8712329 - Flags: feedback?(nalexander)
Hi Malayaleecoder,

Yes, lets scope this bug for those 55 strings.
Flags: needinfo?(vivekb.balakrishnan)
Comment on attachment 8712329 [details] [diff] [review]
Part-1_v1

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

Looks good!

On the subject of the strings: I looked for consumers of "@string/sync" strings using dxr (https://dxr.mozilla.org/mozilla-central/search?q=%22%40string%2Fsync%22+path%3Amobile%2Fandroid&redirect=false&case=false) and I see only a few that will remain, notably https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/AndroidManifest.xml.in#424.  We'll want to keep those existing entities and strings that are in fact used.  Thanks for looking at this!

vivek: Consider landing this and filing a follow-up for the strings.  I'd like to get this cleaned up ASAP.
Attachment #8712329 - Flags: feedback?(nalexander) → feedback+
Attached patch Part-2_v1Splinter Review
Please check part-2 patch and let me know for further improvements.
Flags: needinfo?(vivekb.balakrishnan)
Attachment #8712793 - Flags: review?(vivekb.balakrishnan)
Comment on attachment 8712793 [details] [diff] [review]
Part-2_v1

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

Hi Malayaleecoder,

I have filed a new bug https://bugzilla.mozilla.org/show_bug.cgi?id=1243821 to remove the strings.
Can you file a new patch there addressing the following comments.

::: mobile/android/services/strings.xml.in
@@ +8,1 @@
>    <!-- J-PAKE PIN Screen -->

Remove the comments

@@ -54,3 @@
>    <!-- 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>

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

So don't remove them
Attachment #8712793 - Flags: review?(vivekb.balakrishnan) → feedback+
https://hg.mozilla.org/mozilla-central/rev/f4b78465549d
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
clearing NI flag
Flags: needinfo?(vivekb.balakrishnan)
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.