Fix Java warnings for Fennec and Sync

RESOLVED FIXED in Firefox 11

Status

()

Firefox for Android
General
P4
normal
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: cpeterson, Assigned: cpeterson)

Tracking

(Depends on: 1 bug)

unspecified
Firefox 12
ARM
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox11 fixed, firefox12 fixed, fennec11+)

Details

Attachments

(5 attachments, 11 obsolete attachments)

7.70 KB, patch
blassey
: review+
Details | Diff | Splinter Review
59.65 KB, patch
cpeterson
: review+
Details | Diff | Splinter Review
5.75 KB, patch
cpeterson
: review+
Details | Diff | Splinter Review
19.37 KB, patch
cpeterson
: review+
Details | Diff | Splinter Review
1.95 KB, patch
cpeterson
: review+
Details | Diff | Splinter Review
(Assignee)

Description

5 years ago
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.
(Assignee)

Updated

5 years ago
Assignee: nobody → cpeterson
(Assignee)

Comment 1

5 years ago
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.
Attachment #585466 - Flags: review?(rnewman)
Attachment #585466 - Flags: feedback?(doug.turner)
(Assignee)

Comment 2

5 years ago
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.
Attachment #585467 - Flags: review?(doug.turner)
(Assignee)

Comment 3

5 years ago
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.
Attachment #585469 - Flags: review?(pwalton)
Attachment #585469 - Flags: feedback?(doug.turner)
(Assignee)

Comment 4

5 years ago
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.
Attachment #585470 - Flags: review?(doug.turner)
Attachment #585470 - Flags: feedback?(rnewman)
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 9

5 years ago
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.
Attachment #585467 - Flags: review?(doug.turner) → review+

Updated

5 years ago
Attachment #585469 - Flags: feedback?(doug.turner) → feedback+
(Assignee)

Comment 10

5 years ago
> > -  -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?
(Assignee)

Comment 11

5 years ago
> 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}"?
(Assignee)

Comment 12

5 years ago
> 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.
Blocks: 715047
(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
No longer blocks: 715047
Depends on: 715047

Updated

5 years ago
Priority: -- → P4
(Assignee)

Comment 16

5 years ago
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.
Attachment #585466 - Attachment is obsolete: true
Attachment #585466 - Flags: review?(rnewman)
Attachment #585466 - Flags: feedback?(doug.turner)
Attachment #586199 - Flags: review?(rnewman)
Attachment #586199 - Flags: feedback?(doug.turner)
(Assignee)

Comment 17

5 years ago
Created attachment 586203 [details] [diff] [review]
1-fix-some-java-warnings-v1-rebased.patch

r+=dougt (before I rebased this patch)
Attachment #585467 - Attachment is obsolete: true
Attachment #586203 - Flags: review+
(Assignee)

Comment 18

5 years ago
Created attachment 586206 [details] [diff] [review]
2-fix-java-cast-warnings-v1-rebased.patch

rebased patch
Attachment #585469 - Attachment is obsolete: true
Attachment #585469 - Flags: review?(pwalton)
Attachment #586206 - Flags: review?(pwalton)
Attachment #586206 - Flags: feedback?(doug.turner)
(Assignee)

Comment 19

5 years ago
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.
Attachment #585470 - Attachment is obsolete: true
Attachment #585470 - Flags: review?(doug.turner)
Attachment #585470 - Flags: feedback?(rnewman)
Attachment #586210 - Flags: review?(doug.turner)
Attachment #586210 - Flags: feedback?(rnewman)
(Assignee)

Updated

5 years ago
Status: NEW → ASSIGNED
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?
Attachment #586199 - Flags: review?(rnewman)
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.
Attachment #586210 - Flags: feedback?(rnewman)
(Assignee)

Comment 22

5 years ago
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).
Attachment #586199 - Attachment is obsolete: true
Attachment #586199 - Flags: feedback?(doug.turner)
Attachment #586249 - Flags: review?(rnewman)
(Assignee)

Comment 23

5 years ago
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.
Attachment #586210 - Attachment is obsolete: true
Attachment #586210 - Flags: review?(doug.turner)
Attachment #586301 - Flags: review?(doug.turner)
Attachment #586301 - Flags: feedback?(rnewman)
Attachment #586249 - Flags: review?(rnewman) → review+
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/
Attachment #586301 - Flags: feedback?(rnewman) → feedback+
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().)
Attachment #586206 - Flags: review?(pwalton) → review+
> >      @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/
(Assignee)

Comment 28

5 years ago
: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.
(Assignee)

Comment 30

5 years ago
> 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.
(Assignee)

Comment 31

5 years ago
Created attachment 586571 [details] [diff] [review]
2-fix-java-cast-warnings-v2.patch

patch v2 incorporates pcwalton's and kats' feedback. r=pcwalton
Attachment #586206 - Attachment is obsolete: true
Attachment #586206 - Flags: feedback?(doug.turner)
Attachment #586571 - Flags: review+

Updated

5 years ago
Attachment #586301 - Flags: review?(doug.turner) → review+
(Assignee)

Comment 32

5 years ago
All patches have been r+'d. checkin-needed, please.
Keywords: checkin-needed
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
(Assignee)

Updated

5 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
not FIXED until it lands on mozilla-central
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Status: REOPENED → NEW
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
Status: NEW → RESOLVED
Last Resolved: 5 years ago5 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 12
This bug also touches PanZoomController and may need to go into aurora. Marking as dependency for bug 716673.
Blocks: 716673
tracking-fennec: --- → ?
(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.
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).
Attachment #588378 - Flags: review?(blassey.bugs)
Attachment #588378 - Flags: review?(blassey.bugs) → review+
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.
Attachment #588378 - Flags: approval-mozilla-aurora?
Comment on attachment 588378 [details] [diff] [review]
Changes that touch PanZoomController only

[Triage Comment]
Mobile only - approved for Aurora.
Attachment #588378 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/05f417af1d18
(Assignee)

Comment 42

5 years ago
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.
Attachment #586249 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 43

5 years ago
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.
Attachment #586203 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 44

5 years ago
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.
Attachment #586571 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 45

5 years ago
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.
Attachment #586301 - Flags: approval-mozilla-aurora?
(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.
(Assignee)

Comment 47

5 years ago
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.
Attachment #586249 - Attachment is obsolete: true
Attachment #586249 - Flags: approval-mozilla-aurora?
Attachment #590285 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 48

5 years ago
Comment on attachment 590285 [details] [diff] [review]
bug-714874-part-1-compile-fennec-and-sync-files-separately-v3.patch

r=rnewman r=dougt
Attachment #590285 - Flags: review+
(Assignee)

Comment 49

5 years ago
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.
Attachment #586203 - Attachment is obsolete: true
Attachment #586203 - Flags: approval-mozilla-aurora?
Attachment #590287 - Flags: review+
Attachment #590287 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 50

5 years ago
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.
Attachment #586571 - Attachment is obsolete: true
Attachment #586571 - Flags: approval-mozilla-aurora?
Attachment #590288 - Flags: review+
Attachment #590288 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 51

5 years ago
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.
Attachment #586301 - Attachment is obsolete: true
Attachment #586301 - Flags: approval-mozilla-aurora?
Attachment #590291 - Flags: review+
Attachment #590291 - Flags: approval-mozilla-aurora?
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.
Attachment #590285 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

5 years ago
Attachment #590287 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

5 years ago
Attachment #590288 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

5 years ago
Attachment #590291 - Flags: approval-mozilla-aurora? → approval-mozilla-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
tracking-fennec: ? → 11+
status-firefox11: --- → affected
status-firefox12: --- → fixed
https://hg.mozilla.org/releases/mozilla-aurora/rev/59907c5f60c7
https://hg.mozilla.org/releases/mozilla-aurora/rev/7c89e6d33286
status-firefox11: affected → fixed
You need to log in before you can comment on or make changes to this bug.