From 1836a63e9286c176b2323b38314f51511a797688 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 0d21a064a638c62037205cdc8516abfa32c30308 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 110cb12040f1972062a6cff86e29c05b284d60b3 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(); } } }