From bdd3d3c24b4eebb7af602d508e9210ecb997801b Mon Sep 17 00:00:00 2001 From: Fabian Kozynski Date: Tue, 5 Mar 2019 13:36:26 -0500 Subject: [PATCH] Removed unnecessary calls and creations of Binder The previous behavior registered HotspotController to the WifiManager every time a callback was added in order to trigger an update in the number of current connections. This caused two things: * Every registering with WifiManager creates a new Binder object (see WifiManager#registerSoftApCallback) * Every callback added triggered Callback#onHotspotChanged on all the current callbacks This removes both behaviors while maintaining the information sent to the callback on addCallback by posting an update. We assume that the information currently in HotspotController is up to date, if not, there would be a callback. Test: atest Test: manual (opening and closing QS) Fixes: 123546508 Change-Id: Ica73082beb752a7c20e11b2b2a441fa84bf948a1 --- .../policy/HotspotControllerImpl.java | 42 +++---- .../policy/HotspotControllerImplTest.java | 116 ++++++++++++++++++ 2 files changed, 135 insertions(+), 23 deletions(-) create mode 100644 packages/SystemUI/tests/src/com/android/systemui/statusbar/policy/HotspotControllerImplTest.java diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/policy/HotspotControllerImpl.java b/packages/SystemUI/src/com/android/systemui/statusbar/policy/HotspotControllerImpl.java index b561ac1664c09..db2be0e78e9a3 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/policy/HotspotControllerImpl.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/policy/HotspotControllerImpl.java @@ -93,14 +93,28 @@ public class HotspotControllerImpl implements HotspotController, WifiManager.Sof return null; } + /** + * Adds {@code callback} to the controller. The controller will update the callback on state + * changes. It will immediately trigger the callback added to notify current state. + * @param callback + */ @Override public void addCallback(Callback callback) { synchronized (mCallbacks) { if (callback == null || mCallbacks.contains(callback)) return; if (DEBUG) Log.d(TAG, "addCallback " + callback); mCallbacks.add(callback); - - updateWifiStateListeners(!mCallbacks.isEmpty()); + if (mWifiManager != null) { + if (mCallbacks.size() == 1) { + mWifiManager.registerSoftApCallback(this, mMainHandler); + } else { + // mWifiManager#registerSoftApCallback triggers a call to onNumClientsChanged + // on the Main Handler. In order to always update the callback on added, we + // make this call when adding callbacks after the first. + mMainHandler.post(() -> + callback.onHotspotChanged(isHotspotEnabled(), mNumConnectedDevices)); + } + } } } @@ -110,27 +124,9 @@ public class HotspotControllerImpl implements HotspotController, WifiManager.Sof if (DEBUG) Log.d(TAG, "removeCallback " + callback); synchronized (mCallbacks) { mCallbacks.remove(callback); - updateWifiStateListeners(!mCallbacks.isEmpty()); - } - } - - /** - * Updates the wifi state receiver to either start or stop listening to get updates to the - * hotspot status. Additionally starts listening to wifi manager state to track the number of - * connected devices. - * - * @param shouldListen whether we should start listening to various wifi statuses - */ - private void updateWifiStateListeners(boolean shouldListen) { - if (mWifiManager == null) { - return; - } - if (shouldListen) { - mWifiManager.registerSoftApCallback( - this, - mMainHandler); - } else { - mWifiManager.unregisterSoftApCallback(this); + if (mCallbacks.isEmpty() && mWifiManager != null) { + mWifiManager.unregisterSoftApCallback(this); + } } } diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/policy/HotspotControllerImplTest.java b/packages/SystemUI/tests/src/com/android/systemui/statusbar/policy/HotspotControllerImplTest.java new file mode 100644 index 0000000000000..3e4c4d6a7a8a9 --- /dev/null +++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/policy/HotspotControllerImplTest.java @@ -0,0 +1,116 @@ +/* + * Copyright (C) 2019 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.android.systemui.statusbar.policy; + +import static org.mockito.ArgumentMatchers.anyBoolean; +import static org.mockito.ArgumentMatchers.anyInt; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.any; +import static org.mockito.Mockito.doAnswer; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; + +import android.net.ConnectivityManager; +import android.net.wifi.WifiManager; +import android.os.Handler; +import android.testing.AndroidTestingRunner; +import android.testing.TestableLooper; + +import androidx.test.filters.SmallTest; + +import com.android.systemui.SysuiTestCase; + +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.Mock; +import org.mockito.MockitoAnnotations; +import org.mockito.invocation.InvocationOnMock; + +@SmallTest +@RunWith(AndroidTestingRunner.class) +@TestableLooper.RunWithLooper +public class HotspotControllerImplTest extends SysuiTestCase { + + @Mock + private ConnectivityManager mConnectivityManager; + @Mock + private WifiManager mWifiManager; + @Mock + private HotspotController.Callback mCallback1; + @Mock + private HotspotController.Callback mCallback2; + private HotspotControllerImpl mController; + private TestableLooper mLooper; + + @Before + public void setUp() { + MockitoAnnotations.initMocks(this); + mLooper = TestableLooper.get(this); + + mContext.addMockSystemService(ConnectivityManager.class, mConnectivityManager); + mContext.addMockSystemService(WifiManager.class, mWifiManager); + + doAnswer((InvocationOnMock invocation) -> { + ((WifiManager.SoftApCallback) invocation.getArgument(0)).onNumClientsChanged(1); + return null; + }).when(mWifiManager).registerSoftApCallback(any(WifiManager.SoftApCallback.class), + any(Handler.class)); + + mController = new HotspotControllerImpl(mContext, new Handler(mLooper.getLooper())); + } + + @Test + public void testAddingTwoCallbacksRegistersToWifiManagerOnce() { + mController.addCallback(mCallback1); + mController.addCallback(mCallback2); + + verify(mWifiManager, times(1)).registerSoftApCallback(eq(mController), any()); + } + + @Test + public void testAddingCallbacksDoesntUpdateAll() { + mController.addCallback(mCallback1); + mController.addCallback(mCallback2); + + mLooper.processAllMessages(); + // Each callback should be updated only once + verify(mCallback1, times(1)).onHotspotChanged(anyBoolean(), anyInt()); + verify(mCallback2, times(1)).onHotspotChanged(anyBoolean(), anyInt()); + } + + @Test + public void testRemovingTwoCallbacksUnegistersToWifiManagerOnce() { + mController.addCallback(mCallback1); + mController.addCallback(mCallback2); + + mController.removeCallback(mCallback1); + mController.removeCallback(mCallback2); + + verify(mWifiManager, times(1)).unregisterSoftApCallback(mController); + } + + @Test + public void testDoNotUnregisterIfRemainingCallbacks() { + mController.addCallback(mCallback1); + mController.addCallback(mCallback2); + + verify(mWifiManager, never()).unregisterSoftApCallback(any()); + } + +}