Create directory structure for search activity project

RESOLVED FIXED in Firefox 33

Status

Firefox for Android Graveyard
Search Activity
RESOLVED FIXED
4 years ago
5 months ago

People

(Reporter: Margaret, Assigned: nalexander)

Tracking

Trunk
Firefox 33
All
Android
Dependency tree / graph

Details

Attachments

(4 attachments, 5 obsolete attachments)

514.01 KB, patch
Margaret
: review+
eedens
: feedback+
Details | Diff | Splinter Review
3.20 KB, patch
nalexander
: review+
Details | Diff | Splinter Review
5.37 KB, patch
nalexander
: review+
Details | Diff | Splinter Review
6.56 KB, patch
rnewman
: review+
Details | Diff | Splinter Review
(Reporter)

Description

4 years ago
(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.
Flags: needinfo?(nalexander)
(Reporter)

Updated

4 years ago
Blocks: 1017135
Let's go ahead and call this a build bug.

My vote is for mobile/android/search, fwiw.
Component: General → Build Config & IDE Support
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.
(Reporter)

Comment 3

4 years ago
(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.
(Assignee)

Updated

4 years ago
Component: Build Config & IDE Support → Search Activity
Flags: needinfo?(nalexander)
(Assignee)

Comment 4

4 years ago
First steps pushed to try:

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

Comment 5

4 years ago
(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.
(Assignee)

Updated

4 years ago
Depends on: 1024299
(Assignee)

Comment 6

4 years ago
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.
(Assignee)

Updated

4 years ago
Depends on: 1024527
(Reporter)

Comment 7

4 years ago
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.
(Assignee)

Comment 8

4 years ago
(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 :)
(Assignee)

Comment 10

4 years ago
Here's a minimal stub landing and build integration:

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

Comment 11

4 years ago
And with the Search Activity not integrated into Fennec:

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

Comment 12

4 years ago
(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.
Depends on: 919627
(Assignee)

Comment 13

4 years ago
(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.
(Assignee)

Comment 14

4 years ago
Created attachment 8443560 [details] [diff] [review]
Pre: Build R as a separate Java JAR.
(Assignee)

Comment 15

4 years ago
Created attachment 8443562 [details] [diff] [review]
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.
Attachment #8443562 - Flags: review?(rnewman)
(Assignee)

Comment 16

4 years ago
Created attachment 8443563 [details] [diff] [review]
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?
Attachment #8443563 - Flags: review?(margaret.leibovic)
(Assignee)

Comment 17

4 years ago
Created attachment 8443564 [details] [diff] [review]
Part 3: Include Search Activity strings in Fennec.
(Assignee)

Comment 18

4 years ago
Created attachment 8443566 [details] [diff] [review]
Part 4: Build Search Activity as part of the Fennec APK.
Attachment #8443562 - Flags: review?(rnewman) → review+
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!
Attachment #8443563 - Flags: review+
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.
Attachment #8443564 - Flags: review+
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.
Attachment #8443566 - Flags: review+
(Reporter)

Comment 23

4 years ago
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.
Attachment #8443563 - Flags: review?(margaret.leibovic)
Attachment #8443563 - Flags: review?(eedens)
Attachment #8443563 - Flags: review+
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.
Attachment #8443563 - Flags: review?(eedens) → review+
(Assignee)

Comment 25

4 years ago
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.
Flags: needinfo?(eedens)
(Assignee)

Comment 26

4 years ago
(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 :)
(Assignee)

Comment 27

4 years ago
Created attachment 8446653 [details] [diff] [review]
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.
Attachment #8446653 - Flags: review?(margaret.leibovic)
Attachment #8446653 - Flags: feedback?(eedens)
(Assignee)

Comment 28

4 years ago
Created attachment 8446654 [details] [diff] [review]
Part 2: Add build feature flag MOZ_ANDROID_SEARCH_ACTIVITY. r=rnewman

Carrying forward the r+ from rnewman.
Attachment #8443560 - Attachment is obsolete: true
Attachment #8443562 - Attachment is obsolete: true
Attachment #8443563 - Attachment is obsolete: true
Attachment #8446654 - Flags: review+
(Assignee)

Comment 29

4 years ago
Created attachment 8446655 [details] [diff] [review]
Part 3: Include Search Activity strings in Fennec. r=rnewman

Carrying forward the r+ from rnewman.
Attachment #8443564 - Attachment is obsolete: true
Attachment #8446655 - Flags: review+
(Assignee)

Comment 30

4 years ago
Created attachment 8446656 [details] [diff] [review]
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?
Attachment #8443566 - Attachment is obsolete: true
Attachment #8446656 - Flags: review?(wjohnston)
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.
Attachment #8446653 - Flags: feedback?(eedens) → feedback+
(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!
Flags: needinfo?(eedens)
(Assignee)

Comment 35

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

Updated

4 years ago
Depends on: 1031380

Updated

4 years ago
Depends on: 1031534
(Reporter)

Comment 36

4 years ago
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.
Attachment #8446653 - Flags: review?(margaret.leibovic) → review+
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
Attachment #8446656 - Flags: review?(wjohnston) → review+
(Assignee)

Comment 39

4 years ago
(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.
(Assignee)

Comment 40

4 years ago
Landed, build time configured off.

Updated

5 months ago
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.