722439 - Fennec fails to build with javac 1.7 because of warnings

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect, P3)

Description

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

Attachment: 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

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

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

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

Attachment: Fix most compiler warnings

Comment 3

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

Attachment: Work around javac 1.7 warning suppression bug

Comment 4

[Richard Newman [:rnewman]]

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]]

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]]

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]]

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

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

(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

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/57c9ea722e73

Comment 10

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/57c9ea722e73

Comment 11

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

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

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

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.