[Downloads] Use content:// URIs when sharing

[Downloads] Use content:// URIs when sharing

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

[Theresa, 2016-08-10 17:52:04]

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

[commit-bot: I haz the power, 2016-08-10 17:52:49]

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-10 17:53:51]

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

[Theresa, 2016-08-10 17:53:52]

ptal

https://codereview.chromium.org/2230803004/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadManagerUi.java
(right):

https://codereview.chromium.org/2230803004/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadManagerUi.java:309:
Uri uri = itemWrapper.getUri();
dfalcantara@ - do you think we'll need getUri() for other things? For the share
application, it might make more sense to just use itemWrapper.getFilePath() so
that we're only turning it into a Java file once.

[gone, 2016-08-10 18:14:58]

https://chromiumcodereview.appspot.com/2230803004/diff/1/chrome/android/java/...
File chrome/android/java/res/xml/file_paths.xml (right):

https://chromiumcodereview.appspot.com/2230803004/diff/1/chrome/android/java/...
chrome/android/java/res/xml/file_paths.xml:12: <external-path name="downloads"
path="Download/" />
Not sure if this will work if we eventually allow users to save files elsewhere.
 Can you add a note about that here?

https://chromiumcodereview.appspot.com/2230803004/diff/1/chrome/android/java/...
File
chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadManagerUi.java
(right):

https://chromiumcodereview.appspot.com/2230803004/diff/1/chrome/android/java/...
chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadManagerUi.java:309:
Uri uri = itemWrapper.getUri();
On 2016/08/10 17:53:51, Theresa Wellington wrote:
> dfalcantara@ - do you think we'll need getUri() for other things? For the
share
> application, it might make more sense to just use itemWrapper.getFilePath() so
> that we're only turning it into a Java file once.

Yeah, caching makes sense.

https://chromiumcodereview.appspot.com/2230803004/diff/1/chrome/android/java/...
chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadManagerUi.java:316:
Log.e(TAG, "Could not create content uri: " + e);
If this fails, should we just fall back to using the Uri.fromFile method?

[Theresa, 2016-08-10 18:27:46]

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

[commit-bot: I haz the power, 2016-08-10 18:28:34]

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-10 18:29:52]

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

[Theresa, 2016-08-10 18:29:55]

https://chromiumcodereview.appspot.com/2230803004/diff/1/chrome/android/java/...
File chrome/android/java/res/xml/file_paths.xml (right):

https://chromiumcodereview.appspot.com/2230803004/diff/1/chrome/android/java/...
chrome/android/java/res/xml/file_paths.xml:12: <external-path name="downloads"
path="Download/" />
On 2016/08/10 18:14:57, dfalcantara wrote:
> Not sure if this will work if we eventually allow users to save files
elsewhere.
>  Can you add a note about that here?

Done.

https://chromiumcodereview.appspot.com/2230803004/diff/1/chrome/android/java/...
File
chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadManagerUi.java
(right):

https://chromiumcodereview.appspot.com/2230803004/diff/1/chrome/android/java/...
chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadManagerUi.java:309:
Uri uri = itemWrapper.getUri();
On 2016/08/10 18:14:57, dfalcantara wrote:
> On 2016/08/10 17:53:51, Theresa Wellington wrote:
> > dfalcantara@ - do you think we'll need getUri() for other things? For the
> share
> > application, it might make more sense to just use itemWrapper.getFilePath()
so
> > that we're only turning it into a Java file once.
> 
> Yeah, caching makes sense.

Done.

https://chromiumcodereview.appspot.com/2230803004/diff/1/chrome/android/java/...
chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadManagerUi.java:316:
Log.e(TAG, "Could not create content uri: " + e);
On 2016/08/10 18:14:58, dfalcantara wrote:
> If this fails, should we just fall back to using the Uri.fromFile method?

Done.

[commit-bot: I haz the power, 2016-08-10 18:30:55]

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-10 18:34:18]

lgtm

[qinmin, 2016-08-10 18:54:51]

lgtm % nit

[qinmin, 2016-08-10 18:56:19]

https://chromiumcodereview.appspot.com/2230803004/diff/40001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadManagerUi.java
(right):

https://chromiumcodereview.appspot.com/2230803004/diff/40001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadManagerUi.java:308:
Uri uri;
nit: not needed. 
Just do return in try block, and return Uri.FromFile() outside the try block. No
uri assignment needed in catch block

[commit-bot: I haz the power, 2016-08-10 19:38:59]

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

[commit-bot: I haz the power, 2016-08-10 19:39:02]

Dry run: This issue passed the CQ dry run.

[Theresa, 2016-08-10 20:07:32]

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

[Theresa, 2016-08-10 20:07:33]

+wnwen@ -- I'm adding a StrictMode exception in DownloadManagerUi.java. Getting
the content URI causes a disk read because
ContentUriUtils#getContentUriFromFile() (our method) calls into
FileProvider#getUriForFile() (Android's method) which calls
File#getCanonicalPath() which apparently reads from the disk.

I think allowing disk reads for this is okay. It should be fast, and the other
option is creating the content:// URI on a background thread, which may cause a
delay in firing the share intent after the share button is clicked.

What do you think?

https://chromiumcodereview.appspot.com/2230803004/diff/40001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadManagerUi.java
(right):

https://chromiumcodereview.appspot.com/2230803004/diff/40001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadManagerUi.java:308:
Uri uri;
On 2016/08/10 18:56:18, qinmin wrote:
> nit: not needed. 
> Just do return in try block, and return Uri.FromFile() outside the try block.
No
> uri assignment needed in catch block

I left this alone for now. It turns out that #getContentUriFromFile() causes a
disk read so I want to muck with the StrictMode settings. cc'ing wnwen@ on this
change for his approval.

[Theresa, 2016-08-10 20:07:42]

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

[commit-bot: I haz the power, 2016-08-10 20:08:25]

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-08-10 20:51:03]

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

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

Dry run: This issue passed the CQ dry run.

[Peter Wen, 2016-08-11 15:18:56]

Thanks for checking/fixing this.

lgtm % comment

https://chromiumcodereview.appspot.com/2230803004/diff/60001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadManagerUi.java
(right):

https://chromiumcodereview.appspot.com/2230803004/diff/60001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadManagerUi.java:310:
// #getContentUriFromFile causes a disk read when it calls into
FileProvider#getUriForFile.
Can you add that since this is due to a user action to share selected items, it
is on the critical path and we will have to wait for it even if we run it on a
separate thread? As it depends on user-selected items, we cannot know/preload
which URIs we need until the user presses share.

Just so it's clear for others reading this code that we've thought carefully
about why this is a valid exception.

[Theresa, 2016-08-11 16:50:38]

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

[commit-bot: I haz the power, 2016-08-11 16:51:07]

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-11 16:51:27]

dfalcantara/qinmin - does DownloadManagerUI still look good to you two?

https://codereview.chromium.org/2230803004/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadManagerUi.java
(right):

https://codereview.chromium.org/2230803004/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/download/ui/DownloadManagerUi.java:310:
// #getContentUriFromFile causes a disk read when it calls into
FileProvider#getUriForFile.
On 2016/08/11 15:18:55, Peter Wen wrote:
> Can you add that since this is due to a user action to share selected items,
it
> is on the critical path and we will have to wait for it even if we run it on a
> separate thread? As it depends on user-selected items, we cannot know/preload
> which URIs we need until the user presses share.
> 
> Just so it's clear for others reading this code that we've thought carefully
> about why this is a valid exception.

Done.

[gone, 2016-08-11 16:57:31]

slgtm

[commit-bot: I haz the power, 2016-08-11 17:40:19]

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

[commit-bot: I haz the power, 2016-08-11 17:40:20]

Dry run: This issue passed the CQ dry run.

[Theresa, 2016-08-11 21:02:46]

The CQ bit was checked by twellington@chromium.org

[Theresa, 2016-08-11 21:02:47]

The patchset sent to the CQ was uploaded after l-g-t-m from qinmin@chromium.org,
dfalcantara@chromium.org, wnwen@chromium.org
Link to the patchset: https://codereview.chromium.org/2230803004/#ps120001
(title: "[Downloads] Use content:// URIs when sharing")

[commit-bot: I haz the power, 2016-08-11 21:04:45]

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-11 21:10:50]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-08-11 21:12:53]

Description was changed from

==========
[Downloads] Use content:// URIs when sharing

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

to

==========
[Downloads] Use content:// URIs when sharing

BUG=616324

Committed: https://crrev.com/0a681ef8a944e744d31918da6b156c1159af24d7
Cr-Commit-Position: refs/heads/master@{#411419}
==========

[commit-bot: I haz the power, 2016-08-11 21:12:55]

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