1161195 - Don't hard-code class names in AppConstants.java.in and AndroidManifest.xml

Status: RESOLVED FIXED

(Firefox Build System :: Android Studio and Gradle Integration, enhancement)

Description

[Nathan Toone [:toonetown]]

Sometimes it is helpful to be able to override the main activities and applications with custom java classes when creating a branded build.  One example of this is to be able to do additional work on the android shared preferences (i.e. storing them in a different location).

I would suggest the ability to specify (in confvars.sh) the main browser activity (MOZ_ANDROID_BROWSER_INTENT_CLASS), the search activity (MOZ_ANDROID_SEARCH_INTENT_CLASS) and the overall application class (MOZ_ANDROID_APPLICATION_CLASS).

Also, if the branded directory contains any '*.jar' files, they should be included in the final package as well.

The browser intent and search intent classes are already specified (though hard-coded) in AppConstants.java.in.

Comment 1

[Nathan Toone [:toonetown]]

Attachment: bug-1161195-part1.diff

First part - which reads variables from the environment instead of hard-coding them into AppConstants.java.in

Comment 2

[Nathan Toone [:toonetown]]

Attachment: bug-1161195-part2.diff

Second part - includes jar files from branding directory tree to the final package

Comment 3

[Nathan Toone [:toonetown]]

Attachment: bug-1161195-part3.diff

Third part - rename variables to match the environment variable names

Comment 4

[Nathan Toone [:toonetown]]

Attachment: bug-1161195-docs.diff

This patch just adds some documentation in mobile/android/base/docs around branding and this feature.

Comment 5

[Nathan Toone [:toonetown]]

Attachment: bug-1161195-part1.diff

First part - which reads variables from the environment instead of hard-coding them into AppConstants.java.in (Updated to include the variables in configure.in)

Comment 6

[Nathan Toone [:toonetown]]

Attachment: bug-1161195-part3.diff

Third part - rename variables to match the environment variable names (Updated to also fix the values from Bug 1161134)

Comment 7

[Nathan Toone [:toonetown]]

Try server: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2238afdade1e

Comment 8

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8601221 [details] [diff] [review]
bug-1161195-part1.diff

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

nits only.

::: mobile/android/base/Makefile.in
@@ +30,5 @@
>    -DANDROID_VERSION_CODE=$(ANDROID_VERSION_CODE) \
>    -DMOZ_ANDROID_SHARED_ID="$(MOZ_ANDROID_SHARED_ID)" \
>    -DMOZ_ANDROID_SHARED_ACCOUNT_TYPE="$(MOZ_ANDROID_SHARED_ACCOUNT_TYPE)" \
>    -DMOZ_ANDROID_SHARED_FXACCOUNT_TYPE="$(MOZ_ANDROID_SHARED_FXACCOUNT_TYPE)" \
> +  -DMOZ_ANDROID_APPLICATION_CLASS="$(MOZ_ANDROID_APPLICATION_CLASS)" \

You should export these in moz.build, too.  See other examples where this happens.

::: mobile/android/confvars.sh
@@ +15,5 @@
>  # We support Android SDK version 9 and up by default.
>  # See the --enable-android-min-sdk and --enable-android-max-sdk arguments in configure.in.
>  MOZ_ANDROID_MIN_SDK_VERSION=9
>  
> +# These are the default values of the classes that are loaded - they can be overridden by branding configure.sh

There are several entry points into the Firefox application.  These are the names of the classes that are listed in the Android manifest.  These defaults can be overridden in the branding configure.sh.

Comment 9

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8601166 [details] [diff] [review]
bug-1161195-part2.diff

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

::: mobile/android/base/Makefile.in
@@ +105,5 @@
>    gecko-mozglue.jar \
>    gecko-thirdparty.jar \
>    gecko-util.jar \
>    sync-thirdparty.jar \
> +  $(wildcard $(DEPTH)/$(MOZ_BRANDING_DIRECTORY)/java/*.jar) \

This is not a thing that I support.  But I don't know what a better thing is.  Leaving the review open for me to think about.

Comment 10

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8601223 [details] [diff] [review]
bug-1161195-part3.diff

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

rubber stamp.  It's auto-generated, right?  =:D

::: mobile/android/base/AppConstants.java.in
@@ +79,5 @@
>          public static final boolean preHCMR1 = MAX_SDK_VERSION < 12 || (MIN_SDK_VERSION < 12 && Build.VERSION.SDK_INT < 12);
>          public static final boolean preHC = MAX_SDK_VERSION < 11 || (MIN_SDK_VERSION < 11 && Build.VERSION.SDK_INT < 11);
>      }
>  
>      /**

Update the comment, please.  (And add comments for the other two.)

Comment 11

[Nathan Toone [:toonetown]]

> rubber stamp.  It's auto-generated, right?  =:D

Yes - it was just a search/replace for variable names

> ::: mobile/android/base/AppConstants.java.in
> @@ +79,5 @@
> >          public static final boolean preHCMR1 = MAX_SDK_VERSION < 12 || (MIN_SDK_VERSION < 12 && Build.VERSION.SDK_INT < 12);
> >          public static final boolean preHC = MAX_SDK_VERSION < 11 || (MIN_SDK_VERSION < 11 && Build.VERSION.SDK_INT < 11);
> >      }
> >  
> >      /**
> 
> Update the comment, please.  (And add comments for the other two.)

I'll make the comment change as a part of "part 1" - and leave this patch (part 3) to be just the variable renaming.

Comment 12

[Nathan Toone [:toonetown]]

(In reply to Nick Alexander :nalexander from comment #9)
> Comment on attachment 8601166 [details] [diff] [review]
> bug-1161195-part2.diff
> 
> Review of attachment 8601166 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/Makefile.in
> @@ +105,5 @@
> >    gecko-mozglue.jar \
> >    gecko-thirdparty.jar \
> >    gecko-util.jar \
> >    sync-thirdparty.jar \
> > +  $(wildcard $(DEPTH)/$(MOZ_BRANDING_DIRECTORY)/java/*.jar) \
> 
> This is not a thing that I support.  But I don't know what a better thing
> is.  Leaving the review open for me to think about.

Understood.  I'm open to other alternatives as well.

Just as a note to myself - if this part of the patch doesn't land (but the other parts do, for some reason), then the documentation needs to be updated to reflect that.

Comment 13

[Nathan Toone [:toonetown]]

Attachment: bug-1161195-part1.diff

Update to part1 to take into account suggestions from :nalexander (put the variables in moz.build and update the comments)

Carrying over review flags.

Comment 14

[Nathan Toone [:toonetown]]

Attachment: bug-1161195-part3.diff

Update to part 3 so that it applies cleanly with comment changes from part 1 (and carrying over review flags)

Comment 15

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8601176 [details] [diff] [review]
bug-1161195-docs.diff

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

This is interesting.  Another way we could do this is to have a dedicated branding.mozbuild in mobile/android/base, which by default builds an empty gecko-branding.jar.  Then we add that gecko-branding.jar into the mix, depending on the all the other jars.  That way branding.mozbuild is always available to you and it is very unlikely you would ever see a conflict.  It's also a burden to our future build system flexibility like the wildcard in Makefile.in you added in part 2.

Tricky.  I think what we really want is ALL_JARS to be defined in moz.build, so you can extend it by including another .mozbuild file from within the main moz.build.

I'm happy to add the layer of indirection you want with the class names, but not to bend the build system to your particular use.  That will require time and thought; the ongoing maintainence is a big deal to us.  So let's move part 2 and some of this documentation to a new ticket tracking that, and get the indirection cleaned up and ready for landing.

::: mobile/android/base/docs/branding.rst
@@ +1,1 @@
> +.. -*- Mode: rst; fill-column: 80; -*-

Consider bumping this to 100.  80 is really narrow :)

Also, kill trailing whitespace throughout.

@@ +6,5 @@
> +
> +Much of the branding of the firefox browser can be accomplished by adding
> +variables to ``configure.sh`` in ``$(MOZ_BRANDING_DIRECTORY)``, and including
> +additional or different content, resources, and locales.  However, some 
> +modifications to the browser can better be handled using java classes.  To this

nit: Java.  Throughout.

@@ +11,5 @@
> +end, the ability to extend the "core" activity and application classes for the
> +browser has been added as a part of `Bug 1161195 <https://bugzilla.mozilla.org/show_bug.cgi?id=1161195>`.
> +This document details some of the steps required to implement such extensions.
> +
> +One reason for performing this type extension would be to modify the way that

Huh, this is what you want?  Interesting.

@@ +24,5 @@
> +The first step to override the functionality of the main classes is to extend
> +the provided java classes.  Currently, the following classes can be extended:
> +
> +1. org.mozilla.gecko.GeckoApplication
> + - The main application

s/main/Android/

@@ +28,5 @@
> + - The main application
> + - Overridden via the MOZ_ANDROID_APPLICATION_CLASS variable
> +
> +2. org.mozilla.gecko.BrowserApp
> + - The main browser activity

The browser activity.  This is what opens when you click the Firefox icon on the home screen, or when you Share to Firefox.

@@ +32,5 @@
> + - The main browser activity
> + - Overridden via the MOZ_ANDROID_BROWSER_INTENT_CLASS variable
> +
> +3. org.mozilla.search.SearchActivity
> + - The main search activity

The search activity.  This is what opens when you click the Firefox Search icon on the home screen, or when you swipe-up from the bottom of the device's screen.

@@ +40,5 @@
> +directory's ``java`` folder, and build them as a part of the normal build 
> +process (i.e. include them in ``moz.build``).
> +
> +An example ``$(MOZ_BRANDING_DIRECTORY)/java/BrandedApplication.java`` file 
> +might be::

nit throughout: newline after ::.

@@ +64,5 @@
> +
> +An example ``$(MOZ_BRANDING_DIRECTORY)/java/moz.build` would be::
> +  brandjar = add_java_jar('branding')
> +  brandjar.sources += [ 
> +      'BrandedApplication.java'

nit: always include trailing , in Python lists.  It looks better, makes diffs better, and can prevent errors in copy-paste.  (Since Python silently joins strings, even across lines.)

@@ +75,5 @@
> +  ]
> +
> +During the build process, this will create a jar called ``brandjar.jar`` 
> +containing your custom classes.  This jar will be included in the final 
> +(packaged) Android apk.

nit: drop (packaged), upper-case APK.

@@ +81,5 @@
> +
> +Using custom classes
> +====================
> +
> +You can now set the appropriate ``MOZ_ANDROID_*_CLASS`` variable in your

Wow, this truly is a poor man's dependency injection :(  (Although I guess Android makes this somewhat unavoidable, since the manifest must contain certain class names declared statically.)

Comment 16

[Nathan Toone [:toonetown]]

Removed the review flags - I will rework this as separate bugs as suggested above.

Comment 17

[Nathan Toone [:toonetown]]

Attachment: bug-1161195-fix-part1.diff

This is just the portion of the patch that puts the class names in confvars.sh

Comment 18

[Nathan Toone [:toonetown]]

Attachment: bug-1161195-fix-part2.diff

This is the second part of the patch - which handles the variable renaming.

Comment 19

[Nathan Toone [:toonetown]]

The two latest patches are just reworking of the comments and commit messages of what was originally part 1 and part 3.

Comment 20

[Nathan Toone [:toonetown]]

Try build with new patches: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a90703a67a81

Comment 21

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8601852 [details] [diff] [review]
bug-1161195-fix-part1.diff

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

wfm.

::: mobile/android/base/AppConstants.java.in
@@ +85,2 @@
>       */
> +    public static final String MOZ_ANDROID_APPLICATION_CLASS_NAME = "@MOZ_ANDROID_APPLICATION_CLASS@";

nit: just one line between, delete trailing whitespace.

::: mobile/android/confvars.sh
@@ +16,5 @@
>  # See the --enable-android-min-sdk and --enable-android-max-sdk arguments in configure.in.
>  MOZ_ANDROID_MIN_SDK_VERSION=9
>  
> +# There are several entry points into the Firefox application.  These are the names of some of the classes that are 
> +# listed in the Android manifest.  The are specified in here to avoid hard-coding them in source code files.

nit: delete trailing whitespace.

s/The are/They are/

Comment 22

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8601853 [details] [diff] [review]
bug-1161195-fix-part2.diff

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

rubberstamp.

Comment 23

[Nathan Toone [:toonetown]]

Attachment: bug-1161195-fix-part1.diff

Fix spacing and spelling error from last review.  Copy over review flag.

Comment 24

[Nathan Toone [:toonetown]]

Attachment: bug-1161195-fix-part2.diff

Update patch to apply cleanly after whitespace fixes in part 1.  Carry over review flags

Comment 25

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/df64a65ca1bd
https://hg.mozilla.org/integration/fx-team/rev/b36b6e987110

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/df64a65ca1bd
https://hg.mozilla.org/mozilla-central/rev/b36b6e987110