Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2810773002:
Photo Picker Dialog: Recursively traverse the photo directories. (Closed)
DescriptionPhoto Picker Dialog: Recursively traverse the photo directories.
(Only visible change, since the picker is not decoding yet, is that
it will show as many gray squares as there are photos on disk).
BUG=656015
Review-Url: https://codereview.chromium.org/2810773002
Cr-Commit-Position: refs/heads/master@{#464641}
Committed: https://chromium.googlesource.com/chromium/src/+/a106f0abbf69dad349d4aaf4bcc4f5d376dd2377
Patch Set 1 #Patch Set 2 : Augment comment #
Total comments: 13
Patch Set 3 : Address Theresa's feedback #
Total comments: 14
Patch Set 4 : Address feedback from Theresa #
Total comments: 8
Patch Set 5 : Address feedback from Theresa #
Total comments: 1
Patch Set 6 : Add todo #
Messages
Total messages: 36 (24 generated)
The CQ bit was checked by finnur@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by finnur@chromium.org to run a CQ dry run
Description was changed from ========== Photo Picker Dialog: Recursively traverse the photo directories. (Only visible change, since the picker is not decoding, is that it will show as many gray squares as there are photos on disk). BUG=656015 ========== to ========== Photo Picker Dialog: Recursively traverse the photo directories. (Only visible change, since the picker is not decoding yet, is that it will show as many gray squares as there are photos on disk). BUG=656015 ==========
finnur@chromium.org changed reviewers: + twellington@chromium.org
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2810773002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/AttrAcceptFileFilter.java (right): https://codereview.chromium.org/2810773002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/AttrAcceptFileFilter.java:19: class AttrAcceptFileFilter implements FileFilter { nit: I think ImageFileFilter is a better name since this is specifically filtering for images. https://codereview.chromium.org/2810773002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/AttrAcceptFileFilter.java:70: public boolean acceptsImages() { nit: JavaDoc on this and other two public methods. What are acceptsImages(), acceptsVideos() and acceptsOther() going to be used for. https://codereview.chromium.org/2810773002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/AttrAcceptFileFilter.java:101: String mimeType = mMimeTypeMap.getMimeTypeFromExtension(ext); This requires the file to have an extension type. Is that consistent with the Android photo picker? e.g. If I download an image called "image" with no file extension, would Android's photo picker include "image" in the list of options? https://codereview.chromium.org/2810773002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/FileEnumWorkerTask.java (right): https://codereview.chromium.org/2810773002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/FileEnumWorkerTask.java:68: return true; nit: inline this if (files == null) return true; https://codereview.chromium.org/2810773002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/FileEnumWorkerTask.java:103: Environment.getExternalStoragePublicDirectory(Environment.DIRECTORY_DOWNLOADS), If videos/* is an accepted type, should we also be checking DIRECTORY_MOVIES? What about DIRECTORY_DOCUMENTS? Or the screenshots directory? It may be safest just to check all directories in the public directory. In Android's photo picker, I see standard folders as well as custom ones (e.g. "image_search"). https://codereview.chromium.org/2810773002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerCategoryView.java (right): https://codereview.chromium.org/2810773002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerCategoryView.java:147: setVisibility(View.GONE); If there are no files, we will still display the gallery and camera tiles, right?
The CQ bit was checked by finnur@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL https://codereview.chromium.org/2810773002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/AttrAcceptFileFilter.java (right): https://codereview.chromium.org/2810773002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/AttrAcceptFileFilter.java:19: class AttrAcceptFileFilter implements FileFilter { Agreed. https://codereview.chromium.org/2810773002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/AttrAcceptFileFilter.java:70: public boolean acceptsImages() { > What are acceptsImages(), acceptsVideos() and acceptsOther() going to be used > for. Huh... Nothing apparently. Good catch. https://codereview.chromium.org/2810773002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/AttrAcceptFileFilter.java:101: String mimeType = mMimeTypeMap.getMimeTypeFromExtension(ext); Yes, that is consistent. I used adb shell to rename a photo in order to drop the extension and (after clearing the cache by rebooting) the photo stopped appearing in the list for the stock Android picker intent. https://codereview.chromium.org/2810773002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/FileEnumWorkerTask.java (right): https://codereview.chromium.org/2810773002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/FileEnumWorkerTask.java:68: return true; On 2017/04/10 18:01:14, Theresa wrote: > nit: inline this > > if (files == null) return true; Done. https://codereview.chromium.org/2810773002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/FileEnumWorkerTask.java:103: Environment.getExternalStoragePublicDirectory(Environment.DIRECTORY_DOWNLOADS), videos/* I'll remove, because we don't have good thumbnails for those -- at least not yet. But I'm very hesitant to add the top level folder, not just because it will slow enumeration down, but also because, at least on my phone, there's the high-quality photos but also a *lot* of junk images I don't really care about: downloads, screenshots and images from other apps. I'll start a thread on this, though. https://codereview.chromium.org/2810773002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerCategoryView.java (right): https://codereview.chromium.org/2810773002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerCategoryView.java:147: setVisibility(View.GONE); Ah, yes. I thought I'd deleted that line. Good catch.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2810773002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/FileEnumWorkerTask.java (right): https://codereview.chromium.org/2810773002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/FileEnumWorkerTask.java:103: Environment.getExternalStoragePublicDirectory(Environment.DIRECTORY_DOWNLOADS), On 2017/04/11 11:30:57, Finnur wrote: > videos/* I'll remove, because we don't have good thumbnails for those -- at > least not yet. > > But I'm very hesitant to add the top level folder, not just because it will slow > enumeration down, but also because, at least on my phone, there's the > high-quality photos but also a *lot* of junk images I don't really care about: > downloads, screenshots and images from other apps. I'll start a thread on this, > though. I think screenshots are really important e.g. screenshots of text message threads are often posted online. Thanks for starting the thread -- we can iron out the policy there. https://codereview.chromium.org/2810773002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/FileEnumWorkerTask.java (right): https://codereview.chromium.org/2810773002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/FileEnumWorkerTask.java:56: SAMPLE_DCIM_SOURCE_SUB_DIRECTORY); Instead of explicitly finding "Camera", can we search the entire Environment.DIRECTORY_DCIM directory in case the folder is something like "camera" or "Cámara". I don't think it's safe to assume that the folder name will be in English. https://codereview.chromium.org/2810773002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/FileEnumWorkerTask.java:73: if (!traverseDir(file, pickerBitmaps)) return false; optional nit: I liked the old return traverseDir(file, pickerBitmaps); but the logic is the same so it's your choice. https://codereview.chromium.org/2810773002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/ImageFileFilter.java (right): https://codereview.chromium.org/2810773002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/ImageFileFilter.java:21: private static final String VIDEO_SUPERTYPE = "video"; This isn't used anymore. https://codereview.chromium.org/2810773002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/ImageFileFilter.java:64: } nit: this can be simplified to: if (mimeType != null) { if (mMimeTypes.contains(mimeType) || mMimeSupertypes.contains(getMimeSupertype(mimeType))) { return true; } } https://codereview.chromium.org/2810773002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerCategoryView.java (right): https://codereview.chromium.org/2810773002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerCategoryView.java:219: mWorkerTask.cancel(true); Should we also cancel the work task when the dialog is closed if we haven't received a callback yet? https://codereview.chromium.org/2810773002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerCategoryView.java:222: mWorkerTask = new FileEnumWorkerTask(this, new ImageFileFilter("image/*")); I think the name ImageFileFilter indicates that only images will be accepted. Should we move the "image/*" in-line there until there's a need for for ImageFileFilter to be more generic? Or perhaps my last comment to rename ImageFileFilter was misguided and the class should be more generic now?
The CQ bit was checked by finnur@chromium.org to run a CQ dry run
PTAL. https://codereview.chromium.org/2810773002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/FileEnumWorkerTask.java (right): https://codereview.chromium.org/2810773002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/FileEnumWorkerTask.java:56: SAMPLE_DCIM_SOURCE_SUB_DIRECTORY); I actually modeled this after what Google Photos does. I tried to use DIRECTORY_DCIM instead (the parent directory) but got a lot of junk photos doing that. If it is OK with you I'd like to add a TODO and see what the resolution is on the discussion thread. https://codereview.chromium.org/2810773002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/FileEnumWorkerTask.java:73: if (!traverseDir(file, pickerBitmaps)) return false; I see you fell into the same mind trap I did. The logic isn't the same because the for loop isn't supposed to end when traverseDir returns true (it is supposed to continue with the next file). https://codereview.chromium.org/2810773002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/ImageFileFilter.java (right): https://codereview.chromium.org/2810773002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/ImageFileFilter.java:64: } On 2017/04/11 15:44:49, Theresa wrote: > nit: this can be simplified to: > > if (mimeType != null) { > if (mMimeTypes.contains(mimeType) > || mMimeSupertypes.contains(getMimeSupertype(mimeType))) { > return true; > } > } Done. https://codereview.chromium.org/2810773002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerCategoryView.java (right): https://codereview.chromium.org/2810773002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerCategoryView.java:219: mWorkerTask.cancel(true); Not a bad idea. Done. https://codereview.chromium.org/2810773002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerCategoryView.java:222: mWorkerTask = new FileEnumWorkerTask(this, new ImageFileFilter("image/*")); How about MimeTypeFileFilter?
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2810773002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/FileEnumWorkerTask.java (right): https://codereview.chromium.org/2810773002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/FileEnumWorkerTask.java:56: SAMPLE_DCIM_SOURCE_SUB_DIRECTORY); On 2017/04/11 16:25:23, Finnur wrote: > I actually modeled this after what Google Photos does. I tried to use > DIRECTORY_DCIM instead (the parent directory) but got a lot of junk photos doing > that. If it is OK with you I'd like to add a TODO and see what the resolution is > on the discussion thread. Acknowledged. https://codereview.chromium.org/2810773002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/FileEnumWorkerTask.java:73: if (!traverseDir(file, pickerBitmaps)) return false; On 2017/04/11 16:25:23, Finnur wrote: > I see you fell into the same mind trap I did. The logic isn't the same because > the for loop isn't supposed to end when traverseDir returns true (it is supposed > to continue with the next file). Ah, I see. Thank you for the explanation. https://codereview.chromium.org/2810773002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerCategoryView.java (right): https://codereview.chromium.org/2810773002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerCategoryView.java:222: mWorkerTask = new FileEnumWorkerTask(this, new ImageFileFilter("image/*")); On 2017/04/11 16:25:23, Finnur wrote: > How about MimeTypeFileFilter? Sounds good to me. https://codereview.chromium.org/2810773002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/MimeTypeFileFilter.java (right): https://codereview.chromium.org/2810773002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/MimeTypeFileFilter.java:16: * A file filter for handling extensions and MIME types (images/jpeg) and nit: This description should be updated since it could be any MimeType now (right?) https://codereview.chromium.org/2810773002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/MimeTypeFileFilter.java:25: public MimeTypeFileFilter(@NonNull String acceptAttr) { JavaDoc https://codereview.chromium.org/2810773002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerCategoryView.java (right): https://codereview.chromium.org/2810773002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerCategoryView.java:85: public PickerCategoryView(Context context, AttributeSet attrs, int defStyle) { nit:Are all three of these constructors necessary? You probably only need the 2-param version. https://codereview.chromium.org/2810773002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerCategoryView.java:127: public void teardown() { nit: I think teardown() is typically used in test classes but not regular classes. This could be something like onDialogDismissed() or destroy(). super nit: We check if mWorkerTask != null in prepareBitmaps(), which shouldn't be possible since we only end up calling that method through the constructor. But we don't check if mWorkerTask != null here. Maybe we should also check here that mWorkerTask != null, and set mWorkTask = null when the callback is received?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by finnur@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL https://codereview.chromium.org/2810773002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/MimeTypeFileFilter.java (right): https://codereview.chromium.org/2810773002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/MimeTypeFileFilter.java:16: * A file filter for handling extensions and MIME types (images/jpeg) and On 2017/04/11 17:01:28, Theresa wrote: > nit: This description should be updated since it could be any MimeType now > (right?) Correct. Done. https://codereview.chromium.org/2810773002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/MimeTypeFileFilter.java:25: public MimeTypeFileFilter(@NonNull String acceptAttr) { On 2017/04/11 17:01:28, Theresa wrote: > JavaDoc Done. https://codereview.chromium.org/2810773002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerCategoryView.java (right): https://codereview.chromium.org/2810773002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerCategoryView.java:85: public PickerCategoryView(Context context, AttributeSet attrs, int defStyle) { Only one is being used, currently (the single-param one). https://codereview.chromium.org/2810773002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerCategoryView.java:127: public void teardown() { On 2017/04/11 17:01:29, Theresa wrote: > nit: I think teardown() is typically used in test classes but not regular > classes. This could be something like onDialogDismissed() or destroy(). Done. > super nit: We check if mWorkerTask != null in prepareBitmaps(), which shouldn't > be possible since we only end up calling that method through the constructor. It isn't possible now, but that's only because this is a trimmed CL with sync loading of images. In a future CL, this can be null. > But we don't check if mWorkerTask != null here. Maybe we should also check here > that mWorkerTask != null, and set mWorkTask = null when the callback is > received? Added null-check.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm % one last comment https://codereview.chromium.org/2810773002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/MimeTypeFileFilter.java (right): https://codereview.chromium.org/2810773002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/MimeTypeFileFilter.java:29: public MimeTypeFileFilter(@NonNull String acceptAttr) { Instead of a comma separate string, could this be a List?
Would it be OK if I make a mental TODO and leave this for the next CL? We're heading into a long Easter break here and I'd like to check this in before it bitrots. :)
On 2017/04/13 22:10:55, Finnur wrote: > Would it be OK if I make a mental TODO and leave this for the next CL? We're > heading into a long Easter break here and I'd like to check this in before it > bitrots. :) A TODO is fine with me.
The CQ bit was checked by finnur@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from twellington@chromium.org Link to the patchset: https://codereview.chromium.org/2810773002/#ps100001 (title: "Add todo")
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": 100001, "attempt_start_ts": 1492127496905410,
"parent_rev": "5be07ac89aea29234eb908ec661881fb977bdd9b", "commit_rev":
"a106f0abbf69dad349d4aaf4bcc4f5d376dd2377"}
Message was sent while issue was closed.
Description was changed from ========== Photo Picker Dialog: Recursively traverse the photo directories. (Only visible change, since the picker is not decoding yet, is that it will show as many gray squares as there are photos on disk). BUG=656015 ========== to ========== Photo Picker Dialog: Recursively traverse the photo directories. (Only visible change, since the picker is not decoding yet, is that it will show as many gray squares as there are photos on disk). BUG=656015 Review-Url: https://codereview.chromium.org/2810773002 Cr-Commit-Position: refs/heads/master@{#464641} Committed: https://chromium.googlesource.com/chromium/src/+/a106f0abbf69dad349d4aaf4bcc4... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/a106f0abbf69dad349d4aaf4bcc4... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
