[Downloads] Remove externally deleted items from downloads history

[Downloads] Remove externally deleted items from downloads history

Rather than greying out externally deleted items, delete
the files and remove the entry from history.

BUG=616324
Committed: https://crrev.com/e9334c611ffe8ddf0a7a491d28987f6681956ee1
Cr-Commit-Position: refs/heads/master@{#414276}

[Theresa, 2016-08-24 00:45:14]

The CQ bit was checked by twellington@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-24 00:45:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Theresa, 2016-08-24 00:46:46]

twellington@chromium.org changed reviewers:
+ qinmin@chromium.org

[Theresa, 2016-08-24 00:46:47]

qinmin@ - ptal at download_manager_service.cc changes

holte@ - ptal at actions.xml changes

dfalcantara@ - ptal at everything; I still need to rebase on top of your two
in-flight changes and make some tweaks.

https://codereview.chromium.org/2271913002/diff/20001/tools/metrics/actions/a...
File tools/metrics/actions/actions.xml (left):

https://codereview.chromium.org/2271913002/diff/20001/tools/metrics/actions/a...
tools/metrics/actions/actions.xml:940: <action
name="Android.DownloadManager.Share.Deleted">
This action landed yesterday and hasn't made it into any releases yet, so I
think it's okay to remove it rather than marking it obsolete. Is that correct?

[Theresa, 2016-08-24 00:47:11]

twellington@chromium.org changed reviewers:
+ dfalcantara@chromium.org, holte@google.com

[Theresa, 2016-08-24 00:47:12]

qinmin@ - ptal at download_manager_service.cc changes

holte@ - ptal at actions.xml changes

dfalcantara@ - ptal at everything; I still need to rebase on top of your two
in-flight changes and make some tweaks.

[commit-bot: I haz the power, 2016-08-24 00:48:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-24 00:48:54]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Steven Holte, 2016-08-24 01:48:08]

holte@chromium.org changed reviewers:
+ holte@chromium.org

[Steven Holte, 2016-08-24 01:48:09]

lgtm

https://codereview.chromium.org/2271913002/diff/20001/tools/metrics/actions/a...
File tools/metrics/actions/actions.xml (left):

https://codereview.chromium.org/2271913002/diff/20001/tools/metrics/actions/a...
tools/metrics/actions/actions.xml:940: <action
name="Android.DownloadManager.Share.Deleted">
On 2016/08/24 00:46:46, Theresa Wellington wrote:
> This action landed yesterday and hasn't made it into any releases yet, so I
> think it's okay to remove it rather than marking it obsolete. Is that correct?

That should be fine.

[qinmin, 2016-08-24 16:30:54]

https://codereview.chromium.org/2271913002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadManagerToolbar.java
(right):

https://codereview.chromium.org/2271913002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadManagerToolbar.java:56:
if (mIsSelectionEnabled && !wasSelectionEnabled) {
nit: mIsSelectEnabled appeared twice in the conditions, consider change this to

if (!mIsSelectionEnabled) {
  updateTitle()
} else if (!wasSelectionEnabled) {
   ..................
}

https://codereview.chromium.org/2271913002/diff/20001/chrome/browser/android/...
File chrome/browser/android/download/download_manager_service.cc (right):

https://codereview.chromium.org/2271913002/diff/20001/chrome/browser/android/...
chrome/browser/android/download/download_manager_service.cc:181: if (!manager ||
!is_history_query_complete_)
Nit: there is no need to get the manager if is_history_query_complete_ is false.

So it would be better to change this to:

if (!is_history_query_complete_) 
  return;

DownloadManger* manager = ....
....

[qinmin, 2016-08-24 16:31:05]

lgtm % nits

[Theresa, 2016-08-24 20:23:42]

The CQ bit was checked by twellington@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-24 20:24:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Theresa, 2016-08-24 22:51:55]

The CQ bit was checked by twellington@chromium.org to run a CQ dry run

[Theresa, 2016-08-24 22:52:09]

dfalcantara@ - this is ready now :)

https://codereview.chromium.org/2271913002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadManagerToolbar.java
(right):

https://codereview.chromium.org/2271913002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadManagerToolbar.java:56:
if (mIsSelectionEnabled && !wasSelectionEnabled) {
On 2016/08/24 16:30:53, qinmin wrote:
> nit: mIsSelectEnabled appeared twice in the conditions, consider change this
to
> 
> if (!mIsSelectionEnabled) {
>   updateTitle()
> } else if (!wasSelectionEnabled) {
>    ..................
> }

Done.

https://codereview.chromium.org/2271913002/diff/20001/chrome/browser/android/...
File chrome/browser/android/download/download_manager_service.cc (right):

https://codereview.chromium.org/2271913002/diff/20001/chrome/browser/android/...
chrome/browser/android/download/download_manager_service.cc:181: if (!manager ||
!is_history_query_complete_)
On 2016/08/24 16:30:53, qinmin wrote:
> Nit: there is no need to get the manager if is_history_query_complete_ is
false.
> 
> So it would be better to change this to:
> 
> if (!is_history_query_complete_) 
>   return;
> 
> DownloadManger* manager = ....
> ....

Done.

[commit-bot: I haz the power, 2016-08-24 22:52:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[gone, 2016-08-24 23:51:01]

https://chromiumcodereview.appspot.com/2271913002/diff/80001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadPage.java
(right):

https://chromiumcodereview.appspot.com/2271913002/diff/80001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadPage.java:40:
@SuppressFBWarnings("LI_LAZY_INIT_UPDATE_STATIC")  // Not called from multiple
threads.
nit: remove warning

https://chromiumcodereview.appspot.com/2271913002/diff/80001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadPage.java:57:
DownloadUtils.checkForExternallyRemovedDownloads(
Need to check both incognito and non-incognito when the tab is incognito.

https://chromiumcodereview.appspot.com/2271913002/diff/80001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java
(right):

https://chromiumcodereview.appspot.com/2271913002/diff/80001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java:70:
private static final Map<String, Boolean> sExternallyDeletedItems = new
HashMap<>();
1) Ugh, we need two maps because Incognito and regular downloads might share
GUIDs.

2) Keep track of how many DownloadHistoryAdapters there are and then nuke the
map so that we don't keep it around forever.  Use an AtomicInteger maybe.

https://chromiumcodereview.appspot.com/2271913002/diff/80001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java:114:
if (sExternallyDeletedItems.containsKey(wrapper.getId())) continue;
else if with the other ones?

[Theresa, 2016-08-25 00:21:18]

https://chromiumcodereview.appspot.com/2271913002/diff/80001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadPage.java
(right):

https://chromiumcodereview.appspot.com/2271913002/diff/80001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadPage.java:40:
@SuppressFBWarnings("LI_LAZY_INIT_UPDATE_STATIC")  // Not called from multiple
threads.
On 2016/08/24 23:51:00, dfalcantara wrote:
> nit: remove warning

Done.

https://chromiumcodereview.appspot.com/2271913002/diff/80001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadPage.java:57:
DownloadUtils.checkForExternallyRemovedDownloads(
On 2016/08/24 23:51:00, dfalcantara wrote:
> Need to check both incognito and non-incognito when the tab is incognito.

Changed in checkForExternallyRemovedDownloads.

https://chromiumcodereview.appspot.com/2271913002/diff/80001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java
(right):

https://chromiumcodereview.appspot.com/2271913002/diff/80001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java:70:
private static final Map<String, Boolean> sExternallyDeletedItems = new
HashMap<>();
On 2016/08/24 23:51:01, dfalcantara wrote:
> 1) Ugh, we need two maps because Incognito and regular downloads might share
> GUIDs.
> 
> 2) Keep track of how many DownloadHistoryAdapters there are and then nuke the
> map so that we don't keep it around forever.  Use an AtomicInteger maybe.

Done.

https://chromiumcodereview.appspot.com/2271913002/diff/80001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryAdapter.java:114:
if (sExternallyDeletedItems.containsKey(wrapper.getId())) continue;
On 2016/08/24 23:51:01, dfalcantara wrote:
> else if with the other ones?

Done.

[gone, 2016-08-25 00:28:39]

lgtm

[Theresa, 2016-08-25 00:41:00]

The CQ bit was checked by twellington@chromium.org

[Theresa, 2016-08-25 00:41:01]

The patchset sent to the CQ was uploaded after l-g-t-m from qinmin@chromium.org,
holte@chromium.org
Link to the patchset:
https://chromiumcodereview.appspot.com/2271913002/#ps100001 (title:
"dfalcantara@ review changes")

[commit-bot: I haz the power, 2016-08-25 00:41:50]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-25 02:49:22]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-08-25 02:51:31]

Description was changed from

==========
[Downloads] Remove externally deleted items from downloads history

Rather than greying out externally deleted items, delete
the files and remove the entry from history.

BUG=616324
==========

to

==========
[Downloads] Remove externally deleted items from downloads history

Rather than greying out externally deleted items, delete
the files and remove the entry from history.

BUG=616324

Committed: https://crrev.com/e9334c611ffe8ddf0a7a491d28987f6681956ee1
Cr-Commit-Position: refs/heads/master@{#414276}
==========

[commit-bot: I haz the power, 2016-08-25 02:51:33]

Patchset 6 (id:??) landed as
https://crrev.com/e9334c611ffe8ddf0a7a491d28987f6681956ee1
Cr-Commit-Position: refs/heads/master@{#414276}