1062112 - Build failing for javac version 1.8.0_11

Status: RESOLVED FIXED

(Firefox Build System :: Android Studio and Gradle Integration, defect)

Description

[jahandaniyal]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:30.0) Gecko/20100101 Firefox/30.0
Build ID: 20140608211457

Steps to reproduce:

./mach build - gives error when building with javac 1.8.0_11
java version installed is java-8-oracle


Actual results:

0:06.49 configure:21557: checking for java
 0:06.49 configure:21606: checking for javac
 0:06.49 configure:21655: checking for javah
 0:06.49 configure:21704: checking for jar
 0:06.49 configure:21753: checking for jarsigner
 0:06.49 configure:21802: checking for keytool
 0:06.49 configure:21871: checking for minimum required javac version = 1.7
 0:06.49 configure: error: javac 1.7 is required.
 0:06.49 *** Fix above errors and then restart with\
 0:06.49                "/usr/bin/make -f client.mk build"
 0:06.49 make[2]: *** [configure] Error 1
 0:06.49 make[1]: *** [/home/daniyal/mozilla-central/obj-i386-linux-android/Makefile] Error 2
 0:06.49 make: *** [build] Error 2
 0:06.51 0 compiler warnings present.

Comment 1

[jahandaniyal]

P.S - build is working perfectly fine with javac 1.7.xx

Comment 2

[Nick Alexander :nalexander [he/him]]

This is by design.  The build actually fails (with a real error message) on Java 1.8.  ckitching is the expert here; CCing him here.

Comment 3

[Chris Kitching [:ckitching]]

This happens:


 2:12.95 /home/chris/Fennec/Fennec/mobile/android/base/GeckoAppShell.java:135: warning: auxiliary class GeckoEditableListener in /home/chris/Fennec/Fennec/mobile/android/base/GeckoEditable.java should not be accessed from outside its own source file
 2:12.95     private static GeckoEditableListener editableListener;
 2:12.96                    ^
 2:13.12 /home/chris/Fennec/Fennec/mobile/android/base/GeckoInputConnection.java:44: warning: auxiliary class GeckoEditableListener in /home/chris/Fennec/Fennec/mobile/android/base/GeckoEditable.java should not be accessed from outside its own source file
 2:13.12     implements InputConnectionHandler, GeckoEditableListener {
 2:13.12                                        ^
 2:13.12 /home/chris/Fennec/Fennec/mobile/android/base/GeckoInputConnection.java:224: warning: auxiliary class GeckoEditableClient in /home/chris/Fennec/Fennec/mobile/android/base/GeckoEditable.java should not be accessed from outside its own source file
 2:13.12     private final GeckoEditableClient mEditableClient;
 2:13.12                   ^
 2:13.12 /home/chris/Fennec/Fennec/mobile/android/base/GeckoInputConnection.java:234: warning: auxiliary class GeckoEditableClient in /home/chris/Fennec/Fennec/mobile/android/base/GeckoEditable.java should not be accessed from outside its own source file
 2:13.12                                                GeckoEditableClient editable) {
 2:13.12                                                ^
 2:13.12 /home/chris/Fennec/Fennec/mobile/android/base/GeckoInputConnection.java:233: warning: auxiliary class GeckoEditableListener in /home/chris/Fennec/Fennec/mobile/android/base/GeckoEditable.java should not be accessed from outside its own source file
 2:13.12     public static GeckoEditableListener create(View targetView,
 2:13.12                   ^
 2:13.12 /home/chris/Fennec/Fennec/mobile/android/base/GeckoInputConnection.java:242: warning: auxiliary class GeckoEditableClient in /home/chris/Fennec/Fennec/mobile/android/base/GeckoEditable.java should not be accessed from outside its own source file
 2:13.12                                    GeckoEditableClient editable) {
 2:13.12                                    ^
 2:13.12 /home/chris/Fennec/Fennec/mobile/android/base/GeckoInputConnection.java:134: warning: auxiliary class GeckoEditableClient in /home/chris/Fennec/Fennec/mobile/android/base/GeckoEditable.java should not be accessed from outside its own source file
 2:13.12                                   final GeckoEditableClient client,
 2:13.12                                         ^
 2:13.12 /home/chris/Fennec/Fennec/mobile/android/base/GeckoInputConnection.java:146: warning: auxiliary class GeckoEditableClient in /home/chris/Fennec/Fennec/mobile/android/base/GeckoEditable.java should not be accessed from outside its own source file
 2:13.12                                           final GeckoEditableClient client,
 2:13.12                                                 ^
 2:13.12 /home/chris/Fennec/Fennec/mobile/android/base/GeckoInputConnection.java:156: warning: auxiliary class GeckoEditableClient in /home/chris/Fennec/Fennec/mobile/android/base/GeckoEditable.java should not be accessed from outside its own source file
 2:13.12                                                final GeckoEditableClient client) {
 2:13.12                                                      ^
 2:13.12 /home/chris/Fennec/Fennec/mobile/android/base/GeckoInputConnection.java:1003: warning: auxiliary class GeckoEditableClient in /home/chris/Fennec/Fennec/mobile/android/base/GeckoEditable.java should not be accessed from outside its own source file
 2:13.12                                       GeckoEditableClient editable) {
 2:13.12                                       ^
 2:13.12 /home/chris/Fennec/Fennec/mobile/android/base/GeckoInputConnection.java:1009: warning: auxiliary class GeckoEditableClient in /home/chris/Fennec/Fennec/mobile/android/base/GeckoEditable.java should not be accessed from outside its own source file
 2:13.12                                                GeckoEditableClient editable) {
 2:13.12                                                ^
 2:13.12 /home/chris/Fennec/Fennec/mobile/android/base/GeckoInputConnection.java:1008: warning: auxiliary class GeckoEditableListener in /home/chris/Fennec/Fennec/mobile/android/base/GeckoEditable.java should not be accessed from outside its own source file
 2:13.12     public static GeckoEditableListener create(View targetView,
 2:13.12                   ^
 2:13.13 /home/chris/Fennec/Fennec/mobile/android/base/GeckoSmsManager.java:286: warning: auxiliary class ISmsManager in /home/chris/Fennec/Fennec/mobile/android/base/SmsManager.java should not be accessed from outside its own source file
 2:13.13   implements ISmsManager
 2:13.13              ^
 2:13.32 /home/chris/Fennec/Fennec/mobile/android/base/ChromeCast.java:37: warning: auxiliary class GeckoMediaPlayer in /home/chris/Fennec/Fennec/mobile/android/base/MediaPlayerManager.java should not be accessed from outside its own source file
 2:13.32 class ChromeCast implements GeckoMediaPlayer {
 2:13.32                             ^
 2:13.74 error: warnings found and -Werror specified
 2:14.47 /home/chris/Fennec/Fennec/mobile/android/base/home/HomeConfig.java:100: warning: [overrides] Class PanelConfig overrides equals, but neither it nor any superclass overrides hashCode method
 2:14.48     public static class PanelConfig implements Parcelable {
 2:14.48                   ^
 2:14.55 /home/chris/Fennec/Fennec/mobile/android/base/GeckoApp.java:1379: warning: auxiliary class GeckoEditableListener in /home/chris/Fennec/Fennec/mobile/android/base/GeckoEditable.java should not be accessed from outside its own source file
 2:14.55             GeckoAppShell.notifyIMEContext(GeckoEditableListener.IME_STATE_DISABLED, "", "", "");
 2:14.55                                            ^
 2:15.53 /home/chris/Fennec/Fennec/mobile/android/base/GeckoInputConnection.java:1010: warning: [rawtypes] found raw type: Class
 2:15.53         final Class[] PROXY_INTERFACES = { InputConnection.class,
 2:15.53               ^
 2:15.53   missing type arguments for generic class Class<T>
 2:15.53   where T is a type-variable:
 2:15.53     T extends Object declared in class Class
 2:15.53 /home/chris/Fennec/Fennec/mobile/android/base/GeckoInputConnection.java:1012: warning: auxiliary class GeckoEditableListener in /home/chris/Fennec/Fennec/mobile/android/base/GeckoEditable.java should not be accessed from outside its own source file
 2:15.53                 GeckoEditableListener.class };
 2:15.53                 ^
 2:15.53 /home/chris/Fennec/Fennec/mobile/android/base/GeckoInputConnection.java:1018: warning: auxiliary class GeckoEditableListener in /home/chris/Fennec/Fennec/mobile/android/base/GeckoEditable.java should not be accessed from outside its own source file
 2:15.53         return (GeckoEditableListener)dgic.mProxy;
 2:15.53                 ^
 2:15.53 /home/chris/Fennec/Fennec/mobile/android/base/GeckoInputConnection.java:1031: warning: auxiliary class GeckoEditableListener in /home/chris/Fennec/Fennec/mobile/android/base/GeckoEditable.java should not be accessed from outside its own source file
 2:15.53                     GeckoEditableListener.class, "NOTIFY_IME_", arg));
 2:15.53                     ^
 2:15.53 /home/chris/Fennec/Fennec/mobile/android/base/GeckoInputConnection.java:1034: warning: auxiliary class GeckoEditableListener in /home/chris/Fennec/Fennec/mobile/android/base/GeckoEditable.java should not be accessed from outside its own source file
 2:15.53                     GeckoEditableListener.class, "IME_STATE_", arg));
 2:15.54                     ^
 2:16.19 /home/chris/Fennec/Fennec/mobile/android/base/sync/repositories/domain/ClientRecord.java:14: warning: [overrides] Class ClientRecord overrides equals, but neither it nor any superclass overrides hashCode method
 2:16.19 public class ClientRecord extends Record {
 2:16.19        ^
 2:16.26 /home/chris/Fennec/Fennec/mobile/android/base/background/db/Tab.java:17: warning: [overrides] Class Tab overrides equals, but neither it nor any superclass overrides hashCode method
 2:16.26 public class Tab {
 2:16.26        ^
 2:16.44 /home/chris/Fennec/Fennec/mobile/android/base/sync/CollectionKeys.java:20: warning: [overrides] Class CollectionKeys overrides equals, but neither it nor any superclass overrides hashCode method
 2:16.44 public class CollectionKeys {
 2:16.44        ^

Comment 4

[Chris Kitching [:ckitching]]

In case that's not self-explanatory enough, this thread sums it up pretty well:

http://stackoverflow.com/questions/22521288/what-are-auxiliary-classes

Comment 5

[Chris Kitching [:ckitching]]

After talking with Nick on IRC, let's get builds with JDK 8 working. More compiler warnings are better!

Comment 6

[Chris Kitching [:ckitching]]

Attachment: Part 1: Cleanup some madness.

This is turning into quite the epic cleanup party.
Excellent.


GeckoEditable's classes have been pulled out to eliminate warnings.
Redundant "final" and "static" keywords have been removed (variables in interfaces are implicitly so).

ISmsManager's singleton has been refactored using the thread-safe-lazy-loaded-singleton pattern (see: http://blog.crazybob.org/2007/01/lazy-loading-singletons.html , as well as the new comment).
This has the following advantages:

No possibility of races caused by multiple threads trying to call getInstance at once.
Smaller bytecode.
No function call overheads due to getInstance().
The code that *uses* SmsManager will be deleted if !AppConstants.MOZ_WEBSMS_BACKEND. Previously, the call and null check would likely have survived optimisation (at least some of the time).
It's just plain neater.

This then caused me to look at GeckoSmsManager. An assortment of trivial insanities have been corrected:

          if (mDelivery == null) {
            restrictions.add("type IN ('" + kSmsTypeSentbox + "', '" + kSmsTypeInbox + "')");
          } else if (mDelivery == "sent") {
            restrictions.add("type = " + kSmsTypeSentbox);
          } else if (mDelivery == "received") {
            restrictions.add("type = " + kSmsTypeInbox);
          } else {
            throw new UnexpectedDeliveryStateException();
          }

Checking equality of strings with ==.
Concatenation of strings in a for loop (refactor to use StringBuilder, otherwise it desugars to a StringBuilder for every concatenation. Ew.)
Many instances of literal characters being literal strings. You can get smaller bytecode by using a char literal (where equivalent).
Replacing .size() == 0 with .isEmpty() on collection.
Removing redundant class qualifications, redundant keywords...
Making many fields final that were effectively-final (via static analysis)
Making effectively-static methods static.
PendingIntentUID has been replaced... by an integer.
MessagesListManager.mCursors is no longer initialised with a length of zero. That's dumb, because it means the moment you ever try and use it you're force to reallocate it...
There is no Javadoc tag "@note": this may crash javadoc parsers (if we ever start using one).
Non-static inner exception classes (such as IdTooHighException) are nonsensical.
We probably don't care about serialVersionUID, too...

The check at line 600:
http://mxr.mozilla.org/mozilla-aurora/source/mobile/android/base/GeckoSmsManager.java#598

appears to have been written without remembering that integers are signed in Java.
As such, this check will throw for an id exceeding 2^31-1, but if the comment is correct we're really safe up to 2^32-1.

Why is this file completely ignoring the code style standard?

Lastly, since we now have 7 SMS classes, I shoved them all in a new o.m.g.sms package.

More refactorings needed before we'll build with JDK8, but this is some of the problems resolved and I'm out of time for now.

Comment 7

[Chris Kitching [:ckitching]]

Attachment: Part 2: Fix generics in GeckoInputConnection.

Another warning is whinging about poor use of generics in DebugGeckoInputConnection.

Here's a minimal patch to fix the warning, but  having debug instrumentation deeply entangled with your application code is not something you should have. Particularly not without a single comment explaining what it's for, how it works,  why it's useful, and how it makes my debugging life so much easier I don't need to find and kill the person who wrote it.

Comment 8

[Chris Kitching [:ckitching]]

Attachment: Part 3: Move GeckoMediaPlayer

Gotta move GeckoMediaPlayer into its own file because of the way it's being used (touched from a different compilation unit).

Having multiple top-level classes in the same source file is not a good idea, see JLS section 7.6:
http://docs.oracle.com/javase/specs/jls/se8/html/jls-7.html#jls-7.6

(The *hideously* obtuse bold paragraph and footnote at the end of the section).

This is the same reason I moved a bunch of stuff around in SMS-land. When you have multiple top-level classes in a file, at most one can be accessed from outside the package. The remainder are implicitly package access.
The technical term for the situation is "confusing mess", since it is no longer easy to determine where a particular class is stored. While IDEs obviate the problem of lookup, the spec permits compilers to shout at you for doing this. Javac seems to have decided to start doing so, so let's take the hint and clean up our act.

Comment 9

[Chris Kitching [:ckitching]]

Attachment: Part 4: Add hashCode stubs.

The next batch of problems are annoying:

 0:10.08 /home/chris/Fennec/Fennec/mobile/android/base/home/HomeConfig.java:100: warning: [overrides] Class PanelConfig overrides equals, but neither it nor any superclass overrides hashCode method
 0:10.08     public static class PanelConfig implements Parcelable {
 0:10.08                   ^
 0:10.10 error: warnings found and -Werror specified
 0:11.86 /home/chris/Fennec/Fennec/mobile/android/base/sync/repositories/domain/ClientRecord.java:14: warning: [overrides] Class ClientRecord overrides equals, but neither it nor any superclass overrides hashCode method
 0:11.86 public class ClientRecord extends Record {
 0:11.86        ^
 0:11.91 /home/chris/Fennec/Fennec/mobile/android/base/background/db/Tab.java:17: warning: [overrides] Class Tab overrides equals, but neither it nor any superclass overrides hashCode method
 0:11.91 public class Tab {
 0:11.91        ^
 0:12.12 /home/chris/Fennec/Fennec/mobile/android/base/sync/CollectionKeys.java:20: warning: [overrides] Class CollectionKeys overrides equals, but neither it nor any superclass overrides hashCode method
 0:12.12 public class CollectionKeys {
 0:12.12        ^


Javac is correct, but we don't care. These classes aren't used as keys in HashMaps or similar datastructures, and in any case the performance of Java's default hashCode is pretty hard okay: it spits out the memory address of the object. While you can write your own implementation that gets a more uniform spread, that's not something we're going to meaningfully benefit from here (we're only going to care if we end up with big Maps or such).
So, we want to tell javac to shaddaaap.

We could annotate these classes with @SuppressWarnings("overrides"), but that incurs some collateral damage. See:
http://docs.oracle.com/javase/7/docs/technotes/tools/solaris/javac.html
(The section entitled "Warnings That Can Be Enabled or Disabled with -Xlint Option")

Since some of these classes are nontrivial, I'm going to suggest overriding hashCode and just delegating to super to shut the compiler up. The rubbish will be deleted by Proguard anyway, I guess...

With these four patches applied, we succeed in building with JDK 8 and our current Xlint options.

Comment 10

[Chris Kitching [:ckitching]]

Attachment: Part 5: Have configure accept JDK 8.

Well, aside from this, of course.


Feel free to chuck the reviews to rnewman or somebody more appropriate, Nick.

Comment 11

[Chris Kitching [:ckitching]]

(In reply to Chris Kitching [:ckitching] from comment #9)
> Java's default hashCode is pretty hard okay: it spits out the memory address

s/hard//.
I started with "pretty hard to beat" and then decided I wanted to say "pretty okay" and it all went a bit wrong.

*whistles innocently*

Comment 12

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8484027 [details] [diff] [review]
Part 5: Have configure accept JDK 8.

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

wfm.

Comment 13

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8483890 [details] [diff] [review]
Part 1: Cleanup some madness.

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

This patch is just doing a little too much.

There's GeckoEditable (which looks good); sms (which looks surprising -- can we just push the interfaces into the class and make the class public?); and miscellaneous things like s/"\""/'"'/ that I don't care for.  Can we break these down?

nit throughout: the license headers don't follow the rest of the code.

Comment 14

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8484002 [details] [diff] [review]
Part 2: Fix generics in GeckoInputConnection.

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

Yep.

Comment 15

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8484007 [details] [diff] [review]
Part 3: Move GeckoMediaPlayer

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

WFM.

wesj: why crushing nits that prevent compiling with Java 1.8; multiple top-level types in a file is one such nit.  We're generally extracting interfaces like this into top-level files.  If you prefer to fold into MediaPlayerManager, speak up.

::: mobile/android/base/GeckoMediaPlayer.java
@@ +1,4 @@
> +/*
> + * This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/.

nit: header.

@@ +8,5 @@
> +
> +import org.json.JSONObject;
> +import org.mozilla.gecko.util.EventCallback;
> +
> +/* Wraper for different MediaRouter types supproted by Android. i.e. Chromecast, Miracast, etc. */

nit: fix typos and wrap lines.  "Wraper", "supproted".

::: mobile/android/base/moz.build
@@ +199,5 @@
>      'GeckoEvent.java',
>      'GeckoHalDefines.java',
>      'GeckoInputConnection.java',
>      'GeckoJavaSampler.java',
> +    'GeckoMediaPlayer.java',

This should be conditional, like MediaPlayerManager itself.

Comment 16

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8484013 [details] [diff] [review]
Part 4: Add hashCode stubs.

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

In general, I think calling super.hashCode() is odd; I'd rather do better.  I guess this is what's happening already, though.

Some of these will need back-porting to android-sync.

Comment 17

[Chris Kitching [:ckitching]]

(In reply to Nick Alexander :nalexander from comment #16)
> In general, I think calling super.hashCode() is odd; I'd rather do better. 
> I guess this is what's happening already, though.
> 
> Some of these will need back-porting to android-sync.

Yeah, I don't like any of the options I've come up with for this one. See comment 9 for the message javac prints out if we take no action.

We could:

* Add -Xlint:-overrides to disable this and some similar (but more useful!) warnings we want to keep.
* Add @SuppressWarnings("overrides") to the offending classes to disable this category of warning just for those classes (seems strictly better than the first option)
* Do what this patch does: Add three lines of pointless code to the classes to make the warning go away without incurring any collateral damage to warnings we like.
* Something else....

Perhaps you prefer the annotation to the garbage code?

Comment 18

[Chris Kitching [:ckitching]]

Attachment: Part 1a: Move the interface.

(In reply to Nick Alexander :nalexander from comment #13)
> Comment on attachment 8483890 [details] [diff] [review]
> Part 1: Cleanup some madness.
> 
> Review of attachment 8483890 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This patch is just doing a little too much.

> 
> There's GeckoEditable (which looks good); sms (which looks surprising -- can
> we just push the interfaces into the class and make the class public?); and
> miscellaneous things like s/"\""/'"'/ that I don't care for.  

Ah, ew, I didn't spot it doing that. Got a little overzealous with automatically fixing the character literals. It's a pretty trivial microoptimisation, so I really don't care if we don't do it.

Here's the absolute minimum we can get away with to keep the compiler happy: Shift interface into class and make public.

I'll provide the other cleanup as a series of small patches for you to take or drop. Apologies, I saw bad code and reached for the mop.

Comment 19

[Chris Kitching [:ckitching]]

Attachment: Part 1b: Cleanup SmsManager's singleton.

This approach is shorter, smaller, potentially faster (if the optimiser is dumb) and thread-safe (unlike the old approach, where multiple initialisations of sInstance were possible).
The SmsManager class is now effectively an instance-holder class for the singleton.

Comment 20

[Chris Kitching [:ckitching]]

Attachment: Part 1b: Cleanup SmsManager's singleton. (V2)

Now with more qref.

Comment 21

[Chris Kitching [:ckitching]]

Attachment: Part 1c: Replace PendingIntentGuid with an AtomicInteger.

This one seems fairly self-explanatory...

Comment 22

[Chris Kitching [:ckitching]]

Attachment: Part 1d: Correctly detect when the id is too high in GeckoSmsManager.

This one's an obscure bug:

The check at line 600:
http://mxr.mozilla.org/mozilla-aurora/source/mobile/android/base/GeckoSmsManager.java#598

appears to have been written without remembering that integers are signed in Java.
As such, this check will throw for an id exceeding 2^31-1, but if the comment is correct we're really safe up to 2^32-1.

So, let's fix that.

Comment 23

[Chris Kitching [:ckitching]]

Attachment: Part 1e: Make effectively-final fields final.

These fields are effectively-final. Let's make them final.

Also, let's not declare an ArrayList of size zero. That's stupid, because the moment you try and put anything in it it'll have to be reallocated...

Comment 24

[Chris Kitching [:ckitching]]

Attachment: Part 1f: Stop comparing strings with ==...

Comment 25

[Chris Kitching [:ckitching]]

Attachment: Part 1g: Make exception classes static.

Non-static inner classes get a generated field holding a reference to the enclosing instance, plus a constructor which takes this reference as a parameter and assigns it to the field.

That's handy for making inner classes work, but totally pointless for exception classes. Proguard's probably deleting the junk, but the notion of a non-static Exception class doesn't make much sense, either...

Comment 26

[Chris Kitching [:ckitching]]

Attachment: Part 1h: Tidy up some redundant keywords.

Interface methods are implicitly public.
Remove unnecessarily qualified class references.
Return is unnecessary as the last statement in a void function.

Comment 27

[Chris Kitching [:ckitching]]

Attachment: Part 1h: Tidy up some redundant keywords. (V2)

Now with more qref.

Comment 28

[Chris Kitching [:ckitching]]

Attachment: Part 1i: Remove redundant parameters.

These parameters are never used, and can't be optimised out because JNI.

Also, defining a named class inside a method is highly suspect, especially when all createMessageList is really doing is delegating to the runnable...

Comment 29

[Chris Kitching [:ckitching]]

Attachment: Part 1i: Remove redundant parameters. (V2)

Now with more qref.

Comment 30

[Chris Kitching [:ckitching]]

Attachment: Part 1j: Use StringBuilders instead of concatenating in a loop.

Javac desugars string concatenation to StringBuilder.

Unfortunately, it's stupid and doesn't aggregate them in the obvious way.

So something like:

String cake = "hello!";
for (int i = 0; i < 10; i++) {
   cake += "Bobcat: " + i;
}

Ends up looking something like:

String cake = "hello!";
for (int i = 0; i < 10; i++) {
   StringBuilder $0 = new StringBuilder(cake);
   $0.append("Bobcat: ").append(i);
   cake = $0.toString();
}

When we could do better in the obvious way, which is what I'm doing here.

Comment 31

[Chris Kitching [:ckitching]]

Feel free to take or leave the various cleanup patches. As I said, I saw bad code and reached for a mop. I'll land as many as you care enough about to r+.

Comment 32

[Chris Kitching [:ckitching]]

Attachment: Part 1.5: Move GeckoEditable interfaces to satisfy JDK 8.

Oh, and this, of course.

Comment 33

[Chris Kitching [:ckitching]]

https://tbpl.mozilla.org/?tree=Try&rev=eb7faf366d27

Comment 34

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8484722 [details] [diff] [review]
Part 1.5: Move GeckoEditable interfaces to satisfy JDK 8.

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

wfm; r? to nchen in case he cares or knows about a dependency with the C++ side of Gecko.  I see tests that might need to be updated:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/testANRReporter.java#39

and a spot to verify here.  Since no values have changed, this should be okay.

http://mxr.mozilla.org/mozilla-central/source/widget/nsIWidget.h#500

::: mobile/android/base/GeckoEditable.java
@@ +979,2 @@
>          } else {
> +            sb.append(obj);

This is different when obj is null -- one throws, one doesn't.  I doubt this is an issue, but it is a change.

::: mobile/android/base/GeckoEditableClient.java
@@ +1,2 @@
> +/*
> + * This Source Code Form is subject to the terms of the Mozilla Public

It doesn't look like you addressed the comments from the previous review.  I'll mark a few, but please union the sets before landing.

Take the header from http://dxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoApp.java.  It includes emacs mode lines, for example.

::: mobile/android/base/GeckoEditableListener.java
@@ +5,5 @@
> + */
> +
> +package org.mozilla.gecko;
> +
> +/** interface for the Editable to listen to the Gecko thread

Please fix typos and use full sentences when doing code clean-up.

Comment 35

[Chris Kitching [:ckitching]]

(In reply to Nick Alexander :nalexander from comment #34)
> wfm; r? to nchen in case he cares or knows about a dependency with the C++
> side of Gecko.  I see tests that might need to be updated:
> 
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/
> testANRReporter.java#39

Both of the moved interfaces were named classes. This line refers to GeckoEditable$5. That is, the fifth *anonymous* inner class of GeckoEditable, so we're fine.
Also, a try run has been completed:
https://tbpl.mozilla.org/?tree=Try&rev=02a408d9615c

> and a spot to verify here.  Since no values have changed, this should be
> okay.
> 
> http://mxr.mozilla.org/mozilla-central/source/widget/nsIWidget.h#500

Yeah, I didn't touch any of that stuff.

> ::: mobile/android/base/GeckoEditable.java
> @@ +979,2 @@
> >          } else {
> > +            sb.append(obj);
> 
> This is different when obj is null -- one throws, one doesn't.  I doubt this
> is an issue, but it is a change.

Actually, this whole chunk is sort of pointless. It's evil debugging code that's being deleted by the optimiser anyway. I'd question the usefulness of its continued existence (as opposed to having a class full of handy helper functions for that purpose).
So, yes, blech.

> ::: mobile/android/base/GeckoEditableListener.java
> @@ +5,5 @@
> > + */
> > +
> > +package org.mozilla.gecko;
> > +
> > +/** interface for the Editable to listen to the Gecko thread
> 
> Please fix typos and use full sentences when doing code clean-up.

This was just copypasta, but, you're right, since I'm touching it I should tidy it up.

Comment 36

[Chris Kitching [:ckitching]]

Attachment: Part 1.5: Move GeckoEditable interfaces to satisfy JDK 8

Comment 37

[Chris Kitching [:ckitching]]

Attachment: Part 3: Move GeckoMediaPlayer

Comment 38

[Chris Kitching [:ckitching]]

Attachment: Part 3: Move GeckoMediaPlayer

Comment 39

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8485792 [details] [diff] [review]
Part 1.5: Move GeckoEditable interfaces to satisfy JDK 8

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

Over to nchen, just in case.

::: mobile/android/base/GeckoEditable.java
@@ +974,5 @@
>          } else if (obj instanceof CharSequence) {
>              sb.append("\"").append(obj.toString().replace('\n', '\u21b2')).append("\"");
>          } else if (obj.getClass().isArray()) {
>              sb.append(obj.getClass().getComponentType().getSimpleName()).append("[")
> +              .append(Array.getLength(obj)).append(']');

This is odd -- either do them all, or do none.  I'm cool with doing them all, like your earlier patch.

Comment 40

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8484714 [details] [diff] [review]
Part 1j: Use StringBuilders instead of concatenating in a loop.

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

wfm.

::: mobile/android/base/GeckoSmsManager.java
@@ +768,5 @@
>              restrictions.add("date <= " + mEndDate);
>            }
>  
>            if (mNumbersCount > 0) {
> +            StringBuilder numberRestriction = new StringBuilder("address IN ('");

I'm always a |final|-ista, here and below.

Comment 41

[Chris Kitching [:ckitching]]

(In reply to Nick Alexander :nalexander from comment #39)
> ::: mobile/android/base/GeckoEditable.java
> @@ +974,5 @@
> >          } else if (obj instanceof CharSequence) {
> >              sb.append("\"").append(obj.toString().replace('\n', '\u21b2')).append("\"");
> >          } else if (obj.getClass().isArray()) {
> >              sb.append(obj.getClass().getComponentType().getSimpleName()).append("[")
> > +              .append(Array.getLength(obj)).append(']');
> 
> This is odd -- either do them all, or do none.  I'm cool with doing them
> all, like your earlier patch.

Oh, sorry, I misunderstood an earlier comment and thought you'd taken exception to the idea. I won't bother requesting a new review just for that.
Over to nchen indeed...

Comment 42

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8484690 [details] [diff] [review]
Part 1b: Cleanup SmsManager's singleton. (V2)

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

I see what you're doing here, and I think it's an improvement because sInstance is final, but it hurts to force all consumers to check AppConstants.MOZ_WEBSMS_BACKEND.  And in general, I prefer |getInstance| to reaching into a singleton's managed instance.

At the very least, the commit comment and the SmsManager class need to document the contract.

I'm cool with this; rnewman, as resident concurrency guru, do you have a strong feeling?

::: mobile/android/base/SmsManager.java
@@ +4,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  package org.mozilla.gecko;
>  
>  public class SmsManager {

This should be conditional on MOZ_WEBSMS_BACKEND, null if it's not enabled.  (I want the NPE on bad accesses.)

Add a comment explaining the contract.

Comment 43

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8484693 [details] [diff] [review]
Part 1c: Replace PendingIntentGuid with an AtomicInteger.

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

SHIP IT.

Comment 44

[Nick Alexander :nalexander [he/him]]

Actually, now I'm truly confused: the following exists.  Why?

http://mxr.mozilla.org/mozilla-central/source/embedding/android/GeckoSmsManager.java

blassey, can you explain what this shadow copy of Fennec in embedding/android is used for?

Comment 45

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8484696 [details] [diff] [review]
Part 1d: Correctly detect when the id is too high in GeckoSmsManager.

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

::: mobile/android/base/GeckoSmsManager.java
@@ +592,5 @@
>        long id = ContentUris.parseId(uri);
>  
>        // The DOM API takes a 32bits unsigned int for the id. It's unlikely that
>        // we happen to need more than that but it doesn't cost to check.
> +      if (id > UNSIGNED_INTEGER_MAX_VALUE) {

Curiously, this doesn't really show that it's 32bits.  I leave it to you to decide if this is better than |2**32 - 1|.

Comment 46

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8484697 [details] [diff] [review]
Part 1e: Make effectively-final fields final.

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

Yay for final!

Comment 47

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8484698 [details] [diff] [review]
Part 1f: Stop comparing strings with ==...

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

So, clearly this code isn't being used.  I wonder if we should be deleting it entirely?  Some other day.

Comment 48

[Chris Kitching [:ckitching]]

(In reply to Nick Alexander :nalexander from comment #47)
> So, clearly this code isn't being used.  I wonder if we should be deleting
> it entirely?  Some other day.

It certainly, at the very least, raises some interesting questions about our test coverage.

Comment 49

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8484706 [details] [diff] [review]
Part 1h: Tidy up some redundant keywords. (V2)

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

Make sure this is 1.7 and 1.8 compatible, please.  If your early try already built this, roll on.

::: mobile/android/base/GeckoSmsManager.java
@@ +344,5 @@
>    private final static int kMessageClassClass1  = 2;
>    private final static int kMessageClassClass2  = 3;
>    private final static int kMessageClassClass3  = 4;
>  
> +  private final static String[] kRequiredMessageRows = { "_id", "address", "body", "date", "type", "status" };

Huh, I didn't know this was legal.

Comment 50

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8484711 [details] [diff] [review]
Part 1i: Remove redundant parameters. (V2)

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

I think this is technically correct, but let's not do this just to do it.  I'd rather these JNI interfaces stay stable for as long as possible.

Comment 51

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8485798 [details] [diff] [review]
Part 3: Move GeckoMediaPlayer

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

wfm if it works for you.  Thanks for f+, wesj.

Comment 52

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8484711 [details] [diff] [review]
Part 1i: Remove redundant parameters. (V2)

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

ckitching and I discussed this on IRC; I have no problem landing it, I just don't feel confident reviewing it.

Comment 53

[Chris Kitching [:ckitching]]

Comment on attachment 8484711 [details] [diff] [review]
Part 1i: Remove redundant parameters. (V2)

nchen: What do you think of the JNI interface changes here? Got a good try run, I can haz cleanup approval?

Comment 54

[Chris Kitching [:ckitching]]

(In reply to Nick Alexander :nalexander from comment #42)
> I see what you're doing here, and I think it's an improvement because
> sInstance is final, but it hurts to force all consumers to check
> AppConstants.MOZ_WEBSMS_BACKEND.  

Why does that hurt? It's basically an IFDEF.

I think this is better than the old approach because:

* getInstance() returning null when the flag isn't set and consumers checking for null doesn't make it immediately obvious why we're getting a null.
* Unlike checking the flag, the null check is slightly less trivial to optimise out, so might survive into bytecode.

> And in general, I prefer |getInstance| to
> reaching into a singleton's managed instance.

We can always put in getInstance as a trivial getter. It'll just get inlined anyway.

> At the very least, the commit comment and the SmsManager class need to
> document the contract.
> 
> I'm cool with this; rnewman, as resident concurrency guru, do you have a
> strong feeling?

I should add: This approach is thread safe. The old approach wasn't, because multiple calls to getInstance could race to perform initialisation.

The thread-safety of this approach is guaranteed by a fun property of the JVM specification: that <clinit> (the virtual static method run by the classloader when a class is loaded, which contains the concatenation of all your static blocks and initialisers) is guaranteed to be run exactly once, on one thread, and nothing else can interact with the class until it's done.


I should possibly add that this isn't a concept I just pulled out thin air, it's A Thing (tm):
http://javarevisited.blogspot.com/2012/12/how-to-create-thread-safe-singleton-in-java-example.html

Comment 55

[Richard Newman [:rnewman]]

Comment on attachment 8484690 [details] [diff] [review]
Part 1b: Cleanup SmsManager's singleton. (V2)

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

Good enough. Agree with nalexander's point about sInstance = null.

I also don't like forcing callers to check MOZ_WEBSMS_BACKEND, but there's a limited set of callers and it's more efficient this way, so whatevs.

::: mobile/android/base/GeckoAppShell.java
@@ +2366,1 @@
>              return;

Not a comment on this patch, but: boy, this seems totally wrong, doesn't it?

Comment 56

[Chris Kitching [:ckitching]]

(In reply to Richard Newman [:rnewman] from comment #55)
> Good enough. Agree with nalexander's point about sInstance = null.
> 
> I also don't like forcing callers to check MOZ_WEBSMS_BACKEND, but there's a
> limited set of callers and it's more efficient this way, so whatevs.

I think we could equally-efficiently bundle it up in a method that just return MOZ_WEBSMS_BACKEND. SmsManager.isEnabled() or something.
Proguard would still be able to inline MOZ_WEBSMS_BACKEND, then inline the method, then delete the check.

> ::: mobile/android/base/GeckoAppShell.java
> @@ +2366,1 @@
> >              return;
> 
> Not a comment on this patch, but: boy, this seems totally wrong, doesn't it?

It's a web browser with a JNI binding for an SMS system... called from Javascript.
"wrongness" is relative.


But, yes, it is a bit weird. These methods are being compiled in as empty stubs when SMS is turned off. *shrug*

Comment 57

[Chris Kitching [:ckitching]]

Attachment: Part 1b: Cleanup singleton use in SmsManager

Sugar-coated it a bit. Looking better?

Comment 58

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8484711 [details] [diff] [review]
Part 1i: Remove redundant parameters. (V2)

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

Patch looks good, but I have a feeling that we should "make use of these arguments" rather than "eliminate these arguments". Bug 878533 specifically added these arguments, implying that they do have a purpose. It might be an oversight that these arguments ended up unused. I would ask somebody that works on WebSMS about it. r+ from me if they're okay with removing these arguments.

Comment 59

[Chris Kitching [:ckitching]]

(In reply to Jim Chen [:jchen :nchen] from comment #58)
> Comment on attachment 8484711 [details] [diff] [review]
> Part 1i: Remove redundant parameters. (V2)
> 
> Review of attachment 8484711 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Patch looks good, but I have a feeling that we should "make use of these
> arguments" rather than "eliminate these arguments". Bug 878533 specifically
> added these arguments, implying that they do have a purpose. It might be an
> oversight that these arguments ended up unused. I would ask somebody that
> works on WebSMS about it. r+ from me if they're okay with removing these
> arguments.

Sounds good. I'll file a followup so we don't block the other good patches on this speculative one.

Comment 60

[Chris Kitching [:ckitching]]

https://hg.mozilla.org/integration/fx-team/rev/082f94edf57b
https://hg.mozilla.org/integration/fx-team/rev/6177ae59bd24
https://hg.mozilla.org/integration/fx-team/rev/d2d463e509c3
https://hg.mozilla.org/integration/fx-team/rev/e6163044265f
https://hg.mozilla.org/integration/fx-team/rev/9b6cadbcb21c
https://hg.mozilla.org/integration/fx-team/rev/a8637aba08ca
https://hg.mozilla.org/integration/fx-team/rev/d2791c6007d9
https://hg.mozilla.org/integration/fx-team/rev/ceb1265224fd
https://hg.mozilla.org/integration/fx-team/rev/578355f316ca
https://hg.mozilla.org/integration/fx-team/rev/eedab31c66d4
https://hg.mozilla.org/integration/fx-team/rev/a2e71b3f12b1
https://hg.mozilla.org/integration/fx-team/rev/6bf8f5121c9b
https://hg.mozilla.org/integration/fx-team/rev/f4ebee1534d2
https://hg.mozilla.org/integration/fx-team/rev/3b82fdb751e0
https://hg.mozilla.org/integration/fx-team/rev/8dc3f8821953

Comment 61

[Brad Lassey [:blassey] (use needinfo?)]

Vicamo, see comment 44

Comment 62

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/082f94edf57b
https://hg.mozilla.org/mozilla-central/rev/6177ae59bd24
https://hg.mozilla.org/mozilla-central/rev/d2d463e509c3
https://hg.mozilla.org/mozilla-central/rev/e6163044265f
https://hg.mozilla.org/mozilla-central/rev/9b6cadbcb21c
https://hg.mozilla.org/mozilla-central/rev/a8637aba08ca
https://hg.mozilla.org/mozilla-central/rev/d2791c6007d9
https://hg.mozilla.org/mozilla-central/rev/ceb1265224fd
https://hg.mozilla.org/mozilla-central/rev/578355f316ca
https://hg.mozilla.org/mozilla-central/rev/eedab31c66d4
https://hg.mozilla.org/mozilla-central/rev/a2e71b3f12b1
https://hg.mozilla.org/mozilla-central/rev/6bf8f5121c9b
https://hg.mozilla.org/mozilla-central/rev/f4ebee1534d2
https://hg.mozilla.org/mozilla-central/rev/3b82fdb751e0
https://hg.mozilla.org/mozilla-central/rev/8dc3f8821953

Comment 63

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Nick Alexander :nalexander from comment #44)
> Actually, now I'm truly confused: the following exists.  Why?
> 
> http://mxr.mozilla.org/mozilla-central/source/embedding/android/
> GeckoSmsManager.java
> 
> blassey, can you explain what this shadow copy of Fennec in
> embedding/android is used for?

Hi Nick, Blassey,

In bug 742790 comment 36 (thanks Google), Mounir had suggested that we may remove those duplicated lines in embedding/. But I also remember that I was requested not to remove them somewhere I can't recall. Anyway, bug 1064656 filed for this.