[Downloads] Use content:// URIs in more places

[Downloads] Use content:// URIs in more places

* Use content:// URIs in more places instead of file:// URIs.  This avoids
  problems with Android N+ complaining about passing around file:// URIs.

* Allow Intents for Media Viewer Custom Tabs to have an additional URI that can
  override the one passed along via the Intent's data.  The file:// URI is
  necessary to deal with Custom Tabs displaying "content:" and a database ID
  instead of the filename and its location.

BUG=658012
Review-Url: https://codereview.chromium.org/2671573002
Cr-Commit-Position: refs/heads/master@{#447821}
Committed: https://chromium.googlesource.com/chromium/src/+/ef71dc0ec1f189d366e294f644ceeacbc7dab48a

[gone, 2017-02-01 23:46:24]

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

[gone, 2017-02-01 23:46:25]

Yusuf: Custom tabs changes
Qin & Theresa: Changes to URI handling in downloads code

[Theresa, 2017-02-01 23:59:11]

Looks good overall, just one question

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

https://codereview.chromium.org/2671573002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadUtils.java:457:
if (uri == null) uri = Uri.fromFile(file);
I wonder if this could still cause issues.. I'm not sure what to fall back on if
not a file URI, though.

[gone, 2017-02-02 00:03:15]

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

https://codereview.chromium.org/2671573002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadUtils.java:457:
if (uri == null) uri = Uri.fromFile(file);
On 2017/02/01 23:59:10, Theresa wrote:
> I wonder if this could still cause issues.. I'm not sure what to fall back on
if
> not a file URI, though.

Yeah, honestly wasn't sure what to do here :/  Best I figured we could do is
hope that the Intents we fire grant read access to the URI that we pass along.

[Theresa, 2017-02-02 00:09:08]

On 2017/02/02 00:03:15, dfalcantara (load balance plz) wrote:
>
https://codereview.chromium.org/2671573002/diff/1/chrome/android/java/src/org...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadUtils.java
> (right):
> 
>
https://codereview.chromium.org/2671573002/diff/1/chrome/android/java/src/org...
>
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadUtils.java:457:
> if (uri == null) uri = Uri.fromFile(file);
> On 2017/02/01 23:59:10, Theresa wrote:
> > I wonder if this could still cause issues.. I'm not sure what to fall back
on
> if
> > not a file URI, though.
> 
> Yeah, honestly wasn't sure what to do here :/  Best I figured we could do is
> hope that the Intents we fire grant read access to the URI that we pass along.

lgtm

[qinmin, 2017-02-02 17:29:43]

lgtm % nit

https://codereview.chromium.org/2671573002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabIntentDataProvider.java
(right):

https://codereview.chromium.org/2671573002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabIntentDataProvider.java:186:
mMediaViewerUrl = mIsMediaViewer
is the if condition needed?
IntentUtils.safeGetStringExtra will return null by itself, and mIsMediaViewer
should aklways be true when EXTRA_MEDIA_VIEWER_URL is around?

[Yusuf, 2017-02-02 17:54:17]

lgtm

[gone, 2017-02-02 18:38:50]

https://codereview.chromium.org/2671573002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabIntentDataProvider.java
(right):

https://codereview.chromium.org/2671573002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabIntentDataProvider.java:186:
mMediaViewerUrl = mIsMediaViewer
On 2017/02/02 17:29:42, qinmin wrote:
> is the if condition needed?
> IntentUtils.safeGetStringExtra will return null by itself, and mIsMediaViewer
> should aklways be true when EXTRA_MEDIA_VIEWER_URL is around?
> 

It's a security check; we require the Intent to be trusted (via mIsMediaViewer)
to prevent malicious third-parties from overriding the URL, in case the code
changes later in getUrlToLoad().

[gone, 2017-02-02 18:38:54]

The CQ bit was checked by dfalcantara@chromium.org

[commit-bot: I haz the power, 2017-02-02 18:39: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, 2017-02-02 19:40:40]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1486060734736460, "parent_rev":
"bad0d77fc89ded426e7ee6723fdd3d4433ea5325", "commit_rev":
"ef71dc0ec1f189d366e294f644ceeacbc7dab48a"}

[commit-bot: I haz the power, 2017-02-02 19:41:16]

Description was changed from

==========
[Downloads] Use content:// URIs in more places

* Use content:// URIs in more places instead of file:// URIs.  This avoids
  problems with Android N+ complaining about passing around file:// URIs.

* Allow Intents for Media Viewer Custom Tabs to have an additional URI that can
  override the one passed along via the Intent's data.  The file:// URI is
  necessary to deal with Custom Tabs displaying "content:" and a database ID
  instead of the filename and its location.

BUG=658012
==========

to

==========
[Downloads] Use content:// URIs in more places

* Use content:// URIs in more places instead of file:// URIs.  This avoids
  problems with Android N+ complaining about passing around file:// URIs.

* Allow Intents for Media Viewer Custom Tabs to have an additional URI that can
  override the one passed along via the Intent's data.  The file:// URI is
  necessary to deal with Custom Tabs displaying "content:" and a database ID
  instead of the filename and its location.

BUG=658012

Review-Url: https://codereview.chromium.org/2671573002
Cr-Commit-Position: refs/heads/master@{#447821}
Committed:
https://chromium.googlesource.com/chromium/src/+/ef71dc0ec1f189d366e294f644ce...
==========

[commit-bot: I haz the power, 2017-02-02 19:41:20]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/ef71dc0ec1f189d366e294f644ce...