Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2909633002:
Photo Picker dialog: Show the dialog when image types are being requested. (Closed)
DescriptionPhoto Picker dialog: Show the dialog when image types are being requested.
BUG=656015
Review-Url: https://codereview.chromium.org/2909633002
Cr-Commit-Position: refs/heads/master@{#475503}
Committed: https://chromium.googlesource.com/chromium/src/+/5f19ec17aa243eabd70ee11a6b08d90913bcb778
Patch Set 1 #
Total comments: 6
Patch Set 2 : Address feedback, shuffle things a bit and add test #
Total comments: 2
Patch Set 3 : Address feedback from Ted #
Total comments: 1
Messages
Total messages: 25 (17 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...
finnur@chromium.org changed reviewers: + tedchoc@chromium.org
Ted, do you mind doing this (short) review in Theresa's absence?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2909633002/diff/1/ui/android/java/src/org/chr... File ui/android/java/src/org/chromium/ui/base/SelectFileDialog.java (right): https://codereview.chromium.org/2909633002/diff/1/ui/android/java/src/org/chr... ui/android/java/src/org/chromium/ui/base/SelectFileDialog.java:255: if (activity != null && usePhotoPicker() Could usePhotoPicker() be: TextUtils.equals(getContentIntent.getType(), ALL_IMAGE_TYPES) && !captureCamera() I wonder if the block with noSpecificType() is doing what we want above. Or maybe we would want to set a boolean in the shouldShowImageTypes() branch above. That "seems" what we want. https://codereview.chromium.org/2909633002/diff/1/ui/android/java/src/org/chr... ui/android/java/src/org/chromium/ui/base/SelectFileDialog.java:275: * remove extra blank line
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 unchecked by finnur@chromium.org
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/2909633002/diff/1/ui/android/java/src/org/chr... File ui/android/java/src/org/chromium/ui/base/SelectFileDialog.java (right): https://codereview.chromium.org/2909633002/diff/1/ui/android/java/src/org/chr... ui/android/java/src/org/chromium/ui/base/SelectFileDialog.java:255: if (activity != null && usePhotoPicker() I don't see how that can work. There's two ways to specify images in HTML: with MIME types (image/jpg) or with Extensions (.jpg). Lets take an example, where I specify both, as in: "image/jpg,*.gif". noSpecificType() would in that case return true, which makes the branch point at line 225 above skip lines 226-242, causing getContentIntent to have type */*, and the photo picker wouldn't be shown. I do however, think that this code belongs above all the code that constructs a getContentIntent, so I have moved it higher up (and the UMA to the top, above the early returns). Also added a test. https://codereview.chromium.org/2909633002/diff/1/ui/android/java/src/org/chr... ui/android/java/src/org/chromium/ui/base/SelectFileDialog.java:275: * On 2017/05/26 20:21:14, Ted C wrote: > remove extra blank line Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2909633002/diff/1/ui/android/java/src/org/chr... File ui/android/java/src/org/chromium/ui/base/SelectFileDialog.java (right): https://codereview.chromium.org/2909633002/diff/1/ui/android/java/src/org/chr... ui/android/java/src/org/chromium/ui/base/SelectFileDialog.java:255: if (activity != null && usePhotoPicker() On 2017/05/29 15:18:36, Finnur wrote: > I don't see how that can work. > > There's two ways to specify images in HTML: with MIME types (image/jpg) or with > Extensions (.jpg). > > Lets take an example, where I specify both, as in: "image/jpg,*.gif". > > noSpecificType() would in that case return true, which makes the branch point at > line 225 above skip lines 226-242, causing getContentIntent to have type */*, > and the photo picker wouldn't be shown. > > I do however, think that this code belongs above all the code that constructs a > getContentIntent, so I have moved it higher up (and the UMA to the top, above > the early returns). > > Also added a test. If that is the case, then I'd argue the previous/existing behavior was wrong. With the example you showed, shouldn't shouldShowImageTypes() return true (not in the current form, but conceptually)? It seems odd to me to have two methods that check the same thing. Especially if the existing Android photo picker would handle your example correctly ( filtering to just those file formats). I don't know if it needs to block this anymore, but it seems that we should reconcile this and explain the difference. https://codereview.chromium.org/2909633002/diff/20001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/SelectFileDialogTest.java (right): https://codereview.chromium.org/2909633002/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/SelectFileDialogTest.java:223: assertTrue(SelectFileDialog.usePhotoPicker(Arrays.asList(".jpg", "image/jpeg"))); nit, but I think we should add a test where the mime types wouldn't necessarily resolve to the same (gif and jpg for example)
https://codereview.chromium.org/2909633002/diff/1/ui/android/java/src/org/chr... File ui/android/java/src/org/chromium/ui/base/SelectFileDialog.java (right): https://codereview.chromium.org/2909633002/diff/1/ui/android/java/src/org/chr... ui/android/java/src/org/chromium/ui/base/SelectFileDialog.java:255: if (activity != null && usePhotoPicker() I tend to agree with you. I suspect the previous behavior is wrong, but am not 100% sure. But yeah, we should reconcile this. https://codereview.chromium.org/2909633002/diff/20001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/SelectFileDialogTest.java (right): https://codereview.chromium.org/2909633002/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/SelectFileDialogTest.java:223: assertTrue(SelectFileDialog.usePhotoPicker(Arrays.asList(".jpg", "image/jpeg"))); Sure. Done.
The CQ bit was checked by finnur@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org Link to the patchset: https://codereview.chromium.org/2909633002/#ps40001 (title: "Address feedback from Ted")
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": 40001, "attempt_start_ts": 1496142979562490,
"parent_rev": "42f156958878270e4574544ab7abf9a99b7972ee", "commit_rev":
"5f19ec17aa243eabd70ee11a6b08d90913bcb778"}
Message was sent while issue was closed.
Description was changed from ========== Photo Picker dialog: Show the dialog when image types are being requested. BUG=656015 ========== to ========== Photo Picker dialog: Show the dialog when image types are being requested. BUG=656015 Review-Url: https://codereview.chromium.org/2909633002 Cr-Commit-Position: refs/heads/master@{#475503} Committed: https://chromium.googlesource.com/chromium/src/+/5f19ec17aa243eabd70ee11a6b08... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/5f19ec17aa243eabd70ee11a6b08...
Message was sent while issue was closed.
jbudorick@chromium.org changed reviewers: + jbudorick@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2909633002/diff/40001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/SelectFileDialogTest.java (right): https://codereview.chromium.org/2909633002/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/SelectFileDialogTest.java:213: public void testPhotoPickerLaunchAndMimeTypes() throws Throwable { This test is flaky: https://bugs.chromium.org/p/chromium/issues/detail?id=728247 I'm disabling it for now. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
