Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2799533002:
Use file Uri to launch intent for download on M and below (Closed)
DescriptionUse file Uri to launch intent for download on M and below
Content Uri has permission issues on certain devices
Use file Uri instead.
BUG=705748
Review-Url: https://codereview.chromium.org/2799533002
Cr-Commit-Position: refs/heads/master@{#463698}
Committed: https://chromium.googlesource.com/chromium/src/+/ca6e72620f092e950e6ce788bde57455a4e73eb1
Patch Set 1 #
Total comments: 4
Patch Set 2 : add function in APiCompatibilityUtils #
Total comments: 4
Patch Set 3 : addressing comments #
Total comments: 2
Patch Set 4 : nit #
Messages
Total messages: 20 (7 generated)
qinmin@chromium.org changed reviewers: + dtrainor@chromium.org
PTAL
lgtm % nit. https://codereview.chromium.org/2799533002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/download/DownloadUtils.java (right): https://codereview.chromium.org/2799533002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/download/DownloadUtils.java:466: StrictMode.ThreadPolicy oldPolicy = StrictMode.allowThreadDiskReads(); Unrelated, but we're getting strict mode violations in DownloadManagerDelegate.getContentUriFromDownloadManager(). Should we be doing the same strict mode allow thing there as well? https://codereview.chromium.org/2799533002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/download/DownloadUtils.java:511: Uri uri = Build.VERSION.SDK_INT > Build.VERSION_CODES.M Should this be an ApiCompatibilityUtil method?
qinmin@chromium.org changed reviewers: + nyquist@chromium.org
+nyquist for OWNER of base/android https://codereview.chromium.org/2799533002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/download/DownloadUtils.java (right): https://codereview.chromium.org/2799533002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/download/DownloadUtils.java:466: StrictMode.ThreadPolicy oldPolicy = StrictMode.allowThreadDiskReads(); On 2017/04/05 22:25:48, David Trainor-ping if over 24h wrote: > Unrelated, but we're getting strict mode violations in > DownloadManagerDelegate.getContentUriFromDownloadManager(). Should we be doing > the same strict mode allow thing there as well? I think caller of that function should do that. getContentUriFromDownloadManager() is called in both DownloadManagerService and DownloadBroadcastReceiver. The former will not trigger the strict mode violation, so we only need it for the latter.
https://codereview.chromium.org/2799533002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/download/DownloadUtils.java (right): https://codereview.chromium.org/2799533002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/download/DownloadUtils.java:511: Uri uri = Build.VERSION.SDK_INT > Build.VERSION_CODES.M On 2017/04/05 22:25:48, David Trainor-ping if over 24h wrote: > Should this be an ApiCompatibilityUtil method? Done.
https://codereview.chromium.org/2799533002/diff/20001/base/android/java/src/o... File base/android/java/src/org/chromium/base/ApiCompatibilityUtils.java (right): https://codereview.chromium.org/2799533002/diff/20001/base/android/java/src/o... base/android/java/src/org/chromium/base/ApiCompatibilityUtils.java:624: * Get the URI for a downloaded file Nit: Missing period at the end. https://codereview.chromium.org/2799533002/diff/20001/base/android/java/src/o... File base/android/java/src/org/chromium/base/FileUtils.java (right): https://codereview.chromium.org/2799533002/diff/20001/base/android/java/src/o... base/android/java/src/org/chromium/base/FileUtils.java:105: StrictMode.ThreadPolicy oldPolicy = StrictMode.allowThreadDiskReads(); I'm not sure if we want to have this part in //base. This seems entirely up to the caller of this (//chrome in your case). Could the StrictMode violation handling just be in //chrome instead? To be honest, I would rather want to have an assert !ThreadUtils.runningOnUiThread() in the start of this method than this (you can still block on an async task or something similar in //chrome).
Patchset #3 (id:40001) has been deleted
https://codereview.chromium.org/2799533002/diff/20001/base/android/java/src/o... File base/android/java/src/org/chromium/base/ApiCompatibilityUtils.java (right): https://codereview.chromium.org/2799533002/diff/20001/base/android/java/src/o... base/android/java/src/org/chromium/base/ApiCompatibilityUtils.java:624: * Get the URI for a downloaded file On 2017/04/06 23:53:39, nyquist wrote: > Nit: Missing period at the end. Done. https://codereview.chromium.org/2799533002/diff/20001/base/android/java/src/o... File base/android/java/src/org/chromium/base/FileUtils.java (right): https://codereview.chromium.org/2799533002/diff/20001/base/android/java/src/o... base/android/java/src/org/chromium/base/FileUtils.java:105: StrictMode.ThreadPolicy oldPolicy = StrictMode.allowThreadDiskReads(); On 2017/04/06 23:53:39, nyquist wrote: > I'm not sure if we want to have this part in //base. This seems entirely up to > the caller of this (//chrome in your case). Could the StrictMode violation > handling just be in //chrome instead? > > To be honest, I would rather want to have an assert > !ThreadUtils.runningOnUiThread() in the start of this method than this (you can > still block on an async task or something similar in //chrome). Done. Moved the StrictMode violation to chrome/. I will leave the ThreadUtils.runningOnUiThread() to another Cl. The UI interaction with an AsyncTask raises some issues as mentioned in the comments. I need to check with Theresa/Dan to understand the preload problem with share functionalities.
https://codereview.chromium.org/2799533002/diff/60001/base/android/java/src/o... File base/android/java/src/org/chromium/base/FileUtils.java (right): https://codereview.chromium.org/2799533002/diff/60001/base/android/java/src/o... base/android/java/src/org/chromium/base/FileUtils.java:97: Uri uri = null; Can you file a bug and add a TODO here referring to the bug for adding the assert? In fact, even better, could you do this: public static Uri getUriForFile(File file) // TODO(crbug/FOO): Uncomment this when ... has been fixed. // assert !ThreadUtils.runningOnUiThread();
https://codereview.chromium.org/2799533002/diff/60001/base/android/java/src/o... File base/android/java/src/org/chromium/base/FileUtils.java (right): https://codereview.chromium.org/2799533002/diff/60001/base/android/java/src/o... base/android/java/src/org/chromium/base/FileUtils.java:97: Uri uri = null; On 2017/04/07 19:03:22, nyquist wrote: > Can you file a bug and add a TODO here referring to the bug for adding the > assert? > In fact, even better, could you do this: > > > public static Uri getUriForFile(File file) > // TODO(crbug/FOO): Uncomment this when ... has been fixed. > // assert !ThreadUtils.runningOnUiThread(); Done.
ping, nyquist@, is this CL good to go?
ping, nyquist@, is this CL good to go?
thanks for the ping! lgtm Thanks for doing the follow up too!
The CQ bit was checked by qinmin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dtrainor@chromium.org Link to the patchset: https://codereview.chromium.org/2799533002/#ps80001 (title: "nit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1491932554210640,
"parent_rev": "20017d2b1fcf1d125715da9b32ea695600d45b07", "commit_rev":
"ca6e72620f092e950e6ce788bde57455a4e73eb1"}
Message was sent while issue was closed.
Description was changed from ========== Use file Uri to launch intent for download on M and below Content Uri has permission issues on certain devices Use file Uri instead. BUG=705748 ========== to ========== Use file Uri to launch intent for download on M and below Content Uri has permission issues on certain devices Use file Uri instead. BUG=705748 Review-Url: https://codereview.chromium.org/2799533002 Cr-Commit-Position: refs/heads/master@{#463698} Committed: https://chromium.googlesource.com/chromium/src/+/ca6e72620f092e950e6ce788bde5... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:80001) as https://chromium.googlesource.com/chromium/src/+/ca6e72620f092e950e6ce788bde5... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
