From 76df467cca532f8b560e790735ca9a03d3c5338a Mon Sep 17 00:00:00 2001 From: Neil Fuller Date: Fri, 12 Nov 2021 14:32:20 +0000 Subject: [PATCH] Remove geo suggestion clearing Remove strategy clearing / ignoring geo suggestions within TimeZoneDetectorStrategyImpl when geo detection is disabled / the user is changed. The LocationTimeZoneManager's ControllerImpl withdraws suggestions in these situations anyway. Removing the duplicated functionality from the strategy helps avoid races (i.e. if the TimeZoneDetectorStrategy and LocationTimeZoneManager receive the same "something has changed" signal in an ill-defined order it could lead to suggestions being "missed"). This should open up opportunities for future droidfood testing (i.e. the ability to run geo detection "in the background" and record / report them in metrics during QA, bug 200279201). Test: atest services/tests/servicestests/src/com/android/server/timezonedetector/ Test: atest cts/hostsidetests/time/host/src/android/time/cts/host/ Bug: 200279201 Bug: 197624972 Change-Id: I2e5ea8482216ef204cf2e7ad412eeb0f501e36b9 --- .../TimeZoneDetectorStrategyImpl.java | 44 +++++++---------- .../TimeZoneDetectorStrategyImplTest.java | 48 ++++--------------- 2 files changed, 24 insertions(+), 68 deletions(-) diff --git a/services/core/java/com/android/server/timezonedetector/TimeZoneDetectorStrategyImpl.java b/services/core/java/com/android/server/timezonedetector/TimeZoneDetectorStrategyImpl.java index 1ebb983eef576..1b09f441a87eb 100644 --- a/services/core/java/com/android/server/timezonedetector/TimeZoneDetectorStrategyImpl.java +++ b/services/core/java/com/android/server/timezonedetector/TimeZoneDetectorStrategyImpl.java @@ -289,18 +289,19 @@ public final class TimeZoneDetectorStrategyImpl implements TimeZoneDetectorStrat } Objects.requireNonNull(suggestion); - if (currentUserConfig.getGeoDetectionEnabledBehavior()) { - // Only store a geolocation suggestion if geolocation detection is currently enabled. - // See also handleConfigChanged(), which can clear mLatestGeoLocationSuggestion. - // The suggestion's "effective from" time is ignored: we currently assume suggestions - // are made in a sensible order and the most recent is always the best one to use. - mLatestGeoLocationSuggestion.set(suggestion); + // Geolocation suggestions may be stored but not used during time zone detection if the + // configuration doesn't have geo time zone detection enabled. The caller is expected to + // withdraw a previous suggestion (i.e. submit an "uncertain" suggestion, when geo time zone + // detection is disabled. - // Now perform auto time zone detection. The new suggestion may be used to modify the - // time zone setting. - String reason = "New geolocation time zone suggested. suggestion=" + suggestion; - doAutoTimeZoneDetection(currentUserConfig, reason); - } + // The suggestion's "effective from" time is ignored: we currently assume suggestions + // are made in a sensible order and the most recent is always the best one to use. + mLatestGeoLocationSuggestion.set(suggestion); + + // Now perform auto time zone detection. The new suggestion may be used to modify the + // time zone setting. + String reason = "New geolocation time zone suggested. suggestion=" + suggestion; + doAutoTimeZoneDetection(currentUserConfig, reason); } @Override @@ -365,10 +366,8 @@ public final class TimeZoneDetectorStrategyImpl implements TimeZoneDetectorStrat // Now perform auto time zone detection. The new suggestion may be used to modify the time // zone setting. - if (!currentUserConfig.getGeoDetectionEnabledBehavior()) { - String reason = "New telephony time zone suggested. suggestion=" + suggestion; - doAutoTimeZoneDetection(currentUserConfig, reason); - } + String reason = "New telephony time zone suggested. suggestion=" + suggestion; + doAutoTimeZoneDetection(currentUserConfig, reason); } @Override @@ -427,7 +426,8 @@ public final class TimeZoneDetectorStrategyImpl implements TimeZoneDetectorStrat return; } - // Use the correct algorithm based on the user's current configuration. + // Use the correct algorithm based on the user's current configuration. If it changes, then + // detection will be re-run. if (currentUserConfig.getGeoDetectionEnabledBehavior()) { doGeolocationTimeZoneDetection(detectionReason); } else { @@ -605,18 +605,6 @@ public final class TimeZoneDetectorStrategyImpl implements TimeZoneDetectorStrat ConfigurationInternal currentUserConfig = mEnvironment.getConfigurationInternal(currentUserId); - GeolocationTimeZoneSuggestion latestGeoLocationSuggestion = - mLatestGeoLocationSuggestion.get(); - if (latestGeoLocationSuggestion != null - && !currentUserConfig.getGeoDetectionEnabledBehavior()) { - // The current user's config has geodetection disabled, so clear the latest suggestion. - // This is done to ensure we only ever keep a geolocation suggestion if the user has - // said it is ok to do so. - mLatestGeoLocationSuggestion.set(null); - mTimeZoneChangesLog.log( - "handleConfigChanged: Cleared latest Geolocation suggestion."); - } - // The configuration change may have changed available suggestions or the way suggestions // are used, so re-run detection. doAutoTimeZoneDetection(currentUserConfig, "handleConfigChanged()"); diff --git a/services/tests/servicestests/src/com/android/server/timezonedetector/TimeZoneDetectorStrategyImplTest.java b/services/tests/servicestests/src/com/android/server/timezonedetector/TimeZoneDetectorStrategyImplTest.java index e8af41d1cfded..18268125b2f10 100644 --- a/services/tests/servicestests/src/com/android/server/timezonedetector/TimeZoneDetectorStrategyImplTest.java +++ b/services/tests/servicestests/src/com/android/server/timezonedetector/TimeZoneDetectorStrategyImplTest.java @@ -835,29 +835,6 @@ public class TimeZoneDetectorStrategyImplTest { mTimeZoneDetectorStrategy.getLatestGeolocationSuggestion()); } - @Test - public void testGeoSuggestion_togglingGeoDetectionClearsLastSuggestion() { - GeolocationTimeZoneSuggestion suggestion = - createCertainGeolocationSuggestion("Europe/London"); - - Script script = new Script() - .initializeConfig(CONFIG_INT_AUTO_ENABLED_GEO_ENABLED) - .initializeTimeZoneSetting(ARBITRARY_TIME_ZONE_ID); - - script.simulateGeolocationTimeZoneSuggestion(suggestion) - .verifyTimeZoneChangedAndReset(suggestion); - - // Assert internal service state. - assertEquals(suggestion, mTimeZoneDetectorStrategy.getLatestGeolocationSuggestion()); - - // Turn off geo detection and verify the latest suggestion is cleared. - script.simulateUpdateConfiguration(USER_ID, CONFIG_GEO_DETECTION_DISABLED, true) - .verifyConfigurationChangedAndReset(CONFIG_INT_AUTO_ENABLED_GEO_DISABLED); - - // Assert internal service state. - assertNull(mTimeZoneDetectorStrategy.getLatestGeolocationSuggestion()); - } - /** * Confirms that changing the geolocation time zone detection enabled setting has the expected * behavior, i.e. immediately recompute the detected time zone using different signals. @@ -878,13 +855,12 @@ public class TimeZoneDetectorStrategyImplTest { script.simulateGeolocationTimeZoneSuggestion(geolocationSuggestion) .verifyTimeZoneNotChanged(); - // Geolocation suggestions are only stored when geolocation detection is enabled. - assertNull(mTimeZoneDetectorStrategy.getLatestGeolocationSuggestion()); + assertEquals(geolocationSuggestion, + mTimeZoneDetectorStrategy.getLatestGeolocationSuggestion()); script.simulateTelephonyTimeZoneSuggestion(telephonySuggestion) .verifyTimeZoneNotChanged(); - // Telephony suggestions are always stored. assertEquals(telephonySuggestion, mTimeZoneDetectorStrategy.getLatestTelephonySuggestion(SLOT_INDEX1).suggestion); @@ -894,12 +870,10 @@ public class TimeZoneDetectorStrategyImplTest { script.simulateUpdateConfiguration(USER_ID, CONFIG_AUTO_ENABLED, true /* expectedResult */) .verifyTimeZoneChangedAndReset(telephonySuggestion); - // Changing the detection to enable geo detection won't cause the device tz setting to - // change because the geo suggestion is empty. + // Changing the detection to enable geo detection will cause the device tz setting to + // change to use the latest geolocation suggestion. script.simulateUpdateConfiguration( USER_ID, CONFIG_GEO_DETECTION_ENABLED, true /* expectedResult */) - .verifyTimeZoneNotChanged() - .simulateGeolocationTimeZoneSuggestion(geolocationSuggestion) .verifyTimeZoneChangedAndReset(geolocationSuggestion); // Changing the detection to disable geo detection should cause the device tz setting to @@ -908,7 +882,8 @@ public class TimeZoneDetectorStrategyImplTest { USER_ID, CONFIG_GEO_DETECTION_DISABLED, true /* expectedResult */) .verifyTimeZoneChangedAndReset(telephonySuggestion); - assertNull(mTimeZoneDetectorStrategy.getLatestGeolocationSuggestion()); + assertEquals(geolocationSuggestion, + mTimeZoneDetectorStrategy.getLatestGeolocationSuggestion()); } @Test @@ -947,7 +922,7 @@ public class TimeZoneDetectorStrategyImplTest { .verifyTimeZoneNotChanged(); assertMetricsState(expectedInternalConfig, expectedDeviceTimeZoneId, - manualSuggestion, telephonySuggestion, null /* expectedGeoSuggestion */, + manualSuggestion, telephonySuggestion, geolocationTimeZoneSuggestion, MetricsTimeZoneDetectorState.DETECTION_MODE_MANUAL); // Update the config and confirm that the config metrics state updates also. @@ -957,16 +932,9 @@ public class TimeZoneDetectorStrategyImplTest { .setAutoDetectionEnabled(true) .setGeoDetectionEnabled(true) .build(); - script.simulateUpdateConfiguration(USER_ID, configUpdate, true /* expectedResult */) - .verifyConfigurationChangedAndReset(expectedInternalConfig) - .verifyTimeZoneNotChanged(); - assertMetricsState(expectedInternalConfig, expectedDeviceTimeZoneId, - manualSuggestion, telephonySuggestion, null /* expectedGeoSuggestion */, - MetricsTimeZoneDetectorState.DETECTION_MODE_GEO); - // Now simulate a geo suggestion and confirm it is used and reported in the metrics too. expectedDeviceTimeZoneId = geolocationTimeZoneSuggestion.getZoneIds().get(0); - script.simulateGeolocationTimeZoneSuggestion(geolocationTimeZoneSuggestion) + script.simulateUpdateConfiguration(USER_ID, configUpdate, true /* expectedResult */) .verifyTimeZoneChangedAndReset(expectedDeviceTimeZoneId); assertMetricsState(expectedInternalConfig, expectedDeviceTimeZoneId, manualSuggestion, telephonySuggestion, geolocationTimeZoneSuggestion,