Eliminate delay before contacts lookup for a Notification

The RankingReconsideration produced by ValidateNotificationPeople had getDelay() == 1000 ms, which meant NMS waiting a full second before starting the work. This wasted half of the time available to decide whether a notification should produce sound during DND, for no good reason (despite what the comment claimed, this delay is not used as a timeout).

This CL also moves all the contacts-querying methods in ValidateNotificationPeople to PeopleRankingReconsideration. This makes it clear that the querying is only done as part of the reconsideration, and process() only queries the cache.

(This is a roll-forward with adjusted CTS tests).

Bug: 263839687
Test: atest
Change-Id: I80707fc1381410a5b64c3ac128e448d8d9a35b11

2 files changed, 135 insertions, 113 deletions

diff --git a/services/core/java/com/android/server/notification/ValidateNotificationPeople.java b/services/core/java/com/android/server/notification/ValidateNotificationPeople.java
index 5e0a18039152..8417049bf297 100644
--- a/services/core/java/com/android/server/notification/ValidateNotificationPeople.java
+++ b/services/core/java/com/android/server/notification/ValidateNotificationPeople.java
@@ -62,7 +62,7 @@ import java.util.concurrent.TimeUnit;
 public class ValidateNotificationPeople implements NotificationSignalExtractor {
     // Using a shorter log tag since setprop has a limit of 32chars on variable name.
     private static final String TAG = "ValidateNoPeople";
-    private static final boolean VERBOSE = Log.isLoggable(TAG, Log.VERBOSE);;
+    private static final boolean VERBOSE = Log.isLoggable(TAG, Log.VERBOSE);
     private static final boolean DEBUG = Log.isLoggable(TAG, Log.DEBUG);
 
     private static final boolean ENABLE_PEOPLE_VALIDATOR = true;
@@ -105,12 +105,13 @@ public class ValidateNotificationPeople implements NotificationSignalExtractor {
     private int mEvictionCount;
     private NotificationUsageStats mUsageStats;
 
+    @Override
     public void initialize(Context context, NotificationUsageStats usageStats) {
         if (DEBUG) Slog.d(TAG, "Initializing  " + getClass().getSimpleName() + ".");
         mUserToContextMap = new ArrayMap<>();
         mBaseContext = context;
         mUsageStats = usageStats;
-        mPeopleCache = new LruCache<String, LookupResult>(PEOPLE_CACHE_SIZE);
+        mPeopleCache = new LruCache<>(PEOPLE_CACHE_SIZE);
         mEnabled = ENABLE_PEOPLE_VALIDATOR && 1 == Settings.Global.getInt(
                 mBaseContext.getContentResolver(), SETTING_ENABLE_PEOPLE_VALIDATOR, 1);
         if (mEnabled) {
@@ -134,7 +135,7 @@ public class ValidateNotificationPeople implements NotificationSignalExtractor {
     // For tests: just do the setting of various local variables without actually doing work
     @VisibleForTesting
     protected void initForTests(Context context, NotificationUsageStats usageStats,
-            LruCache peopleCache) {
+            LruCache<String, LookupResult> peopleCache) {
         mUserToContextMap = new ArrayMap<>();
         mBaseContext = context;
         mUsageStats = usageStats;
@@ -142,6 +143,7 @@ public class ValidateNotificationPeople implements NotificationSignalExtractor {
         mEnabled = true;
     }
 
+    @Override
     public RankingReconsideration process(NotificationRecord record) {
         if (!mEnabled) {
             if (VERBOSE) Slog.i(TAG, "disabled");
@@ -272,7 +274,7 @@ public class ValidateNotificationPeople implements NotificationSignalExtractor {
         }
 
         if (VERBOSE) Slog.i(TAG, "Validating: " + key + " for " + context.getUserId());
-        final LinkedList<String> pendingLookups = new LinkedList<String>();
+        final LinkedList<String> pendingLookups = new LinkedList<>();
         int personIdx = 0;
         for (String handle : people) {
             if (TextUtils.isEmpty(handle)) continue;
@@ -320,7 +322,6 @@ public class ValidateNotificationPeople implements NotificationSignalExtractor {
         return Integer.toString(userId) + ":" + handle;
     }
 
-    // VisibleForTesting
     public static String[] getExtraPeople(Bundle extras) {
         String[] peopleList = getExtraPeopleForKey(extras, Notification.EXTRA_PEOPLE_LIST);
         String[] legacyPeople = getExtraPeopleForKey(extras, Notification.EXTRA_PEOPLE);
@@ -417,101 +418,6 @@ public class ValidateNotificationPeople implements NotificationSignalExtractor {
         return null;
     }
 
-    private LookupResult resolvePhoneContact(Context context, final String number) {
-        Uri phoneUri = Uri.withAppendedPath(ContactsContract.PhoneLookup.CONTENT_FILTER_URI,
-                Uri.encode(number));
-        return searchContacts(context, phoneUri);
-    }
-
-    private LookupResult resolveEmailContact(Context context, final String email) {
-        Uri numberUri = Uri.withAppendedPath(
-                ContactsContract.CommonDataKinds.Email.CONTENT_LOOKUP_URI,
-                Uri.encode(email));
-        return searchContacts(context, numberUri);
-    }
-
-    @VisibleForTesting
-    LookupResult searchContacts(Context context, Uri lookupUri) {
-        LookupResult lookupResult = new LookupResult();
-        final Uri corpLookupUri =
-                ContactsContract.Contacts.createCorpLookupUriFromEnterpriseLookupUri(lookupUri);
-        if (corpLookupUri == null) {
-            addContacts(lookupResult, context, lookupUri);
-        } else {
-            addWorkContacts(lookupResult, context, corpLookupUri);
-        }
-        return lookupResult;
-    }
-
-    @VisibleForTesting
-    // Performs a contacts search using searchContacts, and then follows up by looking up
-    // any phone numbers associated with the resulting contact information and merge those
-    // into the lookup result as well. Will have no additional effect if the contact does
-    // not have any phone numbers.
-    LookupResult searchContactsAndLookupNumbers(Context context, Uri lookupUri) {
-        LookupResult lookupResult = searchContacts(context, lookupUri);
-        String phoneLookupKey = lookupResult.getPhoneLookupKey();
-        if (phoneLookupKey != null) {
-            String selection = Contacts.LOOKUP_KEY + " = ?";
-            String[] selectionArgs = new String[] { phoneLookupKey };
-            try (Cursor cursor = context.getContentResolver().query(
-                    ContactsContract.CommonDataKinds.Phone.CONTENT_URI, PHONE_LOOKUP_PROJECTION,
-                    selection, selectionArgs, /* sortOrder= */ null)) {
-                if (cursor == null) {
-                    Slog.w(TAG, "Cursor is null when querying contact phone number.");
-                    return lookupResult;
-                }
-
-                while (cursor.moveToNext()) {
-                    lookupResult.mergePhoneNumber(cursor);
-                }
-            } catch (Throwable t) {
-                Slog.w(TAG, "Problem getting content resolver or querying phone numbers.", t);
-            }
-        }
-        return lookupResult;
-    }
-
-    private void addWorkContacts(LookupResult lookupResult, Context context, Uri corpLookupUri) {
-        final int workUserId = findWorkUserId(context);
-        if (workUserId == -1) {
-            Slog.w(TAG, "Work profile user ID not found for work contact: " + corpLookupUri);
-            return;
-        }
-        final Uri corpLookupUriWithUserId =
-                ContentProvider.maybeAddUserId(corpLookupUri, workUserId);
-        addContacts(lookupResult, context, corpLookupUriWithUserId);
-    }
-
-    /** Returns the user ID of the managed profile or -1 if none is found. */
-    private int findWorkUserId(Context context) {
-        final UserManager userManager = context.getSystemService(UserManager.class);
-        final int[] profileIds =
-                userManager.getProfileIds(context.getUserId(), /* enabledOnly= */ true);
-        for (int profileId : profileIds) {
-            if (userManager.isManagedProfile(profileId)) {
-                return profileId;
-            }
-        }
-        return -1;
-    }
-
-    /** Modifies the given lookup result to add contacts found at the given URI. */
-    private void addContacts(LookupResult lookupResult, Context context, Uri uri) {
-        try (Cursor c = context.getContentResolver().query(
-                uri, LOOKUP_PROJECTION, null, null, null)) {
-            if (c == null) {
-                Slog.w(TAG, "Null cursor from contacts query.");
-                return;
-            }
-            while (c.moveToNext()) {
-                lookupResult.mergeContact(c);
-            }
-        } catch (Throwable t) {
-            Slog.w(TAG, "Problem getting content resolver or performing contacts query.", t);
-        }
-    }
-
     @VisibleForTesting
     protected static class LookupResult {
         private static final long CONTACT_REFRESH_MILLIS = 60 * 60 * 1000;  // 1hr
@@ -619,19 +525,18 @@ public class ValidateNotificationPeople implements NotificationSignalExtractor {
         }
     }
 
-    private class PeopleRankingReconsideration extends RankingReconsideration {
+    @VisibleForTesting
+    class PeopleRankingReconsideration extends RankingReconsideration {
         private final LinkedList<String> mPendingLookups;
         private final Context mContext;
 
-        // Amount of time to wait for a result from the contacts db before rechecking affinity.
-        private static final long LOOKUP_TIME = 1000;
         private float mContactAffinity = NONE;
         private ArraySet<String> mPhoneNumbers = null;
         private NotificationRecord mRecord;
 
         private PeopleRankingReconsideration(Context context, String key,
                 LinkedList<String> pendingLookups) {
-            super(key, LOOKUP_TIME);
+            super(key);
             mContext = context;
             mPendingLookups = pendingLookups;
         }
@@ -642,7 +547,7 @@ public class ValidateNotificationPeople implements NotificationSignalExtractor {
             long timeStartMs = System.currentTimeMillis();
             for (final String handle: mPendingLookups) {
                 final String cacheKey = getCacheKey(mContext.getUserId(), handle);
-                LookupResult lookupResult = null;
+                LookupResult lookupResult;
                 boolean cacheHit = false;
                 synchronized (mPeopleCache) {
                     lookupResult = mPeopleCache.get(cacheKey);
@@ -703,6 +608,102 @@ public class ValidateNotificationPeople implements NotificationSignalExtractor {
             }
         }
 
+        private static LookupResult resolvePhoneContact(Context context, final String number) {
+            Uri phoneUri = Uri.withAppendedPath(ContactsContract.PhoneLookup.CONTENT_FILTER_URI,
+                    Uri.encode(number));
+            return searchContacts(context, phoneUri);
+        }
+
+        private static LookupResult resolveEmailContact(Context context, final String email) {
+            Uri numberUri = Uri.withAppendedPath(
+                    ContactsContract.CommonDataKinds.Email.CONTENT_LOOKUP_URI,
+                    Uri.encode(email));
+            return searchContacts(context, numberUri);
+        }
+
+        @VisibleForTesting
+        static LookupResult searchContacts(Context context, Uri lookupUri) {
+            LookupResult lookupResult = new LookupResult();
+            final Uri corpLookupUri =
+                    ContactsContract.Contacts.createCorpLookupUriFromEnterpriseLookupUri(lookupUri);
+            if (corpLookupUri == null) {
+                addContacts(lookupResult, context, lookupUri);
+            } else {
+                addWorkContacts(lookupResult, context, corpLookupUri);
+            }
+            return lookupResult;
+        }
+
+        @VisibleForTesting
+        // Performs a contacts search using searchContacts, and then follows up by looking up
+        // any phone numbers associated with the resulting contact information and merge those
+        // into the lookup result as well. Will have no additional effect if the contact does
+        // not have any phone numbers.
+        static LookupResult searchContactsAndLookupNumbers(Context context, Uri lookupUri) {
+            LookupResult lookupResult = searchContacts(context, lookupUri);
+            String phoneLookupKey = lookupResult.getPhoneLookupKey();
+            if (phoneLookupKey != null) {
+                String selection = Contacts.LOOKUP_KEY + " = ?";
+                String[] selectionArgs = new String[] { phoneLookupKey };
+                try (Cursor cursor = context.getContentResolver().query(
+                        ContactsContract.CommonDataKinds.Phone.CONTENT_URI, PHONE_LOOKUP_PROJECTION,
+                        selection, selectionArgs, /* sortOrder= */ null)) {
+                    if (cursor == null) {
+                        Slog.w(TAG, "Cursor is null when querying contact phone number.");
+                        return lookupResult;
+                    }
+
+                    while (cursor.moveToNext()) {
+                        lookupResult.mergePhoneNumber(cursor);
+                    }
+                } catch (Throwable t) {
+                    Slog.w(TAG, "Problem getting content resolver or querying phone numbers.", t);
+                }
+            }
+            return lookupResult;
+        }
+
+        private static void addWorkContacts(LookupResult lookupResult, Context context,
+                Uri corpLookupUri) {
+            final int workUserId = findWorkUserId(context);
+            if (workUserId == -1) {
+                Slog.w(TAG, "Work profile user ID not found for work contact: " + corpLookupUri);
+                return;
+            }
+            final Uri corpLookupUriWithUserId =
+                    ContentProvider.maybeAddUserId(corpLookupUri, workUserId);
+            addContacts(lookupResult, context, corpLookupUriWithUserId);
+        }
+
+        /** Returns the user ID of the managed profile or -1 if none is found. */
+        private static int findWorkUserId(Context context) {
+            final UserManager userManager = context.getSystemService(UserManager.class);
+            final int[] profileIds =
+                    userManager.getProfileIds(context.getUserId(), /* enabledOnly= */ true);
+            for (int profileId : profileIds) {
+                if (userManager.isManagedProfile(profileId)) {
+                    return profileId;
+                }
+            }
+            return -1;
+        }
+
+        /** Modifies the given lookup result to add contacts found at the given URI. */
+        private static void addContacts(LookupResult lookupResult, Context context, Uri uri) {
+            try (Cursor c = context.getContentResolver().query(
+                    uri, LOOKUP_PROJECTION, null, null, null)) {
+                if (c == null) {
+                    Slog.w(TAG, "Null cursor from contacts query.");
+                    return;
+                }
+                while (c.moveToNext()) {
+                    lookupResult.mergeContact(c);
+                }
+            } catch (Throwable t) {
+                Slog.w(TAG, "Problem getting content resolver or performing contacts query.", t);
+            }
+        }
+
         @Override
         public void applyChangesLocked(NotificationRecord operand) {
             float affinityBound = operand.getContactAffinity();
diff --git a/services/tests/uiservicestests/src/com/android/server/notification/ValidateNotificationPeopleTest.java b/services/tests/uiservicestests/src/com/android/server/notification/ValidateNotificationPeopleTest.java
index d72cfc70fc02..0564a73bf3fc 100644
--- a/services/tests/uiservicestests/src/com/android/server/notification/ValidateNotificationPeopleTest.java
+++ b/services/tests/uiservicestests/src/com/android/server/notification/ValidateNotificationPeopleTest.java
@@ -15,6 +15,8 @@
  */
 package com.android.server.notification;
 
+import static com.google.common.truth.Truth.assertThat;
+
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNotNull;
@@ -51,6 +53,8 @@ import android.util.LruCache;
 import androidx.test.runner.AndroidJUnit4;
 
 import com.android.server.UiServiceTestCase;
+import com.android.server.notification.ValidateNotificationPeople.LookupResult;
+import com.android.server.notification.ValidateNotificationPeople.PeopleRankingReconsideration;
 
 import org.junit.Test;
 import org.junit.runner.RunWith;
@@ -60,6 +64,7 @@ import org.mockito.stubbing.Answer;
 
 import java.util.ArrayList;
 import java.util.Arrays;
+import java.util.concurrent.TimeUnit;
 
 @SmallTest
 @RunWith(AndroidJUnit4.class)
@@ -215,7 +220,7 @@ public class ValidateNotificationPeopleTest extends UiServiceTestCase {
                 ContactsContract.Contacts.CONTENT_LOOKUP_URI,
                 ContactsContract.Contacts.ENTERPRISE_CONTACT_LOOKUP_PREFIX + contactId);
 
-        new ValidateNotificationPeople().searchContacts(mockContext, lookupUri);
+        PeopleRankingReconsideration.searchContacts(mockContext, lookupUri);
 
         ArgumentCaptor<Uri> queryUri = ArgumentCaptor.forClass(Uri.class);
         verify(mockContentResolver).query(
@@ -242,7 +247,7 @@ public class ValidateNotificationPeopleTest extends UiServiceTestCase {
         final Uri lookupUri = Uri.withAppendedPath(
                 ContactsContract.Contacts.CONTENT_LOOKUP_URI, String.valueOf(contactId));
 
-        new ValidateNotificationPeople().searchContacts(mockContext, lookupUri);
+        PeopleRankingReconsideration.searchContacts(mockContext, lookupUri);
 
         ArgumentCaptor<Uri> queryUri = ArgumentCaptor.forClass(Uri.class);
         verify(mockContentResolver).query(
@@ -277,7 +282,7 @@ public class ValidateNotificationPeopleTest extends UiServiceTestCase {
 
         // call searchContacts and then mergePhoneNumbers, make sure we never actually
         // query the content resolver for a phone number
-        new ValidateNotificationPeople().searchContactsAndLookupNumbers(mockContext, lookupUri);
+        PeopleRankingReconsideration.searchContactsAndLookupNumbers(mockContext, lookupUri);
         verify(mockContentResolver, never()).query(
                 eq(ContactsContract.CommonDataKinds.Phone.CONTENT_URI),
                 eq(ValidateNotificationPeople.PHONE_LOOKUP_PROJECTION),
@@ -320,7 +325,7 @@ public class ValidateNotificationPeopleTest extends UiServiceTestCase {
 
         // call searchContacts and then mergePhoneNumbers, and check that we query
         // once for the
-        new ValidateNotificationPeople().searchContactsAndLookupNumbers(mockContext, lookupUri);
+        PeopleRankingReconsideration.searchContactsAndLookupNumbers(mockContext, lookupUri);
         verify(mockContentResolver, times(1)).query(
                 eq(ContactsContract.CommonDataKinds.Phone.CONTENT_URI),
                 eq(ValidateNotificationPeople.PHONE_LOOKUP_PROJECTION),
@@ -339,7 +344,7 @@ public class ValidateNotificationPeopleTest extends UiServiceTestCase {
 
         // Create validator with empty cache
         ValidateNotificationPeople vnp = new ValidateNotificationPeople();
-        LruCache cache = new LruCache<String, ValidateNotificationPeople.LookupResult>(5);
+        LruCache<String, LookupResult> cache = new LruCache<>(5);
         vnp.initForTests(mockContext, mockNotificationUsageStats, cache);
 
         NotificationRecord record = getNotificationRecord();
@@ -366,9 +371,8 @@ public class ValidateNotificationPeopleTest extends UiServiceTestCase {
         float affinity = 0.7f;
 
         // Create a fake LookupResult for the data we'll pass in
-        LruCache cache = new LruCache<String, ValidateNotificationPeople.LookupResult>(5);
-        ValidateNotificationPeople.LookupResult lr =
-                mock(ValidateNotificationPeople.LookupResult.class);
+        LruCache<String, LookupResult> cache = new LruCache<>(5);
+        LookupResult lr = mock(LookupResult.class);
         when(lr.getAffinity()).thenReturn(affinity);
         when(lr.getPhoneNumbers()).thenReturn(new ArraySet<>(new String[]{lookupTel}));
         when(lr.isExpired()).thenReturn(false);
@@ -392,6 +396,23 @@ public class ValidateNotificationPeopleTest extends UiServiceTestCase {
         assertTrue(record.getPhoneNumbers().contains(lookupTel));
     }
 
+    @Test
+    public void validatePeople_reconsiderationWillNotBeDelayed() {
+        final Context mockContext = mock(Context.class);
+        final ContentResolver mockContentResolver = mock(ContentResolver.class);
+        when(mockContext.getContentResolver()).thenReturn(mockContentResolver);
+        ValidateNotificationPeople vnp = new ValidateNotificationPeople();
+        vnp.initForTests(mockContext, mock(NotificationUsageStats.class), new LruCache<>(5));
+        NotificationRecord record = getNotificationRecord();
+        String[] callNumber = new String[]{"tel:12345678910"};
+        setNotificationPeople(record, callNumber);
+
+        RankingReconsideration rr = vnp.validatePeople(mockContext, record);
+
+        assertThat(rr).isNotNull();
+        assertThat(rr.getDelay(TimeUnit.MILLISECONDS)).isEqualTo(0);
+    }
+
     // Creates a cursor that points to one item of Contacts data with the specified
     // columns.
     private Cursor makeMockCursor(int id, String lookupKey, int starred, int hasPhone) {