Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
450003002:
Pass display names for uploaded content URI files (Closed)
|
Created:
6 years, 4 months ago by qinmin Modified:
6 years, 4 months ago CC:
chromium-reviews, android-webview-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionPass display names for uploaded content URI files
The display names are missing for content URI files.
That will cause some upload to fail.
This CL translates the content URI to displayed name and passes them to native code.
BUG=402537
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=289102
Patch Set 1 #Patch Set 2 : fix NPE since results can be null #
Total comments: 8
Patch Set 3 : addressing comments #
Total comments: 4
Patch Set 4 : move getDisplayName(Uri, ContentResolver) to ContentUriUtils #
Total comments: 3
Patch Set 5 : move queries on ContentResolver to another thread #
Total comments: 4
Patch Set 6 : nits #
Messages
Total messages: 21 (0 generated)
PTAL
I don't really understand this area of code, just some general comments... https://codereview.chromium.org/450003002/diff/20001/android_webview/java/src... File android_webview/java/src/org/chromium/android_webview/AwWebContentsDelegateAdapter.java (right): https://codereview.chromium.org/450003002/diff/20001/android_webview/java/src... android_webview/java/src/org/chromium/android_webview/AwWebContentsDelegateAdapter.java:34: Context mContext; final https://codereview.chromium.org/450003002/diff/20001/android_webview/java/src... android_webview/java/src/org/chromium/android_webview/AwWebContentsDelegateAdapter.java:188: ContentResolver resolver = mContext.getContentResolver(); You are not using this variable here https://codereview.chromium.org/450003002/diff/20001/android_webview/java/src... android_webview/java/src/org/chromium/android_webview/AwWebContentsDelegateAdapter.java:226: private String resolveFileName(String filePath) { how expensive is this to do on the UI thread?
https://codereview.chromium.org/450003002/diff/20001/android_webview/java/src... File android_webview/java/src/org/chromium/android_webview/AwWebContentsDelegateAdapter.java (right): https://codereview.chromium.org/450003002/diff/20001/android_webview/java/src... android_webview/java/src/org/chromium/android_webview/AwWebContentsDelegateAdapter.java:34: Context mContext; On 2014/08/08 00:59:59, boliu wrote: > final Done. https://codereview.chromium.org/450003002/diff/20001/android_webview/java/src... android_webview/java/src/org/chromium/android_webview/AwWebContentsDelegateAdapter.java:188: ContentResolver resolver = mContext.getContentResolver(); On 2014/08/08 00:59:59, boliu wrote: > You are not using this variable here Done. Removed https://codereview.chromium.org/450003002/diff/20001/android_webview/java/src... android_webview/java/src/org/chromium/android_webview/AwWebContentsDelegateAdapter.java:226: private String resolveFileName(String filePath) { No idea, but should be pretty cheap compared to the action for user to select all the files to upload. In Chrome on android, the translation between content URI to display name also happens on UI thread. On 2014/08/08 00:59:59, boliu wrote: > how expensive is this to do on the UI thread?
https://codereview.chromium.org/450003002/diff/40001/android_webview/java/src... File android_webview/java/src/org/chromium/android_webview/AwWebContentsDelegateAdapter.java (right): https://codereview.chromium.org/450003002/diff/40001/android_webview/java/src... android_webview/java/src/org/chromium/android_webview/AwWebContentsDelegateAdapter.java:247: return ""; If we can't get a display name, will the upload fail in the same way that the current flow does (i.e. using the display name out of the content:// URI)? Do we let the user know that they can't upload that kind of file in some way?
https://codereview.chromium.org/450003002/diff/40001/android_webview/java/src... File android_webview/java/src/org/chromium/android_webview/AwWebContentsDelegateAdapter.java (right): https://codereview.chromium.org/450003002/diff/40001/android_webview/java/src... android_webview/java/src/org/chromium/android_webview/AwWebContentsDelegateAdapter.java:247: return ""; I think the first answer depends on website. If display name is not passed, some website will still upload the file, but without displaying the correct name on the file(i think 163 mail is the case). Thus causing other issues while downloading or displaying the uploaded file. Unfortunately, the java side doesn't know whether upload will fail at this stage. The logic rests in the javascript/server side. On 2014/08/08 12:08:58, benm wrote: > If we can't get a display name, will the upload fail in the same way that the > current flow does (i.e. using the display name out of the content:// URI)? > > Do we let the user know that they can't upload that kind of file in some way?
Bo/Ben, is this good to go?
https://codereview.chromium.org/450003002/diff/40001/android_webview/java/src... File android_webview/java/src/org/chromium/android_webview/AwWebContentsDelegateAdapter.java (right): https://codereview.chromium.org/450003002/diff/40001/android_webview/java/src... android_webview/java/src/org/chromium/android_webview/AwWebContentsDelegateAdapter.java:225: private String resolveFileName(String filePath) { Can we expose the method on SelectFileDialog.java instead of duplicating it? Looks like SelectFileDialog.resolveFileName should be static anyway.
https://codereview.chromium.org/450003002/diff/20001/android_webview/java/src... File android_webview/java/src/org/chromium/android_webview/AwWebContentsDelegateAdapter.java (right): https://codereview.chromium.org/450003002/diff/20001/android_webview/java/src... android_webview/java/src/org/chromium/android_webview/AwWebContentsDelegateAdapter.java:226: private String resolveFileName(String filePath) { On 2014/08/08 01:20:58, qinmin wrote: > No idea, but should be pretty cheap compared to the action for user to select > all the files to upload. > In Chrome on android, the translation between content URI to display name also > happens on UI thread. > > On 2014/08/08 00:59:59, boliu wrote: > > how expensive is this to do on the UI thread? > it is not suggested to do contentresolver queries on UI thread (due to latency) http://developer.android.com/guide/topics/providers/content-provider-basics.h... However in this case I did not think on enough to see if it makes sense to use an async task or not. I think you should at least put a comment explaining why it cannot be done async.
On 2014/08/11 17:10:11, sgurun wrote: > https://codereview.chromium.org/450003002/diff/20001/android_webview/java/src... > File > android_webview/java/src/org/chromium/android_webview/AwWebContentsDelegateAdapter.java > (right): > > https://codereview.chromium.org/450003002/diff/20001/android_webview/java/src... > android_webview/java/src/org/chromium/android_webview/AwWebContentsDelegateAdapter.java:226: > private String resolveFileName(String filePath) { > On 2014/08/08 01:20:58, qinmin wrote: > > No idea, but should be pretty cheap compared to the action for user to select > > all the files to upload. > > In Chrome on android, the translation between content URI to display name also > > happens on UI thread. > > > > On 2014/08/08 00:59:59, boliu wrote: > > > how expensive is this to do on the UI thread? > > > > it is not suggested to do contentresolver queries on UI thread (due to latency) > > http://developer.android.com/guide/topics/providers/content-provider-basics.h... > > However in this case I did not think on enough to see if it makes sense to use > an async task or not. I think you should at least put a comment explaining why > it cannot be done async. Do we have a bug number for this? I guess most of the discussion is kept in an email thread?
filed crbug.com/402537 for this https://codereview.chromium.org/450003002/diff/20001/android_webview/java/src... File android_webview/java/src/org/chromium/android_webview/AwWebContentsDelegateAdapter.java (right): https://codereview.chromium.org/450003002/diff/20001/android_webview/java/src... android_webview/java/src/org/chromium/android_webview/AwWebContentsDelegateAdapter.java:226: private String resolveFileName(String filePath) { ok, added some comments and a TODO there. On 2014/08/11 17:10:11, sgurun wrote: > On 2014/08/08 01:20:58, qinmin wrote: > > No idea, but should be pretty cheap compared to the action for user to select > > all the files to upload. > > In Chrome on android, the translation between content URI to display name also > > happens on UI thread. > > > > On 2014/08/08 00:59:59, boliu wrote: > > > how expensive is this to do on the UI thread? > > > > it is not suggested to do contentresolver queries on UI thread (due to latency) > > http://developer.android.com/guide/topics/providers/content-provider-basics.h... > > However in this case I did not think on enough to see if it makes sense to use > an async task or not. I think you should at least put a comment explaining why > it cannot be done async. https://codereview.chromium.org/450003002/diff/40001/android_webview/java/src... File android_webview/java/src/org/chromium/android_webview/AwWebContentsDelegateAdapter.java (right): https://codereview.chromium.org/450003002/diff/40001/android_webview/java/src... android_webview/java/src/org/chromium/android_webview/AwWebContentsDelegateAdapter.java:225: private String resolveFileName(String filePath) { Moved the method to ContentUriUtils.java in base. On 2014/08/11 16:58:07, boliu wrote: > Can we expose the method on SelectFileDialog.java instead of duplicating it? > > Looks like SelectFileDialog.resolveFileName should be static anyway.
I'll let selim stamp this since he has a bit more experience here. I'm ok with PS4 just for matching chrome behavior. I do think it should be happening off UI thread in chrome as well. https://codereview.chromium.org/450003002/diff/60001/android_webview/java/src... File android_webview/java/src/org/chromium/android_webview/AwWebContentsDelegateAdapter.java (right): https://codereview.chromium.org/450003002/diff/60001/android_webview/java/src... android_webview/java/src/org/chromium/android_webview/AwWebContentsDelegateAdapter.java:226: // However, that is incomparable to the latency introduce by user interaction That's a bad argument, because this delay is *after* waiting for user, not *while* waiting for user.
https://codereview.chromium.org/450003002/diff/60001/android_webview/java/src... File android_webview/java/src/org/chromium/android_webview/AwWebContentsDelegateAdapter.java (right): https://codereview.chromium.org/450003002/diff/60001/android_webview/java/src... android_webview/java/src/org/chromium/android_webview/AwWebContentsDelegateAdapter.java:226: // However, that is incomparable to the latency introduce by user interaction agreed with Bo. This is after user already chose the value, let's not increase any jank on UI thread. Please use an asynctask. An asynctask does the processing in a background thread and publishes the result in a UI thread, which then simply can call this. On 2014/08/11 19:58:14, boliu wrote: > That's a bad argument, because this delay is *after* waiting for user, not > *while* waiting for user.
https://codereview.chromium.org/450003002/diff/60001/android_webview/java/src... File android_webview/java/src/org/chromium/android_webview/AwWebContentsDelegateAdapter.java (right): https://codereview.chromium.org/450003002/diff/60001/android_webview/java/src... android_webview/java/src/org/chromium/android_webview/AwWebContentsDelegateAdapter.java:226: // However, that is incomparable to the latency introduce by user interaction Ok, moved all the contentResolver queries to an async task. On 2014/08/11 21:23:24, sgurun wrote: > agreed with Bo. This is after user already chose the value, let's not increase > any jank on UI thread. > Please use an asynctask. An asynctask does the processing in a background thread > and publishes the result in a UI thread, which then simply can call this. > > On 2014/08/11 19:58:14, boliu wrote: > > That's a bad argument, because this delay is *after* waiting for user, not > > *while* waiting for user. >
On 2014/08/11 22:49:29, qinmin wrote: > https://codereview.chromium.org/450003002/diff/60001/android_webview/java/src... > File > android_webview/java/src/org/chromium/android_webview/AwWebContentsDelegateAdapter.java > (right): > > https://codereview.chromium.org/450003002/diff/60001/android_webview/java/src... > android_webview/java/src/org/chromium/android_webview/AwWebContentsDelegateAdapter.java:226: > // However, that is incomparable to the latency introduce by user interaction > Ok, moved all the contentResolver queries to an async task. > > On 2014/08/11 21:23:24, sgurun wrote: > > agreed with Bo. This is after user already chose the value, let's not increase > > any jank on UI thread. > > Please use an asynctask. An asynctask does the processing in a background > thread > > and publishes the result in a UI thread, which then simply can call this. > > > > On 2014/08/11 19:58:14, boliu wrote: > > > That's a bad argument, because this delay is *after* waiting for user, not > > > *while* waiting for user. > > lgtm
+yaron for OWNER of ui/android and base/android
https://codereview.chromium.org/450003002/diff/80001/android_webview/java/src... File android_webview/java/src/org/chromium/android_webview/AwWebContentsDelegateAdapter.java (right): https://codereview.chromium.org/450003002/diff/80001/android_webview/java/src... android_webview/java/src/org/chromium/android_webview/AwWebContentsDelegateAdapter.java:218: private class GetDisplayNameTask extends AsyncTask<Void, Void, String[]> { Why not static? https://codereview.chromium.org/450003002/diff/80001/base/android/java/src/or... File base/android/java/src/org/chromium/base/ContentUriUtils.java (right): https://codereview.chromium.org/450003002/diff/80001/base/android/java/src/or... base/android/java/src/org/chromium/base/ContentUriUtils.java:85: public static String getDisplayName(Uri uri, ContentResolver contentResolver) { This seems like a misnomer as it it's hard-coded to MediaStore.MediaColumns.DISPLAY_NAME. I'd be happier with layering if that column was passed in, but is this a generalizable concept or are you just putting it here for layering reasons?
https://codereview.chromium.org/450003002/diff/80001/android_webview/java/src... File android_webview/java/src/org/chromium/android_webview/AwWebContentsDelegateAdapter.java (right): https://codereview.chromium.org/450003002/diff/80001/android_webview/java/src... android_webview/java/src/org/chromium/android_webview/AwWebContentsDelegateAdapter.java:218: private class GetDisplayNameTask extends AsyncTask<Void, Void, String[]> { On 2014/08/12 00:28:54, Yaron wrote: > Why not static? Done. https://codereview.chromium.org/450003002/diff/80001/base/android/java/src/or... File base/android/java/src/org/chromium/base/ContentUriUtils.java (right): https://codereview.chromium.org/450003002/diff/80001/base/android/java/src/or... base/android/java/src/org/chromium/base/ContentUriUtils.java:85: public static String getDisplayName(Uri uri, ContentResolver contentResolver) { Ok, changed the method to pass the column name into this method. Since most user uploads are uploading photos/videos from the gallary, so far MediaColumns.DISPLAY_NAME is the only thing we know that is being affected. There might be others in the future, so we probably might have to add checks to the URI if that happens. On 2014/08/12 00:28:54, Yaron wrote: > This seems like a misnomer as it it's hard-coded to > MediaStore.MediaColumns.DISPLAY_NAME. I'd be happier with layering if that > column was passed in, but is this a generalizable concept or are you just > putting it here for layering reasons?
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/450003002/140001
Message was sent while issue was closed.
Change committed as 289102 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
