Show download error message if sdcard is not available

Show download error message if sdcard is not available

Currently when downloading a file without sdcard, Chrome shows the overwrite
infobar.
And if user acks that infobar, the file will be downloaded to Chrome's own
dir.
And the downloaded file cannot be opened by other apps, making it
useless.
This CL fixes that behavior by showing a "missing sdcard" message
when sdcard is not available.
Chrome will no longer try to generate a path under its own dir if sdcard
is not available.
If snackbar is available, the error message will be displayed on
snackbar.
Otherwise, a toast will be shown.

BUG=624912
Committed: https://crrev.com/7cf649da41128daf23ce3f631fe19f89b8da6a76
Cr-Commit-Position: refs/heads/master@{#405833}

[qinmin, 2016-07-06 23:15:30]

qinmin@chromium.org changed reviewers:
+ asanka@chromium.org, isherman@chromium.org, yfriedman@chromium.org

[qinmin, 2016-07-06 23:15:31]

PTAL

[Ilya Sherman, 2016-07-06 23:43:59]

histograms.xml lgtm

[Yaron, 2016-07-07 19:24:24]

https://codereview.chromium.org/2117343007/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerService.java
(right):

https://codereview.chromium.org/2117343007/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerService.java:1433:
private void onDownloadCanceled(String fileName, boolean
isExternalStorageMissing) {
If you make this static you can avoid the double round-trip through JNI.

https://codereview.chromium.org/2117343007/diff/1/chrome/browser/download/dow...
File chrome/browser/download/download_target_determiner.cc (right):

https://codereview.chromium.org/2117343007/diff/1/chrome/browser/download/dow...
chrome/browser/download/download_target_determiner.cc:336: // TODO(qinmin): show
an error toast to the user. http://crbug.com/581106.
aren't you fixing this? OnDownloadCanceled will toast

[qinmin, 2016-07-07 21:09:48]

https://codereview.chromium.org/2117343007/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerService.java
(right):

https://codereview.chromium.org/2117343007/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerService.java:1433:
private void onDownloadCanceled(String fileName, boolean
isExternalStorageMissing) {
On 2016/07/07 19:24:23, Yaron wrote:
> If you make this static you can avoid the double round-trip through JNI.

Done.

https://codereview.chromium.org/2117343007/diff/1/chrome/browser/download/dow...
File chrome/browser/download/download_target_determiner.cc (right):

https://codereview.chromium.org/2117343007/diff/1/chrome/browser/download/dow...
chrome/browser/download/download_target_determiner.cc:336: // TODO(qinmin): show
an error toast to the user. http://crbug.com/581106.
On 2016/07/07 19:24:23, Yaron wrote:
> aren't you fixing this? OnDownloadCanceled will toast

Done.

[asanka, 2016-07-07 21:12:48]

Should we add code to clean up the inaccessible files that may have been
downloaded?

https://codereview.chromium.org/2117343007/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerService.java
(right):

https://codereview.chromium.org/2117343007/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerService.java:1434:
int reason = isExternalStorageMissing ? DownloadManager.ERROR_DEVICE_NOT_FOUND
Nit: DEVICE_NOT_FOUND implies something much worse than there being no SD card.

https://codereview.chromium.org/2117343007/diff/1/chrome/browser/download/dow...
File chrome/browser/download/download_path_reservation_tracker.cc (right):

https://codereview.chromium.org/2117343007/diff/1/chrome/browser/download/dow...
chrome/browser/download/download_path_reservation_tracker.cc:204: #if
defined(OS_ANDROID)
This code is conditioned on OS_ANDROID while the only logic that intercepts the
resulting state (!verified && path.empty()) is conditioned on
BUILDFLAG(ANDROID_JAVA_UI). Let's pick one.

https://codereview.chromium.org/2117343007/diff/1/chrome/browser/download/dow...
chrome/browser/download/download_path_reservation_tracker.cc:206: // Chrome's
own directory.
The comment should mention that DIR_USER_DOCUMENTS is in reality a subdirectory
of DIR_ANDROID_APP_DATA which isn't accessible by other apps.

Also explicitly clear |reserved_path| here.

https://codereview.chromium.org/2117343007/diff/1/chrome/browser/download/dow...
File chrome/browser/download/download_target_determiner.cc (right):

https://codereview.chromium.org/2117343007/diff/1/chrome/browser/download/dow...
chrome/browser/download/download_target_determiner.cc:331: } else if
(!download_->GetWebContents()) {
Nit: 'else' unnecessary.

[Yaron, 2016-07-07 22:00:58]

lgtm

[qinmin, 2016-07-07 22:47:28]

On 2016/07/07 21:12:48, asanka wrote:
> Should we add code to clean up the inaccessible files that may have been
> downloaded?
> 

added the logic to clean up documents dir if sdcard was not found when a
download is triggered

>
https://codereview.chromium.org/2117343007/diff/1/chrome/android/java/src/org...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerService.java
> (right):
> 
>
https://codereview.chromium.org/2117343007/diff/1/chrome/android/java/src/org...
>
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerService.java:1434:
> int reason = isExternalStorageMissing ? DownloadManager.ERROR_DEVICE_NOT_FOUND
> Nit: DEVICE_NOT_FOUND implies something much worse than there being no SD
card.
> 
>
https://codereview.chromium.org/2117343007/diff/1/chrome/browser/download/dow...
> File chrome/browser/download/download_path_reservation_tracker.cc (right):
> 
>
https://codereview.chromium.org/2117343007/diff/1/chrome/browser/download/dow...
> chrome/browser/download/download_path_reservation_tracker.cc:204: #if
> defined(OS_ANDROID)
> This code is conditioned on OS_ANDROID while the only logic that intercepts
the
> resulting state (!verified && path.empty()) is conditioned on
> BUILDFLAG(ANDROID_JAVA_UI). Let's pick one.
> 
>
https://codereview.chromium.org/2117343007/diff/1/chrome/browser/download/dow...
> chrome/browser/download/download_path_reservation_tracker.cc:206: // Chrome's
> own directory.
> The comment should mention that DIR_USER_DOCUMENTS is in reality a
subdirectory
> of DIR_ANDROID_APP_DATA which isn't accessible by other apps.
> 
> Also explicitly clear |reserved_path| here.
> 
>
https://codereview.chromium.org/2117343007/diff/1/chrome/browser/download/dow...
> File chrome/browser/download/download_target_determiner.cc (right):
> 
>
https://codereview.chromium.org/2117343007/diff/1/chrome/browser/download/dow...
> chrome/browser/download/download_target_determiner.cc:331: } else if
> (!download_->GetWebContents()) {
> Nit: 'else' unnecessary.

[qinmin, 2016-07-07 22:49:05]

Patchset #3 (id:40001) has been deleted

[qinmin, 2016-07-07 22:49:37]

https://codereview.chromium.org/2117343007/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerService.java
(right):

https://codereview.chromium.org/2117343007/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerService.java:1434:
int reason = isExternalStorageMissing ? DownloadManager.ERROR_DEVICE_NOT_FOUND
On 2016/07/07 21:12:48, asanka wrote:
> Nit: DEVICE_NOT_FOUND implies something much worse than there being no SD
card.

Yes, the naming is really confusing. According to the DownloadManager API, the
constant actually refers to missing sdcard:
https://developer.android.com/reference/android/app/DownloadManager.html#ERRO....
I will leave this as it is now. Once we deprecated Android DownloadManager, I
will introduce our own constants.

https://codereview.chromium.org/2117343007/diff/1/chrome/browser/download/dow...
File chrome/browser/download/download_path_reservation_tracker.cc (right):

https://codereview.chromium.org/2117343007/diff/1/chrome/browser/download/dow...
chrome/browser/download/download_path_reservation_tracker.cc:204: #if
defined(OS_ANDROID)
On 2016/07/07 21:12:48, asanka wrote:
> This code is conditioned on OS_ANDROID while the only logic that intercepts
the
> resulting state (!verified && path.empty()) is conditioned on
> BUILDFLAG(ANDROID_JAVA_UI). Let's pick one.

Done.

https://codereview.chromium.org/2117343007/diff/1/chrome/browser/download/dow...
chrome/browser/download/download_path_reservation_tracker.cc:206: // Chrome's
own directory.
On 2016/07/07 21:12:48, asanka wrote:
> The comment should mention that DIR_USER_DOCUMENTS is in reality a
subdirectory
> of DIR_ANDROID_APP_DATA which isn't accessible by other apps.
> 
> Also explicitly clear |reserved_path| here.

Done.

https://codereview.chromium.org/2117343007/diff/1/chrome/browser/download/dow...
File chrome/browser/download/download_target_determiner.cc (right):

https://codereview.chromium.org/2117343007/diff/1/chrome/browser/download/dow...
chrome/browser/download/download_target_determiner.cc:331: } else if
(!download_->GetWebContents()) {
On 2016/07/07 21:12:48, asanka wrote:
> Nit: 'else' unnecessary.

Done.

[qinmin, 2016-07-11 16:59:36]

ping, asanka@, would you please take another look. Would like to submit this
before you go on vacation

[asanka, 2016-07-12 17:07:10]

lgtm % comment change. I'm a bit concerned that removing DIR_USER_DOCUMENTS with
no UI may result in unexpected data loss. But I'll defer the decision to Android
folks. If no other component uses this directory, then I guess it's ok.

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

https://codereview.chromium.org/2117343007/diff/60001/chrome/browser/android/...
chrome/browser/android/download/download_manager_service.cc:36: void
ClearUnusedDownloads() {
This one is a bit tricky. Are there other components that may have written to
this directory? Any files written here would not be visible externally without
extra work. But it's not quite a given that a) DIR_USER_DOCUMENTS consists
solely of downloaded files, and b) all such files can be safely removed.

I'll defer to Android folks about the safety of what's been done here.

https://codereview.chromium.org/2117343007/diff/60001/chrome/browser/download...
File chrome/browser/download/download_target_determiner.cc (right):

https://codereview.chromium.org/2117343007/diff/60001/chrome/browser/download...
chrome/browser/download/download_target_determiner.cc:336: // file.
This comment seems wrong. How does a resumption attempt following an overwrite
result in this situation?

[qinmin, 2016-07-12 18:13:04]

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

https://codereview.chromium.org/2117343007/diff/60001/chrome/browser/android/...
chrome/browser/android/download/download_manager_service.cc:36: void
ClearUnusedDownloads() {
On 2016/07/12 17:07:10, asanka wrote:
> This one is a bit tricky. Are there other components that may have written to
> this directory? Any files written here would not be visible externally without
> extra work. But it's not quite a given that a) DIR_USER_DOCUMENTS consists
> solely of downloaded files, and b) all such files can be safely removed.
> 
> I'll defer to Android folks about the safety of what's been done here.

Seems only download is using Documents dir, I added a TODO to remove this
function in the next chrome release.
Will ping tedchoc/yfriedman to confirm this is ok.

https://codereview.chromium.org/2117343007/diff/60001/chrome/browser/download...
File chrome/browser/download/download_target_determiner.cc (right):

https://codereview.chromium.org/2117343007/diff/60001/chrome/browser/download...
chrome/browser/download/download_target_determiner.cc:336: // file.
On 2016/07/12 17:07:10, asanka wrote:
> This comment seems wrong. How does a resumption attempt following an overwrite
> result in this situation?

should use created instead of overwritten.
When resumption is called, Chrome still checks for path conflict. For example,
if user download file a, then a.crdownload is created. If user kills chrome, and
another process creates a file with name "a" under the download dir, then when
user resumes the download, there will be a conflict.  And since this is
background resumption, webcontents is null.
Modified the comments to describe the situation better

[qinmin, 2016-07-12 18:16:59]

Yaron, is it ok to nuke the Documents dir under Chrome app dir? 

This is for some historical reasons. When sdcard is not around, previous chrome
versions will save the downloads to Document dir. However, the Document Dir is
not accessible to other apps, which makes the downloads useless and they will
take up space forever unless user clear the app data. 

So this CL includes a function to nuke the Documents dir since I cannot find
another feature that writes to it. I added a comment to remove that logic in the
next chrome version.

If this is not desired, I can remove that logic from this CL.

[qinmin, 2016-07-14 21:31:27]

The CQ bit was checked by qinmin@chromium.org

[qinmin, 2016-07-14 21:31:28]

The patchset sent to the CQ was uploaded after l-g-t-m from
isherman@chromium.org, yfriedman@chromium.org, asanka@chromium.org
Link to the patchset: https://codereview.chromium.org/2117343007/#ps100001
(title: "remove document dir cleaning logic, will do it in a separate CL")

[commit-bot: I haz the power, 2016-07-14 21:32:12]

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-07-14 23:19:30]

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

[commit-bot: I haz the power, 2016-07-14 23:19:32]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[qinmin, 2016-07-15 17:48:09]

Patchset #5 (id:100001) has been deleted

[qinmin, 2016-07-15 17:48:15]

The CQ bit was checked by qinmin@chromium.org

[qinmin, 2016-07-15 17:48:15]

The patchset sent to the CQ was uploaded after l-g-t-m from asanka@chromium.org,
yfriedman@chromium.org, isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/2117343007/#ps120001
(title: "fix test, remove document dir cleaning code, will do it in another CL")

[commit-bot: I haz the power, 2016-07-15 17:49:05]

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-07-15 17:51:36]

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

[commit-bot: I haz the power, 2016-07-15 17:51:37]

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-device-gn on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)

[qinmin, 2016-07-15 18:46:34]

The CQ bit was checked by qinmin@chromium.org

[qinmin, 2016-07-15 18:46:35]

The patchset sent to the CQ was uploaded after l-g-t-m from asanka@chromium.org,
yfriedman@chromium.org, isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/2117343007/#ps140001
(title: "rebase")

[commit-bot: I haz the power, 2016-07-15 18:47:20]

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-07-15 19:56:57]

Committed patchset #6 (id:140001)

[commit-bot: I haz the power, 2016-07-15 19:57:15]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-15 19:59:30]

Description was changed from

==========
Show download error message if sdcard is not available

Currently when downloading a file without sdcard, Chrome shows the overwrite
infobar.
And if user acks that infobar, the file will be downloaded to Chrome's own
dir.
And the downloaded file cannot be opened by other apps, making it
useless.
This CL fixes that behavior by showing a "missing sdcard" message
when sdcard is not available.
Chrome will no longer try to generate a path under its own dir if sdcard
is not available.
If snackbar is available, the error message will be displayed on
snackbar.
Otherwise, a toast will be shown.

BUG=624912
==========

to

==========
Show download error message if sdcard is not available

Currently when downloading a file without sdcard, Chrome shows the overwrite
infobar.
And if user acks that infobar, the file will be downloaded to Chrome's own
dir.
And the downloaded file cannot be opened by other apps, making it
useless.
This CL fixes that behavior by showing a "missing sdcard" message
when sdcard is not available.
Chrome will no longer try to generate a path under its own dir if sdcard
is not available.
If snackbar is available, the error message will be displayed on
snackbar.
Otherwise, a toast will be shown.

BUG=624912

Committed: https://crrev.com/7cf649da41128daf23ce3f631fe19f89b8da6a76
Cr-Commit-Position: refs/heads/master@{#405833}
==========

[commit-bot: I haz the power, 2016-07-15 19:59:31]

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