856163 - Reduce the number of preprocessed files

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Brian Nicholson (:bnicholson)]

For the sake of code purity and better compatibility with other builders such as Eclipse, we should try to avoid unnecessary preprocessing. Rather than having entire files be preprocessed because they have sections of #ifdefs or preprocessor variables, we can take a cue from Sync and move these all into a constants file. There also appear to be a number of classes that are needlessly in the @ANDROID_PACKAGE_NAME@ package; I propose we move as many of these to org.mozilla.gecko as possible. For historical reasons, we probably shouldn't change the package of App.java.in since it's the main entrypoint to Fennec and could break any intents (such as shortcuts) that launch it, but I think all other activities can be changed.

Comment 1

[Brian Nicholson (:bnicholson)]

Attachment: Part 1: Remove unused classes

Cleanup patch that removes unused code.

Comment 2

[Brian Nicholson (:bnicholson)]

Attachment: Part 2: Move preprocessed code to FennecConstants

This is the main patch that moves most of the constants to FennecConstants. I left out a few more specialized changes that the patches after this will address.

In http://developer.android.com/guide/topics/manifest/activity-element.html, the spec recommends not changing the <activity> name after the application has been published. This patch ignores that suggestion for CrashReporter and Restarter with the assumption that no third-party code -- from Mozilla or elsewhere -- has created any shortcuts/intents/other dependencies on these Activities. Let me know if you think this is a bad idea.

I've mostly left code in test/ untouched as changing the package names of some of those classes would break our tests, and also because it will likely require its own constants file since it doesn't have access to the Fennec library. If/once this lands, we can continue investigation for removing preprocessing from the test framework code.

Richard, you're the lucky reviewer since you've already done this with Sync :)

Comment 3

[Brian Nicholson (:bnicholson)]

Attachment: Part 3: Remove GeckoAppInfo

GeckoAppInfo seems mostly redundant after adding FennecConstants, so this patch removes it. If we do need to keep it around for any reason, we could still change it to use FennecConstants so it doesn't need to be preprocessed.

Comment 4

[Brian Nicholson (:bnicholson)]

Attachment: Part 4: Use gecko package for database classes

I don't think the database classes need to be in the @ANDROID_PACKAGE_NAME@ package do they? On the other hand, I think the exposed authorities must still be in the special package to prevent provider collisions with multi-channel installations.

Comment 5

[Brian Nicholson (:bnicholson)]

Attachment: Part 5: Reduce preprocessing in WebApp

Changes the package of and removes some preprocessing from webapps. This will break any existing webapp shortcuts, but Wes pointed out that webapps haven't shipped past Aurora.

Comment 6

[Brian Nicholson (:bnicholson)]

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

Comment 7

[Richard Newman [:rnewman]]

(In reply to Brian Nicholson (:bnicholson) from comment #4)
> Created attachment 731309 [details] [diff] [review]
> Part 4: Use gecko package for database classes
> 
> I don't think the database classes need to be in the @ANDROID_PACKAGE_NAME@
> package do they? On the other hand, I think the exposed authorities must
> still be in the special package to prevent provider collisions with
> multi-channel installations.

I think it's worth keeping the implementing classes in the same namespace as the provider URIs; seems like an easy debugging win, no?

Note that what you're doing here is going to collide mightily with Nick's work in Bug 854530. You probably want to wait for him to finish doing that, because it'll make your patches shorter!

Comment 8

[Brian Nicholson (:bnicholson)]

Oops -- rebase fail. I'll upload new patches with a new try push shortly.

Comment 9

[Brian Nicholson (:bnicholson)]

(In reply to Richard Newman [:rnewman] from comment #7)
> (In reply to Brian Nicholson (:bnicholson) from comment #4)
> > Created attachment 731309 [details] [diff] [review]
> > Part 4: Use gecko package for database classes
> > 
> > I don't think the database classes need to be in the @ANDROID_PACKAGE_NAME@
> > package do they? On the other hand, I think the exposed authorities must
> > still be in the special package to prevent provider collisions with
> > multi-channel installations.
> 
> I think it's worth keeping the implementing classes in the same namespace as
> the provider URIs; seems like an easy debugging win, no?

Can you elaborate on what you mean by a debugging win?

> Note that what you're doing here is going to collide mightily with Nick's
> work in Bug 854530. You probably want to wait for him to finish doing that,
> because it'll make your patches shorter!

I was under the impression that this would only collide with Makefile.in, which doesn't seem too bad. I'll have to rebase each patch since they all touch Makefile.in, but I didn't think it would be too terrible to replay those changes onto moz.build.

Comment 10

[Richard Newman [:rnewman]]

(In reply to Brian Nicholson (:bnicholson) from comment #9)

> > I think it's worth keeping the implementing classes in the same namespace as
> > the provider URIs; seems like an easy debugging win, no?
> 
> Can you elaborate on what you mean by a debugging win?

I mean when we get user-reported (or automatically submitted) stacks like this:

E/GeckoAppShell( 7258): >>> REPORTING UNCAUGHT EXCEPTION FROM THREAD 683 ("GeckoBackgroundThread")
E/GeckoAppShell( 7258): android.database.sqlite.SQLiteDatabaseLockedException: database is locked
E/GeckoAppShell( 7258): 	at android.database.sqlite.SQLiteStatement.native_executeSql(Native Method)
E/GeckoAppShell( 7258): 	at android.database.sqlite.SQLiteStatement.executeUpdateDelete(SQLiteStatement.java:100)
E/GeckoAppShell( 7258): 	at android.database.sqlite.SQLiteDatabase.executeSql(SQLiteDatabase.java:1899)
E/GeckoAppShell( 7258): 	at android.database.sqlite.SQLiteDatabase.execSQL(SQLiteDatabase.java:1839)
E/GeckoAppShell( 7258): 	at android.database.sqlite.SQLiteDatabase.endTransaction(SQLiteDatabase.java:714)
E/GeckoAppShell( 7258): 	at org.mozilla.firefox.db.BrowserProvider.update(BrowserProvider.java:1586)
E/GeckoAppShell( 7258): 	at android.content.ContentProvider$Transport.update(ContentProvider.java:219)
E/GeckoAppShell( 7258): 	at android.content.ContentResolver.update(ContentResolver.java:856)
E/GeckoAppShell( 7258): 	at org.mozilla.gecko.db.LocalBrowserDB.updateHistoryTitle(LocalBrowserDB.java:222)
E/GeckoAppShell( 7258): 	at org.mozilla.gecko.db.BrowserDB.updateHistoryTitle(BrowserDB.java:109)
E/GeckoAppShell( 7258): 	at org.mozilla.gecko.GlobalHistory.update(GlobalHistory.java:131)
E/GeckoAppShell( 7258): 	at org.mozilla.gecko.Tab$4.run(Tab.java:303)
E/GeckoAppShell( 7258): 	at android.os.Handler.handleCallback(Handler.java:605)
E/GeckoAppShell( 7258): 	at android.os.Handler.dispatchMessage(Handler.java:92)
E/GeckoAppShell( 7258): 	at android.os.Looper.loop(Looper.java:137)
E/GeckoAppShell( 7258): 	at org.mozilla.gecko.util.GeckoBackgroundThread.run(GeckoBackgroundThread.java:31)


it's nice to see a specific class package name. This will be particularly relevant should multi-profile support land, and/or users are syncing more than one Firefox at a time.

Comment 11

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

Comment on attachment 731301 [details] [diff] [review]
Part 2: Move preprocessed code to FennecConstants

Please, don't use "Fennec" in any code or file names. Maybe use "Gecko", but I am getting tired of that too.

I think "BuildConstants" works best and is closer to the real context.

Comment 12

[Richard Newman [:rnewman]]

Comment on attachment 731301 [details] [diff] [review]
Part 2: Move preprocessed code to FennecConstants

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

A few things to change, and a few questions to answer.

Thanks for making the world a better place! :D

::: mobile/android/base/App.java.in
@@ +42,5 @@
>              deviceType = "Tablet";
> +        return "Mozilla/5.0 (Android; " + deviceType + "; rv:" +
> +                FennecConstants.MOZ_APP_VERSION + ") Gecko/" +
> +                FennecConstants.MOZ_APP_VERSION + " Firefox/" +
> +                FennecConstants.MOZ_APP_VERSION;

Let's go whole-hog and define FENNEC_USER_AGENT in FennecConstants.

You'll see that we do exactly that for Sync:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/sync/SyncConstants.java.in#21

and for Product Announcements:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/background/announcements/AnnouncementsConstants.java.in#48

@@ +52,1 @@
>          // client-side redirect from t.co. This bot-like UA gives us a 

Kill trailing whitespace while you're here. :D

::: mobile/android/base/CrashReporter.java.in
@@ +3,4 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +package org.mozilla.gecko;

Could you get snorp to OK this change? I'm not intimately familiar with how the CrashReporter works.

::: mobile/android/base/FennecConstants.java.in
@@ +34,5 @@
> +#ifdef MOZ_ANDROID_ANR_REPORTER
> +        MOZ_ANDROID_ANR_REPORTER = true;
> +#else
> +        MOZ_ANDROID_ANR_REPORTER = false;
> +#endif

You could just do

  public static final MOZ_ANDROID_ANR_REPORTER =
#ifdef MOZ_ANDROID_ANR_REPORTER
    true;
#else
    false;
#endif

I betcha could also extend the text preprocessor to support attemptSubstitution for bools…

::: mobile/android/base/GeckoActivity.java.in
@@ +39,5 @@
>      @Override
>      public void onCreate(android.os.Bundle savedInstanceState) {
>          super.onCreate(savedInstanceState);
> +        if (FennecConstants.MOZ_ANDROID_ANR_REPORTER) {
> +            ANRReporter.register(getApplicationContext());

ANRReporter isn't even built if this flag isn't set:

  ifdef MOZ_ANDROID_ANR_REPORTER
  DEFINES += -DMOZ_ANDROID_ANR_REPORTER=1
  FENNEC_JAVA_FILES += ANRReporter.java
  endif

so I would be surprised if this built and/or dexed without warnings…

::: mobile/android/base/GeckoApp.java
@@ +2159,4 @@
>          Log.d(LOGTAG, "doRestart(\"" + action + "\")");
>          try {
>              Intent intent = new Intent(action);
> +            intent.setClassName(getPackageName(), "org.mozilla.gecko.Restarter");

Lift that magic constant to somewhere other than two thousand lines into the file :D

::: mobile/android/base/Makefile.in
@@ -217,1 @@
>  endif

If you're emptying the clause, kill the whole thing. But I note: by no longer conditionalizing here, you're either regressing our package size, or putting expectations on ProGuard. Please verify the APK size and installed size before and after these changes…

@@ +296,4 @@
>    -DMOZ_APP_BUILDID=$(MOZ_APP_BUILDID) \
>    -DMOZ_APP_ABI=$(TARGET_XPCOM_ABI) \
>    -DMOZ_BUILD_TIMESTAMP=$(MOZ_BUILD_TIMESTAMP) \
> +  -DMOZ_PKG_SPECIAL=$(MOZ_PKG_SPECIAL) \

Hang on, is this change really equivalent? Isn't MOZ_PKG_SPECIAL now always defined?

@@ +511,1 @@
>  	$(SYNC_RES_XML) \

Nit: indenting.

::: mobile/android/base/Restarter.java.in
@@ +14,5 @@
>      private static final String LOGTAG = "GeckoRestarter";
>  
>      @Override
>      public void onCreate(Bundle savedInstanceState) {
> +        Log.i(LOGTAG, "trying to restart " + FennecConstants.MOZ_APP_NAME);

While you're breaking blame, capitalize "Trying", wouldya?

@@ +41,5 @@
>          }
>          try {
>              Intent intent = new Intent(Intent.ACTION_MAIN);
> +            intent.setClassName(FennecConstants.ANDROID_PACKAGE_NAME,
> +                                FennecConstants.ANDROID_PACKAGE_NAME + ".App");

Just put this in FennecConstants, a la

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/background/common/GlobalConstants.java.in#30

::: mobile/android/base/UpdateServiceHelper.java.in
@@ +48,4 @@
>      // Name of the Intent extra that holds the APK path, used with ACTION_APPLY_UPDATE
>      public static final String EXTRA_PACKAGE_PATH_NAME = "packagePath";
>  
> +    public static final String UPDATE_CHANNEL = FennecConstants.MOZ_UPDATE_CHANNEL;

Just inline this.

@@ +70,1 @@
>      

Trailing whitespace.

@@ +112,5 @@
> +        if (FennecConstants.MOZ_UPDATER) {
> +            return true;
> +        } else {
> +            return false;
> +        }

return FennecConstants.MOZ_UPDATER;

::: mobile/android/base/db/BrowserContract.java.in
@@ +21,2 @@
>      public static final Uri FORM_HISTORY_AUTHORITY_URI = Uri.parse("content://" + FORM_HISTORY_AUTHORITY);
>      

Whitespace plz.

::: mobile/android/base/resources/xml/preferences.xml.in
@@ +90,4 @@
>                              android:defaultValue="true"
>                              android:persistent="false" />
>  
> +        <CheckBoxPreference android:key="toolkit.telemetry.enabled"

This is going away anyway (FHR).

Comment 13

[Richard Newman [:rnewman]]

(In reply to Mark Finkle (:mfinkle) from comment #11)

> I think "BuildConstants" works best and is closer to the real context.

WFM. Take a look at the other *Constants.java.in files in the mobile tree…

Comment 14

[Brian Nicholson (:bnicholson)]

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

Comment 15

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

(In reply to Richard Newman [:rnewman] from comment #13)
> (In reply to Mark Finkle (:mfinkle) from comment #11)
> 
> > I think "BuildConstants" works best and is closer to the real context.
> 
> WFM. Take a look at the other *Constants.java.in files in the mobile tree…

Given the convention of "SyncConstants", "AnnouncementsConstants" and "GlobalConstants" I would also accept "AppConstants"

Comment 16

[James Willcox (:snorp) ([e-mail]) (he/him)]

> ::: mobile/android/base/CrashReporter.java.in
> @@ +3,4 @@
> >   * License, v. 2.0. If a copy of the MPL was not distributed with this
> >   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> >  
> > +package org.mozilla.gecko;
> 
> Could you get snorp to OK this change? I'm not intimately familiar with how
> the CrashReporter works.
> 

I don't know anything about the CrashReporter either :)

Comment 17

[Richard Newman [:rnewman]]

Well, drat! bnicholson, you might need to hg blame to figure out who the right sucker is…

Comment 18

[Nick Alexander :nalexander [he/him]]

Ditto what rnewman said: thanks for making the world a better place.  And thanks for helping with the "great mobile/android moz.build push of Q2"!

We want to land this at the beginning of the next cycle, and it will definitely need to co-operate with Bug 854530, but I don't think either has priority.  So let's collect reviews and try to co-ordinate to lessen our rebase pain.

Two notes:

* I don't know how the updater works, but I'd hate to leave behind users by changing intent names.  (This might happen if we registered a Pending Intent and then switched class names.)
* what rnewman means by "debugging win" is that it is nice if tracebacks look like "org.mozilla.firefox_beta" instead of all looking like "org.mozilla.gecko". I hadn't thought of that.  In Sync we send all logging to the tag "FxSync" and append ":: org.mozilla.firefox_beta" to differentiate packages.  This makes it easy to debug multiple concurrent Fennecs accessing the same account.

Now I wonder if we should move to @ANDROID_PACKAGE_NAME@ through-out.  I see that Java requires package declarations, so we can't just ignore it :(

Comment 19

[Richard Newman [:rnewman]]

Comment on attachment 731301 [details] [diff] [review]
Part 2: Move preprocessed code to FennecConstants

Looks like kats is the victim…

Comment 20

[Brian Nicholson (:bnicholson)]

Attachment: Part 2: Move preprocessed code to AppConstants

Thanks for the review! I've renamed FennecConstants to AppConstants as suggested by Mark, addressed Richard's suggestions, and made some other minor changes along the way. One change, for instance, was moving all overridden methods from App.java.in to BrowserApp.java (which is probably where the code should have been in the first place) so that App is nothing more than a namespace wrapper for BrowserApp.

(In reply to Richard Newman [:rnewman] from comment #12)
> Comment on attachment 731301 [details] [diff] [review]
> Part 2: Move preprocessed code to FennecConstants
> 
> Review of attachment 731301 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> A few things to change, and a few questions to answer.
> 
> Thanks for making the world a better place! :D
> 
> ::: mobile/android/base/App.java.in
> @@ +42,5 @@
> >              deviceType = "Tablet";
> > +        return "Mozilla/5.0 (Android; " + deviceType + "; rv:" +
> > +                FennecConstants.MOZ_APP_VERSION + ") Gecko/" +
> > +                FennecConstants.MOZ_APP_VERSION + " Firefox/" +
> > +                FennecConstants.MOZ_APP_VERSION;
> 
> Let's go whole-hog and define FENNEC_USER_AGENT in FennecConstants.

In Fennec, since we need to use GeckoApp#isTablet() to build the UA, I felt it it was more fitting to have this method in GeckoApp (and we also have getUAStringForHost in that file). I moved it from App to GeckoApp since the same code was duplicated in App and WebApp. But I can still move it to AppConstants -- either as a method or as a String with a static initializer -- if you have a strong opinion about it.

> ANRReporter isn't even built if this flag isn't set:
> 
>   ifdef MOZ_ANDROID_ANR_REPORTER
>   DEFINES += -DMOZ_ANDROID_ANR_REPORTER=1
>   FENNEC_JAVA_FILES += ANRReporter.java
>   endif
> 
> so I would be surprised if this built and/or dexed without warnings…

Oops, I had moved that definition outside of the #ifdef in the following patch. I moved that change to this patch.

> ::: mobile/android/base/Makefile.in
> @@ -217,1 @@
> >  endif
> 
> If you're emptying the clause, kill the whole thing. But I note: by no
> longer conditionalizing here, you're either regressing our package size, or
> putting expectations on ProGuard. Please verify the APK size and installed
> size before and after these changes…

The APK is ~13kB larger with these changes. I figured a small APK increase in the APK size would just be a price we'd have to pay for purer code. I don't know, however, what the acceptable threshold is for APK inflation that we would still be consider to be outweighed by the benefits. It's uglier, but one alternative would be to move bits like [1] and [2] into methods in AppConstants so that the calling code would be agnostic to these definitions. What do you guys think?

[1] http://hg.mozilla.org/mozilla-central/file/0b7c27024048/mobile/android/base/GeckoActivity.java.in#l45
[2] http://hg.mozilla.org/mozilla-central/file/0b7c27024048/mobile/android/base/SmsManager.java.in#l19

> @@ +296,4 @@
> >    -DMOZ_APP_BUILDID=$(MOZ_APP_BUILDID) \
> >    -DMOZ_APP_ABI=$(TARGET_XPCOM_ABI) \
> >    -DMOZ_BUILD_TIMESTAMP=$(MOZ_BUILD_TIMESTAMP) \
> > +  -DMOZ_PKG_SPECIAL=$(MOZ_PKG_SPECIAL) \
> 
> Hang on, is this change really equivalent? Isn't MOZ_PKG_SPECIAL now always
> defined?

Thanks - fixed.

Comment 21

[Brian Nicholson (:bnicholson)]

Attachment: Part 3: Remove GeckoAppInfo, v2

Rebased

Comment 22

[Brian Nicholson (:bnicholson)]

Attachment: Part 4: Use gecko package for database classes

Regarding the debugging issue, one workaround would be to create a wrapper class in the generated namespace that extends the org.mozilla.gecko class, which is what we do with App/BrowserApp. A brief snippet from IRC about doing this: http://www.pastebin.mozilla.org/2264166

Comment 23

[Brian Nicholson (:bnicholson)]

Attachment: Part 5: Reduce preprocessing in WebApp, v2

Rebased

Comment 24

[Brian Nicholson (:bnicholson)]

(In reply to Nick Alexander :nalexander from comment #18)
> * I don't know how the updater works, but I'd hate to leave behind users by
> changing intent names.  (This might happen if we registered a Pending Intent
> and then switched class names.)

I'm trying to envision how this might happen. The updater package name will change only once they've installed a new version of Fennec, and after that, any future pending intents should refer to the new package name, right?

> * what rnewman means by "debugging win" is that it is nice if tracebacks
> look like "org.mozilla.firefox_beta" instead of all looking like
> "org.mozilla.gecko". I hadn't thought of that.  In Sync we send all logging
> to the tag "FxSync" and append ":: org.mozilla.firefox_beta" to
> differentiate packages.  This makes it easy to debug multiple concurrent
> Fennecs accessing the same account.
> 
> Now I wonder if we should move to @ANDROID_PACKAGE_NAME@ through-out.  I see
> that Java requires package declarations, so we can't just ignore it :(

So use the preprocessed name everywhere? I hope we don't need to do that -- that feels like going backwards! That would also break Eclipse editing support for every file unless we hacked in some workarounds, such as the preprocessing translation on save script...

Comment 25

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 731778 [details] [diff] [review]
Part 3: Remove GeckoAppInfo, v2

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

Looks good! Thanks!

::: mobile/android/base/ANRReporter.java
@@ +592,5 @@
> +        // Having a different locale than system locale is not
> +        // supported right now; assume we are using the system locale
> +        return Locale.getDefault().toString().replace('_', '-');
> +    }
> +

Can you move getLocale() to right below getTotalMem()?

Comment 26

[Richard Newman [:rnewman]]

(In reply to Brian Nicholson (:bnicholson) from comment #24)

> I'm trying to envision how this might happen. The updater package name will
> change only once they've installed a new version of Fennec, and after that,
> any future pending intents should refer to the new package name, right?

That's the theory. I would experimentally verify!

> > Now I wonder if we should move to @ANDROID_PACKAGE_NAME@ through-out.  I see
> > that Java requires package declarations, so we can't just ignore it :(
> 
> So use the preprocessed name everywhere?

Naw, it'd just show up one level deeper in the stack.

> That would also break Eclipse editing
> support for every file unless we hacked in some workarounds, such as the
> preprocessing translation on save script...

Yeah, it's hard to mix "files with interesting logic" and "files that are preprocessed" :D

Comment 27

[Kartikaya Gupta (email:[e-mail])]

(In reply to Brian Nicholson (:bnicholson) from comment #20)
> In Fennec, since we need to use GeckoApp#isTablet() to build the UA, I felt
> it it was more fitting to have this method in GeckoApp (and we also have
> getUAStringForHost in that file). I moved it from App to GeckoApp since the
> same code was duplicated in App and WebApp. But I can still move it to
> AppConstants -- either as a method or as a String with a static initializer
> -- if you have a strong opinion about it.

I haven't looked at the patches yet but this may affect your decision: GeckoApp.isTablet() doesn't belong in GeckoApp. It should be moved out (along with isSmallTablet() and isLargeTablet() and isTouchDevice()) into some util class.

> > Hang on, is this change really equivalent? Isn't MOZ_PKG_SPECIAL now always
> > defined?

Is it? It should only be defined for ARMv6 builds. We have a bunch of stuff (e.g. ifdefs in mobile/android/app/mobile.js) that depends on that being true, so if that's not the case then something somewhere is broken.

Comment 28

[Richard Newman [:rnewman]]

(In reply to Brian Nicholson (:bnicholson) from comment #20)

> > Let's go whole-hog and define FENNEC_USER_AGENT in FennecConstants.
> 
> In Fennec, since we need to use GeckoApp#isTablet() to build the UA, I felt
> it it was more fitting to have this method in GeckoApp (and we also have
> getUAStringForHost in that file). I moved it from App to GeckoApp since the
> same code was duplicated in App and WebApp. But I can still move it to
> AppConstants -- either as a method or as a String with a static initializer
> -- if you have a strong opinion about it.

I'd go for

  if (isTablet()) { 
    return FENNEC_TABLET_USER_AGENT;
  } else {
    return FENNEC_USER_AGENT;
  }

UA feels like the kind of thing that lives as a preprocessed constant.


> The APK is ~13kB larger with these changes. I figured a small APK increase
> in the APK size would just be a price we'd have to pay for purer code. I
> don't know, however, what the acceptable threshold is for APK inflation that
> we would still be consider to be outweighed by the benefits. It's uglier,
> but one alternative would be to move bits like [1] and [2] into methods in
> AppConstants so that the calling code would be agnostic to these
> definitions. What do you guys think?

If we're starting down that road, we might as well be intellectually honest and define a PreprocessorLayer.java.in that conditionally accesses methods on classes that might be preprocessed away. AppConstants probably isn't the place for that.

The win of doing this will perhaps mostly eat the gain of not including every file, so might need more work here. Perhaps a follow-up (APKShrink!).

I no longer see any reference to ProGuard in the tree. Am I missing something?

Another way to do this is to do what Android does with the build version numbers. Define constants like

  MOZ_ANR_REPORTER_PRESENT = true;

(which you do), then always compile code with the ANR reporter, but don't always ship it. That seems like it would be equivalent to a class built against a later Android SDK but running on an old device, no?

That should win back our APK size.

Comment 29

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 731777 [details] [diff] [review]
Part 2: Move preprocessed code to AppConstants

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

Minusing for what I think is breakage in GeckoApp.java, but otherwise this looks pretty good. Nice work!

::: mobile/android/base/AndroidManifest.xml.in
@@ -71,4 @@
>  		 android:debuggable="true">
>  #endif
>  
> -        <activity android:name="App"

How did this work before? Ditto for Restarter and CrashReporter. Shouldn't their component names previously have been the un-namespaced versions? Why can I start fenenc using "adb shell am start org.mozilla.fennec/.App" then? So many questions!

::: mobile/android/base/GeckoApp.java
@@ +2152,5 @@
>          return Boolean.TRUE;
>      } 
>  
> +    public String getPackageName() {
> +        return "@ANDROID_PACKAGE_NAME@";

Why exactly are we overriding getPackageName()? This seems unnecessary. But if it is, use AppConstants.ANDROID_PACKAGE_NAME, because GeckoApp.java isn't preprocessed and this is probably broken. Also put back the @Override

@@ +2156,5 @@
> +        return "@ANDROID_PACKAGE_NAME@";
> +    }
> +
> +    public String getContentProcessName() {
> +        return "@MOZ_CHILD_PROCESS_NAME@";

Ditto, GeckoApp.java is not preprocessed, so you can't do this. Move it to AppConstants.

@@ +2180,5 @@
>      public void doRestart(String action) {
>          Log.d(LOGTAG, "doRestart(\"" + action + "\")");
>          try {
>              Intent intent = new Intent(action);
> +            intent.setClassName(getPackageName(), RESTARTER_CLASS);

Preferably use something other than getPackageName() here.. like AppConstants.ANDROID_PACKAGE_NAME.

::: mobile/android/base/GeckoAppShell.java
@@ +619,4 @@
>                      shortcutIntent.setAction(GeckoApp.ACTION_BOOKMARK);
>                      shortcutIntent.setData(Uri.parse(aURI));
>                      shortcutIntent.setClassName(GeckoApp.mAppContext,
> +                                                AppConstants.BROWSER_INTENT_CLASS);

While you're here, replace the first argument with AppConstants.ANDROID_PACKAGE_NAME so we reduce unnecessary usage of GeckoApp.mAppContext.

@@ +658,4 @@
>                      shortcutIntent = new Intent();
>                      shortcutIntent.setAction(GeckoApp.ACTION_BOOKMARK);
>                      shortcutIntent.setClassName(GeckoApp.mAppContext,
> +                                                AppConstants.BROWSER_INTENT_CLASS);

Ditto.

::: mobile/android/base/GeckoPreferences.java
@@ +59,5 @@
> +    public static String PREFS_CATEGORY_PRIVACY = "category_privacy";
> +    public static String PREFS_MENU_CHAR_ENCODING = "browser.menu.showCharacterEncoding";
> +    public static String PREFS_MP_ENABLED         = "privacy.masterpassword.enabled";
> +    public static String PREFS_TELEMETRY_ENABLED = "toolkit.telemetry.enabled";
> +    public static String PREFS_TELEMETRY_ENABLED_PRERELEASE = "toolkit.telemetry.enabledPreRelease";

Either line up all of the equals or (my preference) don't line up any of them. Just leave one space on either side of each equals sign.

Comment 30

[Richard Newman [:rnewman]]

Comment on attachment 731777 [details] [diff] [review]
Part 2: Move preprocessed code to AppConstants

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

Not much to add over mine and kats' earlier comments.

::: mobile/android/base/CrashReporter.java.in
@@ +278,1 @@
>                  sb.append("nothumb Build\n");

Can we stick some braces here while you're breaking blame?

::: mobile/android/base/GeckoApp.java
@@ +1821,5 @@
>      }
>  
> +    public String getDefaultUAString() {
> +        String deviceType = isTablet() ? "Tablet" : "Mobile";
> +        return "Mozilla/5.0 (Android; " + deviceType + "; rv:" +

To second earlier comment: yeah, I'd definitely prefer as much of this work as possible to be done once, and this method simply to dispatch between two constants.

And is there a reason why this method isn't static?

Comment 31

[Brian Nicholson (:bnicholson)]

Attachment: Part 2: Move preprocessed code to AppConstants, v2

(In reply to Richard Newman [:rnewman] from comment #28)
> (In reply to Brian Nicholson (:bnicholson) from comment #20)
> 
> If we're starting down that road, we might as well be intellectually honest
> and define a PreprocessorLayer.java.in that conditionally accesses methods
> on classes that might be preprocessed away. AppConstants probably isn't the
> place for that.
> 
> The win of doing this will perhaps mostly eat the gain of not including
> every file, so might need more work here. Perhaps a follow-up (APKShrink!).

Filed bug 856791.

> I no longer see any reference to ProGuard in the tree. Am I missing
> something?

According to Chris, we've never actually had ProGuard support (see bug 709230).

> Another way to do this is to do what Android does with the build version
> numbers. Define constants like
> 
>   MOZ_ANR_REPORTER_PRESENT = true;
> 
> (which you do), then always compile code with the ANR reporter, but don't
> always ship it. That seems like it would be equivalent to a class built
> against a later Android SDK but running on an old device, no?
> 
> That should win back our APK size.

Sounds nicer than having an additional PreprocessorLayer; we should look at this in bug 856791, or maybe even in bug 854530.


(In reply to Kartikaya Gupta (email:[e-mail]) from comment #29)
> Comment on attachment 731777 [details] [diff] [review]
> Part 2: Move preprocessed code to AppConstants
> 
> Review of attachment 731777 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/AndroidManifest.xml.in
> @@ -71,4 @@
> >  		 android:debuggable="true">
> >  #endif
> >  
> > -        <activity android:name="App"
> 
> How did this work before? Ditto for Restarter and CrashReporter. Shouldn't
> their component names previously have been the un-namespaced versions? Why
> can I start fenenc using "adb shell am start org.mozilla.fennec/.App" then?
> So many questions!

They actually have the same result (see http://androidxref.com/4.2.2_r1/xref/frameworks/base/tools/aapt/Resource.cpp#719). But I figured we should use the dot notation as described in the android:name section at http://developer.android.com/guide/topics/manifest/activity-element.html since the previous format's behavior is undefined at the API level.

> ::: mobile/android/base/GeckoApp.java
> @@ +2152,5 @@
> >          return Boolean.TRUE;
> >      } 
> >  
> > +    public String getPackageName() {
> > +        return "@ANDROID_PACKAGE_NAME@";
> 
> Why exactly are we overriding getPackageName()? This seems unnecessary.

Good question. This was done in bug 620584 as hack, and I'm not sure it's still necessary. I didn't even realize it was already defined in Context until you pointed it out -- hence the missing @Overrides.

> @@ +2156,5 @@
> > +        return "@ANDROID_PACKAGE_NAME@";
> > +    }
> > +
> > +    public String getContentProcessName() {
> > +        return "@MOZ_CHILD_PROCESS_NAME@";
> 
> Ditto, GeckoApp.java is not preprocessed, so you can't do this. Move it to
> AppConstants.

Oops, more rebase fail. I had the AppConstants change in the final patch here. Moved it to this one.


(In reply to Richard Newman [:rnewman] from comment #30)
> Comment on attachment 731777 [details] [diff] [review]
> Part 2: Move preprocessed code to AppConstants
> 
> Review of attachment 731777 [details] [diff] [review]:
> -----------------------------------------------------------------
> ::: mobile/android/base/GeckoApp.java
> @@ +1821,5 @@
> >      }
> >  
> > +    public String getDefaultUAString() {
> > +        String deviceType = isTablet() ? "Tablet" : "Mobile";
> > +        return "Mozilla/5.0 (Android; " + deviceType + "; rv:" +
> 
> To second earlier comment: yeah, I'd definitely prefer as much of this work
> as possible to be done once, and this method simply to dispatch between two
> constants.

Done.

> And is there a reason why this method isn't static?

It calls isTablet(), which in turn calls isLargeTablet() and isSmallTablet() -- none of which are static. A workaround would be to call GeckoAppShell.isTablet(), but that seems kind of dumb since that method is just a static wrapper for the one in GeckoApp. I think we can clean all of this up in bug 856756.


CC'ing Brad to make sure the getPackageName() override is safe to remove.

Comment 32

[Brian Nicholson (:bnicholson)]

Attachment: Part 3: Remove GeckoAppInfo, v3. r=jchen

Rebased and addressed Jim's comment.

Comment 33

[Brian Nicholson (:bnicholson)]

Attachment: Part 5: Reduce preprocessing in WebApp, v3

Rebased.

Comment 34

[Brian Nicholson (:bnicholson)]

Comment on attachment 732052 [details] [diff] [review]
Part 5: Reduce preprocessing in WebApp, v3

Sending review to Mark since he didn't sound happy with these changes.

Comment 35

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 732047 [details] [diff] [review]
Part 2: Move preprocessed code to AppConstants, v2

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

LGTM

Comment 36

[Richard Newman [:rnewman]]

Comment on attachment 732047 [details] [diff] [review]
Part 2: Move preprocessed code to AppConstants, v2

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

Looks good to me. Extra points if you kill the trailing whitespace that shows up nearby :D

::: mobile/android/base/GeckoActivity.java.in
@@ +71,4 @@
>          ComponentName component = intent.getComponent();
>          return (component != null
>                  && component.getPackageName() != null
> +                && component.getPackageName().equals(AppConstants.ANDROID_PACKAGE_NAME));

You can replace these two lines with

  && AppConstants.ANDROID_PACKAGE_NAME.equals(component.getPackageName());

::: mobile/android/base/GeckoApp.java
@@ +1830,5 @@
> +        // client-side redirect from t.co. This bot-like UA gives us a
> +        // 301 response code
> +        if ("t.co".equals(host))
> +            return "Redirector/" + AppConstants.MOZ_APP_VERSION +
> +                    " (Android; rv:" + AppConstants.MOZ_APP_VERSION + ")";

Braces, and can we have a BOT_LIKE_UA string constant?

::: mobile/android/base/GeckoAppShell.java
@@ +580,5 @@
>          Intent intent = new Intent();
>          intent.setAction(GeckoApp.ACTION_WEBAPP_PREFIX + aIndex);
>          intent.setData(Uri.parse(aURI));
> +        intent.setClassName(AppConstants.ANDROID_PACKAGE_NAME,
> +                AppConstants.ANDROID_PACKAGE_NAME + ".WebApps$WebApp" + aIndex);

Align params vertically.

::: mobile/android/base/UpdateServiceHelper.java.in
@@ +62,5 @@
> +        }
> +        UPDATE_URL = "https://aus2.mozilla.org/update/4/" + AppConstants.MOZ_APP_BASENAME + "/" +
> +            AppConstants.MOZ_APP_VERSION + "/%BUILDID%/Android_ " + AppConstants.MOZ_APP_ABI +
> +            pkgSpecial + "/%LOCALE%/" + AppConstants.MOZ_UPDATE_CHANNEL +
> +            "/%OS_VERSION%/default/default/" + AppConstants.MOZ_APP_VERSION + "/update.xml";

Spaces, not tabs, please. (And check throughout!)

Can we align and pair this up a little bit? Perhaps

 UPDATE_URL = "https://aus2.mozilla.org/update/4/" + AppConstants.MOZ_APP_BASENAME + "/" +
              AppConstants.MOZ_APP_VERSION     +
              "/%BUILDID%/Android_ "           + AppConstants.MOZ_APP_ABI + pkgSpecial +
              "/%LOCALE%/"                     + AppConstants.MOZ_UPDATE_CHANNEL +
              "/%OS_VERSION%/default/default/" + AppConstants.MOZ_APP_VERSION +
              "/update.xml";

Comment 37

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 732047 [details] [diff] [review]
Part 2: Move preprocessed code to AppConstants, v2

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

::: mobile/android/base/WebApp.java.in
@@ -145,4 @@
>      }
>  
>      @Override
> -    public String getPackageName() {

yup, no problem removing the overrides

Comment 38

[Lucas Rocha (:lucasr)]

Comment on attachment 731779 [details] [diff] [review]
Part 4: Use gecko package for database classes

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

Looks fine to me. Just make sure this doesn't break sync somehow.

::: mobile/android/base/AndroidManifest.xml.in
@@ +229,4 @@
>                    android:configChanges="orientation|screenSize"
>                    android:excludeFromRecents="true"/>
>  
> +        <provider android:name="org.mozilla.gecko.db.BrowserProvider"

Won't this change any users of the content provider such as sync?

Comment 39

[Richard Newman [:rnewman]]

(In reply to Lucas Rocha (:lucasr) from comment #38)

> > +        <provider android:name="org.mozilla.gecko.db.BrowserProvider"
> 
> Won't this change any users of the content provider such as sync?

I should probably be really, really explicit and say for the record: Brian, please test this with Nightly, Aurora, Beta, and release builds, making sure both Sync and our search integration continue to function :D

(Had kinda assumed that, but worth being clear)

Comment 40

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

Comment on attachment 732047 [details] [diff] [review]
Part 2: Move preprocessed code to AppConstants, v2

>diff --git a/mobile/android/base/App.java.in b/mobile/android/base/App.java.in

> #filter substitution
> package @ANDROID_PACKAGE_NAME@;

Is this still needed?

>diff --git a/mobile/android/base/GeckoAppShell.java b/mobile/android/base/GeckoAppShell.java

>         intent.setData(Uri.parse(aURI));
>-        intent.setClassName(GeckoApp.mAppContext, GeckoApp.mAppContext.getPackageName() + ".WebApps$WebApp" + aIndex);
>+        intent.setClassName(AppConstants.ANDROID_PACKAGE_NAME,
>+                AppConstants.ANDROID_PACKAGE_NAME + ".WebApps$WebApp" + aIndex);

Just know. We will need an alias or something in the AndroidManifest to not break launching existing webapps

>                     shortcutIntent.setAction(GeckoApp.ACTION_BOOKMARK);
>                     shortcutIntent.setData(Uri.parse(aURI));
>-                    shortcutIntent.setClassName(GeckoApp.mAppContext,
>-                                                GeckoApp.mAppContext.getPackageName() + ".App");
>+                    shortcutIntent.setClassName(AppConstants.ANDROID_PACKAGE_NAME,
>+                                                AppConstants.BROWSER_INTENT_CLASS);

We will need to do something to not break opening existing bookmarks

Comment 41

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

Comment on attachment 732052 [details] [diff] [review]
Part 5: Reduce preprocessing in WebApp, v3

>diff --git a/mobile/android/base/GeckoAppShell.java b/mobile/android/base/GeckoAppShell.java
>index 30853db..de11f58 100644
>--- a/mobile/android/base/GeckoAppShell.java
>+++ b/mobile/android/base/GeckoAppShell.java
>@@ -581,7 +581,7 @@ public class GeckoAppShell
>         intent.setAction(GeckoApp.ACTION_WEBAPP_PREFIX + aIndex);
>         intent.setData(Uri.parse(aURI));
>         intent.setClassName(AppConstants.ANDROID_PACKAGE_NAME,
>-                AppConstants.ANDROID_PACKAGE_NAME + ".WebApps$WebApp" + aIndex);
>+                "org.mozilla.gecko.WebApps$WebApp" + aIndex);
>         return intent;
>     }

Remember: Don't break existing web apps

Comment 42

[Brian Nicholson (:bnicholson)]

(In reply to Mark Finkle (:mfinkle) from comment #40)
> Comment on attachment 732047 [details] [diff] [review]
> Part 2: Move preprocessed code to AppConstants, v2
> 
> >diff --git a/mobile/android/base/App.java.in b/mobile/android/base/App.java.in
> 
> > #filter substitution
> > package @ANDROID_PACKAGE_NAME@;
> 
> Is this still needed?

Yes; App.java.in is a namespace alias to BrowserApp. We need to keep it in the org.mozilla.fennec package since the app's package/name (org.mozilla.fennec.App) cannot change after it's been published.

> >diff --git a/mobile/android/base/GeckoAppShell.java b/mobile/android/base/GeckoAppShell.java
> 
> >         intent.setData(Uri.parse(aURI));
> >-        intent.setClassName(GeckoApp.mAppContext, GeckoApp.mAppContext.getPackageName() + ".WebApps$WebApp" + aIndex);
> >+        intent.setClassName(AppConstants.ANDROID_PACKAGE_NAME,
> >+                AppConstants.ANDROID_PACKAGE_NAME + ".WebApps$WebApp" + aIndex);
> 
> Just know. We will need an alias or something in the AndroidManifest to not
> break launching existing webapps

Okay, I can do this as a quick follow-up.

> 
> >                     shortcutIntent.setAction(GeckoApp.ACTION_BOOKMARK);
> >                     shortcutIntent.setData(Uri.parse(aURI));
> >-                    shortcutIntent.setClassName(GeckoApp.mAppContext,
> >-                                                GeckoApp.mAppContext.getPackageName() + ".App");
> >+                    shortcutIntent.setClassName(AppConstants.ANDROID_PACKAGE_NAME,
> >+                                                AppConstants.BROWSER_INTENT_CLASS);
> 
> We will need to do something to not break opening existing bookmarks

The class name for bookmarks isn't changing (BROWSER_INTENT_CLASS is the package name + ".App"), so this shouldn't break anything.

Comment 43

[Brian Nicholson (:bnicholson)]

Try push was green and local testing hasn't revealed any problems, so here goes:

https://hg.mozilla.org/integration/mozilla-inbound/rev/9518bed6e1b8
https://hg.mozilla.org/integration/mozilla-inbound/rev/5966e48d3c63
https://hg.mozilla.org/integration/mozilla-inbound/rev/948b0710f727
https://hg.mozilla.org/integration/mozilla-inbound/rev/b11be362d439
https://hg.mozilla.org/integration/mozilla-inbound/rev/8cd1bf407495

Comment 44

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/9518bed6e1b8
https://hg.mozilla.org/mozilla-central/rev/5966e48d3c63
https://hg.mozilla.org/mozilla-central/rev/948b0710f727
https://hg.mozilla.org/mozilla-central/rev/b11be362d439
https://hg.mozilla.org/mozilla-central/rev/8cd1bf407495