To keep resources for both Australis and Photon at the same time, we'll duplicate a copy of resources for Photon. Any visual refresh for Photon should only modify the duplicated files. That would make us easily maintain two APKs.

The duplicate resource files/folders are just temporary on 56 because currently we have to keep both Australis and Photon available. We should remove all legacy resources of Australis on 57.

Comment on attachment 8877018 [details] Bug 1372486 - Part 1: Move Fennec resources from base/resources to app/src/main/res. https://reviewboard.mozilla.org/r/148352/#review152856

Comment on attachment 8877019 [details] Bug 1372486 - Part 2: Fix Fennec resource path in build/lint scripts. https://reviewboard.mozilla.org/r/148354/#review152888

Please loop me in to all of these build changes. As it stands, this approach isn't needed. It will be much better if we: - overlay Photon resources on Australis resources, both in Gradle and in moz.build; - overlay Australis resources onto a set of shared resources, both in Gradle and in moz.build; rather than copying everything. The best approach will be to make the moz.build setup use the Gradle directory structure, which is the end goal. So: we should move: - all of the _shared_ resources to mobile/android/app/src/main/res - all of the _Photon-only_ (or replaced in Photon) resources to mobile/android/app/src/australis/res - all of the _Australis-only_ (or replaced in Australis) resources to mobile/android/app/src/photon/res That should "just work" for Gradle. Now, for moz.build, we need to opt-in to the new directories, updating the directories around http://searchfox.org/mozilla-central/source/mobile/android/base/moz.build#1374 based on the flags. Now, earlier directories should override later directories, so make sure to put the Photon-only and Australis-only directories _before_ the shared directory. From |aapt|: ``` -S directory in which to find resources. Multiple directories will be scanned and the first match found (left to right) will take precedence. ``` Again, please loop me in to _every_ build change. This system is very fragile and I have a plan on how to evolve towards Gradle; doing a bunch of things that don't align with that plan will only keep us away from that goal.

Comment on attachment 8877019 [details] Bug 1372486 - Part 2: Fix Fennec resource path in build/lint scripts. https://reviewboard.mozilla.org/r/148354/#review153010 I want to see this before it lands. ::: mobile/android/app/build.gradle:208 (Diff revision 3) > } > srcDir "${topsrcdir}/mobile/android/app/assets" > } > } > > + australis { Per my comment in the ticket, let's move to using the "Gradle default" locations, which I think I got right in the ticket, and make `moz.build` agree. ::: mobile/android/base/moz.build:1372 (Diff revision 3) > CONFIG['ANDROID_SUPPORT_V4_AAR_LIB'], > 'sync-thirdparty.jar', > ] > > # Putting branding earlier allows branders to override default resources. > +if CONFIG['MOZ_ANDROID_PHOTON_SKIN']: Prefer: ``` ANDROID_RES_DIRS += [...] if CONFIG[...] ANDROID_RES_DIRS += [...] else: ANDROID_RES_DIRS += [...] ANDROID_RES_DIRS += [...] ``` ::: mobile/android/moz.configure:114 (Diff revision 3) > +@depends('--enable-photon') > +def photon_skin(value): > + if value: > + return True > + > +set_config('MOZ_ANDROID_PHOTON_SKIN', photon_skin) Please make the `MOZ_ANDROID_XXX` agree with `--enable-XXX`. There's no advantage to making readers look up the mapping in the `moz.configure` file.

(In reply to Nick Alexander :nalexander from comment #9) > Please loop me in to all of these build changes. As it stands, this > approach isn't needed. It will be much better if we: > > - overlay Photon resources on Australis resources, both in Gradle and in > moz.build; > - overlay Australis resources onto a set of shared resources, both in Gradle > and in moz.build; I think the main difference between our approach is 1. We copy all resources at beginning and will drop resources in Australis eventually when Photon is ready, and 2. You prefer to use shared resources until Photon needs to modify them. The benefit of whole resource duplication is for easy development: We don't have to figure out which resource is shared/Australis-only/Photon-only right now. Since some visual design aren't finalized, fully duplication is the easiest way to keep Australis resource freeze. For each photon bugs, we can just add new resources into its resource folder, and remove unused files according to lint warning. Any changes in Photon won't effect Australis, and vice versa. Also since we will drop Australis when Photon is ready, all duplicated resources in Australis will be removed eventually.

Comment on attachment 8877018 [details] Bug 1372486 - Part 1: Move Fennec resources from base/resources to app/src/main/res. https://reviewboard.mozilla.org/r/148352/#review153330

(In reply to Jing-wei Wu [:jwu] from comment #11) > (In reply to Nick Alexander :nalexander from comment #9) > > Please loop me in to all of these build changes. As it stands, this > > approach isn't needed. It will be much better if we: > > > > - overlay Photon resources on Australis resources, both in Gradle and in > > moz.build; > > - overlay Australis resources onto a set of shared resources, both in Gradle > > and in moz.build; > > I think the main difference between our approach is > 1. We copy all resources at beginning and will drop resources in Australis > eventually when Photon is ready, and > 2. You prefer to use shared resources until Photon needs to modify them. > > The benefit of whole resource duplication is for easy development: > > We don't have to figure out which resource is > shared/Australis-only/Photon-only right now. Since some visual design aren't > finalized, fully duplication is the easiest way to keep Australis resource > freeze. You're thinking of this only in one direction: you're thinking about making Photon-specific changes and not wanting to touch Australis. But there will be Australis/generic development, and you're making every Australis/generic change touch _two_ resources now. I see 803 files in the shared resource folder. You have something like 14 weeks to do the Photon work. To decide what to do with each file, you will have to replace or delete ~10 resource files a day just to get through the list. (And some are very complicated, like styles.xml!) I don't think that's going to happen; you're going to have a tiny amount of Photon work on top of a _lot_ of shared/Australis resources. I believe you will be much better off tactically adding and replacing resources in photon/res than this bulk copy. Now, we may find that you need to copy-and-edit some files (like styles.xml) due to limitations in the aapt merging in moz.build. But that will still be much better than this bulk copy, where the sets of files will immediately diverge and you will have a much harder time just understanding _what's changed_ between the two resource sets. > For each photon bugs, we can just add new resources into its resource > folder, and remove unused files according to lint warning. Any changes in > Photon won't effect Australis, and vice versa. This is also true if you make photon/res an overlay on top of main/res. > Also since we will drop Australis when Photon is ready, all duplicated > resources in Australis will be removed eventually. This is also true if you remove australis/res when Australis is removed, and move australis/res/** into main/res. Show me work-in-progress patches that you think require this copy operation, rather than an overlay operation. As it is, this approach is not optimal, for the reasons I have pointed out.

Comment on attachment 8877019 [details] Bug 1372486 - Part 2: Fix Fennec resource path in build/lint scripts. https://reviewboard.mozilla.org/r/148354/#review153736 Please request again once the design is finalized. Thank you!

Comment on attachment 8877019 [details] Bug 1372486 - Part 2: Fix Fennec resource path in build/lint scripts. https://reviewboard.mozilla.org/r/148354/#review153866 ::: mobile/android/app/build.gradle:208 (Diff revision 3) > } > srcDir "${topsrcdir}/mobile/android/app/assets" > } > } > > + australis { Okay, I'll file new patches ASAP that 1. move resources from base/resources to app/src/main/res, and 2. let Australis/Photon can put their specific resources into their own folders.

Comment on attachment 8877019 [details] Bug 1372486 - Part 2: Fix Fennec resource path in build/lint scripts. https://reviewboard.mozilla.org/r/148354/#review154028 ::: mobile/android/app/build.gradle (Diff revision 4) > } > > res { > srcDir "${topsrcdir}/${mozconfig.substs.MOZ_BRANDING_DIRECTORY}/res" > srcDir "${project.buildDir}/generated/source/preprocessed_resources" // See syncPreprocessedResources. > - srcDir "${topsrcdir}/mobile/android/base/resources" Huh, I wonder if we just changed the order that Gradle's resource merger will find things. It shouldn't matter for Fennec, but it might matter for the one person I know who was customizing resources using `MOZ_BRANDING_DIRECTORY`. In any case, roll on! ::: mobile/android/base/moz.build:1371 (Diff revision 4) > ] > > # Putting branding earlier allows branders to override default resources. > ANDROID_RES_DIRS += [ > '/' + CONFIG['MOZ_BRANDING_DIRECTORY'] + '/res', > - 'resources', > + '/mobile/android/app/src/main/res', Same comment about ordering. This is definitely correct for `moz.build`, so roll on.

Comment on attachment 8877951 [details] Bug 1372486 - Part 3: Support mach/gradle build for Photon with its resources. https://reviewboard.mozilla.org/r/149358/#review154032 r- just to make sure I got the Gradle directories correct. I just checked, using `./mach gradle sourceSets`: ``` australis --------- Compile configuration: australisCompile build.gradle name: android.sourceSets.australis Java sources: [mobile/android/app/src/australis/java] Manifest file: mobile/android/app/src/australis/AndroidManifest.xml Android resources: [mobile/android/app/src/australis/res] Assets: [mobile/android/app/src/australis/assets] AIDL sources: [mobile/android/app/src/australis/aidl] RenderScript sources: [mobile/android/app/src/australis/rs] JNI sources: [mobile/android/app/src/australis/jni] JNI libraries: [mobile/android/app/src/australis/jniLibs] Java-style resources: [mobile/android/app/src/australis/resources] ``` ::: mobile/android/app/build.gradle:204 (Diff revision 1) > } > } > > + australis { > + res { > + srcDir "${topsrcdir}/mobile/android/australis/src/main/res" I see where you're going, but the Gradle way of doing this is to use the per-flavor dimension directories, which in this case should be: mobile/android/app/src/australis/res mobile/android/app/src/photon/res Those will get used by the Gradle build system automatically, and you shouldn't need any `res.srcDir` declarations at all.

Comment on attachment 8877965 [details] Bug 1372486 - Part 4: Create a dummy files for empty resource folders. https://reviewboard.mozilla.org/r/149364/#review154034 I'd prefer to land some meaningful file, like `res/values/skin_strings.xml` that has a single `<string skin="australis">` or, even better an actual resources that's different between Australis and Photon, but I won't block on it. The `.mkdir.done` files are produced by the build system in the object directory, so I definitely don't want to check something with that name into the source directory. If you really need these dummy files, choose something like `.not-part-of-the-build` or similar.

Comment on attachment 8877018 [details] Bug 1372486 - Part 1: Move Fennec resources from base/resources to app/src/main/res. https://reviewboard.mozilla.org/r/148352/#review154024 Assuming this is just `hg mv ...`, roll on! ::: commit-message-04981:1 (Diff revision 3) > +Bug 1372486 - Part 1: Move Fennec resources from base/resources to app/src/main/res. r?nalexander,maliu,nechen,walkingice I find it helpful to include the hg commands in the commit message for such things: Trailing slashes can matter for hg mv; did you do: hg mv mobile/android/base/resources/ app/src/main/res/

(In reply to Nick Alexander :nalexander from comment #23) > Comment on attachment 8877018 [details] > Bug 1372486 - Part 1: Move Fennec resources from base/resources to > app/src/main/res. > > https://reviewboard.mozilla.org/r/148352/#review154024 > > Assuming this is just `hg mv ...`, roll on! > > ::: commit-message-04981:1 > (Diff revision 3) > > +Bug 1372486 - Part 1: Move Fennec resources from base/resources to app/src/main/res. r?nalexander,maliu,nechen,walkingice > > I find it helpful to include the hg commands in the commit message for such > things: > > Trailing slashes can matter for hg mv; did you do: > > hg mv mobile/android/base/resources/ app/src/main/res/ The command I used is `hg mv mobile/android/base/resources app/src/main/res`, What is the difference between with and without trailing slashes? Should I file another patch?

(In reply to Jing-wei Wu [:jwu] from comment #24) > (In reply to Nick Alexander :nalexander from comment #23) > > Comment on attachment 8877018 [details] > > Bug 1372486 - Part 1: Move Fennec resources from base/resources to > > app/src/main/res. > > > > https://reviewboard.mozilla.org/r/148352/#review154024 > > > > Assuming this is just `hg mv ...`, roll on! > > > > ::: commit-message-04981:1 > > (Diff revision 3) > > > +Bug 1372486 - Part 1: Move Fennec resources from base/resources to app/src/main/res. r?nalexander,maliu,nechen,walkingice > > > > I find it helpful to include the hg commands in the commit message for such > > things: > > > > Trailing slashes can matter for hg mv; did you do: > > > > hg mv mobile/android/base/resources/ app/src/main/res/ > > The command I used is `hg mv mobile/android/base/resources > app/src/main/res`, What is the difference between with and without trailing > slashes? Should I file another patch? Nope, not if what you did works! The slashes mostly have to do with what happens when the target directory already exists; it's the difference between moving foo/** into bar/ or moving foo/ into bar/foo/, IIRC.

Comment on attachment 8877951 [details] Bug 1372486 - Part 3: Support mach/gradle build for Photon with its resources. https://reviewboard.mozilla.org/r/149358/#review154522 This looks good! If it's green on try, it's good for me! I will draft a mail to mobile-firefox-dev explaining where the existing resources are and what the Australis/Photon/Gradle plan is. I might send that without your review, since you (and others!) can correct me on the list; or I might get your feedback first. Thanks for bringing this into line with my thinking!

Comment on attachment 8877965 [details] Bug 1372486 - Part 4: Create a dummy files for empty resource folders. https://reviewboard.mozilla.org/r/149364/#review154528 Looks good! ::: commit-message-8840b:3 (Diff revision 2) > +Bug 1372486 - Part 4: Create a dummy files for empty resource folders. r?nalexander,maliu,nechen,walkingice > + > +Since Mercurial doesn't support empty folder, a dummy file is added to each res folers. nit: s/folers/folders/ ::: commit-message-8840b:4 (Diff revision 2) > +Bug 1372486 - Part 4: Create a dummy files for empty resource folders. r?nalexander,maliu,nechen,walkingice > + > +Since Mercurial doesn't support empty folder, a dummy file is added to each res folers. > +Them should be removed after we put Photon/Australis specific resources in their own res folders. nit: s/Them/These files/

Try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=76623b1e5c2f3446db6d6827eb310c98fff0a632

(In reply to Jing-wei Wu [:jwu] from comment #32) > Try result: > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=76623b1e5c2f3446db6d6827eb310c98fff0a632 I added a few jobs, most importantly the android-api-15-gradle job, which is the actual Photon-enabled build.

Comment on attachment 8877965 [details] Bug 1372486 - Part 4: Create a dummy files for empty resource folders. https://reviewboard.mozilla.org/r/149364/#review154736

Comment on attachment 8877951 [details] Bug 1372486 - Part 3: Support mach/gradle build for Photon with its resources. https://reviewboard.mozilla.org/r/149358/#review154738

Comment on attachment 8877019 [details] Bug 1372486 - Part 2: Fix Fennec resource path in build/lint scripts. https://reviewboard.mozilla.org/r/148354/#review154740

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/3649a9346e36 Part 1: Move Fennec resources from base/resources to app/src/main/res. r=nalexander,nechen,walkingice https://hg.mozilla.org/integration/autoland/rev/511276ae38a7 Part 2: Fix Fennec resource path in build/lint scripts. r=nalexander,nechen,walkingice https://hg.mozilla.org/integration/autoland/rev/d60f1d602dca Part 3: Support mach/gradle build for Photon with its resources. r=nalexander,nechen https://hg.mozilla.org/integration/autoland/rev/da771468e363 Part 4: Create a dummy files for empty resource folders. r=nalexander,nechen

Backed out for build bustage failing TestMozbuildReading.test_orphan_file_patterns for mobile/android/base/moz.build: https://hg.mozilla.org/integration/autoland/rev/ca11e8883c527f1d311e9f8995eea884075a4500 https://hg.mozilla.org/integration/autoland/rev/f26e5ef35d118ceeded19fe1ee1ba12552c9b1ff https://hg.mozilla.org/integration/autoland/rev/f362fd82fd6005b357065992c7e0471532bd9ecd https://hg.mozilla.org/integration/autoland/rev/b5bdd69dfa60f745727fb3d594199ddc8ce374b1 Push with bustage: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=da771468e3635f4488d08cf72144c8d64f25c3dd&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable Build log: https://treeherder.mozilla.org/logviewer.html#?job_id=107918137&repo=autoland TEST-UNEXPECTED-FAIL | /home/worker/workspace/build/src/config/tests/test_mozbuild_reading.py | TestMozbuildReading.test_orphan_file_patterns, line 105: The pattern 'resources/**' in a Files() entry in '/home/worker/workspace/build/src/mobile/android/base/moz.build' corresponds to no files in the tree. AssertionError: The pattern 'resources/**' in a Files() entry in '/home/worker/workspace/build/src/mobile/android/base/moz.build' corresponds to no files in the tree.

Add a new patch to fix the resource path defined in mobile/android/base/moz.build. The new try command that covers the test is `mach try -b do -p android-api-15,android-x86,linux64 -u robocop -t none`, and the result is here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=55b789ba84618ada9cb6f92e74d289043f18e524&selectedJob=108074408

Comment on attachment 8878995 [details] Bug 1372486 - Part 5: Fix Fennec resources path defined in moz.build to pass `mach build check`. https://reviewboard.mozilla.org/r/150268/#review154910

Comment on attachment 8878995 [details] Bug 1372486 - Part 5: Fix Fennec resources path defined in moz.build to pass `mach build check`. https://reviewboard.mozilla.org/r/150268/#review155174 ::: mobile/android/base/moz.build:91 (Diff revision 1) > BUG_COMPONENT = ('Firefox for Android', 'Profile Handling') > > with Files('locales/**'): > BUG_COMPONENT = ('Firefox for Android', 'General') > > -with Files('resources/**'): > +with Files('../app/src/main/res/**'): I think we should add '../app/src/australis/**' to 'General', and '../app/src/photon/**' to _something_ -- do we have a Photon component? If not, let's add it to General. We might make these _all_ '../app/src/*/res/...', since roughly speaking main, photon, australis will all divide into the same pieces. Make sense?

Comment on attachment 8878995 [details] Bug 1372486 - Part 5: Fix Fennec resources path defined in moz.build to pass `mach build check`. https://reviewboard.mozilla.org/r/150268/#review155174 > I think we should add '../app/src/australis/**' to 'General', and '../app/src/photon/**' to _something_ -- do we have a Photon component? If not, let's add it to General. > > We might make these _all_ '../app/src/*/res/...', since roughly speaking main, photon, australis will all divide into the same pieces. > > Make sense? Yeah, use `*` for main/australis/photon makes more sense.

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/cf646df15e71 Part 1: Move Fennec resources from base/resources to app/src/main/res. r=nalexander,nechen,walkingice https://hg.mozilla.org/integration/autoland/rev/935cc1064edd Part 2: Fix Fennec resource path in build/lint scripts. r=nalexander,nechen,walkingice https://hg.mozilla.org/integration/autoland/rev/c0962acec73d Part 3: Support mach/gradle build for Photon with its resources. r=nalexander,nechen https://hg.mozilla.org/integration/autoland/rev/a4636f4caa44 Part 4: Create a dummy files for empty resource folders. r=nalexander,nechen https://hg.mozilla.org/integration/autoland/rev/770a7a380874 Part 5: Fix Fennec resources path defined in moz.build to pass `mach build check`. r=nalexander,nechen

https://hg.mozilla.org/mozilla-central/rev/cf646df15e71 https://hg.mozilla.org/mozilla-central/rev/935cc1064edd https://hg.mozilla.org/mozilla-central/rev/c0962acec73d https://hg.mozilla.org/mozilla-central/rev/a4636f4caa44 https://hg.mozilla.org/mozilla-central/rev/770a7a380874

Comment on attachment 8877951 [details] Bug 1372486 - Part 3: Support mach/gradle build for Photon with its resources. https://reviewboard.mozilla.org/r/149358/#review156642

Comment on attachment 8877965 [details] Bug 1372486 - Part 4: Create a dummy files for empty resource folders. https://reviewboard.mozilla.org/r/149364/#review156644

Comment on attachment 8878995 [details] Bug 1372486 - Part 5: Fix Fennec resources path defined in moz.build to pass `mach build check`. https://reviewboard.mozilla.org/r/150268/#review156646