Removed timestamps from PhoneLookup APIs.

Also added onSuccessfulBulkUpdate method.

It is safer for each PhoneLookup to keep track of its own last processed time via shared prefs where the value saved is the actual last processed timestamp from the underlying data. This is because it is difficult or impossible to select a single time that spans lookups due to queries being run and processed at different times.

The onSuccessfulBulkUpdate method is provided as a hook for PhoneLookups to persist their shared pref once they know the result of bulkUpdate have been successfully saved.

Finally, removed usage of the lastModified timestamp from PhoneLookupHistory in PhoneLookupDataSource since I believe the cases it was originally intended to cover are now handled by populateInserts().

Bug: 34672501
Test: unit
PiperOrigin-RevId: 177891586
Change-Id: I072409fc217e4d7e36816548862e8b358aebf165

3 files changed, 87 insertions, 24 deletions

diff --git a/java/com/android/dialer/phonelookup/PhoneLookup.java b/java/com/android/dialer/phonelookup/PhoneLookup.java
index 66f166d6d..183277569 100644
--- a/java/com/android/dialer/phonelookup/PhoneLookup.java
+++ b/java/com/android/dialer/phonelookup/PhoneLookup.java
@@ -27,8 +27,8 @@ import com.google.common.util.concurrent.ListenableFuture;
  * Provides operations related to retrieving information about phone numbers.
  *
  * <p>Some operations defined by this interface are generally targeted towards specific use cases;
- * for example {@link #isDirty(ImmutableSet, long)} and {@link #bulkUpdate(ImmutableMap, long)} are
- * generally intended to be used by the call log.
+ * for example {@link #isDirty(ImmutableSet)}, {@link #bulkUpdate(ImmutableMap)}, and {@link
+ * #onSuccessfulBulkUpdate()} are generally intended to be used by the call log.
  */
 public interface PhoneLookup {
 
@@ -43,14 +43,14 @@ public interface PhoneLookup {
 
   /**
    * Returns a future which returns true if the information for any of the provided phone numbers
-   * has changed since {@code lastModified} according to this {@link PhoneLookup}.
+   * has changed, usually since {@link #onSuccessfulBulkUpdate()} was last invoked.
    */
-  ListenableFuture<Boolean> isDirty(
-      ImmutableSet<DialerPhoneNumber> phoneNumbers, long lastModified);
+  ListenableFuture<Boolean> isDirty(ImmutableSet<DialerPhoneNumber> phoneNumbers);
 
   /**
-   * Given a set of existing information and a timestamp, returns a set of information with any
-   * changes made since the timestamp according to this {@link PhoneLookup}.
+   * Performs a bulk update of this {@link PhoneLookup}. The returned map must contain the exact
+   * same keys as the provided map. Most implementations will rely on last modified timestamps to
+   * efficiently only update the data which needs to be updated.
    *
    * <p>If there are no changes required, it is valid for this method to simply return the provided
    * {@code existingInfoMap}.
@@ -59,5 +59,15 @@ public interface PhoneLookup {
    * deleted) the returned map should contain an empty {@link PhoneLookupInfo} for that number.
    */
   ListenableFuture<ImmutableMap<DialerPhoneNumber, PhoneLookupInfo>> bulkUpdate(
-      ImmutableMap<DialerPhoneNumber, PhoneLookupInfo> existingInfoMap, long lastModified);
+      ImmutableMap<DialerPhoneNumber, PhoneLookupInfo> existingInfoMap);
+
+  /**
+   * Called when the results of the {@link #bulkUpdate(ImmutableMap)} have been applied by the
+   * caller.
+   *
+   * <p>Typically implementations will use this to store a "last processed" timestamp so that future
+   * invocations of {@link #isDirty(ImmutableSet)} and {@link #bulkUpdate(ImmutableMap)} can be
+   * efficiently implemented.
+   */
+  ListenableFuture<Void> onSuccessfulBulkUpdate();
 }
diff --git a/java/com/android/dialer/phonelookup/composite/CompositePhoneLookup.java b/java/com/android/dialer/phonelookup/composite/CompositePhoneLookup.java
index 59a845774..520c46f9e 100644
--- a/java/com/android/dialer/phonelookup/composite/CompositePhoneLookup.java
+++ b/java/com/android/dialer/phonelookup/composite/CompositePhoneLookup.java
@@ -70,11 +70,10 @@ public final class CompositePhoneLookup implements PhoneLookup {
   }
 
   @Override
-  public ListenableFuture<Boolean> isDirty(
-      ImmutableSet<DialerPhoneNumber> phoneNumbers, long lastModified) {
+  public ListenableFuture<Boolean> isDirty(ImmutableSet<DialerPhoneNumber> phoneNumbers) {
     List<ListenableFuture<Boolean>> futures = new ArrayList<>();
     for (PhoneLookup phoneLookup : phoneLookups) {
-      futures.add(phoneLookup.isDirty(phoneNumbers, lastModified));
+      futures.add(phoneLookup.isDirty(phoneNumbers));
     }
     // Executes all child lookups (possibly in parallel), completing when the first composite lookup
     // which returns "true" completes, and cancels the others.
@@ -90,11 +89,11 @@ public final class CompositePhoneLookup implements PhoneLookup {
    */
   @Override
   public ListenableFuture<ImmutableMap<DialerPhoneNumber, PhoneLookupInfo>> bulkUpdate(
-      ImmutableMap<DialerPhoneNumber, PhoneLookupInfo> existingInfoMap, long lastModified) {
+      ImmutableMap<DialerPhoneNumber, PhoneLookupInfo> existingInfoMap) {
     List<ListenableFuture<ImmutableMap<DialerPhoneNumber, PhoneLookupInfo>>> futures =
         new ArrayList<>();
     for (PhoneLookup phoneLookup : phoneLookups) {
-      futures.add(phoneLookup.bulkUpdate(existingInfoMap, lastModified));
+      futures.add(phoneLookup.bulkUpdate(existingInfoMap));
     }
     return Futures.transform(
         Futures.allAsList(futures),
@@ -118,4 +117,14 @@ public final class CompositePhoneLookup implements PhoneLookup {
         },
         MoreExecutors.directExecutor());
   }
+
+  @Override
+  public ListenableFuture<Void> onSuccessfulBulkUpdate() {
+    List<ListenableFuture<Void>> futures = new ArrayList<>();
+    for (PhoneLookup phoneLookup : phoneLookups) {
+      futures.add(phoneLookup.onSuccessfulBulkUpdate());
+    }
+    return Futures.transform(
+        Futures.allAsList(futures), unused -> null, MoreExecutors.directExecutor());
+  }
 }
diff --git a/java/com/android/dialer/phonelookup/cp2/Cp2PhoneLookup.java b/java/com/android/dialer/phonelookup/cp2/Cp2PhoneLookup.java
index 4ebd401c3..307f998ea 100644
--- a/java/com/android/dialer/phonelookup/cp2/Cp2PhoneLookup.java
+++ b/java/com/android/dialer/phonelookup/cp2/Cp2PhoneLookup.java
@@ -17,11 +17,13 @@
 package com.android.dialer.phonelookup.cp2;
 
 import android.content.Context;
+import android.content.SharedPreferences;
 import android.database.Cursor;
 import android.provider.ContactsContract.CommonDataKinds.Phone;
 import android.provider.ContactsContract.Contacts;
 import android.provider.ContactsContract.DeletedContacts;
 import android.support.annotation.NonNull;
+import android.support.annotation.Nullable;
 import android.support.v4.util.ArrayMap;
 import android.support.v4.util.ArraySet;
 import android.telecom.Call;
@@ -33,6 +35,7 @@ import com.android.dialer.phonelookup.PhoneLookup;
 import com.android.dialer.phonelookup.PhoneLookupInfo;
 import com.android.dialer.phonelookup.PhoneLookupInfo.Cp2Info;
 import com.android.dialer.phonelookup.PhoneLookupInfo.Cp2Info.Cp2ContactInfo;
+import com.android.dialer.storage.Unencrypted;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.util.concurrent.ListenableFuture;
@@ -46,6 +49,9 @@ import javax.inject.Inject;
 /** PhoneLookup implementation for local contacts. */
 public final class Cp2PhoneLookup implements PhoneLookup {
 
+  private static final String PREF_LAST_TIMESTAMP_PROCESSED =
+      "cp2PhoneLookupLastTimestampProcessed";
+
   private static final String[] CP2_INFO_PROJECTION =
       new String[] {
         Phone.DISPLAY_NAME_PRIMARY, // 0
@@ -64,10 +70,14 @@ public final class Cp2PhoneLookup implements PhoneLookup {
   private static final int CP2_INFO_CONTACT_ID_INDEX = 5;
 
   private final Context appContext;
+  private final SharedPreferences sharedPreferences;
+  @Nullable private Long currentLastTimestampProcessed;
 
   @Inject
-  Cp2PhoneLookup(@ApplicationContext Context appContext) {
+  Cp2PhoneLookup(
+      @ApplicationContext Context appContext, @Unencrypted SharedPreferences sharedPreferences) {
     this.appContext = appContext;
+    this.sharedPreferences = sharedPreferences;
   }
 
   @Override
@@ -76,14 +86,13 @@ public final class Cp2PhoneLookup implements PhoneLookup {
   }
 
   @Override
-  public ListenableFuture<Boolean> isDirty(
-      ImmutableSet<DialerPhoneNumber> phoneNumbers, long lastModified) {
+  public ListenableFuture<Boolean> isDirty(ImmutableSet<DialerPhoneNumber> phoneNumbers) {
     // TODO(calderwoodra): consider a different thread pool
-    return MoreExecutors.newDirectExecutorService()
-        .submit(() -> isDirtyInternal(phoneNumbers, lastModified));
+    return MoreExecutors.newDirectExecutorService().submit(() -> isDirtyInternal(phoneNumbers));
   }
 
-  private boolean isDirtyInternal(ImmutableSet<DialerPhoneNumber> phoneNumbers, long lastModified) {
+  private boolean isDirtyInternal(ImmutableSet<DialerPhoneNumber> phoneNumbers) {
+    long lastModified = sharedPreferences.getLong(PREF_LAST_TIMESTAMP_PROCESSED, 0L);
     return contactsUpdated(queryPhoneTableForContactIds(phoneNumbers), lastModified)
         || contactsDeleted(lastModified);
   }
@@ -149,7 +158,12 @@ public final class Cp2PhoneLookup implements PhoneLookup {
 
     return appContext
         .getContentResolver()
-        .query(Contacts.CONTENT_URI, new String[] {Contacts._ID}, where, args, null);
+        .query(
+            Contacts.CONTENT_URI,
+            new String[] {Contacts._ID, Contacts.CONTACT_LAST_UPDATED_TIMESTAMP},
+            where,
+            args,
+            null);
   }
 
   /** Returns true if any contacts were deleted after {@code lastModified}. */
@@ -169,13 +183,16 @@ public final class Cp2PhoneLookup implements PhoneLookup {
 
   @Override
   public ListenableFuture<ImmutableMap<DialerPhoneNumber, PhoneLookupInfo>> bulkUpdate(
-      ImmutableMap<DialerPhoneNumber, PhoneLookupInfo> existingInfoMap, long lastModified) {
+      ImmutableMap<DialerPhoneNumber, PhoneLookupInfo> existingInfoMap) {
     return MoreExecutors.newDirectExecutorService()
-        .submit(() -> bulkUpdateInternal(existingInfoMap, lastModified));
+        .submit(() -> bulkUpdateInternal(existingInfoMap));
   }
 
   private ImmutableMap<DialerPhoneNumber, PhoneLookupInfo> bulkUpdateInternal(
-      ImmutableMap<DialerPhoneNumber, PhoneLookupInfo> existingInfoMap, long lastModified) {
+      ImmutableMap<DialerPhoneNumber, PhoneLookupInfo> existingInfoMap) {
+    currentLastTimestampProcessed = null;
+    long lastModified = sharedPreferences.getLong(PREF_LAST_TIMESTAMP_PROCESSED, 0L);
+
     // Build a set of each DialerPhoneNumber that was associated with a contact, and is no longer
     // associated with that same contact.
     Set<DialerPhoneNumber> deletedPhoneNumbers =
@@ -214,6 +231,21 @@ public final class Cp2PhoneLookup implements PhoneLookup {
     return newInfoMapBuilder.build();
   }
 
+  @Override
+  public ListenableFuture<Void> onSuccessfulBulkUpdate() {
+    return MoreExecutors.newDirectExecutorService()
+        .submit(
+            () -> {
+              if (currentLastTimestampProcessed != null) {
+                sharedPreferences
+                    .edit()
+                    .putLong(PREF_LAST_TIMESTAMP_PROCESSED, currentLastTimestampProcessed)
+                    .apply();
+              }
+              return null;
+            });
+  }
+
   /**
    * 1. get all contact ids. if the id is unset, add the number to the list of contacts to look up.
    * 2. reduce our list of contact ids to those that were updated after lastModified. 3. Now we have
@@ -261,12 +293,18 @@ public final class Cp2PhoneLookup implements PhoneLookup {
     // after lastModified, such that Contacts._ID is in our set of contact IDs we build above.
     try (Cursor cursor = queryContactsTableForContacts(contactIds, lastModified)) {
       int contactIdIndex = cursor.getColumnIndex(Contacts._ID);
+      int lastUpdatedIndex = cursor.getColumnIndex(Contacts.CONTACT_LAST_UPDATED_TIMESTAMP);
       cursor.moveToPosition(-1);
       while (cursor.moveToNext()) {
         // Find the DialerPhoneNumber for each contact id and add it to our updated numbers set.
         // These, along with our number not associated with any Cp2ContactInfo need to be updated.
         long contactId = cursor.getLong(contactIdIndex);
         updatedNumbers.addAll(getDialerPhoneNumber(existingInfoMap, contactId));
+        long lastUpdatedTimestamp = cursor.getLong(lastUpdatedIndex);
+        if (currentLastTimestampProcessed == null
+            || currentLastTimestampProcessed < lastUpdatedTimestamp) {
+          currentLastTimestampProcessed = lastUpdatedTimestamp;
+        }
       }
     }
 
@@ -379,7 +417,7 @@ public final class Cp2PhoneLookup implements PhoneLookup {
         .getContentResolver()
         .query(
             DeletedContacts.CONTENT_URI,
-            new String[] {DeletedContacts.CONTACT_ID},
+            new String[] {DeletedContacts.CONTACT_ID, DeletedContacts.CONTACT_DELETED_TIMESTAMP},
             where,
             args,
             null);
@@ -389,11 +427,17 @@ public final class Cp2PhoneLookup implements PhoneLookup {
   private Set<DialerPhoneNumber> findDeletedPhoneNumbersIn(
       ImmutableMap<DialerPhoneNumber, PhoneLookupInfo> existingInfoMap, Cursor cursor) {
     int contactIdIndex = cursor.getColumnIndexOrThrow(DeletedContacts.CONTACT_ID);
+    int deletedTimeIndex = cursor.getColumnIndexOrThrow(DeletedContacts.CONTACT_DELETED_TIMESTAMP);
     Set<DialerPhoneNumber> deletedPhoneNumbers = new ArraySet<>();
     cursor.moveToPosition(-1);
     while (cursor.moveToNext()) {
       long contactId = cursor.getLong(contactIdIndex);
       deletedPhoneNumbers.addAll(getDialerPhoneNumber(existingInfoMap, contactId));
+      long deletedTime = cursor.getLong(deletedTimeIndex);
+      if (currentLastTimestampProcessed == null || currentLastTimestampProcessed < deletedTime) {
+        // TODO(zachh): There's a problem here if a contact for a new row is deleted?
+        currentLastTimestampProcessed = deletedTime;
+      }
     }
     return deletedPhoneNumbers;
   }