Last Comment Bug 722439 - Fennec fails to build with javac 1.7 because of warnings
: Fennec fails to build with javac 1.7 because of warnings
Status: RESOLVED FIXED
:
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: unspecified
: All Android
: P3 normal (vote)
: Firefox 13
Assigned To: away[Nov24,Dec5) Kartikaya Gupta (email:kats@mozilla.com)
:
: Sebastian Kaspari (:sebastian)
Mentors:
Depends on: 727121
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-30 12:39 PST by away[Nov24,Dec5) Kartikaya Gupta (email:kats@mozilla.com)
Modified: 2016-07-29 14:22 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
affected
affected
fixed
+


Attachments
warnings (12.82 KB, text/plain)
2012-01-30 12:39 PST, away[Nov24,Dec5) Kartikaya Gupta (email:kats@mozilla.com)
no flags Details
Fix most compiler warnings (27.62 KB, patch)
2012-02-14 09:18 PST, away[Nov24,Dec5) Kartikaya Gupta (email:kats@mozilla.com)
chrislord.net: review+
Details | Diff | Splinter Review
Work around javac 1.7 warning suppression bug (2.41 KB, patch)
2012-02-14 09:19 PST, away[Nov24,Dec5) Kartikaya Gupta (email:kats@mozilla.com)
chrislord.net: review+
Details | Diff | Splinter Review

Description away[Nov24,Dec5) Kartikaya Gupta (email:kats@mozilla.com) 2012-01-30 12:39:35 PST
Created attachment 592827 [details]
warnings

Attached are the warnings output by the 1.7 compiler as reported by bgirard when he tried to build with javac 1.7. We should fix these as many (if not all) are legitimate warnings.

Also we should prevent regressing these by turning on any necessary -Xlint options in the 1.6 compile path.
Comment 1 away[Nov24,Dec5) Kartikaya Gupta (email:kats@mozilla.com) 2012-01-30 13:00:45 PST
Adding :lucasr since a bunch of these warnings are in his code and I'm not sure offhand what some of the List and Map types should be specific-ized as.
Comment 2 away[Nov24,Dec5) Kartikaya Gupta (email:kats@mozilla.com) 2012-02-14 09:18:39 PST
Created attachment 597048 [details] [diff] [review]
Fix most compiler warnings
Comment 3 away[Nov24,Dec5) Kartikaya Gupta (email:kats@mozilla.com) 2012-02-14 09:19:07 PST
Created attachment 597049 [details] [diff] [review]
Work around javac 1.7 warning suppression bug
Comment 4 Richard Newman [:rnewman] 2012-02-14 09:22:36 PST
With respect, this is the wrong fix.

+                @SuppressWarnings("rawtypes")
                 Map map = (Map) selectedItem;

Add generic types instead. The only use of suppressing this kind of thing should be for library code that we don't control.
Comment 5 Chris Lord [:cwiiis] 2012-02-14 09:41:50 PST
Comment on attachment 597048 [details] [diff] [review]
Fix most compiler warnings

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

Looks good to me. The @SuppressWarnings stuff is unfortunate, but I assume it's necessary.
Comment 6 Richard Newman [:rnewman] 2012-02-14 09:53:27 PST
Comment on attachment 597049 [details] [diff] [review]
Work around javac 1.7 warning suppression bug

Obsoleted by Bug 727121. Feel free to land the patch from there if you need a fix before I commit it.
Comment 7 Chris Lord [:cwiiis] 2012-02-14 10:14:11 PST
Comment on attachment 597049 [details] [diff] [review]
Work around javac 1.7 warning suppression bug

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

Looks good to me.

::: mobile/android/base/sync/setup/activities/SetupSyncActivity.java
@@ +301,5 @@
>      String syncKey   = mAccountManager.getUserData(account, Constants.OPTION_SYNCKEY);
>      String serverURL = mAccountManager.getUserData(account, Constants.OPTION_SERVER);
>  
>      JSONObject jAccount = new JSONObject();
> +    Map<String, String> jAccountMap = (Map<String, String>)jAccount;

Maybe add a comment as to why this is being done?
Comment 8 away[Nov24,Dec5) Kartikaya Gupta (email:kats@mozilla.com) 2012-02-14 12:18:39 PST
(In reply to Richard Newman [:rnewman] from comment #4)
> With respect, this is the wrong fix.
> 
> +                @SuppressWarnings("rawtypes")
>                  Map map = (Map) selectedItem;
> 

So I tracked down where this map was coming from and was able to cast it like so:

Map<String,Object> map = (Map<String,Object>)selectedItem

but then it just gave me an unchecked warning instead of a raw types warning, which I had to suppress anyway. So I ended up leaving it this way. The problem is mostly that this Map gets put into the Android view hierarchy and so when we get it back it's just a plain Object, and we need to do some sort of cast to make it usable.
Comment 9 away[Nov24,Dec5) Kartikaya Gupta (email:kats@mozilla.com) 2012-02-14 12:30:58 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/57c9ea722e73
Comment 10 Marco Bonardo [::mak] 2012-02-15 09:03:18 PST
https://hg.mozilla.org/mozilla-central/rev/57c9ea722e73
Comment 11 away[Nov24,Dec5) Kartikaya Gupta (email:kats@mozilla.com) 2012-02-15 09:13:14 PST
Comment on attachment 597048 [details] [diff] [review]
Fix most compiler warnings

[Approval Request Comment]
Regression caused by (bug #): none
User impact if declined: none
Testing completed (on m-c, etc.): on m-c
Risk to taking this patch (and alternatives if risky): future patches may not apply cleanly. alternative is to rebase those future patches, which shouldn't be too hard.
String changes made by this patch: none
Comment 12 away[Nov24,Dec5) Kartikaya Gupta (email:kats@mozilla.com) 2012-02-15 09:35:12 PST
Comment on attachment 597048 [details] [diff] [review]
Fix most compiler warnings

Cancelling uplift request based on latest status meeting, since this isn't really a critical bug or anything.

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