android: Use new proxy from PROXY_CHANGE intent

net/android/java/src/org/chromium/net/ProxyChangeListener.java

Index: net/android/java/src/org/chromium/net/ProxyChangeListener.java
diff --git a/net/android/java/src/org/chromium/net/ProxyChangeListener.java b/net/android/java/src/org/chromium/net/ProxyChangeListener.java
index 9c59bcccf6df36c4ed2032a3d9b517656a89fb3d..8d19422412e69399337af617bd18e6c436b07472 100644
--- a/net/android/java/src/org/chromium/net/ProxyChangeListener.java
+++ b/net/android/java/src/org/chromium/net/ProxyChangeListener.java
@@ -9,6 +9,10 @@ import android.content.Context;
 import android.content.Intent;
 import android.content.IntentFilter;
 import android.net.Proxy;
+import android.util.Log;
+
+import java.lang.reflect.Method;
+import java.util.Set;

[Philippe, 2013/10/15 14:33:21]

Nit: nor that one.

[Elly Fong-Jones, 2013/10/15 16:37:15]

Done.

 
 import org.chromium.base.CalledByNative;
 import org.chromium.base.JNINamespace;
@@ -26,6 +30,15 @@ public class ProxyChangeListener {
     private ProxyReceiver mProxyReceiver;
     private Delegate mDelegate;
 
+    private class ProxyConfig {

[Philippe, 2013/10/15 14:33:21]

Nit: you can make this 'private static class' since you never reference the
outer ProxyChangeListener instance in this inner class.

[Elly Fong-Jones, 2013/10/15 16:37:15]

Done.

+        public ProxyConfig(String host, int port) {
+            host_ = host;
+            port_ = port;
+        }
+        public String host_;

[cbentzel, 2013/10/15 12:29:07]

Nit: Java uses mHost rather than host_;

FindBugs may be complaining because these fields aren't private and get/set via
accessor methods.

[Philippe, 2013/10/15 14:33:21]

+1

Also consider either making those fields 'final' or dropping the constructor (I
would be more in favor of making them final personally).

[Elly Fong-Jones, 2013/10/15 16:37:15]

Done.

+        public int port_;
+    };
+
     public interface Delegate {
         public void proxySettingsChanged();
     }
@@ -66,15 +79,50 @@ public class ProxyChangeListener {
     }
 
     private class ProxyReceiver extends BroadcastReceiver {
+

[Philippe, 2013/10/15 14:33:21]

Nit: other places in this file don't have this blank line.

[Elly Fong-Jones, 2013/10/15 16:37:15]

Done.

         @Override
         public void onReceive(Context context, Intent intent) {
             if (intent.getAction().equals(Proxy.PROXY_CHANGE_ACTION)) {
-                proxySettingsChanged();
+                ProxyConfig cfg = extractNewProxy(intent);
+                if (cfg != null) {
+                    proxySettingsChanged(cfg);
+                }
+            }
+        }
+
+        // Extract a ProxyConfig object from the supplied Intent's extra data
+        // bundle. The android.net.ProxyProperties class is not exported from
+        // tne Android SDK, so we have to use reflection to get at it and invoke
+        // methods on it.
+        // TODO(ellyjones): once android.net.ProxyProperties is exported,
+        // rewrite this.
+        private ProxyConfig extractNewProxy(Intent intent) {

[Philippe, 2013/10/15 14:33:21]

Nit: you can make this method 'static' if you never reference 'this'. This will
mainly improve readability since there is no way in Java to enforce immutability
of the instance (other than making the method 'static' which seems to be
possible here).

[Elly Fong-Jones, 2013/10/15 16:37:15]

Alas:

../net/android/java/src/org/chromium/net/ProxyChangeListener.java:96: inner
classes cannot have static declarations

[Philippe, 2013/10/15 16:45:59]

Wow, didn't know about that!

+            try {
+                final String kClassName = "android.net.ProxyProperties";

[Philippe, 2013/10/15 14:33:21]

Nit: I believe we use capital letters for constants.

[Elly Fong-Jones, 2013/10/15 16:37:15]

Done.

+                final String kGetHostName = "getHost";
+                final String kGetPortName = "getPort";
+                Object props = intent.getExtras().get("proxy");
+                if (props == null) {
+                    return null;
+                }
+                Class cls = Class.forName(kClassName);
+                Method gethost = cls.getDeclaredMethod(kGetHostName);

[Philippe, 2013/10/15 14:33:21]

Nit: s/gethost/getHostMethod maybe (same below).

[Elly Fong-Jones, 2013/10/15 16:37:15]

Done.

+                Method getport = cls.getDeclaredMethod(kGetPortName);
+
+                Object host_obj = gethost.invoke(props);

[Philippe, 2013/10/15 14:33:21]

Nit: feel free to drop these two lines (that shouldn't use '_' for identifiers)
and directly write below:

String host = (String) getHostMethod.invoke(props);
int port = (Integer) getPortMethod.invoke(props);
return new ProxyConfig(host, port);

[Elly Fong-Jones, 2013/10/15 16:37:15]

Done.

+                Object port_obj = getport.invoke(props);
+
+                String host = (String)host_obj;
+                int port = ((Integer)port_obj);
+
+                return new ProxyConfig(host, port);
+            } catch (Exception ex) {
+                return null;
             }
         }
     }
 
-    private void proxySettingsChanged() {
+    private void proxySettingsChanged(ProxyConfig cfg) {
         if (!sEnabled) {
             return;
         }
@@ -86,7 +134,7 @@ public class ProxyChangeListener {
         }
         // Note that this code currently runs on a MESSAGE_LOOP_UI thread, but
         // the C++ code must run the callbacks on the network thread.
-        nativeProxySettingsChanged(mNativePtr);
+        nativeProxySettingsChanged(mNativePtr, cfg);
     }
 
     private void registerReceiver() {
@@ -111,5 +159,6 @@ public class ProxyChangeListener {
      * See net/proxy/proxy_config_service_android.cc
      */
     @NativeClassQualifiedName("ProxyConfigServiceAndroid::JNIDelegate")
-    private native void nativeProxySettingsChanged(int nativePtr);
+    private native void nativeProxySettingsChanged(int nativePtr,
+                                                   ProxyConfig cfg);

[Philippe, 2013/10/15 14:33:21]

We only pass PODs or Java "primitive" types (e.g. String) to JNI. You can make
this method take the host as a string and the port as an int. This will let you
get rid of the reflection code in the .cc file (that we heavily discourage).
This will also help you get rid of the findbugs warnings :)

[Elly Fong-Jones, 2013/10/15 16:37:15]

Done.

 }