(Just filing this in General until bug 1021829 is fixed) Right now, eedens has a search activity prototype that lives here: https://github.com/ericedens/FirefoxSearch We want to create a more official place for this project to live, such that it can be built with Firefox for Android if we do desire (e.g. get it into Nightly builds for easy testing with daily updates). nalexander, what way do you think we should proceeed here? On IRC we talked about making a new mobile/android/search directory in m-c. Should we just start with a patch for m-c? Or should we also explore having this be developed in a separate repo but auto-magically merged into m-c. I'm in favor of just working in m-c, since that's what I know best, but I'm open to new ideas for better workflow. Either way, we should definitely maintain this as a separate Android project from Fennec, such that it could be run/developed on its own.

Let's go ahead and call this a build bug. My vote is for mobile/android/search, fwiw.

Make sure we don't forget that we'll need to make this l10n friendly. Don't make choices that back us into a corner for localization.

(In reply to Mark Finkle (:mfinkle) from comment #2) > Make sure we don't forget that we'll need to make this l10n friendly. Don't > make choices that back us into a corner for localization. Good call. l10n is another reason to make this part of mozilla-central, since localizers can use the same tools they would use to localize Fennec.

First steps pushed to try: http://tbpl.mozilla.org/?tree=Try&rev=7e916a07ee6c

(In reply to Nick Alexander :nalexander from comment #4) > First steps pushed to try: > > http://tbpl.mozilla.org/?tree=Try&rev=7e916a07ee6c B and R* green, first shot! The search-activity APK (debug) was built but not uploaded (partially because of Bug 919627), but it runs locally. I'll look into integrating with Fennec tomorrow.

WIP posted at: https://github.com/ncalexan/gecko-dev/tree/nalexander/bug-1021864-land-search-activity This lands the pre-req libraries into mobile/android/thirdparty [1], and lands the search activity code in mobile/android/search. As part of the build, it builds search-activity.apk [2]. I'll get started integrating the activity into the Fennec APK today. [1] Note to self: a few tweaks were required, in scribe and elsewhere. Remember to separate the tweaks from the initial landing. [2] Note to self: remember to call that search-activity-debug.apk, and sign search-activity.apk in packaging. If we even build the separate search activity APK.

Whoa, awesome progress! This is a great proof of concept of landing all the things we need in the tree, but I think we should take a step back to think about what things we actually *will* land in the tree. I don't think we should land the full-blown prototype, but rather rebuild it from lessons learned. As someone who will likely review this code, it would be nice to start with some incremental commits :) Also, I think we should asses the third-party libraries we want to use before landing them in the tree. Using them for the prototype is totally cool, but we should assess whether or not these are the best options going forward. However, we can always replace things in the future, so this doesn't need to be a final decision.

(In reply to :Margaret Leibovic from comment #7) > Whoa, awesome progress! > > This is a great proof of concept of landing all the things we need in the > tree, but I think we should take a step back to think about what things we > actually *will* land in the tree. Agreed. Preparing patches for landing all the things is the fastest way for me to get to the *interesting* bits about integration into Fennec: resources, strings and l10n, manifest merging, etc. I don't think we should land in this form, for certain. > I don't think we should land the full-blown prototype, but rather rebuild it > from lessons learned. As someone who will likely review this code, it would > be nice to start with some incremental commits :) I somewhat agree, but I'm not a huge stickler for this. For new functionality of a certain complexity, a big code drop is just easier. > Also, I think we should asses the third-party libraries we want to use > before landing them in the tree. Using them for the prototype is totally > cool, but we should assess whether or not these are the best options going > forward. However, we can always replace things in the future, so this > doesn't need to be a final decision. My guess is we'll want to keep at least a few (OAuth? AsyncHttp?) and remove others (EventBus). I'm not going to do surgery on the prototype code base, so I started by getting them all in and getting the APK building in tree. Excision is for later :)

Here's a minimal stub landing and build integration: https://tbpl.mozilla.org/?tree=Try&rev=625ecbdbf80c

And with the Search Activity not integrated into Fennec: http://tbpl.mozilla.org/?tree=Try&rev=e3194564f5a5

(In reply to Nick Alexander :nalexander from comment #10) > Here's a minimal stub landing and build integration: > > https://tbpl.mozilla.org/?tree=Try&rev=625ecbdbf80c This is actually fine, it's just that we're still not allowed to upload APKs (I got thrown off by geckoview_example being uploaded). See Bug 919627.

(In reply to Nick Alexander :nalexander from comment #11) > And with the Search Activity not integrated into Fennec: > > http://tbpl.mozilla.org/?tree=Try&rev=e3194564f5a5 Oh, I know what happened here. Building mobile/android/search should not be conditional. We might want to split the build flag into MOZ_ANDROID_SEARCH_ACTIVITY and MOZ_ANDROID_SEARCH_ACTIVITY in Fennec.

rnewman: I know your queue is long, but you're the right person to provide feedback on "does this feel like android-sync, with lessons learned". One flag for all the patches you care to look at.

margaret: you can see the structure of mobile/android/search pretty easily in this commit. Look decent?

Comment on attachment 8443560 [details] [diff] [review] Pre: Build R as a separate Java JAR. Review of attachment 8443560 [details] [diff] [review]: ----------------------------------------------------------------- ::: config/makefiles/java-build.mk @@ +23,5 @@ > > GENERATED_DIRS += classes > > +JAVA_JAR_TARGETS += R > +R_DEST := $(ANDROID_APK_NAME)-R.jar Does this have any effect on the Eclipse project generator? @@ +36,4 @@ > classes.dex: $(ANDROID_APK_NAME).ap_ > classes.dex: $(ANDROID_EXTRA_JARS) > classes.dex: $(JAVAFILES) > + $(if $(filter %.java,$^),\ Just checking my understanding: this doesn't need to include .java.in, because this is only working with the already preprocessed tree. @@ +57,5 @@ > .aapt.deps: $(android_manifest) $(android_res_files) $(wildcard $(ANDROID_ASSETS_DIR)) > @$(TOUCH) $@ > $(AAPT) package -f -M $< -I $(ANDROID_SDK)/android.jar $(_ANDROID_RES_FLAG) $(_ANDROID_ASSETS_FLAG) \ > + --auto-add-overlay \ > + --non-constant-id \ Does this have any perf impacts? ::: mobile/android/tests/browser/junit3/src/tests/TestRawResource.java @@ +65,5 @@ > } > > assertEquals(RAW_CONTENTS, result); > + > + System.out.println("app_name: " + R.string.app_name); Isn't there an Asserter method you should be using instead?

Comment on attachment 8443563 [details] [diff] [review] Part 2: Land stub org.mozilla.search APK. Review of attachment 8443563 [details] [diff] [review]: ----------------------------------------------------------------- Looks fine to me, albeit progressively more rubberstampy as it gets into packaging!

Comment on attachment 8443564 [details] [diff] [review] Part 3: Include Search Activity strings in Fennec. Review of attachment 8443564 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/locales/Makefile.in @@ +76,1 @@ > $(SYNCSTRINGSPATH) \ After SYNCSTRINGSPATH for consistency.

Comment on attachment 8443566 [details] [diff] [review] Part 4: Build Search Activity as part of the Fennec APK. Review of attachment 8443566 [details] [diff] [review]: ----------------------------------------------------------------- Trusting your judgment to get another build person to review if necessary. ::: mobile/android/base/AndroidManifest.xml.in @@ +18,5 @@ > #include ../services/manifests/SyncAndroidManifest_permissions.xml.in > > +#ifdef MOZ_ANDROID_SEARCH_ACTIVITY > +#include ../search/manifests/SearchAndroidManifest_permissions.xml.in > +#endif An alternative way to do this is to wrap the entirety of each .in with the ifdef, so the included text is empty if not defined. Up to you. ::: mobile/android/base/Makefile.in @@ +93,5 @@ > > +ifdef MOZ_ANDROID_SEARCH_ACTIVITY > +extra_packages += org.mozilla.search > +ALL_JARS += ../search/search-activity.jar > +generated/org/mozilla/search/R.java: .aapt.deps ; Build systems scare me.

Comment on attachment 8443563 [details] [diff] [review] Part 2: Land stub org.mozilla.search APK. Review of attachment 8443563 [details] [diff] [review]: ----------------------------------------------------------------- This looks like a fine start to me. Eric, does this look reasonable to you? We can always change things after they land, so I think it's fine to land something basic like this to start.

Comment on attachment 8443563 [details] [diff] [review] Part 2: Land stub org.mozilla.search APK. Review of attachment 8443563 [details] [diff] [review]: ----------------------------------------------------------------- I'm able to compile / install this against MC tip using r8e, SDK 19, Java 7. The build failed with Java 6, similar to bug 951968; but upgrading to Java 7 fixed that.

Status update: the Search Activity is going to use Geckoview. That means that the above build integration approach needs to change dramatically: Fennec and Geckoview are essentially equivalent, so we can't build the Search Activity *before* Fennec. We need to build it essentially concurrently. I'm working on patches right now. eedens: an ask for you: it is easiest to integrate resources if they all start with a common prefix. So, search_styles.xml, search_card.xml, search_app_name string, etc. This is because the resources get merged with the Fennec resources, and there is no provision for ordering/renaming, etc.

(In reply to Richard Newman [:rnewman] from comment #22) > Comment on attachment 8443566 [details] [diff] [review] > Part 4: Build Search Activity as part of the Fennec APK. > > Review of attachment 8443566 [details] [diff] [review]: > ----------------------------------------------------------------- > > Trusting your judgment to get another build person to review if necessary. > > ::: mobile/android/base/AndroidManifest.xml.in > @@ +18,5 @@ > > #include ../services/manifests/SyncAndroidManifest_permissions.xml.in > > > > +#ifdef MOZ_ANDROID_SEARCH_ACTIVITY > > +#include ../search/manifests/SearchAndroidManifest_permissions.xml.in > > +#endif > > An alternative way to do this is to wrap the entirety of each .in with the > ifdef, so the included text is empty if not defined. Up to you. Originally, I wanted to be able to build the Search Activity stand-alone (like android-sync.apk) and as part of Fennec, and have the stand-alone always built, so that approach wouldn't work. I think I pref this way even without a stand-alone mode. > ::: mobile/android/base/Makefile.in > @@ +93,5 @@ > > > > +ifdef MOZ_ANDROID_SEARCH_ACTIVITY > > +extra_packages += org.mozilla.search > > +ALL_JARS += ../search/search-activity.jar > > +generated/org/mozilla/search/R.java: .aapt.deps ; > > Build systems scare me. Indeed. Make is a frightening beast. wesj will understand this :)

This is a version of eedens github repo. There are some resource renamings (basically, prefixing files (not ids!) with search_) that I'll upstream as part of some scripts for that repo that I'm writing. margaret, eedens: a quick once over for sanity is all that's needed here.

Carrying forward the r+ from rnewman.

Carrying forward the r+ from rnewman.

wesj: the Search Activity is Java package org.mozilla.search. The Github repo builds a stand-alone APK with Android package org.mozilla.search. The code lands in Fennec (under the org.mozilla.fennec* Android package), built as a separate JAR in moz.build. The Makefile lines are the minimum necessary to add a new Android resource library using the mechanism you implemented. Look okay?

Comment on attachment 8446653 [details] [diff] [review] Part 1: Land mobile/android/search. r=margaret Review of attachment 8446653 [details] [diff] [review]: ----------------------------------------------------------------- From a quick sanity check, it looks good to me. From our search activity meeting yesterday (Jun 25), we decided to punt on the packaged wordlist. I'm fine with landing it now, and then removing it as part of a later patch.

(In reply to Nick Alexander :nalexander from comment #25) > eedens: an ask for you: it is easiest to integrate resources if they all > start with a common prefix. So, search_styles.xml, search_card.xml, > search_app_name string, etc. This is because the resources get merged with > the Fennec resources, and there is no provision for ordering/renaming, etc. This makes sense; using a prefix of "search" is fine with me!

With search activity: https://tbpl.mozilla.org/?tree=Try&rev=3807aea88f18

And without: https://tbpl.mozilla.org/?tree=Try&rev=e9a985643dfd

First landing needs https://github.com/ericedens/FirefoxSearch/issues/6 for sure. We might get help from Grunt or the android-sync repo.

Comment on attachment 8446653 [details] [diff] [review] Part 1: Land mobile/android/search. r=margaret Review of attachment 8446653 [details] [diff] [review]: ----------------------------------------------------------------- Rubber stamp to get something in the tree.

Comment on attachment 8446656 [details] [diff] [review] Part 4: Build Search Activity as part of the Fennec APK. r=wesj Review of attachment 8446656 [details] [diff] [review]: ----------------------------------------------------------------- I'm happy to rubberstamp this. If it builds, it builds. ::: mobile/android/base/moz.build @@ +555,5 @@ > > +if CONFIG['MOZ_ANDROID_SEARCH_ACTIVITY']: > + # The Search Activity is mostly independent of Fennec proper, but > + # it does depend on Geckoview. Therefore, we build it as a jar > + # that depends on the Geckoview jars. s/Geckoview/GeckoView

https://hg.mozilla.org/integration/fx-team/rev/3e8565120b1b https://hg.mozilla.org/integration/fx-team/rev/7d404850f5f2 https://hg.mozilla.org/integration/fx-team/rev/a7a8ea9e736b https://hg.mozilla.org/integration/fx-team/rev/bc6a8b22530a https://hg.mozilla.org/integration/fx-team/rev/9395fb24971c

(In reply to Richard Newman [:rnewman] from comment #37) > Comment on attachment 8446656 [details] [diff] [review] > Part 4: Build Search Activity as part of the Fennec APK. r=wesj > > Review of attachment 8446656 [details] [diff] [review]: > ----------------------------------------------------------------- > > I'm happy to rubberstamp this. If it builds, it builds. > > ::: mobile/android/base/moz.build > @@ +555,5 @@ > > > > +if CONFIG['MOZ_ANDROID_SEARCH_ACTIVITY']: > > + # The Search Activity is mostly independent of Fennec proper, but > > + # it does depend on Geckoview. Therefore, we build it as a jar > > + # that depends on the Geckoview jars. > > s/Geckoview/GeckoView Argh, I forgot to update this. I'll catch it some other time I'm in the area.

Landed, build time configured off.

https://hg.mozilla.org/mozilla-central/rev/3e8565120b1b https://hg.mozilla.org/mozilla-central/rev/7d404850f5f2 https://hg.mozilla.org/mozilla-central/rev/a7a8ea9e736b https://hg.mozilla.org/mozilla-central/rev/bc6a8b22530a https://hg.mozilla.org/mozilla-central/rev/9395fb24971c