1021864 - Create directory structure for search activity project

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Search Activity, defect)

Description

[:Margaret Leibovic]

(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.

Comment 1

[Richard Newman [:rnewman]]

Let's go ahead and call this a build bug.

My vote is for mobile/android/search, fwiw.

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

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.

Comment 3

[:Margaret Leibovic]

(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.

Comment 4

[Nick Alexander :nalexander [he/him]]

First steps pushed to try:

http://tbpl.mozilla.org/?tree=Try&rev=7e916a07ee6c

Comment 5

[Nick Alexander :nalexander [he/him]]

(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.

Comment 6

[Nick Alexander :nalexander [he/him]]

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.

Comment 7

[:Margaret Leibovic]

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.

Comment 8

[Nick Alexander :nalexander [he/him]]

(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 :)

Comment 9

[Nick Alexander :nalexander [he/him]]

http://tbpl.mozilla.org/?tree=Try&rev=81ff91b4db45

Comment 10

[Nick Alexander :nalexander [he/him]]

Here's a minimal stub landing and build integration:

https://tbpl.mozilla.org/?tree=Try&rev=625ecbdbf80c

Comment 11

[Nick Alexander :nalexander [he/him]]

And with the Search Activity not integrated into Fennec:

http://tbpl.mozilla.org/?tree=Try&rev=e3194564f5a5

Comment 12

[Nick Alexander :nalexander [he/him]]

(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.

Comment 13

[Nick Alexander :nalexander [he/him]]

(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.

Comment 14

[Nick Alexander :nalexander [he/him]]

Attachment: Pre: Build R as a separate Java JAR.

Comment 15

[Nick Alexander :nalexander [he/him]]

Attachment: Part 1: Add build feature flag MOZ_ANDROID_SEARCH_ACTIVITY.

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.

Comment 16

[Nick Alexander :nalexander [he/him]]

Attachment: Part 2: Land stub org.mozilla.search APK.

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

Comment 17

[Nick Alexander :nalexander [he/him]]

Attachment: Part 3: Include Search Activity strings in Fennec.

Comment 18

[Nick Alexander :nalexander [he/him]]

Attachment: Part 4: Build Search Activity as part of the Fennec APK.

Comment 19

[Richard Newman [:rnewman]]

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 20

[Richard Newman [:rnewman]]

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 21

[Richard Newman [:rnewman]]

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 22

[Richard Newman [:rnewman]]

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 23

[:Margaret Leibovic]

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 24

[Eric Edens [:eedens]]

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.

Comment 25

[Nick Alexander :nalexander [he/him]]

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.

Comment 26

[Nick Alexander :nalexander [he/him]]

(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 :)

Comment 27

[Nick Alexander :nalexander [he/him]]

Attachment: Part 1: Land mobile/android/search. r=margaret

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.

Comment 28

[Nick Alexander :nalexander [he/him]]

Attachment: Part 2: Add build feature flag MOZ_ANDROID_SEARCH_ACTIVITY. r=rnewman

Carrying forward the r+ from rnewman.

Comment 29

[Nick Alexander :nalexander [he/him]]

Attachment: Part 3: Include Search Activity strings in Fennec. r=rnewman

Carrying forward the r+ from rnewman.

Comment 30

[Nick Alexander :nalexander [he/him]]

Attachment: Part 4: Build Search Activity as part of the Fennec APK. r=wesj

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 31

[Eric Edens [:eedens]]

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.

Comment 32

[Eric Edens [:eedens]]

(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!

Comment 33

[Nick Alexander :nalexander [he/him]]

With search activity:

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

Comment 34

[Nick Alexander :nalexander [he/him]]

And without:

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

Comment 35

[Nick Alexander :nalexander [he/him]]

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

Comment 36

[:Margaret Leibovic]

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 37

[Richard Newman [:rnewman]]

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

Comment 38

[Nick Alexander :nalexander [he/him]]

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

Comment 39

[Nick Alexander :nalexander [he/him]]

(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.

Comment 40

[Nick Alexander :nalexander [he/him]]

Landed, build time configured off.

Comment 41

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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