[Download Home] Add missing files to map

Problematically, new DownloadItems are created with no file name
when a download first starts.  This means that the
DownloadHistoryItemWrapper is added to the wrong part of the
FilePathsToDownloadItemsMap and can't be removed until the user
re-opens Download Home, which correctly records the DownloadItem
in the correct place.

* Change the behavior so a download completion event also triggers
  the item to be added to the map -- this time with the correct
  file path.

* Also make the FilePathsToDownloadItemsMap use a HashSet for its
  secondary data structure.  It ends up slightly cleaner IMO.

* Cleans up the StubbedProvider so that it can take in specific
  download percentages more easily.

* Add a new test for this behavior.

BUG=670583
TEST=DownloadHistoryAdapterTest*

Review-Url: https://codereview.chromium.org/2626903005
Cr-Commit-Position: refs/heads/master@{#442960}

chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java

@@ -26,6 +26,7 @@ import org.chromium.chrome.browser.widget.selection.SelectionDelegate;
 import org.chromium.content_public.browser.DownloadState;
 
 import java.util.List;
+import java.util.Set;
 
 /** Bridges the user's download history and the UI used to display it. */
 public class DownloadHistoryAdapter extends DateDividedAdapter implements DownloadUiObserver {
@@ -248,12 +249,21 @@ public class DownloadHistoryAdapter extends DateDividedAdapter implements Downlo
         DownloadHistoryItemWrapper existingWrapper = list.get(index);
         boolean isUpdated = existingWrapper.replaceItem(item);
 
+        // Re-add the file mapping once it finishes downloading. This accounts for the backend
+        // creating DownloadItems with a null file path, then updating it after the download starts.
+        // Doing it once after completion instead of at every update is a compromise that prevents
+        // us from rapidly and repeatedly updating the map with the same info.
+        if (item.getDownloadInfo().state() == DownloadState.COMPLETE) {
+            mFilePathsToItemsMap.addItem(existingWrapper);
+        }
+
         if (item.getDownloadInfo().state() == DownloadState.CANCELLED) {
             // The old one is being removed.
             filter(mFilter);
         } else if (existingWrapper.isVisibleToUser(mFilter)) {
             if (existingWrapper.getPosition() == TimedItem.INVALID_POSITION) {
                 filter(mFilter);
+                for (TestObserver observer : mObservers) observer.onDownloadItemUpdated(item);
             } else if (isUpdated) {
                 notifyItemChanged(existingWrapper.getPosition());
                 for (TestObserver observer : mObservers) observer.onDownloadItemUpdated(item);
@@ -307,7 +317,7 @@ public class DownloadHistoryAdapter extends DateDividedAdapter implements Downlo
      * @param filePath The file path used to retrieve items.
      * @return DownloadHistoryItemWrappers associated with filePath.
      */
-    List<DownloadHistoryItemWrapper> getItemsForFilePath(String filePath) {
+    Set<DownloadHistoryItemWrapper> getItemsForFilePath(String filePath) {
         return mFilePathsToItemsMap.getItemsForFilePath(filePath);
     }
 

chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadManagerUi.java

@@ -428,7 +428,7 @@ public class DownloadManagerUi implements OnMenuItemClickListener {
 
         for (DownloadHistoryItemWrapper item : selectedItems) {
             if (!filePathsToRemove.contains(item.getFilePath())) {
-                List<DownloadHistoryItemWrapper> itemsForFilePath =
+                Set<DownloadHistoryItemWrapper> itemsForFilePath =
                         mHistoryAdapter.getItemsForFilePath(item.getFilePath());
                 if (itemsForFilePath != null) {
                     itemsToRemove.addAll(itemsForFilePath);

chrome/android/java/src/org/chromium/chrome/browser/download/ui/FilePathsToDownloadItemsMap.java

@@ -4,10 +4,12 @@
 
 package org.chromium.chrome.browser.download.ui;
 
-import java.util.ArrayList;
+import android.text.TextUtils;
+
 import java.util.HashMap;
-import java.util.List;
+import java.util.HashSet;
 import java.util.Map;
+import java.util.Set;
 
 /**
  * Multiple download items may reference the same location on disk. This class maintains a mapping
@@ -15,16 +17,17 @@ import java.util.Map;
  * TODO(twellington): remove this class after the backend handles duplicate removal.
  */
 class FilePathsToDownloadItemsMap {
-    private final Map<String, ArrayList<DownloadHistoryItemWrapper>> mMap = new HashMap<>();
+    private final Map<String, Set<DownloadHistoryItemWrapper>> mMap = new HashMap<>();
 
     /**
-     * Adds a DownloadHistoryItemWrapper to the map. This method does not check whether the item
-     * already exists in the map.
+     * Adds a DownloadHistoryItemWrapper to the map if it has a valid path.
      * @param wrapper The item to add to the map.
      */
     void addItem(DownloadHistoryItemWrapper wrapper) {
+        if (TextUtils.isEmpty(wrapper.getFilePath())) return;
+
         if (!mMap.containsKey(wrapper.getFilePath())) {
-            mMap.put(wrapper.getFilePath(), new ArrayList<DownloadHistoryItemWrapper>());
+            mMap.put(wrapper.getFilePath(), new HashSet<DownloadHistoryItemWrapper>());
         }
         mMap.get(wrapper.getFilePath()).add(wrapper);
     }
@@ -35,21 +38,15 @@ class FilePathsToDownloadItemsMap {
      * @param wrapper The item to remove from the map.
      */
     void removeItem(DownloadHistoryItemWrapper wrapper) {
-        ArrayList<DownloadHistoryItemWrapper> matchingItems = mMap.get(wrapper.getFilePath());
-        if (matchingItems == null) return;
-
-        for (int i = 0; i < matchingItems.size(); i++) {
-            if (!matchingItems.get(i).equals(wrapper)) continue;
+        Set<DownloadHistoryItemWrapper> matchingItems = mMap.get(wrapper.getFilePath());
+        if (matchingItems == null || !matchingItems.contains(wrapper)) return;
 
+        if (matchingItems.size() == 1) {
             // If this is the only DownloadHistoryItemWrapper that references the file path,
             // remove the file path from the map.
-            if (matchingItems.size() == 1) {
-                mMap.remove(wrapper.getFilePath());
-            } else {
-                matchingItems.remove(i);
-            }
-
-            return;
+            mMap.remove(wrapper.getFilePath());
+        } else {
+            matchingItems.remove(wrapper);
         }
     }
 
@@ -58,7 +55,7 @@ class FilePathsToDownloadItemsMap {
      * @param filePath The file path used to retrieve items.
      * @return DownloadHistoryItemWrappers associated with filePath.
      */
-    List<DownloadHistoryItemWrapper> getItemsForFilePath(String filePath) {
+    Set<DownloadHistoryItemWrapper> getItemsForFilePath(String filePath) {
         return mMap.get(filePath);
     }
 }

chrome/android/javatests/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapterTest.java

@@ -19,6 +19,8 @@ import org.chromium.chrome.browser.offlinepages.downloads.OfflinePageDownloadIte
 import org.chromium.content.browser.test.NativeLibraryTestBase;
 import org.chromium.content_public.browser.DownloadState;
 
+import java.util.Set;
+
 /**
  * Tests a DownloadHistoryAdapter that is isolated from the real bridges.
  */
@@ -194,7 +196,7 @@ public class DownloadHistoryAdapterTest extends NativeLibraryTestBase {
         // Add a third item with the same date as the second item.
         assertEquals(2, mObserver.onDownloadItemCreatedCallback.getCallCount());
         DownloadItem item2 = StubbedProvider.createDownloadItem(
-                2, "19840117 18:00", false, DownloadState.IN_PROGRESS);
+                2, "19840117 18:00", false, DownloadState.IN_PROGRESS, 0);
         mAdapter.onDownloadItemCreated(item2);
         mObserver.onDownloadItemCreatedCallback.waitForCallback(2);
         assertEquals(mObserver.createdItem, item2);
@@ -205,7 +207,7 @@ public class DownloadHistoryAdapterTest extends NativeLibraryTestBase {
         // but it should now be visible.
         int callCount = mObserver.onDownloadItemUpdatedCallback.getCallCount();
         DownloadItem item3 = StubbedProvider.createDownloadItem(
-                2, "19840117 18:00", false, DownloadState.COMPLETE);
+                2, "19840117 18:00", false, DownloadState.COMPLETE, 100);
         mAdapter.onDownloadItemUpdated(item3);
         mObserver.onDownloadItemUpdatedCallback.waitForCallback(callCount);
         assertEquals(mObserver.updatedItem, item3);
@@ -233,7 +235,7 @@ public class DownloadHistoryAdapterTest extends NativeLibraryTestBase {
         // Initialize the DownloadHistoryAdapter with three items in two date buckets.
         DownloadItem regularItem = StubbedProvider.createDownloadItem(0, "19840116 18:00");
         DownloadItem offTheRecordItem = StubbedProvider.createDownloadItem(
-                1, "19840116 12:00", true, DownloadState.COMPLETE);
+                1, "19840116 12:00", true, DownloadState.COMPLETE, 100);
         OfflinePageDownloadItem offlineItem =
                 StubbedProvider.createOfflineItem(2, "19840117 12:01");
         mDownloadDelegate.regularItems.add(regularItem);
@@ -334,6 +336,52 @@ public class DownloadHistoryAdapterTest extends NativeLibraryTestBase {
         assertEquals(0, mAdapter.getItemCount());
     }
 
+    @SmallTest
+    public void testInProgress_FilePathMapAccurate() throws Exception {
+        Set<DownloadHistoryItemWrapper> toDelete;
+
+        initializeAdapter(false);
+        assertEquals(0, mAdapter.getItemCount());
+        assertEquals(0, mAdapter.getTotalDownloadSize());
+
+        // Simulate the creation of a new item by providing a DownloadItem without a path.
+        DownloadItem itemCreated = StubbedProvider.createDownloadItem(
+                9, "19840118 12:01", false, DownloadState.IN_PROGRESS, 0);
+        mAdapter.onDownloadItemCreated(itemCreated);
+        mObserver.onDownloadItemCreatedCallback.waitForCallback(0);
+        assertEquals(mObserver.createdItem, itemCreated);
+
+        checkAdapterContents();
+        toDelete = mAdapter.getItemsForFilePath(itemCreated.getDownloadInfo().getFilePath());
+        assertNull(toDelete);
+
+        // Update the Adapter with new information about the item.
+        int callCount = mObserver.onDownloadItemUpdatedCallback.getCallCount();
+        DownloadItem itemUpdated = StubbedProvider.createDownloadItem(
+                10, "19840118 12:01", false, DownloadState.IN_PROGRESS, 50);
+        mAdapter.onDownloadItemUpdated(itemUpdated);
+        mObserver.onDownloadItemUpdatedCallback.waitForCallback(callCount);
+        assertEquals(mObserver.updatedItem, itemUpdated);
+
+        checkAdapterContents(null, itemUpdated);
+        toDelete = mAdapter.getItemsForFilePath(itemUpdated.getDownloadInfo().getFilePath());
+        assertNull(toDelete);
+
+        // Tell the Adapter that the item has finished downloading.
+        callCount = mObserver.onDownloadItemUpdatedCallback.getCallCount();
+        DownloadItem itemCompleted = StubbedProvider.createDownloadItem(
+                10, "19840118 12:01", false, DownloadState.COMPLETE, 100);
+        mAdapter.onDownloadItemUpdated(itemCompleted);
+        mObserver.onDownloadItemUpdatedCallback.waitForCallback(callCount);
+        assertEquals(mObserver.updatedItem, itemCompleted);
+        checkAdapterContents(null, itemCompleted);
+
+        // Confirm that the file now shows up when trying to delete it.
+        toDelete = mAdapter.getItemsForFilePath(itemCompleted.getDownloadInfo().getFilePath());
+        assertEquals(1, toDelete.size());
+        assertEquals(itemCompleted.getId(), toDelete.iterator().next().getId());
+    }
+
     /** Checks that the adapter has the correct items in the right places. */
     private void checkAdapterContents(Object... expectedItems) {
         assertEquals(expectedItems.length, mAdapter.getItemCount());

chrome/android/javatests/src/org/chromium/chrome/browser/download/ui/StubbedProvider.java

@@ -199,118 +199,114 @@ public class StubbedProvider implements BackendProvider {
     @Override
     public void destroy() {}
 
-    /** See {@link #createDownloadItem(int, String, boolean)}. */
+    /** See {@link #createDownloadItem(int, String, boolean, int, int)}. */
     public static DownloadItem createDownloadItem(int which, String date) throws Exception {
-        return createDownloadItem(which, date, false, DownloadState.COMPLETE);
+        return createDownloadItem(which, date, false, DownloadState.COMPLETE, 100);
     }
 
     /** Creates a new DownloadItem with pre-defined values. */
     public static DownloadItem createDownloadItem(
-            int which, String date, boolean isIncognito, int state) throws Exception {
-        DownloadItem item = null;
+            int which, String date, boolean isIncognito, int state, int percent) throws Exception {
+        DownloadInfo.Builder builder = null;
         if (which == 0) {
-            item = new DownloadItem(false, new DownloadInfo.Builder()
+            builder = new DownloadInfo.Builder()
                     .setUrl("https://google.com")
                     .setContentLength(1)
                     .setFileName("first_file.jpg")
                     .setFilePath("/storage/fake_path/Downloads/first_file.jpg")
                     .setDownloadGuid("first_guid")
-                    .setMimeType("image/jpeg")
-                    .setState(state)
-                    .setIsOffTheRecord(isIncognito)
-                    .build());
+                    .setMimeType("image/jpeg");
         } else if (which == 1) {
-            item = new DownloadItem(false, new DownloadInfo.Builder()
+            builder = new DownloadInfo.Builder()
                     .setUrl("https://one.com")
                     .setContentLength(10)
                     .setFileName("second_file.gif")
                     .setFilePath("/storage/fake_path/Downloads/second_file.gif")
                     .setDownloadGuid("second_guid")
-                    .setMimeType("image/gif")
-                    .setState(state)
-                    .setIsOffTheRecord(isIncognito)
-                    .build());
+                    .setMimeType("image/gif");
         } else if (which == 2) {
-            item = new DownloadItem(false, new DownloadInfo.Builder()
+            builder = new DownloadInfo.Builder()
                     .setUrl("https://is.com")
                     .setContentLength(100)
                     .setFileName("third_file")
                     .setFilePath("/storage/fake_path/Downloads/third_file")
                     .setDownloadGuid("third_guid")
-                    .setMimeType("text/plain")
-                    .setState(state)
-                    .setIsOffTheRecord(isIncognito)
-                    .build());
+                    .setMimeType("text/plain");
         } else if (which == 3) {
-            item = new DownloadItem(false, new DownloadInfo.Builder()
+            builder = new DownloadInfo.Builder()
                     .setUrl("https://the.com")
                     .setContentLength(5)
                     .setFileName("four.webm")
                     .setFilePath("/storage/fake_path/Downloads/four.webm")
                     .setDownloadGuid("fourth_guid")
-                    .setMimeType("video/webm")
-                    .setState(state)
-                    .setIsOffTheRecord(isIncognito)
-                    .build());
+                    .setMimeType("video/webm");
         } else if (which == 4) {
-            item = new DownloadItem(false, new DownloadInfo.Builder()
+            builder = new DownloadInfo.Builder()
                     .setUrl("https://loneliest.com")
                     .setContentLength(50)
                     .setFileName("five.mp3")
                     .setFilePath("/storage/fake_path/Downloads/five.mp3")
                     .setDownloadGuid("fifth_guid")
-                    .setMimeType("audio/mp3")
-                    .setState(state)
-                    .setIsOffTheRecord(isIncognito)
-                    .build());
+                    .setMimeType("audio/mp3");
         } else if (which == 5) {
-            item = new DownloadItem(false, new DownloadInfo.Builder()
+            builder = new DownloadInfo.Builder()
                     .setUrl("https://number.com")
                     .setContentLength(500)
                     .setFileName("six.mp3")
                     .setFilePath("/storage/fake_path/Downloads/six.mp3")
                     .setDownloadGuid("sixth_guid")
-                    .setMimeType("audio/mp3")
-                    .setState(state)
-                    .setIsOffTheRecord(isIncognito)
-                    .build());
+                    .setMimeType("audio/mp3");
         } else if (which == 6) {
-            item = new DownloadItem(false, new DownloadInfo.Builder()
+            builder = new DownloadInfo.Builder()
                     .setUrl("https://sigh.com")
                     .setContentLength(ONE_GIGABYTE)
                     .setFileName("huge_image.png")
                     .setFilePath("/storage/fake_path/Downloads/huge_image.png")
                     .setDownloadGuid("seventh_guid")
-                    .setMimeType("image/png")
-                    .setState(state)
-                    .build());
+                    .setMimeType("image/png");
         } else if (which == 7) {
-            item = new DownloadItem(false, new DownloadInfo.Builder()
+            builder = new DownloadInfo.Builder()
                     .setUrl("https://sleepy.com")
                     .setContentLength(ONE_GIGABYTE / 2)
                     .setFileName("sleep.pdf")
                     .setFilePath("/storage/fake_path/Downloads/sleep.pdf")
                     .setDownloadGuid("eighth_guid")
-                    .setMimeType("application/pdf")
-                    .setState(state)
-                    .setIsOffTheRecord(isIncognito)
-                    .build());
+                    .setMimeType("application/pdf");
         } else if (which == 8) {
-            // This is a duplicate of item 7 above.
-            item = new DownloadItem(false, new DownloadInfo.Builder()
+            // This is a duplicate of item 7 above with a different GUID.
+            builder = new DownloadInfo.Builder()
                     .setUrl("https://sleepy.com")
                     .setContentLength(ONE_GIGABYTE / 2)
                     .setFileName("sleep.pdf")
                     .setFilePath("/storage/fake_path/Downloads/sleep.pdf")
                     .setDownloadGuid("ninth_guid")
-                    .setMimeType("application/pdf")
-                    .setState(state)
-                    .setIsOffTheRecord(isIncognito)
-                    .build());
+                    .setMimeType("application/pdf");
+        } else if (which == 9) {
+            builder = new DownloadInfo.Builder()
+                    .setUrl("https://totallynew.com")
+                    .setContentLength(ONE_GIGABYTE / 10)
+                    .setFileName("forserious.jpg")
+                    .setFilePath(null)
+                    .setDownloadGuid("tenth_guid")
+                    .setMimeType("image/jpg");
+        } else if (which == 10) {
+            // Duplicate version of #9, but the file path has been set.
+            builder = new DownloadInfo.Builder()
+                    .setUrl("https://totallynew.com")
+                    .setContentLength(ONE_GIGABYTE / 10)
+                    .setFileName("forserious.jpg")
+                    .setFilePath("/storage/fake_path/Downloads/forserious.jpg")
+                    .setDownloadGuid("tenth_guid")
+                    .setMimeType("image/jpg");
         } else {
             return null;
         }
 
+        builder.setIsOffTheRecord(isIncognito);
+        builder.setPercentCompleted(percent);
+        builder.setState(state);
+
+        DownloadItem item = new DownloadItem(false, builder.build());
         item.setStartTime(dateToEpoch(date));
         return item;
     }