Last Comment Bug 714874 - Fix Java warnings for Fennec and Sync
: Fix Java warnings for Fennec and Sync
Status: RESOLVED FIXED
:
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: unspecified
: ARM Android
: P4 normal (vote)
: Firefox 12
Assigned To: Chris Peterson [:cpeterson]
:
: Sebastian Kaspari (:sebastian)
Mentors:
Depends on: 715047
Blocks: 716673
  Show dependency treegraph
 
Reported: 2012-01-03 10:52 PST by Chris Peterson [:cpeterson]
Modified: 2012-01-26 19:35 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
fixed
11+


Attachments
0-compile-fennec-and-sync-files-separately.patch (4.77 KB, patch)
2012-01-03 10:58 PST, Chris Peterson [:cpeterson]
no flags Details | Diff | Splinter Review
1-fix-some-java-warnings.patch (6.06 KB, patch)
2012-01-03 10:59 PST, Chris Peterson [:cpeterson]
doug.turner: review+
Details | Diff | Splinter Review
2-fix-java-cast-warnings.patch (17.71 KB, patch)
2012-01-03 11:00 PST, Chris Peterson [:cpeterson]
doug.turner: feedback+
Details | Diff | Splinter Review
3-disable-some-java-warnings.patch (2.07 KB, patch)
2012-01-03 11:02 PST, Chris Peterson [:cpeterson]
no flags Details | Diff | Splinter Review
0-compile-fennec-and-sync-files-separately-v2.patch (59.61 KB, patch)
2012-01-05 13:31 PST, Chris Peterson [:cpeterson]
no flags Details | Diff | Splinter Review
1-fix-some-java-warnings-v1-rebased.patch (5.76 KB, patch)
2012-01-05 13:34 PST, Chris Peterson [:cpeterson]
cpeterson: review+
Details | Diff | Splinter Review
2-fix-java-cast-warnings-v1-rebased.patch (17.42 KB, patch)
2012-01-05 13:36 PST, Chris Peterson [:cpeterson]
pwalton: review+
Details | Diff | Splinter Review
patches-fix-warnings-v2/3-disable-some-java-warnings-v2.patch (2.08 KB, patch)
2012-01-05 13:38 PST, Chris Peterson [:cpeterson]
no flags Details | Diff | Splinter Review
0-compile-fennec-and-sync-files-separately-v3.patch (59.65 KB, patch)
2012-01-05 15:36 PST, Chris Peterson [:cpeterson]
rnewman: review+
Details | Diff | Splinter Review
3-disable-some-java-warnings-v3.patch (1.95 KB, patch)
2012-01-05 18:13 PST, Chris Peterson [:cpeterson]
doug.turner: review+
rnewman: feedback+
Details | Diff | Splinter Review
2-fix-java-cast-warnings-v2.patch (19.37 KB, patch)
2012-01-06 14:23 PST, Chris Peterson [:cpeterson]
cpeterson: review+
Details | Diff | Splinter Review
Changes that touch PanZoomController only (7.70 KB, patch)
2012-01-13 04:52 PST, Kartikaya Gupta (email:kats@mozilla.com)
blassey.bugs: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
bug-714874-part-1-compile-fennec-and-sync-files-separately-v3.patch (59.65 KB, patch)
2012-01-20 12:14 PST, Chris Peterson [:cpeterson]
cpeterson: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
bug-714874-part-2-fix-some-java-warnings-v1-rebased.patch (5.75 KB, patch)
2012-01-20 12:16 PST, Chris Peterson [:cpeterson]
cpeterson: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
bug-714874-part-3-fix-java-cast-warnings-v2.patch (19.37 KB, patch)
2012-01-20 12:17 PST, Chris Peterson [:cpeterson]
cpeterson: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
bug-714874-part-4-disable-some-java-warnings-v3.patch (1.95 KB, patch)
2012-01-20 12:18 PST, Chris Peterson [:cpeterson]
cpeterson: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Chris Peterson [:cpeterson] 2012-01-03 10:52:55 PST
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.
Comment 1 Chris Peterson [:cpeterson] 2012-01-03 10:58:13 PST
Created attachment 585466 [details] [diff] [review]
0-compile-fennec-and-sync-files-separately.patch

Split Fennec and Sync java file lists to allow different compile flags.
Comment 2 Chris Peterson [:cpeterson] 2012-01-03 10:59:14 PST
Created attachment 585467 [details] [diff] [review]
1-fix-some-java-warnings.patch

* 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.
Comment 3 Chris Peterson [:cpeterson] 2012-01-03 11:00:24 PST
Created attachment 585469 [details] [diff] [review]
2-fix-java-cast-warnings.patch

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.
Comment 4 Chris Peterson [:cpeterson] 2012-01-03 11:02:14 PST
Created attachment 585470 [details] [diff] [review]
3-disable-some-java-warnings.patch

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.
Comment 5 Richard Newman [:rnewman] 2012-01-03 11:20:49 PST
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.
Comment 6 Brad Lassey [:blassey] (use needinfo?) 2012-01-03 11:28:08 PST
(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?
Comment 7 Richard Newman [:rnewman] 2012-01-03 11:32:39 PST
(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?
Comment 8 Brad Lassey [:blassey] (use needinfo?) 2012-01-03 11:39:10 PST
> 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 9 Doug Turner (:dougt) 2012-01-03 11:49:56 PST
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.
Comment 10 Chris Peterson [:cpeterson] 2012-01-03 12:02:21 PST
> > -  -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?
Comment 11 Chris Peterson [:cpeterson] 2012-01-03 12:25:39 PST
> 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}"?
Comment 12 Chris Peterson [:cpeterson] 2012-01-03 15:19:38 PST
> 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.
Comment 13 Richard Newman [:rnewman] 2012-01-03 18:04:07 PST
(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.
Comment 14 Richard Newman [:rnewman] 2012-01-03 18:12:19 PST
(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.
Comment 15 Brad Lassey [:blassey] (use needinfo?) 2012-01-03 18:13:59 PST
(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
Comment 16 Chris Peterson [:cpeterson] 2012-01-05 13:31:24 PST
Created attachment 586199 [details] [diff] [review]
0-compile-fennec-and-sync-files-separately-v2.patch

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.
Comment 17 Chris Peterson [:cpeterson] 2012-01-05 13:34:39 PST
Created attachment 586203 [details] [diff] [review]
1-fix-some-java-warnings-v1-rebased.patch

r+=dougt (before I rebased this patch)
Comment 18 Chris Peterson [:cpeterson] 2012-01-05 13:36:30 PST
Created attachment 586206 [details] [diff] [review]
2-fix-java-cast-warnings-v1-rebased.patch

rebased patch
Comment 19 Chris Peterson [:cpeterson] 2012-01-05 13:38:42 PST
Created attachment 586210 [details] [diff] [review]
patches-fix-warnings-v2/3-disable-some-java-warnings-v2.patch

patch v2 compiles Fennec and Sync .java files with -Werror and third-party code without any warning flags enabled.
Comment 20 Richard Newman [:rnewman] 2012-01-05 14:31:33 PST
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 21 Richard Newman [:rnewman] 2012-01-05 14:32:59 PST
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.
Comment 22 Chris Peterson [:cpeterson] 2012-01-05 15:36:30 PST
Created attachment 586249 [details] [diff] [review]
0-compile-fennec-and-sync-files-separately-v3.patch

oops: I forgot to "hg qrefresh" my patch queue! This patch v3 compiles $(SYNC_JAVA_FILES).
Comment 23 Chris Peterson [:cpeterson] 2012-01-05 18:13:22 PST
Created attachment 586301 [details] [diff] [review]
3-disable-some-java-warnings-v3.patch

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 24 Richard Newman [:rnewman] 2012-01-06 09:12:52 PST
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 25 Patrick Walton (:pcwalton) 2012-01-06 11:04:56 PST
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().)
Comment 26 Kartikaya Gupta (email:kats@mozilla.com) 2012-01-06 11:40:13 PST
> >      @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).
Comment 27 Kartikaya Gupta (email:kats@mozilla.com) 2012-01-06 11:41:25 PST
s/where we check for log exception/where we log the exception/
Comment 28 Chris Peterson [:cpeterson] 2012-01-06 11:58:55 PST
: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)|.
Comment 29 Kartikaya Gupta (email:kats@mozilla.com) 2012-01-06 12:37:42 PST
(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.
Comment 30 Chris Peterson [:cpeterson] 2012-01-06 13:14:01 PST
> 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.
Comment 31 Chris Peterson [:cpeterson] 2012-01-06 14:23:57 PST
Created attachment 586571 [details] [diff] [review]
2-fix-java-cast-warnings-v2.patch

patch v2 incorporates pcwalton's and kats' feedback. r=pcwalton
Comment 32 Chris Peterson [:cpeterson] 2012-01-06 14:49:59 PST
All patches have been r+'d. checkin-needed, please.
Comment 34 Mark Finkle (:mfinkle) (use needinfo?) 2012-01-07 06:09:03 PST
not FIXED until it lands on mozilla-central
Comment 36 Kartikaya Gupta (email:kats@mozilla.com) 2012-01-12 20:07:31 PST
This bug also touches PanZoomController and may need to go into aurora. Marking as dependency for bug 716673.
Comment 37 Richard Newman [:rnewman] 2012-01-12 21:22:22 PST
(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.
Comment 38 Kartikaya Gupta (email:kats@mozilla.com) 2012-01-13 04:52:20 PST
Created attachment 588378 [details] [diff] [review]
Changes that touch PanZoomController only

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 39 Kartikaya Gupta (email:kats@mozilla.com) 2012-01-19 00:24:25 PST
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 40 Alex Keybl [:akeybl] 2012-01-19 12:03:22 PST
Comment on attachment 588378 [details] [diff] [review]
Changes that touch PanZoomController only

[Triage Comment]
Mobile only - approved for Aurora.
Comment 41 Kartikaya Gupta (email:kats@mozilla.com) 2012-01-19 13:16:45 PST
https://hg.mozilla.org/releases/mozilla-aurora/rev/05f417af1d18
Comment 42 Chris Peterson [:cpeterson] 2012-01-19 14:23:26 PST
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 43 Chris Peterson [:cpeterson] 2012-01-19 14:23:40 PST
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 44 Chris Peterson [:cpeterson] 2012-01-19 14:23:55 PST
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 45 Chris Peterson [:cpeterson] 2012-01-19 14:24:06 PST
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.
Comment 46 Kartikaya Gupta (email:kats@mozilla.com) 2012-01-19 14:25:32 PST
(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.
Comment 47 Chris Peterson [:cpeterson] 2012-01-20 12:14:15 PST
Created attachment 590285 [details] [diff] [review]
bug-714874-part-1-compile-fennec-and-sync-files-separately-v3.patch

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 48 Chris Peterson [:cpeterson] 2012-01-20 12:15:14 PST
Comment on attachment 590285 [details] [diff] [review]
bug-714874-part-1-compile-fennec-and-sync-files-separately-v3.patch

r=rnewman r=dougt
Comment 49 Chris Peterson [:cpeterson] 2012-01-20 12:16:28 PST
Created attachment 590287 [details] [diff] [review]
bug-714874-part-2-fix-some-java-warnings-v1-rebased.patch

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.
Comment 50 Chris Peterson [:cpeterson] 2012-01-20 12:17:35 PST
Created attachment 590288 [details] [diff] [review]
bug-714874-part-3-fix-java-cast-warnings-v2.patch

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.
Comment 51 Chris Peterson [:cpeterson] 2012-01-20 12:18:34 PST
Created attachment 590291 [details] [diff] [review]
bug-714874-part-4-disable-some-java-warnings-v3.patch

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 52 Alex Keybl [:akeybl] 2012-01-23 09:19:48 PST
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.
Comment 53 Brad Lassey [:blassey] (use needinfo?) 2012-01-24 22:55:29 PST
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

Note You need to log in before you can comment on or make changes to this bug.