Closed Bug 1161195 Opened 5 years ago Closed 5 years ago

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

Categories

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

enhancement
Not set
normal

Tracking

(firefox40 fixed)

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: toonetown, Assigned: toonetown)

References

Details

Attachments

(2 files, 10 obsolete files)

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.
Assignee: nobody → nathan
Attached patch bug-1161195-part1.diff (obsolete) — Splinter Review
First part - which reads variables from the environment instead of hard-coding them into AppConstants.java.in
Attachment #8601165 - Flags: review?(rnewman)
Attachment #8601165 - Flags: review?(nalexander)
Attached patch bug-1161195-part2.diff (obsolete) — Splinter Review
Second part - includes jar files from branding directory tree to the final package
Attachment #8601166 - Flags: review?(rnewman)
Attachment #8601166 - Flags: review?(nalexander)
Attached patch bug-1161195-part3.diff (obsolete) — Splinter Review
Third part - rename variables to match the environment variable names
Attachment #8601167 - Flags: review?(rnewman)
Attachment #8601167 - Flags: review?(nalexander)
Attached patch bug-1161195-docs.diff (obsolete) — Splinter Review
This patch just adds some documentation in mobile/android/base/docs around branding and this feature.
Attachment #8601176 - Flags: review?(rnewman)
Attachment #8601176 - Flags: review?(nalexander)
Attached patch bug-1161195-part1.diff (obsolete) — Splinter Review
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)
Attachment #8601165 - Attachment is obsolete: true
Attachment #8601165 - Flags: review?(rnewman)
Attachment #8601165 - Flags: review?(nalexander)
Attachment #8601221 - Flags: review?(rnewman)
Attachment #8601221 - Flags: review?(nalexander)
Attached patch bug-1161195-part3.diff (obsolete) — Splinter Review
Third part - rename variables to match the environment variable names (Updated to also fix the values from Bug 1161134)
Attachment #8601167 - Attachment is obsolete: true
Attachment #8601167 - Flags: review?(rnewman)
Attachment #8601167 - Flags: review?(nalexander)
Attachment #8601223 - Flags: review?(rnewman)
Attachment #8601223 - Flags: review?(nalexander)
Depends on: 1161134
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.
Attachment #8601221 - Flags: review?(nalexander) → review+
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 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.)
Attachment #8601223 - Flags: review?(nalexander) → review+
> 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.
(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.
Attached patch bug-1161195-part1.diff (obsolete) — Splinter Review
Update to part1 to take into account suggestions from :nalexander (put the variables in moz.build and update the comments)

Carrying over review flags.
Attachment #8601221 - Attachment is obsolete: true
Attachment #8601221 - Flags: review?(rnewman)
Attachment #8601650 - Flags: review?(rnewman)
Attached patch bug-1161195-part3.diff (obsolete) — Splinter Review
Update to part 3 so that it applies cleanly with comment changes from part 1 (and carrying over review flags)
Attachment #8601223 - Attachment is obsolete: true
Attachment #8601223 - Flags: review?(rnewman)
Attachment #8601652 - Flags: review?(rnewman)
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.)
Attachment #8601176 - Flags: review?(nalexander) → review-
Attachment #8601176 - Flags: review?(rnewman)
Attachment #8601166 - Flags: review?(rnewman)
Attachment #8601166 - Flags: review?(nalexander)
Attachment #8601650 - Flags: review?(rnewman)
Attachment #8601652 - Flags: review?(rnewman)
Removed the review flags - I will rework this as separate bugs as suggested above.
Flags: needinfo?(nathan)
Flags: needinfo?(nathan)
Summary: Add ability to "brand" some of the java classes → Don't hard-code class names in AppConstants.java.in and AndroidManifest.xml
Attached patch bug-1161195-fix-part1.diff (obsolete) — Splinter Review
This is just the portion of the patch that puts the class names in confvars.sh
Attachment #8601166 - Attachment is obsolete: true
Attachment #8601176 - Attachment is obsolete: true
Attachment #8601650 - Attachment is obsolete: true
Attachment #8601652 - Attachment is obsolete: true
Attachment #8601852 - Flags: review?(nalexander)
Attached patch bug-1161195-fix-part2.diff (obsolete) — Splinter Review
This is the second part of the patch - which handles the variable renaming.
Attachment #8601853 - Flags: review?(nalexander)
The two latest patches are just reworking of the comments and commit messages of what was originally part 1 and part 3.
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/
Attachment #8601852 - Flags: review?(nalexander) → review+
Comment on attachment 8601853 [details] [diff] [review]
bug-1161195-fix-part2.diff

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

rubberstamp.
Attachment #8601853 - Flags: review?(nalexander) → review+
Fix spacing and spelling error from last review.  Copy over review flag.
Attachment #8601852 - Attachment is obsolete: true
Attachment #8601876 - Flags: review+
Keywords: checkin-needed
Update patch to apply cleanly after whitespace fixes in part 1.  Carry over review flags
Attachment #8601853 - Attachment is obsolete: true
Attachment #8601877 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/df64a65ca1bd
https://hg.mozilla.org/mozilla-central/rev/b36b6e987110
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
Product: Firefox for Android → Firefox Build System
Target Milestone: Firefox 40 → mozilla40
You need to log in before you can comment on or make changes to this bug.