New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 654630 link

Starred by 3 users
Status: Fixed
Owner:
█ dfalcant...@chromium.org
Last visit > 30 days ago
Closed: Dec 2016
Cc:
█ ian...@chromium.org
twelling...@chromium.org
█ dah...@chromium.org
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug

Blocking:
issue 658246



Sign in to add a comment

[Download Home] Clean up frontend code

Project Member Reported by dfalcant...@chromium.org, Oct 11 2016 
With the mad dash to get every case we didn't account for in, we have a bunch of code that could be cleaned up.  Off the top of my head:

* DownloadHistoryAdapter has to track three different backends (regular downloads, incognito downloads, and offline pages) and has a bunch of duplicated code to do it.

* Download Home uses the DownloadManagerService class to handle all of its JNI; it's probably worth revisiting this decision because there's a lot of things that aren't related to the DownloadManagerService here.

* Code that fires Intents to open downloads and share items is currently splayed across at least three different places (DownloadManagerDelegate, DownloadHistoryItemAdapter, ShareHelper), each of which is slightly different.  We should see what can be pulled out here and why they're different.

Comment 1 by dfalcant...@chromium.org, Nov 14 2016

Blocking: 658246
Labels: -Pri-3 M-57 Pri-2
Tried to tackle 658246 without doing this, but DownloadHistoryAdapter is a hot mess.  I'm going to have to tidy up a bit before making it worse for download progress updates.
Project Member

Comment 2 by bugdroid1@chromium.org, Nov 15 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3d622f398a16812eee8f4fdfde7aa58b0c70b7fe

commit 3d622f398a16812eee8f4fdfde7aa58b0c70b7fe
Author: dfalcantara <dfalcantara@chromium.org>
Date: Tue Nov 15 04:47:46 2016

[Download Home] Pull out DeletedFileTracker from DownloadHistoryAdapter

* Pull out the maps that track whether a DownloadItem has been
  deleted externally.

* Clean up the logic slightly so that the DownloadItem is
  queried about its incognito status instead of passing around
  a separate boolean.

* Update the tests to account for the DownloadItem tracking
  incognito state.

BUG= 654630 

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

[modify] https://crrev.com/3d622f398a16812eee8f4fdfde7aa58b0c70b7fe/chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerService.java
[add] https://crrev.com/3d622f398a16812eee8f4fdfde7aa58b0c70b7fe/chrome/android/java/src/org/chromium/chrome/browser/download/ui/DeletedFileTracker.java
[modify] https://crrev.com/3d622f398a16812eee8f4fdfde7aa58b0c70b7fe/chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java
[modify] https://crrev.com/3d622f398a16812eee8f4fdfde7aa58b0c70b7fe/chrome/android/java_sources.gni
[modify] https://crrev.com/3d622f398a16812eee8f4fdfde7aa58b0c70b7fe/chrome/android/javatests/src/org/chromium/chrome/browser/download/DownloadActivityTest.java
[modify] https://crrev.com/3d622f398a16812eee8f4fdfde7aa58b0c70b7fe/chrome/android/javatests/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapterTest.java
[modify] https://crrev.com/3d622f398a16812eee8f4fdfde7aa58b0c70b7fe/chrome/android/javatests/src/org/chromium/chrome/browser/download/ui/StubbedProvider.java
Project Member

Comment 3 by bugdroid1@chromium.org, Nov 17 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b5e76d744d772f1ae8d6fe9b929c0a2257c258ef

commit b5e76d744d772f1ae8d6fe9b929c0a2257c258ef
Author: dfalcantara <dfalcantara@chromium.org>
Date: Thu Nov 17 18:26:05 2016

[Download Home] More cleaning

* More uniformly handle how each of the lists of downloaded items
  from the three backends are manipulated.

* Add a new BackendItems class to encapsulate more of the list
  manipulating behavior.

* Pull out the LoadingStateDelegate into its own file.

* Stop differentiating between DownloadItemWrappers and
  OfflineItemWrappers unless it's necessary.

BUG= 654630 

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

[add] https://crrev.com/b5e76d744d772f1ae8d6fe9b929c0a2257c258ef/chrome/android/java/src/org/chromium/chrome/browser/download/ui/BackendItems.java
[modify] https://crrev.com/b5e76d744d772f1ae8d6fe9b929c0a2257c258ef/chrome/android/java/src/org/chromium/chrome/browser/download/ui/DeletedFileTracker.java
[modify] https://crrev.com/b5e76d744d772f1ae8d6fe9b929c0a2257c258ef/chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java
[modify] https://crrev.com/b5e76d744d772f1ae8d6fe9b929c0a2257c258ef/chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryItemWrapper.java
[modify] https://crrev.com/b5e76d744d772f1ae8d6fe9b929c0a2257c258ef/chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadManagerUi.java
[add] https://crrev.com/b5e76d744d772f1ae8d6fe9b929c0a2257c258ef/chrome/android/java/src/org/chromium/chrome/browser/download/ui/LoadingStateDelegate.java
[modify] https://crrev.com/b5e76d744d772f1ae8d6fe9b929c0a2257c258ef/chrome/android/java_sources.gni
Project Member

Comment 4 by bugdroid1@chromium.org, Nov 18 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/13fa9f45a129fb1404d39b11e30fa911dbc534f8

commit 13fa9f45a129fb1404d39b11e30fa911dbc534f8
Author: dfalcantara <dfalcantara@chromium.org>
Date: Fri Nov 18 20:22:08 2016

[Downloads] Add support for tracking incomplete downloads

* Start tracking most DownloadItems on the Java side, but
  display only completed downloads in the UI.

* Start monitoring for the creation of DownloadItems, which
  happens when new downloads are triggered.

* Change how deletion is done and undone:
  Instead of straight up removing items from the adapter,
  hide the DownloadItems from view until we are alerted that
  the downloads have been deleted by the backends.  This
  allows us to keep track and simplifies a few data structure
  interactions.

* Add proper support for monitoring changes to DownloadItems,
  and add functions for checking if updates should be
  communicated to the UI.

BUG=658246, 654630 

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

[modify] https://crrev.com/13fa9f45a129fb1404d39b11e30fa911dbc534f8/chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerService.java
[modify] https://crrev.com/13fa9f45a129fb1404d39b11e30fa911dbc534f8/chrome/android/java/src/org/chromium/chrome/browser/download/ui/BackendItems.java
[modify] https://crrev.com/13fa9f45a129fb1404d39b11e30fa911dbc534f8/chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java
[modify] https://crrev.com/13fa9f45a129fb1404d39b11e30fa911dbc534f8/chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryItemWrapper.java
[modify] https://crrev.com/13fa9f45a129fb1404d39b11e30fa911dbc534f8/chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadManagerUi.java
[modify] https://crrev.com/13fa9f45a129fb1404d39b11e30fa911dbc534f8/chrome/android/java/src/org/chromium/chrome/browser/download/ui/FilePathsToDownloadItemsMap.java
[modify] https://crrev.com/13fa9f45a129fb1404d39b11e30fa911dbc534f8/chrome/android/javatests/src/org/chromium/chrome/browser/download/DownloadActivityTest.java
[modify] https://crrev.com/13fa9f45a129fb1404d39b11e30fa911dbc534f8/chrome/android/javatests/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapterTest.java
[modify] https://crrev.com/13fa9f45a129fb1404d39b11e30fa911dbc534f8/chrome/android/javatests/src/org/chromium/chrome/browser/download/ui/StubbedProvider.java
[modify] https://crrev.com/13fa9f45a129fb1404d39b11e30fa911dbc534f8/chrome/browser/android/download/download_manager_service.cc
[modify] https://crrev.com/13fa9f45a129fb1404d39b11e30fa911dbc534f8/chrome/browser/android/download/download_manager_service.h
Project Member

Comment 5 by bugdroid1@chromium.org, Nov 21 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d4c7abab4259e2aba07659b3f71b288ff61ccec6

commit d4c7abab4259e2aba07659b3f71b288ff61ccec6
Author: dfalcantara <dfalcantara@chromium.org>
Date: Mon Nov 21 21:07:55 2016

[Downloads] Consolidate Java DownloadItem/Info creation

Consolidates how DownloadInfos are created across
DownloadManagerService and DownloadController.

Differences:
* DownloadItems created for DownloadController previously used
  DownloadItem::GetURL().  They've been changed to use
  DownloadItem::GetTabURL() instead to match DownloadManagerService.

* Similiarly, DownloadController used to use the
  DownloadItem::GetTargetFilePath().BaseName().  This has been
  switched to DownloadItem::GetFileNameToReportUser().

* "Referrer" is more common than "referer" in the codebase,
  despite the historical spec misspelling.  Switch the
  DownloadInfo class to use that instead.  Request headers
  are created using the traditionally wrong "referer".

Removed:
* There was an unused method in DownloadManagerService.  Nuked it.

BUG= 654630 

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

[modify] https://crrev.com/d4c7abab4259e2aba07659b3f71b288ff61ccec6/chrome/android/java/src/org/chromium/chrome/browser/download/DownloadController.java
[modify] https://crrev.com/d4c7abab4259e2aba07659b3f71b288ff61ccec6/chrome/android/java/src/org/chromium/chrome/browser/download/DownloadInfo.java
[modify] https://crrev.com/d4c7abab4259e2aba07659b3f71b288ff61ccec6/chrome/android/java/src/org/chromium/chrome/browser/download/DownloadItem.java
[modify] https://crrev.com/d4c7abab4259e2aba07659b3f71b288ff61ccec6/chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerService.java
[modify] https://crrev.com/d4c7abab4259e2aba07659b3f71b288ff61ccec6/chrome/browser/BUILD.gn
[modify] https://crrev.com/d4c7abab4259e2aba07659b3f71b288ff61ccec6/chrome/browser/android/download/download_controller.cc
[modify] https://crrev.com/d4c7abab4259e2aba07659b3f71b288ff61ccec6/chrome/browser/android/download/download_manager_service.cc
[modify] https://crrev.com/d4c7abab4259e2aba07659b3f71b288ff61ccec6/chrome/browser/android/download/download_manager_service.h

Comment 6 by dfalcant...@chromium.org, Dec 12 2016

Status: Fixed (was: Assigned)
About as clean as it's going to get for a transition.  Marking closed.
►

Sign in to add a comment