From 221019a4c6ad56037e939c1fbfbbb045b09c213a Mon Sep 17 00:00:00 2001 From: Chalard Jean Date: Fri, 8 Jun 2018 19:39:24 +0900 Subject: [PATCH 1/3] [PT14] No-op refactoring of sendProxyBroadcast If mGlobalProxy is non-null, then getDefaultProxy returns mGlobalProxy so the first change is a no-op. If mGlobalProxy is null and mDefaultProxyEnabled is true, then getDefaultProxy returns mDefaultProxy, which has just been set to proxyInfo, so the second change is a no-op. If mGlobalProxy is null and mDefaultProxyEnabled is true, then getDefaultProxy returns mDefaultProxy ; if mGlobalProxy is null and mDefaultProxyEnabled is false, then getDefaultProxy returns null, therefore the third change is a no-op. Test: runtest Change-Id: I7c21062302bf54f4fc917c82e0175975051a55ec --- .../android/server/connectivity/ProxyTracker.java | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/services/core/java/com/android/server/connectivity/ProxyTracker.java b/services/core/java/com/android/server/connectivity/ProxyTracker.java index 47e85b5fd3..a90bba00c0 100644 --- a/services/core/java/com/android/server/connectivity/ProxyTracker.java +++ b/services/core/java/com/android/server/connectivity/ProxyTracker.java @@ -126,9 +126,9 @@ public class ProxyTracker { public ProxyInfo getDefaultProxy() { // This information is already available as a world read/writable jvm property. synchronized (mProxyLock) { - final ProxyInfo ret = mGlobalProxy; - if ((ret == null) && mDefaultProxyEnabled) return mDefaultProxy; - return ret; + if (mGlobalProxy != null) return mGlobalProxy; + if (mDefaultProxyEnabled) return mDefaultProxy; + return null; } } @@ -296,14 +296,14 @@ public class ProxyTracker { && (!Uri.EMPTY.equals(proxyInfo.getPacFileUrl())) && proxyInfo.getPacFileUrl().equals(mGlobalProxy.getPacFileUrl())) { mGlobalProxy = proxyInfo; - sendProxyBroadcast(mGlobalProxy); + sendProxyBroadcast(getDefaultProxy()); return; } mDefaultProxy = proxyInfo; if (mGlobalProxy != null) return; if (mDefaultProxyEnabled) { - sendProxyBroadcast(proxyInfo); + sendProxyBroadcast(getDefaultProxy()); } } } @@ -320,7 +320,7 @@ public class ProxyTracker { if (mDefaultProxyEnabled != enabled) { mDefaultProxyEnabled = enabled; if (mGlobalProxy == null && mDefaultProxy != null) { - sendProxyBroadcast(enabled ? mDefaultProxy : null); + sendProxyBroadcast(getDefaultProxy()); } } } From 9be9a4b6ef055a9933b189111d9b50db8fcee09c Mon Sep 17 00:00:00 2001 From: Chalard Jean Date: Fri, 8 Jun 2018 19:45:27 +0900 Subject: [PATCH 2/3] [PT15] Tiny bugfix in setGlobalProxy This bug has existed for a long time. If mDefaultProxyEnabled is false, then the mDefaultProxy member should obviously not be used in the broadcast. Test: runtest Change-Id: I599a2ff9f96d4667e824cf000c2125f86010bb02 --- .../core/java/com/android/server/connectivity/ProxyTracker.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/services/core/java/com/android/server/connectivity/ProxyTracker.java b/services/core/java/com/android/server/connectivity/ProxyTracker.java index a90bba00c0..de8ae35226 100644 --- a/services/core/java/com/android/server/connectivity/ProxyTracker.java +++ b/services/core/java/com/android/server/connectivity/ProxyTracker.java @@ -269,7 +269,7 @@ public class ProxyTracker { Binder.restoreCallingIdentity(token); } - sendProxyBroadcast(mGlobalProxy == null ? mDefaultProxy : proxyInfo); + sendProxyBroadcast(getDefaultProxy()); } } From 07c5bab28d07692dd6a0a0edf5e042ec4a03d215 Mon Sep 17 00:00:00 2001 From: Chalard Jean Date: Fri, 8 Jun 2018 19:46:44 +0900 Subject: [PATCH 3/3] [PT16] Simplification of sendProxyBroadcast. sendProxyBroadcast is always called with the same argument, and it would make no sense with another argument anyway. Remove it. This concludes the ProxyTracker refactoring with 227 lines removed from ConnectivityService, a lot clarified, and some bugs removed. Things can still be improved, but presumably at a much higher cost. Next steps are : write tests, now that ProxyTracker is both testable and mockable. And try to pour some gasoline on the PROXY_CHANGE_ACTION broadcast, see if it burns well. Test: runtest Change-Id: I66e40b4bf5cfd0b2dc4fa37ea97b3429fe1b7e6c --- .../com/android/server/ConnectivityService.java | 2 +- .../android/server/connectivity/ProxyTracker.java | 15 +++++++-------- 2 files changed, 8 insertions(+), 9 deletions(-) diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index 4b77c69aba..d9394e810e 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -3478,7 +3478,7 @@ public class ConnectivityService extends IConnectivityManager.Stub ProxyInfo oldProxyInfo = oldLp == null ? null : oldLp.getHttpProxy(); if (!ProxyTracker.proxyInfoEqual(newProxyInfo, oldProxyInfo)) { - mProxyTracker.sendProxyBroadcast(mProxyTracker.getDefaultProxy()); + mProxyTracker.sendProxyBroadcast(); } } diff --git a/services/core/java/com/android/server/connectivity/ProxyTracker.java b/services/core/java/com/android/server/connectivity/ProxyTracker.java index de8ae35226..15468ff05c 100644 --- a/services/core/java/com/android/server/connectivity/ProxyTracker.java +++ b/services/core/java/com/android/server/connectivity/ProxyTracker.java @@ -204,11 +204,10 @@ public class ProxyTracker { * * Confusingly this method also sets the PAC file URL. TODO : separate this, it has nothing * to do in a "sendProxyBroadcast" method. - * @param proxyInfo the proxy spec, or null for no proxy. */ - // TODO : make the argument NonNull final and the method private - public void sendProxyBroadcast(@Nullable ProxyInfo proxyInfo) { - if (proxyInfo == null) proxyInfo = new ProxyInfo("", 0, ""); + public void sendProxyBroadcast() { + final ProxyInfo defaultProxy = getDefaultProxy(); + final ProxyInfo proxyInfo = null != defaultProxy ? defaultProxy : new ProxyInfo("", 0, ""); if (mPacManager.setCurrentProxyScriptUrl(proxyInfo)) return; if (DBG) Slog.d(TAG, "sending Proxy Broadcast for " + proxyInfo); Intent intent = new Intent(Proxy.PROXY_CHANGE_ACTION); @@ -269,7 +268,7 @@ public class ProxyTracker { Binder.restoreCallingIdentity(token); } - sendProxyBroadcast(getDefaultProxy()); + sendProxyBroadcast(); } } @@ -296,14 +295,14 @@ public class ProxyTracker { && (!Uri.EMPTY.equals(proxyInfo.getPacFileUrl())) && proxyInfo.getPacFileUrl().equals(mGlobalProxy.getPacFileUrl())) { mGlobalProxy = proxyInfo; - sendProxyBroadcast(getDefaultProxy()); + sendProxyBroadcast(); return; } mDefaultProxy = proxyInfo; if (mGlobalProxy != null) return; if (mDefaultProxyEnabled) { - sendProxyBroadcast(getDefaultProxy()); + sendProxyBroadcast(); } } } @@ -320,7 +319,7 @@ public class ProxyTracker { if (mDefaultProxyEnabled != enabled) { mDefaultProxyEnabled = enabled; if (mGlobalProxy == null && mDefaultProxy != null) { - sendProxyBroadcast(getDefaultProxy()); + sendProxyBroadcast(); } } }