Append late URLs at the end of the nearby URL list

When ListUrlsActivity is refreshed we take the current list of known nearby
URLs and send it to our URL resolution/metadata service. Previous to this
change, if new URLs were discovered after the initial list was sent, they
would not be sent to the resolution service or displayed in the activity.
This caused buggy behavior where nearby URLs would sometimes not be shown
if they were discovered too late.

With this change, URLs discovered after the initial resolution step will be
appended at the end of the list.

BUG=579099

Review URL: https://codereview.chromium.org/1634213002

Cr-Commit-Position: refs/heads/master@{#371838}

chrome/android/java/src/org/chromium/chrome/browser/physicalweb/ListUrlsActivity.java

@@ -28,14 +28,13 @@ import org.chromium.chrome.R;
 import org.chromium.chrome.browser.ChromeApplication;
 
 import java.util.Collection;
-import java.util.HashSet;
 
 /**
  * This activity displays a list of nearby URLs as stored in the {@link UrlManager}.
  * This activity does not and should not rely directly or indirectly on the native library.
  */
-public class ListUrlsActivity extends AppCompatActivity
-        implements AdapterView.OnItemClickListener, SwipeRefreshWidget.OnRefreshListener {
+public class ListUrlsActivity extends AppCompatActivity implements AdapterView.OnItemClickListener,
+        SwipeRefreshWidget.OnRefreshListener, UrlManager.Listener {
     public static final String REFERER_KEY = "referer";
     public static final int NOTIFICATION_REFERER = 1;
     public static final int OPTIN_REFERER = 2;
@@ -127,6 +126,12 @@ public class ListUrlsActivity extends AppCompatActivity
         startRefresh(true, true);
     }
 
+    @Override
+    protected void onStop() {
+        UrlManager.getInstance(this).removeObserver(this);
+        super.onStop();
+    }
+
     private void resolve(Collection<String> urls) {
         final long timestamp = SystemClock.elapsedRealtime();
         mPwsClient.resolve(urls, new PwsClient.ResolveScanCallback() {
@@ -136,13 +141,11 @@ public class ListUrlsActivity extends AppCompatActivity
                 PhysicalWebUma.onForegroundPwsResolution(ListUrlsActivity.this, duration);
 
                 // filter out duplicate site URLs.
-                Collection<String> siteUrls = new HashSet<>();
                 for (PwsResult pwsResult : pwsResults) {
                     String siteUrl = pwsResult.siteUrl;
                     String iconUrl = pwsResult.iconUrl;
 
-                    if (siteUrl != null && !siteUrls.contains(siteUrl)) {
-                        siteUrls.add(siteUrl);
+                    if (siteUrl != null && !mAdapter.hasSiteUrl(siteUrl)) {
                         mAdapter.add(pwsResult);
 
                         if (iconUrl != null && !mAdapter.hasIcon(iconUrl)) {
@@ -170,6 +173,15 @@ public class ListUrlsActivity extends AppCompatActivity
         mContext.startActivity(intent);
     }
 
+    /**
+     * Called when new nearby URLs are found.
+     * @param urls The set of newly-found nearby URLs.
+     */
+    @Override
+    public void onDisplayableUrlsAdded(Collection<String> urls) {
+        resolve(urls);
+    }
+
     private void startRefresh(boolean isUserInitiated, boolean isSwipeInitiated) {
         if (mIsRefreshing) {
             return;
@@ -181,7 +193,10 @@ public class ListUrlsActivity extends AppCompatActivity
         // Clear the list adapter to trigger the empty list display.
         mAdapter.clear();
 
-        Collection<String> urls = UrlManager.getInstance(this).getUrls(true);
+        UrlManager urlManager = UrlManager.getInstance(this);
+        urlManager.addObserver(this);
+
+        Collection<String> urls = urlManager.getUrls(true);
         if (urls.isEmpty()) {
             finishRefresh();
         } else {
@@ -208,6 +223,8 @@ public class ListUrlsActivity extends AppCompatActivity
     }
 
     private void finishRefresh() {
+        UrlManager.getInstance(this).removeObserver(this);
+
         // Hide the busy indicator.
         mSwipeRefreshWidget.setRefreshing(false);
 

chrome/android/java/src/org/chromium/chrome/browser/physicalweb/NearbyUrlsAdapter.java

@@ -81,4 +81,21 @@ class NearbyUrlsAdapter extends ArrayAdapter<PwsResult> {
 
         return view;
     }
+
+    /**
+     * Gets whether the specified site URL is in the list.
+     * @param siteUrl A string containing the site URL.
+     * @return Boolean true if the specified site URL is already in the list.
+     */
+    public boolean hasSiteUrl(String siteUrl) {
+        int itemCount = getCount();
+        for (int position = 0; position < itemCount; ++position) {
+            PwsResult pwsResult = getItem(position);
+            if (siteUrl.equals(pwsResult.siteUrl)) {
+                return true;
+            }
+        }
+
+        return false;
+    }
 }

chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java

@@ -20,12 +20,14 @@ import android.os.SystemClock;
 import android.support.v4.app.NotificationCompat;
 
 import org.chromium.base.Log;
+import org.chromium.base.ObserverList;
 import org.chromium.base.VisibleForTesting;
 import org.chromium.chrome.R;
 import org.chromium.chrome.browser.notifications.NotificationConstants;
 import org.chromium.chrome.browser.notifications.NotificationManagerProxy;
 import org.chromium.chrome.browser.notifications.NotificationManagerProxyImpl;
 
+import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collection;
 import java.util.Collections;
@@ -57,6 +59,19 @@ class UrlManager {
     private final Context mContext;
     private NotificationManagerProxy mNotificationManager;
     private PwsClient mPwsClient;
+    private final ObserverList<Listener> mObservers;
+
+    /**
+     * Interface for observers that should be notified when the nearby URL list changes.
+     */
+    public interface Listener {
+        /**
+         * Callback called when one or more URLs are added to the URL list.
+         * @param urls A set of strings containing nearby URLs resolvable with our resolution
+         * service.
+         */
+        void onDisplayableUrlsAdded(Collection<String> urls);
+    }
 
     /**
      * Construct the UrlManager.
@@ -67,6 +82,7 @@ class UrlManager {
         mNotificationManager = new NotificationManagerProxyImpl(
                 (NotificationManager) context.getSystemService(Context.NOTIFICATION_SERVICE));
         mPwsClient = new PwsClientImpl();
+        mObservers = new ObserverList<Listener>();
         initSharedPreferences();
     }
 
@@ -82,6 +98,22 @@ class UrlManager {
         return sInstance;
     }
 
+    /**
+     * Add an observer to be notified on changes to the nearby URL list.
+     * @param observer The observer to add.
+     */
+    public void addObserver(Listener observer) {
+        mObservers.addObserver(observer);
+    }
+
+    /**
+     * Remove an observer from the observer list.
+     * @param observer The observer to remove.
+     */
+    public void removeObserver(Listener observer) {
+        mObservers.removeObserver(observer);
+    }
+
     /**
      * Add a URL to the store of URLs.
      * This method additionally updates the Physical Web notification.
@@ -93,8 +125,21 @@ class UrlManager {
         boolean isOnboarding = PhysicalWeb.isOnboarding(mContext);
         Set<String> nearbyUrls = getCachedNearbyUrls();
 
-        boolean isUrlListEmptyBefore = (isOnboarding && nearbyUrls.isEmpty())
-                || (!isOnboarding && getUrls().isEmpty());
+        // A URL is displayable if it is both nearby and resolved through our resolution service.
+        // When new displayable URLs are found we tell our observers. In onboarding mode we do not
+        // use our resolution service so the displayable list should always be empty. However, we
+        // still want to track the nearby URL count so we can show an opt-in notification.
+        // In normal operation, both the notification and observers are updated for changes to the
+        // displayable list.
+
+        int displayableUrlsBefore;
+        int notificationUrlsBefore;
+        if (isOnboarding) {
+            displayableUrlsBefore = 0;
+            notificationUrlsBefore = nearbyUrls.size();
+        } else {
+            displayableUrlsBefore = notificationUrlsBefore = getUrls().size();
+        }
 
         nearbyUrls.add(url);
         putCachedNearbyUrls(nearbyUrls);
@@ -103,8 +148,17 @@ class UrlManager {
             resolveUrl(url);
         }
 
-        boolean isUrlListEmptyAfter = !isOnboarding && getUrls().isEmpty();
-        updateNotification(isUrlListEmptyBefore, isUrlListEmptyAfter);
+        int displayableUrlsAfter;
+        int notificationUrlsAfter;
+        if (isOnboarding) {
+            displayableUrlsAfter = 0;
+            notificationUrlsAfter = nearbyUrls.size();
+        } else {
+            displayableUrlsAfter = notificationUrlsAfter = getUrls().size();
+        }
+
+        updateNotification(notificationUrlsBefore == 0, notificationUrlsAfter == 0);
+        notifyDisplayableUrlsChanged(displayableUrlsBefore, displayableUrlsAfter, url);
     }
 
     /**
@@ -120,9 +174,8 @@ class UrlManager {
         nearbyUrls.remove(url);
         putCachedNearbyUrls(nearbyUrls);
 
-        boolean isUrlListEmptyAfter = (isOnboarding && nearbyUrls.isEmpty())
-                || (!isOnboarding && getUrls().isEmpty());
-        updateNotification(false, isUrlListEmptyAfter);
+        int notificationUrlsAfter = isOnboarding ? nearbyUrls.size() : getUrls().size();
+        updateNotification(false, notificationUrlsAfter == 0);
     }
 
     /**
@@ -177,11 +230,14 @@ class UrlManager {
     private void addResolvedUrl(String url) {
         Log.d(TAG, "PWS resolved: %s", url);
         Set<String> resolvedUrls = getCachedResolvedUrls();
-        boolean isUrlListEmptyBefore = getUrls().isEmpty();
+        int displayableUrlsBefore = getUrls().size();
 
         resolvedUrls.add(url);
         putCachedResolvedUrls(resolvedUrls);
-        updateNotification(isUrlListEmptyBefore, getUrls().isEmpty());
+
+        int displayableUrlsAfter = getUrls().size();
+        updateNotification(displayableUrlsBefore == 0, displayableUrlsAfter == 0);
+        notifyDisplayableUrlsChanged(displayableUrlsBefore, displayableUrlsAfter, url);
     }
 
     private void removeResolvedUrl(String url) {
@@ -377,6 +433,18 @@ class UrlManager {
         alarmManager.cancel(pendingIntent);
     }
 
+    private void notifyDisplayableUrlsChanged(int displayCountBefore, int displayCountAfter,
+            String url) {
+        if (displayCountAfter > displayCountBefore) {
+            Collection<String> urls = new ArrayList<String>();
+            urls.add(url);
+            Collection<String> wrappedUrls = Collections.unmodifiableCollection(urls);
+            for (Listener observer : mObservers) {
+                observer.onDisplayableUrlsAdded(wrappedUrls);
+            }
+        }
+    }
+
     @VisibleForTesting
     void overridePwsClientForTesting(PwsClient pwsClient) {
         mPwsClient = pwsClient;