Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
489053003:
Use content URI to upload photos taken by camera (Closed)
|
Created:
6 years, 4 months ago by qinmin Modified:
6 years, 3 months ago CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionUse content URI to upload photos taken by camera
Currently chrome creates a file path when uploading a photo taken through camera
However, this doesn't always work.
Instead, we should use content URI. See more details in the crbug.
BUG=405593
Committed: https://crrev.com/22b41158f1be60572f4eaa1da8676c36f69cb394
Cr-Commit-Position: refs/heads/master@{#292257}
Patch Set 1 : #
Total comments: 10
Patch Set 2 : addressing tedchoc's comments #
Total comments: 6
Patch Set 3 : addressing palmer's comments #Patch Set 4 : moving support.v4 import from ui/ to chrome/ #
Total comments: 10
Patch Set 5 : use static variable to replace singleton #Patch Set 6 : add thread check #
Messages
Total messages: 43 (0 generated)
PTAL
overall seems reasonable, but I think you'll want a pair of security eyes since you're exposing a new provider (+cpalmer for a quick look). https://codereview.chromium.org/489053003/diff/40001/chrome/android/shell/jav... File chrome/android/shell/java/AndroidManifest.xml (right): https://codereview.chromium.org/489053003/diff/40001/chrome/android/shell/jav... chrome/android/shell/java/AndroidManifest.xml:225: <meta-data android:name="android.support.FILE_PROVIDER_PATHS" based on this name alone, I assume this is needed by the support library? https://codereview.chromium.org/489053003/diff/40001/chrome/android/shell/jav... chrome/android/shell/java/AndroidManifest.xml:226: android:resource="@xml/file_paths" /> +3 indent https://codereview.chromium.org/489053003/diff/40001/ui/android/java/src/org/... File ui/android/java/src/org/chromium/ui/base/SelectFileDialog.java (right): https://codereview.chromium.org/489053003/diff/40001/ui/android/java/src/org/... ui/android/java/src/org/chromium/ui/base/SelectFileDialog.java:45: private static final String IMAGE_FILE_PATH = "images"; can this not be read from the xml file? https://codereview.chromium.org/489053003/diff/40001/ui/android/java/src/org/... ui/android/java/src/org/chromium/ui/base/SelectFileDialog.java:72: camera.setFlags(Intent.FLAG_GRANT_READ_URI_PERMISSION + although these are 1 & 2 as values, it seems like |'ing is the correct thing to do for flags https://codereview.chromium.org/489053003/diff/40001/ui/android/java/src/org/... ui/android/java/src/org/chromium/ui/base/SelectFileDialog.java:79: ClipData.newUri(context.getContentResolver(), "images", mCameraOutputUri)); and should this "images" be a reference to the constant up above?
palmer@ or cpalmer@? I think you mean palmer@
https://codereview.chromium.org/489053003/diff/40001/chrome/android/shell/jav... File chrome/android/shell/java/AndroidManifest.xml (right): https://codereview.chromium.org/489053003/diff/40001/chrome/android/shell/jav... chrome/android/shell/java/AndroidManifest.xml:225: <meta-data android:name="android.support.FILE_PROVIDER_PATHS" On 2014/08/21 00:54:51, Ted C wrote: > based on this name alone, I assume this is needed by the support library? Yes, or otherwise chrome will not start. https://codereview.chromium.org/489053003/diff/40001/chrome/android/shell/jav... chrome/android/shell/java/AndroidManifest.xml:226: android:resource="@xml/file_paths" /> On 2014/08/21 00:54:51, Ted C wrote: > +3 indent Done. https://codereview.chromium.org/489053003/diff/40001/ui/android/java/src/org/... File ui/android/java/src/org/chromium/ui/base/SelectFileDialog.java (right): https://codereview.chromium.org/489053003/diff/40001/ui/android/java/src/org/... ui/android/java/src/org/chromium/ui/base/SelectFileDialog.java:45: private static final String IMAGE_FILE_PATH = "images"; the <file-path> will not generate a resource id in R.java. So we have to code it here. On 2014/08/21 00:54:51, Ted C wrote: > can this not be read from the xml file? https://codereview.chromium.org/489053003/diff/40001/ui/android/java/src/org/... ui/android/java/src/org/chromium/ui/base/SelectFileDialog.java:72: camera.setFlags(Intent.FLAG_GRANT_READ_URI_PERMISSION + On 2014/08/21 00:54:51, Ted C wrote: > although these are 1 & 2 as values, it seems like |'ing is the correct thing to > do for flags Done. https://codereview.chromium.org/489053003/diff/40001/ui/android/java/src/org/... ui/android/java/src/org/chromium/ui/base/SelectFileDialog.java:79: ClipData.newUri(context.getContentResolver(), "images", mCameraOutputUri)); This value doesn't seem to impact anything. In the document referenced in the crbug, they mentioned that this line is just a hack for JB/K devices. Anyway, changed it to use the constant above. On 2014/08/21 00:54:51, Ted C wrote: > and should this "images" be a reference to the constant up above?
palmer@, would you please take a look at the security impacts on this CL? Please check the crbug for the problem description. Thanks
lgtm from my side (although I will admit I know little of the provider side of things here).
https://codereview.chromium.org/489053003/diff/60001/ui/android/java/src/org/... File ui/android/java/src/org/chromium/ui/base/SelectFileDialog.java (right): https://codereview.chromium.org/489053003/diff/60001/ui/android/java/src/org/... ui/android/java/src/org/chromium/ui/base/SelectFileDialog.java:45: private static final String IMAGE_FILE_PATH = "images"; Comment that this has to be the same as the resource in the XML? https://codereview.chromium.org/489053003/diff/60001/ui/android/java/src/org/... ui/android/java/src/org/chromium/ui/base/SelectFileDialog.java:145: File photoFile = new File(path + File.separator + System.currentTimeMillis() + There is an extremely tiny chance that this method could try to create the same pathname twice. Additionally, the pathname is predictable; that probably doesn't matter in this case since the pathname is under context.getFilesDir(). Instead, use File.createTempFile. https://codereview.chromium.org/489053003/diff/60001/ui/android/ui_android.gyp File ui/android/ui_android.gyp (right): https://codereview.chromium.org/489053003/diff/60001/ui/android/ui_android.gy... ui/android/ui_android.gyp:48: '../../third_party/android_tools/android_tools.gyp:android_support_v13_javalib', Out of curiosity, what is this for?
https://codereview.chromium.org/489053003/diff/60001/ui/android/java/src/org/... File ui/android/java/src/org/chromium/ui/base/SelectFileDialog.java (right): https://codereview.chromium.org/489053003/diff/60001/ui/android/java/src/org/... ui/android/java/src/org/chromium/ui/base/SelectFileDialog.java:45: private static final String IMAGE_FILE_PATH = "images"; On 2014/08/21 22:21:28, Chromium Palmer wrote: > Comment that this has to be the same as the resource in the XML? Done. https://codereview.chromium.org/489053003/diff/60001/ui/android/java/src/org/... ui/android/java/src/org/chromium/ui/base/SelectFileDialog.java:145: File photoFile = new File(path + File.separator + System.currentTimeMillis() + On 2014/08/21 22:21:28, Chromium Palmer wrote: > There is an extremely tiny chance that this method could try to create the same > pathname twice. Additionally, the pathname is predictable; that probably doesn't > matter in this case since the pathname is under context.getFilesDir(). Instead, > use File.createTempFile. Done. https://codereview.chromium.org/489053003/diff/60001/ui/android/ui_android.gyp File ui/android/ui_android.gyp (right): https://codereview.chromium.org/489053003/diff/60001/ui/android/ui_android.gy... ui/android/ui_android.gyp:48: '../../third_party/android_tools/android_tools.gyp:android_support_v13_javalib', On 2014/08/21 22:21:28, Chromium Palmer wrote: > Out of curiosity, what is this for? this is for the android.support.v4.content.FileProvider import. v4 lib is included in v13. We have it for chrome but not base/ui
lgtm
The CQ bit was checked by qinmin@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/qinmin@chromium.org/489053003/80001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_aosp on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...) android_chromium_gn_compile_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...) android_clang_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...) android_dbg_tests_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_chromium_gn_compile_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
hey min, sorry for the slow response here :-( We are going to need something like this for WebView too, however i'm not quite sure how we hook up the FileProvider there. Let me try a few things - would you mind holding off landing just now? Until we can check the fix works for us too and we don't need to change it later?
On 2014/08/22 10:00:18, benm wrote: > hey min, sorry for the slow response here :-( > > We are going to need something like this for WebView too, however i'm not quite > sure how we hook up the FileProvider there. Let me try a few things - would you > mind holding off landing just now? Until we can check the fix works for us too > and we don't need to change it later? ok, i will hold off the change now. What's special about FileProvider? Is it because the support.v4 library?
On 2014/08/22 15:23:17, qinmin wrote: > On 2014/08/22 10:00:18, benm wrote: > > hey min, sorry for the slow response here :-( > > > > We are going to need something like this for WebView too, however i'm not > quite > > sure how we hook up the FileProvider there. Let me try a few things - would > you > > mind holding off landing just now? Until we can check the fix works for us too > > and we don't need to change it later? > > ok, i will hold off the change now. What's special about FileProvider? Is it > because the support.v4 library? Sorry, I forgot that WebView no longer uses this code path, so we can fix this up ourselves and this change won't affect us. My concern was not the FileProvider code, it's the xml paths file that you have to specify and how we might do that for WebView given it's not an app itself.
Patchset #5 (id:120001) has been deleted
Patchset #4 (id:100001) has been deleted
qinmin@chromium.org changed reviewers: + yfriedman@chromium.org
After chatting with @boliu, the support.v4 library cannot be imported by any classes in ui/. So moving that import into chrome/. WebView doesn't use this camera upload path, so it is not impacted. +yfriedman for OWNER of base/android/ +jochen for OWNER of chrome/ +weitaosu for OWNER of remoting/ (support.v4 is included in support.v13)
qinmin@chromium.org changed reviewers: + thakis@chromium.org, weitaosu@chromium.org
+thakis since jochen is out. +weitaosu PTAL
https://codereview.chromium.org/489053003/diff/140001/chrome/chrome.gyp File chrome/chrome.gyp (right): https://codereview.chromium.org/489053003/diff/140001/chrome/chrome.gyp#newco... chrome/chrome.gyp:656: '../third_party/android_tools/android_tools.gyp:android_support_v13_javalib', Huh, do we really have to support at two different versions of the same library?
https://codereview.chromium.org/489053003/diff/140001/chrome/chrome.gyp File chrome/chrome.gyp (right): https://codereview.chromium.org/489053003/diff/140001/chrome/chrome.gyp#newco... chrome/chrome.gyp:656: '../third_party/android_tools/android_tools.gyp:android_support_v13_javalib', These are not simply two different versions, the java classes are different. V13 includes all the java imports for android.support.v13.* and android.support.v4.* v7 includes all the java imports for android.support.v7.* On 2014/08/25 18:25:10, Nico (hiding) wrote: > Huh, do we really have to support at two different versions of the same library?
Ok, chrome/ lgtm. Thanks for explaining :-)
https://codereview.chromium.org/489053003/diff/140001/base/android/java/src/o... File base/android/java/src/org/chromium/base/ContentUriUtils.java (right): https://codereview.chromium.org/489053003/diff/140001/base/android/java/src/o... base/android/java/src/org/chromium/base/ContentUriUtils.java:25: * Class for providing functionalities to translate a file into content URI. Provides functionality to translate a file into a content URI for use with a content provider. https://codereview.chromium.org/489053003/diff/140001/base/android/java/src/o... base/android/java/src/org/chromium/base/ContentUriUtils.java:39: public static ContentUriUtils getInstance() { How is this related to FileProviderUtil? Why do we need a singleton? https://codereview.chromium.org/489053003/diff/140001/base/android/java/src/o... base/android/java/src/org/chromium/base/ContentUriUtils.java:48: if (mFileProviderUtil != null) { Which threads do you anticipate being involved? If more than one, this is currently racey
https://codereview.chromium.org/489053003/diff/140001/base/android/java/src/o... File base/android/java/src/org/chromium/base/ContentUriUtils.java (right): https://codereview.chromium.org/489053003/diff/140001/base/android/java/src/o... base/android/java/src/org/chromium/base/ContentUriUtils.java:25: * Class for providing functionalities to translate a file into content URI. On 2014/08/25 19:13:05, Yaron wrote: > Provides functionality to translate a file into a content URI for use with a > content provider. Done. https://codereview.chromium.org/489053003/diff/140001/base/android/java/src/o... base/android/java/src/org/chromium/base/ContentUriUtils.java:39: public static ContentUriUtils getInstance() { Since FileProviderUtil implementation exits in chrome/, we need to pass it to ui/. We can either: 1. Declare this as a singleton so ui/ can get FileProviderUtil from the singleton instance. 2. Declare mFileProviderUtil as static and have a static setter function. Switched to option 2, anyway On 2014/08/25 19:13:05, Yaron wrote: > How is this related to FileProviderUtil? Why do we need a singleton? https://codereview.chromium.org/489053003/diff/140001/base/android/java/src/o... base/android/java/src/org/chromium/base/ContentUriUtils.java:48: if (mFileProviderUtil != null) { This should be on the UI thread. On 2014/08/25 19:13:05, Yaron wrote: > Which threads do you anticipate being involved? If more than one, this is > currently racey
lgtm https://codereview.chromium.org/489053003/diff/140001/base/android/java/src/o... File base/android/java/src/org/chromium/base/ContentUriUtils.java (right): https://codereview.chromium.org/489053003/diff/140001/base/android/java/src/o... base/android/java/src/org/chromium/base/ContentUriUtils.java:48: if (mFileProviderUtil != null) { On 2014/08/25 20:27:19, qinmin wrote: > This should be on the UI thread. > > On 2014/08/25 19:13:05, Yaron wrote: > > Which threads do you anticipate being involved? If more than one, this is > > currently racey > Then please add ThreadUtils.assertOnUiThread
https://codereview.chromium.org/489053003/diff/140001/base/android/java/src/o... File base/android/java/src/org/chromium/base/ContentUriUtils.java (right): https://codereview.chromium.org/489053003/diff/140001/base/android/java/src/o... base/android/java/src/org/chromium/base/ContentUriUtils.java:48: if (mFileProviderUtil != null) { On 2014/08/25 23:37:33, Yaron wrote: > On 2014/08/25 20:27:19, qinmin wrote: > > This should be on the UI thread. > > > > On 2014/08/25 19:13:05, Yaron wrote: > > > Which threads do you anticipate being involved? If more than one, this is > > > currently racey > > > > Then please add ThreadUtils.assertOnUiThread Done.
@weitaosu, would you please take a look at remoting/? we shouldn't use the individual jar file under android_tools, the v4 imports are included in v13 library
qinmin@chromium.org changed reviewers: + lambroslambrou@chromium.org, sergeyu@chromium.org - weitaosu@chromium.org
+sergeyu and lambroslambrou for OWNER of remoting/
Patchset #6 (id:180001) has been deleted
Patchset #6 (id:200001) has been deleted
ping, @sergeyu, would you please take a look at remoting/? thanks
remoting lgtm
The CQ bit was checked by qinmin@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/qinmin@chromium.org/489053003/220001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: ios_dbg_simulator on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Message was sent while issue was closed.
Committed patchset #6 (id:220001) as d394007088233f0182e775f1b1ca7cfb641af757
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/22b41158f1be60572f4eaa1da8676c36f69cb394 Cr-Commit-Position: refs/heads/master@{#292257} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
