android: Use new proxy from PROXY_CHANGE intent

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

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 package org.chromium.net;
 
 import android.content.BroadcastReceiver;
 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;
 import org.chromium.base.NativeClassQualifiedName;
 
 // This class partners with native ProxyConfigServiceAndroid to listen for
 // proxy change notifications from Android.
 @JNINamespace("net")
 public class ProxyChangeListener {
     private static final String TAG = "ProxyChangeListener";
     private static boolean sEnabled = true;
 
     private int mNativePtr;
     private Context mContext;
     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();
     }
 
     private ProxyChangeListener(Context context) {
         mContext = context;
     }
 
     public static void setEnabled(boolean enabled) {
         sEnabled = enabled;
@@ skipping 20 matching lines @@
         registerReceiver();
     }
 
     @CalledByNative
     public void stop() {
         mNativePtr = 0;
         unregisterReceiver();
     }
 
     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;
         }
         if (mDelegate != null) {
           mDelegate.proxySettingsChanged();
         }
         if (mNativePtr == 0) {
           return;
         }
         // 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() {
         if (mProxyReceiver != null) {
             return;
         }
         IntentFilter filter = new IntentFilter();
         filter.addAction(Proxy.PROXY_CHANGE_ACTION);
         mProxyReceiver = new ProxyReceiver();
         mContext.getApplicationContext().registerReceiver(mProxyReceiver, filter);
     }
 
     private void unregisterReceiver() {
         if (mProxyReceiver == null) {
             return;
         }
         mContext.unregisterReceiver(mProxyReceiver);
         mProxyReceiver = null;
     }
 
     /**
      * 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.

 }