Closed Bug 923519 Opened 11 years ago Closed 11 years ago

Remove resource preprocessing

Categories

(Firefox for Android Graveyard :: General, defect)

All
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 27

People

(Reporter: bnicholson, Assigned: bnicholson)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

We have a several *.xml.in resource files that are preprocessed using #filter substitution with make variables. We can factor these make variables out into strings.xml.in and reference the strings in the resource files (instead of the make vars directly) to remove the preprocessing.
Attached patch Remove resource preprocessing (obsolete) — Splinter Review
Attachment #813608 - Flags: review?(nalexander)
Comment on attachment 813608 [details] [diff] [review]
Remove resource preprocessing

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

lgtm.  I'll test locally, and I'll land with the required android-sync github changes.

::: mobile/android/base/android-services-files.mk
@@ +349,3 @@
>    $(NULL)
>  
>  SYNC_PP_RES_XML := \

This should go.  But it requires android-sync github changes, so I'll land with this removed.

::: mobile/android/base/locales/Makefile.in
@@ +69,2 @@
>  	  -DBRANDPATH="$(BRANDPATH)" \
> +	  -DMOZ_ANDROID_SHARED_ACCOUNT_TYPE=$(MOZ_ANDROID_SHARED_ACCOUNT_TYPE) \

I'm not confident that MOZ_ANDROID_SHARED_ACCOUNT_TYPE is defined here.
Attachment #813608 - Flags: review?(nalexander) → review+
(In reply to Nick Alexander :nalexander from comment #2)
> ::: mobile/android/base/locales/Makefile.in
> @@ +69,2 @@
> >  	  -DBRANDPATH="$(BRANDPATH)" \
> > +	  -DMOZ_ANDROID_SHARED_ACCOUNT_TYPE=$(MOZ_ANDROID_SHARED_ACCOUNT_TYPE) \
> 
> I'm not confident that MOZ_ANDROID_SHARED_ACCOUNT_TYPE is defined here.

I didn't get any build errors with this patch, so I think we're safe.
(In reply to Brian Nicholson (:bnicholson) from comment #3)
> (In reply to Nick Alexander :nalexander from comment #2)
> > ::: mobile/android/base/locales/Makefile.in
> > @@ +69,2 @@
> > >  	  -DBRANDPATH="$(BRANDPATH)" \
> > > +	  -DMOZ_ANDROID_SHARED_ACCOUNT_TYPE=$(MOZ_ANDROID_SHARED_ACCOUNT_TYPE) \
> > 
> > I'm not confident that MOZ_ANDROID_SHARED_ACCOUNT_TYPE is defined here.
> 
> I didn't get any build errors with this patch, so I think we're safe.

You won't get a compile error; make treats a missing value as an empty string, so this will compile and Sync will fail.  (Check $OBJDIR/mobile/android/base/res/values/strings.xml; you'll see the shared account type is null.)  So we'll need to fix this, which I don't have time to do right now.
Comment on attachment 814142 [details] [diff] [review]
Remove resource preprocessing. r=nalexander

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

This is a slightly updated version of bnicholson's patch.
Attachment #814142 - Flags: review+
Attachment #813608 - Attachment is obsolete: true
Attachment #814141 - Flags: review?(gps)
Comment on attachment 814142 [details] [diff] [review]
Remove resource preprocessing. r=nalexander

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

::: mobile/android/base/android-services-files.mk
@@ +347,2 @@
>    res/xml/sync_syncadapter.xml \
>    res/xml/sync_options.xml \

Nit for your modifications: It looks like you wanted to reorder this alphabetically since you put sync_authenticator first, so you should move sync_options before sync_syncadapter.
Comment on attachment 814141 [details] [diff] [review]
Pre: Expose MOZ_ANDROID_SHARED_{ID,ACCOUNT_TYPE} to mobile/android/**/Makefile.in. r=gps

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

As much as I dislike defs.mk, r+.
Attachment #814141 - Flags: review?(gps) → review+
https://hg.mozilla.org/mozilla-central/rev/4d996eac6432
https://hg.mozilla.org/mozilla-central/rev/085d56ad055d
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 27
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: