The Apache Common lib added by Sync produces tons of Java warnings. My patches do the following: 1. Split Fennec and Sync java file lists to allow different compile flags. 2. Fix some Java warnings. 3. Fix Java warnings about redundant casts. 4. Disable some java warnings. Fennec and Sync will then compile without Java warnings.

Split Fennec and Sync java file lists to allow different compile flags.

* Fix character encoding warnings in third-party Apache code by compiling as UTF8, not ASCII. * Replace casts to parameterized container types because Java type erasure prevents the runtime casts from knowing whether a Map is a Map<String,Object>. * Suppress some warnings about switch statement fallthroughs.

Remove redundant casts. Most warnings complained about code that was casting method return values that were already the desired type. Also refactor some PanZoomController code to consolidate some copy/paste of extra casts. Add a comment about why the type is important, even without casts.

Disable some java warnings. Compile Fennec java files with -Werror because we can fix warnings in our code. Compile Sync java files without -Werror because Sync includes third-party code with warnings that should be fixed by upstream maintainers.

This is half a dupe of Bug 712799. Thanks for addressing this! Disabling warnings for Sync as well as our dependencies isn't the ideal final solution; I would much rather implement a proper fix for Bug 712799, either by fixing upstream libs or preprocessing the dependencies that we've imported (as we already do for Commons). If we're happy to split our source list vars, I can spit out different manifests for Sync code and its dependencies, which would allow a better version of part 0 that doesn't disable warnings for Sync itself. The dependencies already live in base/{httpclientandroidlib,json-simple,apache} and so can be compiled separately. Sync's preprocessed files should not be compiled without warnings; those are Sync only. More after this flight.

(In reply to Richard Newman [:rnewman] from comment #5) > This is half a dupe of Bug 712799. Thanks for addressing this! > > Disabling warnings for Sync as well as our dependencies isn't the ideal > final solution; I would much rather implement a proper fix for Bug 712799, > either by fixing upstream libs or preprocessing the dependencies that we've > imported (as we already do for Commons). Why are we using the external libs? The standard Android Java APIs do both HTTP and JSON, what do these external libs offer that the standard ones don't?

(In reply to Brad Lassey [:blassey] from comment #6) > (In reply to Richard Newman [:rnewman] from comment #5) > > This is half a dupe of Bug 712799. Thanks for addressing this! > > > > Disabling warnings for Sync as well as our dependencies isn't the ideal > > final solution; I would much rather implement a proper fix for Bug 712799, > > either by fixing upstream libs or preprocessing the dependencies that we've > > imported (as we already do for Commons). > Why are we using the external libs? The standard Android Java APIs do both > HTTP and JSON, what do these external libs offer that the standard ones > don't? I think we have been over this several times. The built in libraries are 8 year old SVN snapshots, buggy and don't match any released version. Conventional wisdom is to shade current versions. If Android shipped a CVS snapshot of Necko from 2004, would you use it?

> I think we have been over this several times. The built in libraries are 8 > year old SVN snapshots, buggy and don't match any released version. > Conventional wisdom is to shade current versions. I don't think so, since I haven't asked the question before nor heard the answer. What bugs do the Android version of these libs have? > If Android shipped a CVS snapshot of Necko from 2004, would you use it? Perhaps, what bugs were fixed in the interim and what is the size cost we pay by shipping our own library? I don't have either answer for these external libs sync is shipping.

Comment on attachment 585467 [details] [diff] [review] 1-fix-some-java-warnings.patch Review of attachment 585467 [details] [diff] [review]: ----------------------------------------------------------------- ::: config/android-common.mk @@ +68,5 @@ > -target $(JAVA_VERSION) \ > -source $(JAVA_VERSION) \ > -classpath $(JAVA_CLASSPATH) \ > -bootclasspath $(JAVA_BOOTCLASSPATH) \ > + -encoding UTF8 \ no idea what this change implies.

> > - -encoding ascii \ > > + -encoding UTF8 \ > > no idea what this change implies. The Apache Commons lib (added for Sync) includes Unicode characters in some of its .java files. Which produces ~100 warnings such as: /builds/slave/try-andrd/build/mobile/android/base/apache/commons/codec/language/ColognePhonetic.java:31: warning: unmappable character for encoding ascii * Implements the <a href="http://de.wikipedia.org/wiki/K%C3%B6lner_Phonetik">???K??lner Phonetic???</a> (Cologne Phonetic) If you prefer a more conservative approach, I can continue to compile $(FENNEC_PP_JAVA_FILES) with -encoding ascii and only compile $(SYNC_JAVA_FILES) (or the Apache files) with -encoding UTF8?

> If we're happy to split our source list vars, I can spit out different manifests for Sync > code and its dependencies, which would allow a better version of part 0 that doesn't > disable warnings for Sync itself. The dependencies already live in > > base/{httpclientandroidlib,json-simple,apache} That sounds good. If you are going to create a new .mn manifest (third_party.mn?) for these dependencies, what if we moved the external libs from "mobile/android/base/{httpclientandroidlib,json-simple,apache}" to a separate directory like "mobile/android/third_party/{httpclientandroidlib,json-simple,apache}"?

> what is the size cost we pay by shipping our own library? @blassey, Sync was checked into m-c on 12/21. The nightly .apk from 12/22 is about 600 KB larger than 12/21. Note that this size increase includes both the Sync feature and external libs.

(In reply to Brad Lassey [:blassey] from comment #8) > I don't think so, since I haven't asked the question before nor heard the > answer. What bugs do the Android version of these libs have? The guy who maintains httpclientandroidlib doesn't do it for fun! Little things like "making auth caching and Basic Auth work". Seriously, Android ships with a very outdated nightly snapshot of HttpClient, and the consequences are scattered throughout mailing list posts and Stack Overflow questions, which end with "you should bundle your own version". The modern version of the library isn't big and brings lots of niceness and fixes, as well as useful features like "actually able to run the same code in our desktop unit tests", which is massively important for maintaining quality. Believe me: I didn't put on my trololol hat and say "let's take this dependency for kicks!". In any case, this isn't the right bug for this discussion, and I already made the decision, so let's move on.

(In reply to Chris Peterson (:cpeterson) from comment #11) > If you are going to create a new .mn manifest (third_party.mn?) for these > dependencies, what if we moved the external libs from > "mobile/android/base/{httpclientandroidlib,json-simple,apache}" to a > separate directory like > "mobile/android/third_party/{httpclientandroidlib,json-simple,apache}"? I can do that for the next drop, sure, but given that we're already using the manifest approach it doesn't buy us much. Up to you. If you'd like to land this fix in the mean time, I'm happy to split the manifests now in m-c and verify that they'll work; we can roll that into part 0. (Or you can do this; it's straightforward.) I'll then adjust my export script to match. (In reply to Chris Peterson (:cpeterson) from comment #12) > @blassey, Sync was checked into m-c on 12/21. The nightly .apk from 12/22 is > about 600 KB larger than 12/21. Note that this size increase includes both > the Sync feature and external libs. … and assets. Java code is a fairly small part of Fennec's built artifacts. If my filesystem isn't lying to me, the final classes.dex for the entirety of Fennec and Sync comes to 1MB, and the APK is 19MB.

(In reply to Richard Newman [:rnewman] from comment #13) > (In reply to Brad Lassey [:blassey] from comment #8) > > > I don't think so, since I haven't asked the question before nor heard the > > answer. What bugs do the Android version of these libs have? > > The guy who maintains httpclientandroidlib doesn't do it for fun! Little > things like "making auth caching and Basic Auth work". So sync needs these bugs to be fixed in order to work? > Seriously, Android ships with a very outdated nightly snapshot of > HttpClient, and the consequences are scattered throughout mailing list posts > and Stack Overflow questions, which end with "you should bundle your own > version". This is not a good justification > > The modern version of the library isn't big and brings lots of niceness and > fixes, as well as useful features like "actually able to run the same code > in our desktop unit tests", which is massively important for maintaining > quality. Again, not winning me over > Believe me: I didn't put on my trololol hat and say "let's take this > dependency for kicks!". > > In any case, this isn't the right bug for this discussion fine, filed bug 715047 for that >, and I already > made the decision, so let's move on. that's not how these things work

patch v2 moves Sync's third-party .java file list to a separate .mn manifest file. This split will allow patch part4 to specify different javac warning flags for third-party code. Compile third-party .java files before our code, so our code can link with the third-party .class files.

r+=dougt (before I rebased this patch)

rebased patch

patch v2 compiles Fennec and Sync .java files with -Werror and third-party code without any warning flags enabled.

Comment on attachment 586199 [details] [diff] [review] 0-compile-fennec-and-sync-files-separately-v2.patch Review of attachment 586199 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/Makefile.in @@ +561,3 @@ > $(NSINSTALL) -D classes > + $(JAVAC) $(JAVAC_FLAGS) -Xlint:unchecked -Xlint:deprecation -d classes $(addprefix $(srcdir)/,$(SYNC_THIRDPARTY_JAVA_FILES)) > + $(JAVAC) $(JAVAC_FLAGS) -Xlint:unchecked -Xlint:deprecation -d classes -classpath classes $(addprefix $(srcdir)/,$(FENNEC_JAVA_FILES)) $(FENNEC_PP_JAVA_FILES) $(SYNC_PP_JAVA_FILES) R.java I might be going blind, but I don't see you actually compiling SYNC_JAVA_FILES... could you check?

Comment on attachment 586210 [details] [diff] [review] patches-fix-warnings-v2/3-disable-some-java-warnings-v2.patch Review of attachment 586210 [details] [diff] [review]: ----------------------------------------------------------------- Clarify the changeset comment, plz. ::: mobile/android/base/Makefile.in @@ +559,5 @@ > # indices. > classes.dex: $(FENNEC_JAVA_FILES) $(FENNEC_PP_JAVA_FILES) $(SYNC_JAVA_FILES) $(SYNC_PP_JAVA_FILES) $(SYNC_THIRDPARTY_JAVA_FILES) R.java > $(NSINSTALL) -D classes > + $(JAVAC) $(JAVAC_FLAGS) -d classes $(addprefix $(srcdir)/,$(SYNC_THIRDPARTY_JAVA_FILES)) > + $(JAVAC) $(JAVAC_FLAGS) -Xlint:all,-deprecation -Werror -d classes -classpath classes $(addprefix $(srcdir)/,$(FENNEC_JAVA_FILES)) $(FENNEC_PP_JAVA_FILES) $(SYNC_PP_JAVA_FILES) R.java Same comment as for previous patch.

oops: I forgot to "hg qrefresh" my patch queue! This patch v3 compiles $(SYNC_JAVA_FILES).

patch v3 adds SYNC_JAVA_FILES and removes -Werror ("treat warnings as errors"). The Sync code has a few compiler warnings, so we can enable -Werror after those warnings are fixed. :rnewman asked me to create new bugs to track the Sync warnings and enabling -Werror.

Comment on attachment 586301 [details] [diff] [review] 3-disable-some-java-warnings-v3.patch Review of attachment 586301 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. Please don't forget to file that follow-up that we discussed! \o/

Comment on attachment 586206 [details] [diff] [review] 2-fix-java-cast-warnings-v1-rebased.patch Review of attachment 586206 [details] [diff] [review]: ----------------------------------------------------------------- The AwesomeBar and AwesomeBarTabs aren't my code. For the others, r=me with nits addressed. ::: mobile/android/base/ui/PanZoomController.java @@ +1004,5 @@ > } > > @Override > public void onLongPress(MotionEvent motionEvent) { > + JSONObject ret = null; Do you need the = null here? I thought Java's typestate would be smart enough to know that it can't be null after the try. Also, this isn't the return value, so I wouldn't call it "ret". @@ +1026,5 @@ > } > > @Override > public boolean onDown(MotionEvent motionEvent) { > + JSONObject ret = null; See above. @@ +1065,5 @@ > } > > @Override > public boolean onDoubleTap(MotionEvent motionEvent) { > + JSONObject ret = null; Likewise. @@ +1079,5 @@ > GeckoAppShell.sendEventToGecko(e); > return true; > } > > + private static JSONObject convertPointToJSON(PointF point) throws JSONException { Please add this to gfx/PointUtils.java instead. (I'd call it PointUtils.toJSON().)

> > @Override > > public void onLongPress(MotionEvent motionEvent) { > > + JSONObject ret = null; > > Do you need the = null here? I thought Java's typestate would be smart > enough to know that it can't be null after the try. > Also, this isn't the return value, so I wouldn't call it "ret". > Note that onLongPress is different from the others in that it *can* be null, because the catch block just does a Log.w and continues merrily on. The others throw a RuntimeException. That inconsistency should also be removed; I would prefer the graceful handling approach, where we check for log exception and then handle the null case, rather than blowing up on the UI thread (which will crash Fennec).

s/where we check for log exception/where we log the exception/

:kats, so you are suggesting that the other MotionEvent handlers *not* re-throw a RuntimeException? If you like, I can open a new bug to track your suggestion. This bug is just trying to get Fennec compiling without Java warnings. <:) btw, PanZoomController's MotionEvent handlers have a lot of repeated code. They could be refactored to just 1-2 line methods that call a private helper method like |sendGeckoGestureEvent(String geckoEventName, MotionEvent motionEvent)|.

(In reply to Chris Peterson (:cpeterson) from comment #28) > :kats, so you are suggesting that the other MotionEvent handlers *not* > re-throw a RuntimeException? If you like, I can open a new bug to track your > suggestion. This bug is just trying to get Fennec compiling without Java > warnings. <:) Yes, and please do open a new bug to track that. But also keep in mind that your patch changes the behaviour in onLongPress, so now it might throw a NullPointerException where before it wouldn't. (In the case where the JSON serialization throws an exception, and ret.toString() happens on a null ret) > > btw, PanZoomController's MotionEvent handlers have a lot of repeated code. > They could be refactored to just 1-2 line methods that call a private helper > method like |sendGeckoGestureEvent(String geckoEventName, MotionEvent > motionEvent)|. True. This can also be done as part of the separate bug from above.

> Yes, and please do open a new bug to track that. But also keep in mind that your patch > changes the behaviour in onLongPress, so now it might throw a NullPointerException where > before it wouldn't. (In the case where the JSON serialization throws an exception, and > ret.toString() happens on a null ret) I have revised my patch so onLongPress() will return on exception to avoid a null JSON string. However, this too is new behavior. The old code could (in theory) pass null to GeckoEvent() constructor because JSONObject.toString() returns null from some (unlikely) code paths.

patch v2 incorporates pcwalton's and kats' feedback. r=pcwalton

All patches have been r+'d. checkin-needed, please.

Landed on m-i: https://hg.mozilla.org/integration/mozilla-inbound/rev/ccf8194f6c26 https://hg.mozilla.org/integration/mozilla-inbound/rev/c00505771db1 https://hg.mozilla.org/integration/mozilla-inbound/rev/16f5cf776059 https://hg.mozilla.org/integration/mozilla-inbound/rev/a79e588b6fbd

not FIXED until it lands on mozilla-central

https://hg.mozilla.org/mozilla-central/rev/ccf8194f6c26 https://hg.mozilla.org/mozilla-central/rev/c00505771db1 https://hg.mozilla.org/mozilla-central/rev/16f5cf776059 https://hg.mozilla.org/mozilla-central/rev/a79e588b6fbd

This bug also touches PanZoomController and may need to go into aurora. Marking as dependency for bug 716673.

(In reply to Kartikaya Gupta (:kats) from comment #36) > This bug also touches PanZoomController and may need to go into aurora. > Marking as dependency for bug 716673. Note that Android Sync's first drop was not granted Aurora landing approval, so not all of these patches would make sense to land.

I extracted the changes from these patches that touch PanZoomController into a separate patch that can be landed on aurora without landing everything. This is so that bug 716673 applies to aurora cleanly. (This patch is aurora-only, and should not go into m-i or m-c).

Comment on attachment 588378 [details] [diff] [review] Changes that touch PanZoomController only [Approval Request Comment] Regression caused by (bug #): none User impact if declined: no direct user impact; code is only fixing java warnings. however, if we don't take it then bug 716673 will not land cleanly/easily, introducing some risk there. Testing completed (on m-c, etc.): on m-c for a while Risk to taking this patch (and alternatives if risky): no real risk since it fixes warnings only.

Comment on attachment 588378 [details] [diff] [review] Changes that touch PanZoomController only [Triage Comment] Mobile only - approved for Aurora.

Comment on attachment 586249 [details] [diff] [review] 0-compile-fennec-and-sync-files-separately-v3.patch [Approval Request Comment] Regression caused by (bug #): N/A User impact if declined: None. This patch fixes some Java compiler warnings. Testing completed (on m-c, etc.): Landed on m-c 2012-01-07 Risk to taking this patch (and alternatives if risky): Merge conflicts at next Aurora uplift.

Comment on attachment 586203 [details] [diff] [review] 1-fix-some-java-warnings-v1-rebased.patch [Approval Request Comment] Regression caused by (bug #): N/A User impact if declined: None. This patch fixes some Java compiler warnings. Testing completed (on m-c, etc.): Landed on m-c 2012-01-07 Risk to taking this patch (and alternatives if risky): Merge conflicts at next Aurora uplift.

Comment on attachment 586571 [details] [diff] [review] 2-fix-java-cast-warnings-v2.patch [Approval Request Comment] Regression caused by (bug #): N/A User impact if declined: None. This patch fixes some Java compiler warnings. Testing completed (on m-c, etc.): Landed on m-c 2012-01-07 Risk to taking this patch (and alternatives if risky): Merge conflicts at next Aurora uplift.

Comment on attachment 586301 [details] [diff] [review] 3-disable-some-java-warnings-v3.patch [Approval Request Comment] Regression caused by (bug #): N/A User impact if declined: None. This patch fixes some Java compiler warnings. Testing completed (on m-c, etc.): Landed on m-c 2012-01-07 Risk to taking this patch (and alternatives if risky): Merge conflicts at next Aurora uplift.

(In reply to Kartikaya Gupta (:kats) from comment #41) > https://hg.mozilla.org/releases/mozilla-aurora/rev/05f417af1d18 I also landed the changes to PointUtils to fix build bustage on aurora caused by above patch. https://hg.mozilla.org/releases/mozilla-aurora/rev/e3fd154d6038 Apparently somewhere along the way I missed a hg qref when putting together my PZC change for aurora, and it got left out.

Removed premature a=name. r=rnewman r=dougt [Approval Request Comment] Regression caused by (bug #): N/A User impact if declined: None. This patch fixes some Java compiler warnings. Testing completed (on m-c, etc.): Landed on m-c 2012-01-07 Risk to taking this patch (and alternatives if risky): Merge conflicts at next Aurora uplift.

Comment on attachment 590285 [details] [diff] [review] bug-714874-part-1-compile-fennec-and-sync-files-separately-v3.patch r=rnewman r=dougt

Removed premature a=name. r=dougt [Approval Request Comment] Regression caused by (bug #): N/A User impact if declined: None. This patch fixes some Java compiler warnings. Testing completed (on m-c, etc.): Landed on m-c 2012-01-07 Risk to taking this patch (and alternatives if risky): Merge conflicts at next Aurora uplift.

Removed premature a=name. r=pcwalton r=dougt [Approval Request Comment] Regression caused by (bug #): N/A User impact if declined: None. This patch fixes some Java compiler warnings. Testing completed (on m-c, etc.): Landed on m-c 2012-01-07 Risk to taking this patch (and alternatives if risky): Merge conflicts at next Aurora uplift.

Removed premature a=name. r=rnewman r=dougt [Approval Request Comment] Regression caused by (bug #): N/A User impact if declined: None. This patch fixes some Java compiler warnings. Testing completed (on m-c, etc.): Landed on m-c 2012-01-07 Risk to taking this patch (and alternatives if risky): Merge conflicts at next Aurora uplift.

Comment on attachment 590285 [details] [diff] [review] bug-714874-part-1-compile-fennec-and-sync-files-separately-v3.patch [Triage Comment] Mobile only - approved for Aurora.

https://hg.mozilla.org/releases/mozilla-aurora/rev/007aa5c56b0e https://hg.mozilla.org/releases/mozilla-aurora/rev/254807e32941 leaving status-firefox11 as affected since there is more to land

https://hg.mozilla.org/releases/mozilla-aurora/rev/59907c5f60c7 https://hg.mozilla.org/releases/mozilla-aurora/rev/7c89e6d33286