[Downloads UI] Batch delete selected items

[Downloads UI] Batch delete selected items

Batch delete selected items in one AsyncTask rather than kicking
off a new AsyncTask for each file that needs to be deleted. This
moves the responsibility for deleting files associated with
download items from DownloadItemWrapper to DownloadManagerUi.

Android's THREAD_POOL_EXECUTOR has a finite number of AsyncTasks
that can be queued. If we queue too many AsyncTasks (e.g. 200
items are selected for deletion) an exception will be thrown.

BUG=643811
Committed: https://crrev.com/99e93701687fb2619a57e3ab8b3ef3621b21480e
Cr-Commit-Position: refs/heads/master@{#417612}

[Theresa, 2016-09-02 22:49:35]

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

[commit-bot: I haz the power, 2016-09-02 22:50:12]

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

[Theresa, 2016-09-02 23:01:34]

Description was changed from

==========
[Downloads UI] Batch delete selected items

Batch delete selected items on one AsyncTask rather than kicking
off a new AsyncTask for each file that needs to be deleted. This
moves the responsibility for deleting files associated with
download items from the DownloadItemWrapper to DownloadManagerUi.

Android's thread pool has a finite number of AsyncTasks that can
be queued. If we queue too many AsyncTasks (e.g. 200 items are
selected for deletion) an exception will be thrown.

BUG=643811
==========

to

==========
[Downloads UI] Batch delete selected items

Batch delete selected items in one AsyncTask rather than kicking
off a new AsyncTask for each file that needs to be deleted. This
moves the responsibility for deleting files associated with
download items from DownloadItemWrapper to DownloadManagerUi.

Android's THREAD_POOL_EXECUTOR has a finite number of AsyncTasks
that can be queued. If we queue too many AsyncTasks (e.g. 200
items are selected for deletion) an exception will be thrown.

BUG=643811
==========

[Theresa, 2016-09-02 23:35:19]

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

[commit-bot: I haz the power, 2016-09-02 23:35:54]

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

[Theresa, 2016-09-02 23:36:47]

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

[Theresa, 2016-09-02 23:36:47]

ptal

[gone, 2016-09-02 23:55:46]

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

https://codereview.chromium.org/2305123002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryItemWrapper.java:212:
public boolean fileDeletedOnRemove() {
Should remove() just return a boolean?

[Theresa, 2016-09-03 00:03:10]

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

[commit-bot: I haz the power, 2016-09-03 00:03:20]

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

[Theresa, 2016-09-03 00:03:35]

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

https://codereview.chromium.org/2305123002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadHistoryItemWrapper.java:212:
public boolean fileDeletedOnRemove() {
On 2016/09/02 23:55:46, dfalcantara wrote:
> Should remove() just return a boolean?

Done.

[gone, 2016-09-03 00:11:49]

lgtm

[Theresa, 2016-09-03 00:19:30]

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

[Theresa, 2016-09-03 00:19:31]

+nyquist@ for FileUtils.java

[nyquist, 2016-09-03 00:37:19]

https://codereview.chromium.org/2305123002/diff/40001/base/android/java/src/o...
File base/android/java/src/org/chromium/base/FileUtils.java (right):

https://codereview.chromium.org/2305123002/diff/40001/base/android/java/src/o...
base/android/java/src/org/chromium/base/FileUtils.java:47: for (int i = 0; i <
files.size(); i++) {
Could you just do for (File currentFile : files) { ... } here? I don't see what
using an old-school for-loop buys us here. A bad case would be if it's a
LinkedList, the get(i) call below is an O(N) operation.

Also, at that point maybe just |file| as the variable name?

[commit-bot: I haz the power, 2016-09-03 00:43:58]

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

[commit-bot: I haz the power, 2016-09-03 00:43:59]

Dry run: This issue passed the CQ dry run.

[Theresa, 2016-09-03 01:21:22]

https://codereview.chromium.org/2305123002/diff/40001/base/android/java/src/o...
File base/android/java/src/org/chromium/base/FileUtils.java (right):

https://codereview.chromium.org/2305123002/diff/40001/base/android/java/src/o...
base/android/java/src/org/chromium/base/FileUtils.java:47: for (int i = 0; i <
files.size(); i++) {
On 2016/09/03 00:37:18, nyquist wrote:
> Could you just do for (File currentFile : files) { ... } here? I don't see
what
> using an old-school for-loop buys us here. A bad case would be if it's a
> LinkedList, the get(i) call below is an O(N) operation.
> 
> Also, at that point maybe just |file| as the variable name?

For ArrayLists a regular loop saves allocation of an iterator which makes it
better than a for each loop. Since this is a generic interface, however, I agree
that for each is the way to go. I'll change it.

[Theresa, 2016-09-06 16:15:52]

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

[Theresa, 2016-09-06 16:16:14]

https://codereview.chromium.org/2305123002/diff/40001/base/android/java/src/o...
File base/android/java/src/org/chromium/base/FileUtils.java (right):

https://codereview.chromium.org/2305123002/diff/40001/base/android/java/src/o...
base/android/java/src/org/chromium/base/FileUtils.java:47: for (int i = 0; i <
files.size(); i++) {
On 2016/09/03 01:21:22, Theresa Wellington wrote:
> On 2016/09/03 00:37:18, nyquist wrote:
> > Could you just do for (File currentFile : files) { ... } here? I don't see
> what
> > using an old-school for-loop buys us here. A bad case would be if it's a
> > LinkedList, the get(i) call below is an O(N) operation.
> > 
> > Also, at that point maybe just |file| as the variable name?
> 
> For ArrayLists a regular loop saves allocation of an iterator which makes it
> better than a for each loop. Since this is a generic interface, however, I
agree
> that for each is the way to go. I'll change it.

Done.

[commit-bot: I haz the power, 2016-09-06 16:16:16]

Dry run: 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-09-06 16:53:37]

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

[commit-bot: I haz the power, 2016-09-06 16:53:38]

Dry run: This issue passed the CQ dry run.

[Theresa, 2016-09-08 18:50:10]

nyquist@ - ptal

[nyquist, 2016-09-09 16:34:43]

base lgtm

[Theresa, 2016-09-09 16:35:11]

The CQ bit was checked by twellington@chromium.org

[Theresa, 2016-09-09 16:35:11]

The patchset sent to the CQ was uploaded after l-g-t-m from
dfalcantara@chromium.org
Link to the patchset: https://codereview.chromium.org/2305123002/#ps60001
(title: "foreach")

[commit-bot: I haz the power, 2016-09-09 16:35:37]

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-09-09 17:18:05]

Description was changed from

==========
[Downloads UI] Batch delete selected items

Batch delete selected items in one AsyncTask rather than kicking
off a new AsyncTask for each file that needs to be deleted. This
moves the responsibility for deleting files associated with
download items from DownloadItemWrapper to DownloadManagerUi.

Android's THREAD_POOL_EXECUTOR has a finite number of AsyncTasks
that can be queued. If we queue too many AsyncTasks (e.g. 200
items are selected for deletion) an exception will be thrown.

BUG=643811
==========

to

==========
[Downloads UI] Batch delete selected items

Batch delete selected items in one AsyncTask rather than kicking
off a new AsyncTask for each file that needs to be deleted. This
moves the responsibility for deleting files associated with
download items from DownloadItemWrapper to DownloadManagerUi.

Android's THREAD_POOL_EXECUTOR has a finite number of AsyncTasks
that can be queued. If we queue too many AsyncTasks (e.g. 200
items are selected for deletion) an exception will be thrown.

BUG=643811
==========

[commit-bot: I haz the power, 2016-09-09 17:18:06]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-09-09 17:20:51]

Description was changed from

==========
[Downloads UI] Batch delete selected items

Batch delete selected items in one AsyncTask rather than kicking
off a new AsyncTask for each file that needs to be deleted. This
moves the responsibility for deleting files associated with
download items from DownloadItemWrapper to DownloadManagerUi.

Android's THREAD_POOL_EXECUTOR has a finite number of AsyncTasks
that can be queued. If we queue too many AsyncTasks (e.g. 200
items are selected for deletion) an exception will be thrown.

BUG=643811
==========

to

==========
[Downloads UI] Batch delete selected items

Batch delete selected items in one AsyncTask rather than kicking
off a new AsyncTask for each file that needs to be deleted. This
moves the responsibility for deleting files associated with
download items from DownloadItemWrapper to DownloadManagerUi.

Android's THREAD_POOL_EXECUTOR has a finite number of AsyncTasks
that can be queued. If we queue too many AsyncTasks (e.g. 200
items are selected for deletion) an exception will be thrown.

BUG=643811

Committed: https://crrev.com/99e93701687fb2619a57e3ab8b3ef3621b21480e
Cr-Commit-Position: refs/heads/master@{#417612}
==========

[commit-bot: I haz the power, 2016-09-09 17:20:51]

Patchset 4 (id:??) landed as
https://crrev.com/99e93701687fb2619a57e3ab8b3ef3621b21480e
Cr-Commit-Position: refs/heads/master@{#417612}