Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2737393002:
Implement the new Photo picker, part one. (Closed)
DescriptionImplement the new Photo picker, part one.
This is the first step towards implementing a new Photo Picker for Chrome for
Android, broken down into multiple steps for easier reviewing.
First part includes simply replacing the stock Android picker with a Toast
saying "Not implemented".
Includes a Finch tie-in and a flag to turn this on manually ("Enable new
photopicker" in chrome://flags).
BUG=656015
Review-Url: https://codereview.chromium.org/2737393002
Cr-Commit-Position: refs/heads/master@{#457741}
Committed: https://chromium.googlesource.com/chromium/src/+/a6a921c6657a051d484306088a1d85b992f1a64f
Patch Set 1 #Patch Set 2 : Sync #
Total comments: 25
Patch Set 3 : Address comments #
Total comments: 16
Patch Set 4 : Address comments #Patch Set 5 : Sync #Patch Set 6 : Address feedback #
Total comments: 20
Patch Set 7 : Address Theresa's comments #
Total comments: 2
Patch Set 8 : git cl upload #Messages
Total messages: 63 (44 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 unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
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: + mvanouwerkerk@chromium.org
If it helps, I also have a big-picture reference CL (all the changes in one CL) so you can see where I'm heading. Let me know if you think it is useful and I'll provide it.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Ping...
After changing the flag config you'll also need to resolve the bot failures. https://codereview.chromium.org/2737393002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeSwitches.java (right): https://codereview.chromium.org/2737393002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeSwitches.java:144: * Enables the new picker for selecting photos from the device. Please refer to the native switch here like the others in this file do. https://codereview.chromium.org/2737393002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/init/ProcessInitializationHandler.java (right): https://codereview.chromium.org/2737393002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/init/ProcessInitializationHandler.java:62: static final String PHOTO_PICKER_TRIAL = "photo_picker_trial"; nit: these fields can be private. https://codereview.chromium.org/2737393002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/init/ProcessInitializationHandler.java:62: static final String PHOTO_PICKER_TRIAL = "photo_picker_trial"; Maybe rename to NEW_PHOTO_PICKER_STUDY_NAME = "NewPhotoPicker" because this is the value of the name attribute of the study in a finch config, and that name is normally reflected in the config file names, and uses UpperCamelCase. https://codereview.chromium.org/2737393002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/init/ProcessInitializationHandler.java:63: static final String PHOTO_PICKER_ENABLED = "photo_picker_enabled"; and then maybe this becomes NEW_PHOTO_PICKER_ENABLED = "new_photo_picker_enabled" https://codereview.chromium.org/2737393002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/init/ProcessInitializationHandler.java:166: boolean showPhotoPicker = maybe rename to "useNewPhotoPicker" https://codereview.chromium.org/2737393002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/init/ProcessInitializationHandler.java:167: TextUtils.equals("true", VariationsAssociatedData.getVariationParamValue( I'd suggest using ChromeFeatureList.getFieldTrialParamByFeatureAsBoolean instead. This way the study name has no influence, just the feature name. This means you could run multiple studies on the same feature. This seems like a project that might be large enough to run more than one study per channel. Also, in C++ this hits GetVariationParams in variations_associated_data.h which has been deprecated. https://codereview.chromium.org/2737393002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/init/ProcessInitializationHandler.java:171: showPhotoPicker = commandLine.hasSwitch(ChromeSwitches.ENABLE_NEW_PHOTO_PICKER); You shouldn't need to read from the command line, you can configure chrome:flags to toggle the feature, so just read the feature and its params with ChromeFeatureList. https://codereview.chromium.org/2737393002/diff/20001/chrome/browser/about_fl... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/2737393002/diff/20001/chrome/browser/about_fl... chrome/browser/about_flags.cc:801: SINGLE_VALUE_TYPE(switches::kEnableNewPhotoPicker)}, Use a FEATURE_VALUE_TYPE flag. See components/flags_ui/feature_entry.h for docs. https://codereview.chromium.org/2737393002/diff/20001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/OnPhotoPickerListener.java (right): https://codereview.chromium.org/2737393002/diff/20001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/OnPhotoPickerListener.java:14: * The action the user took in the picker. nit: use one space after * https://codereview.chromium.org/2737393002/diff/20001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/OnPhotoPickerListener.java:42: Map<String, Long> getFilesForTesting(); @Nullable https://codereview.chromium.org/2737393002/diff/20001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/UiUtils.java (right): https://codereview.chromium.org/2737393002/diff/20001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/UiUtils.java:59: /** A delegate that shows the file picker. */ It does more than showing the picker, so let's give it a more general doc: A delegate for the tile picker. https://codereview.chromium.org/2737393002/diff/20001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/UiUtils.java:79: public interface FilePickerDelegate { This is called FilePicker but lots of places say photo picker. Could you make these consistent? https://codereview.chromium.org/2737393002/diff/20001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/UiUtils.java:103: * Allows setting a delegate to override the default software keyboard visibility detection. These docs seem to have been copied from the method above.
Patchset #3 (id:40001) has been deleted
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
Patchset #3 (id:60001) has been deleted
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/2737393002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeSwitches.java (right): https://codereview.chromium.org/2737393002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeSwitches.java:144: * Enables the new picker for selecting photos from the device. On 2017/03/13 14:04:17, Michael van Ouwerkerk wrote: > Please refer to the native switch here like the others in this file do. Done. https://codereview.chromium.org/2737393002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/init/ProcessInitializationHandler.java (right): https://codereview.chromium.org/2737393002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/init/ProcessInitializationHandler.java:62: static final String PHOTO_PICKER_TRIAL = "photo_picker_trial"; On 2017/03/13 14:04:17, Michael van Ouwerkerk wrote: > nit: these fields can be private. Both removed. https://codereview.chromium.org/2737393002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/init/ProcessInitializationHandler.java:63: static final String PHOTO_PICKER_ENABLED = "photo_picker_enabled"; On 2017/03/13 14:04:17, Michael van Ouwerkerk wrote: > and then maybe this becomes NEW_PHOTO_PICKER_ENABLED = > "new_photo_picker_enabled" Ditto. https://codereview.chromium.org/2737393002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/init/ProcessInitializationHandler.java:166: boolean showPhotoPicker = On 2017/03/13 14:04:17, Michael van Ouwerkerk wrote: > maybe rename to "useNewPhotoPicker" Redundant now. https://codereview.chromium.org/2737393002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/init/ProcessInitializationHandler.java:167: TextUtils.equals("true", VariationsAssociatedData.getVariationParamValue( Opted for the simpler ChromeFeatureList.isEnabled. https://codereview.chromium.org/2737393002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/init/ProcessInitializationHandler.java:171: showPhotoPicker = commandLine.hasSwitch(ChromeSwitches.ENABLE_NEW_PHOTO_PICKER); Removed. https://codereview.chromium.org/2737393002/diff/20001/chrome/browser/about_fl... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/2737393002/diff/20001/chrome/browser/about_fl... chrome/browser/about_flags.cc:801: SINGLE_VALUE_TYPE(switches::kEnableNewPhotoPicker)}, On 2017/03/13 14:04:17, Michael van Ouwerkerk wrote: > Use a FEATURE_VALUE_TYPE flag. See components/flags_ui/feature_entry.h for docs. Done. https://codereview.chromium.org/2737393002/diff/20001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/OnPhotoPickerListener.java (right): https://codereview.chromium.org/2737393002/diff/20001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/OnPhotoPickerListener.java:14: * The action the user took in the picker. On 2017/03/13 14:04:17, Michael van Ouwerkerk wrote: > nit: use one space after * Done. https://codereview.chromium.org/2737393002/diff/20001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/OnPhotoPickerListener.java:42: Map<String, Long> getFilesForTesting(); On 2017/03/13 14:04:17, Michael van Ouwerkerk wrote: > @Nullable Done. https://codereview.chromium.org/2737393002/diff/20001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/UiUtils.java (right): https://codereview.chromium.org/2737393002/diff/20001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/UiUtils.java:59: /** A delegate that shows the file picker. */ On 2017/03/13 14:04:17, Michael van Ouwerkerk wrote: > It does more than showing the picker, so let's give it a more general doc: A > delegate for the tile picker. Done. https://codereview.chromium.org/2737393002/diff/20001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/UiUtils.java:79: public interface FilePickerDelegate { On 2017/03/13 14:04:17, Michael van Ouwerkerk wrote: > This is called FilePicker but lots of places say photo picker. Could you make > these consistent? Done. https://codereview.chromium.org/2737393002/diff/20001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/UiUtils.java:103: * Allows setting a delegate to override the default software keyboard visibility detection. Yeah, special two-for one price at the store. Couldn't resit. Fixed.
PTAL
On 2017/03/14 14:36:35, Finnur wrote: > PTAL Oops, didn't mean to excessively ping you. :) Or, as Sean Connery would have it: https://www.youtube.com/watch?v=7O398Ubx-Gs
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
https://codereview.chromium.org/2737393002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeSwitches.java (right): https://codereview.chromium.org/2737393002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeSwitches.java:147: public static final String ENABLE_NEW_PHOTO_PICKER = "enable-new-photo-picker"; You can enable features with existing command line flags, no need to add this. Something like: build/android/adb_chrome_public_command_line --force-fieldtrials=TestTrial/TestGroup '--enable-features=NewPhotoPicker<TestTrial' I wrote a relevant document for Zine on this topic: go/zine-features-command-line https://codereview.chromium.org/2737393002/diff/80001/chrome/browser/about_fl... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/2737393002/diff/80001/chrome/browser/about_fl... chrome/browser/about_flags.cc:815: FEATURE_VALUE_TYPE(chrome::android::kEnableNewPhotoPicker)}, just kNewPhotoPicker seems cleaner https://codereview.chromium.org/2737393002/diff/80001/content/public/common/c... File content/public/common/content_switches.cc (right): https://codereview.chromium.org/2737393002/diff/80001/content/public/common/c... content/public/common/content_switches.cc:424: const char kEnableNewPhotoPicker[] = "enable-new-photo-picker"; I don't think you need this. https://codereview.chromium.org/2737393002/diff/80001/content/public/common/c... File content/public/common/content_switches.h (right): https://codereview.chromium.org/2737393002/diff/80001/content/public/common/c... content/public/common/content_switches.h:138: CONTENT_EXPORT extern const char kEnableNewPhotoPicker[]; I don't think you need this. https://codereview.chromium.org/2737393002/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2737393002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:99142: + <int value="-924582104" label="enable-new-photo-picker"/> Looks like the test wants something a little different: C 238.352s Main [FAIL] AboutFlagsHistogramTest.CheckHistograms: C 238.352s Main [ RUN ] AboutFlagsHistogramTest.CheckHistograms C 238.352s Main ../../chrome/browser/about_flags_unittest.cc:304: Failure C 238.352s Main Value of: enum_entry != histograms_xml_switches_ids.end() && enum_entry->first == flag C 238.352s Main Actual: false C 238.352s Main Expected: true C 238.352s Main histograms.xml enum LoginCustomFlags doesn't contain switch 'NewPhotoPicker:disabled' (value=1960169775 expected). Consider adding entry: C 238.352s Main <int value="1960169775" label="NewPhotoPicker:disabled"/> C 238.352s Main ../../chrome/browser/about_flags_unittest.cc:304: Failure C 238.352s Main Value of: enum_entry != histograms_xml_switches_ids.end() && enum_entry->first == flag C 238.352s Main Actual: false C 238.352s Main Expected: true C 238.352s Main histograms.xml enum LoginCustomFlags doesn't contain switch 'NewPhotoPicker:enabled' (value=1928407249 expected). Consider adding entry: C 238.352s Main <int value="1928407249" label="NewPhotoPicker:enabled"/> C 238.352s Main [ FAILED ] AboutFlagsHistogramTest.CheckHistograms (321 ms) C 238.352s Main [----------] 1 test from AboutFlagsHistogramTest (321 ms total) https://codereview.chromium.org/2737393002/diff/80001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/UiUtils.java (right): https://codereview.chromium.org/2737393002/diff/80001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/UiUtils.java:77: * A delegate interface for showing the photo picker. It does not just show, also dismiss. A more general description might be: "A delegate interface for the photo picker." https://codereview.chromium.org/2737393002/diff/80001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/UiUtils.java:105: * @ param delegate A {@link PhotoPickerDelegate} instance. nit: no space after @
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 commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
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 commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2737393002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeSwitches.java (right): https://codereview.chromium.org/2737393002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeSwitches.java:147: public static final String ENABLE_NEW_PHOTO_PICKER = "enable-new-photo-picker"; On 2017/03/14 15:57:27, Michael van Ouwerkerk wrote: > You can enable features with existing command line flags, no need to add this. > Something like: build/android/adb_chrome_public_command_line > --force-fieldtrials=TestTrial/TestGroup > '--enable-features=NewPhotoPicker<TestTrial' > > I wrote a relevant document for Zine on this topic: > go/zine-features-command-line Done. https://codereview.chromium.org/2737393002/diff/80001/chrome/browser/about_fl... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/2737393002/diff/80001/chrome/browser/about_fl... chrome/browser/about_flags.cc:815: FEATURE_VALUE_TYPE(chrome::android::kEnableNewPhotoPicker)}, ../../chrome/browser/about_flags.cc:815:25: error: use of undeclared identifier 'kEnableNewPhotoPicker'; did you mean 'chrome::android::kEnableNewPhotoPicker'? FEATURE_VALUE_TYPE(kEnableNewPhotoPicker)}, https://codereview.chromium.org/2737393002/diff/80001/content/public/common/c... File content/public/common/content_switches.cc (right): https://codereview.chromium.org/2737393002/diff/80001/content/public/common/c... content/public/common/content_switches.cc:424: const char kEnableNewPhotoPicker[] = "enable-new-photo-picker"; On 2017/03/14 15:57:27, Michael van Ouwerkerk wrote: > I don't think you need this. Done. https://codereview.chromium.org/2737393002/diff/80001/content/public/common/c... File content/public/common/content_switches.h (right): https://codereview.chromium.org/2737393002/diff/80001/content/public/common/c... content/public/common/content_switches.h:138: CONTENT_EXPORT extern const char kEnableNewPhotoPicker[]; On 2017/03/14 15:57:27, Michael van Ouwerkerk wrote: > I don't think you need this. Done. https://codereview.chromium.org/2737393002/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2737393002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:99142: + <int value="-924582104" label="enable-new-photo-picker"/> Heh, yeah. I noticed this too late yesterday. It is a case of me getting the test to work with the about_flags.cc I had, and then changing about_flags.cc. :) https://codereview.chromium.org/2737393002/diff/80001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/UiUtils.java (right): https://codereview.chromium.org/2737393002/diff/80001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/UiUtils.java:77: * A delegate interface for showing the photo picker. Yeah, sorry. I didn't follow your initial comment to the letter. https://codereview.chromium.org/2737393002/diff/80001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/UiUtils.java:105: * @ param delegate A {@link PhotoPickerDelegate} instance. On 2017/03/14 15:57:27, Michael van Ouwerkerk wrote: > nit: no space after @ Done.
lgtm with nit https://codereview.chromium.org/2737393002/diff/80001/chrome/browser/about_fl... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/2737393002/diff/80001/chrome/browser/about_fl... chrome/browser/about_flags.cc:815: FEATURE_VALUE_TYPE(chrome::android::kEnableNewPhotoPicker)}, On 2017/03/15 13:25:06, Finnur wrote: > ../../chrome/browser/about_flags.cc:815:25: error: use of undeclared identifier > 'kEnableNewPhotoPicker'; did you mean 'chrome::android::kEnableNewPhotoPicker'? > FEATURE_VALUE_TYPE(kEnableNewPhotoPicker)}, What I meant was that none of the other features in chrome_feature_list.cc have 'Enable' as part of their name, so you should rename it to kNewPhotoPicker.
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...
Description was changed from
==========
Implement the new Photo picker, part one.
This is the first step towards implementing a new Photo Picker for Chrome for
Android, broken down into multiple steps for easier reviewing.
First part includes simply replacing the stock Android picker with a Toast
saying "Not implemented".
Includes a Finch tie-in and a flag to turn this on manually ("Enable new
photopicker" in chrome://flags).
BUG=656015
==========
to
==========
Implement the new Photo picker, part one.
This is the first step towards implementing a new Photo Picker for Chrome for
Android, broken down into multiple steps for easier reviewing.
First part includes simply replacing the stock Android picker with a Toast
saying "Not implemented".
Includes a Finch tie-in and a flag to turn this on manually ("Enable new
photopicker" in chrome://flags).
BUG=656015
==========
finnur@chromium.org changed reviewers: + twellington@chromium.org
Theresa, mind taking a quick look? Screenshots from my phone are here: https://bugs.chromium.org/p/chromium/issues/detail?id=656015#c6 (showing roughly how it will look in the end, this CL shows just a toast). Design doc here: https://docs.google.com/document/d/1oenFck8kfcWmrwBUCFv1Wi7-cJxEmlxbIx3jpjyZj... Mocks and PRD available if requested. https://codereview.chromium.org/2737393002/diff/80001/chrome/browser/about_fl... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/2737393002/diff/80001/chrome/browser/about_fl... chrome/browser/about_flags.cc:815: FEATURE_VALUE_TYPE(chrome::android::kEnableNewPhotoPicker)}, Ah, right! Done. Thanks for the review, Michael!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2737393002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/init/ProcessInitializationHandler.java (right): https://codereview.chromium.org/2737393002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/init/ProcessInitializationHandler.java:168: @Override nit: add a blank line before this one https://codereview.chromium.org/2737393002/diff/140001/chrome/app/generated_r... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2737393002/diff/140001/chrome/app/generated_r... chrome/app/generated_resources.grd:5553: + Enable new Photopicker nit: s/Photopicker/photo picker Do we want translateable="false" to avoid the cost of translating this chrome://flags string? https://codereview.chromium.org/2737393002/diff/140001/chrome/browser/android... File chrome/browser/android/chrome_feature_list.h (left): https://codereview.chromium.org/2737393002/diff/140001/chrome/browser/android... chrome/browser/android/chrome_feature_list.h:26: extern const base::Feature kContextualSearchUrlActions; Thank you for moving this. https://codereview.chromium.org/2737393002/diff/140001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/OnPhotoPickerListener.java (right): https://codereview.chromium.org/2737393002/diff/140001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/OnPhotoPickerListener.java:14: public interface OnPhotoPickerListener { nit: I like "PhotoPickerListener" better than "OnPhotoPickerListener" for the interface name. https://codereview.chromium.org/2737393002/diff/140001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/OnPhotoPickerListener.java:32: * above. nit: this should fit on the line above. https://codereview.chromium.org/2737393002/diff/140001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/UiUtils.java (right): https://codereview.chromium.org/2737393002/diff/140001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/UiUtils.java:83: * @param listener The listener that will be notified of the selection. nit: ... notified of the action the user took in the picker? https://codereview.chromium.org/2737393002/diff/140001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/UiUtils.java:205: public static boolean showPhotoPicker( nit: let's move the setPhotoPickerDelegate() method above these two to keep them grouped together and avoid splitting up setKeyboardShowingDelegate and the keyboard methods. https://codereview.chromium.org/2737393002/diff/140001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/UiUtils.java:212: public static void dismissPhotoPicker() { This method and the one above need JavaDocs. https://codereview.chromium.org/2737393002/diff/140001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/UiUtils.java:214: sPhotoPickerDelegate.dismissPhotoPicker(); In Android N+ multi-window, will it be possible for the user to have two photo pickers displayed at once? If so, how will we know which one is being dismissed? If not, how will we prevent two from being created? https://codereview.chromium.org/2737393002/diff/140001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/base/SelectFileDialog.java (right): https://codereview.chromium.org/2737393002/diff/140001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/SelectFileDialog.java:248: if (UiUtils.showPhotoPicker(activity, this, mAllowMultiple)) { nit: combine this with the line above: if (activity != null && UiUtils....) { return; }
finnur@chromium.org changed reviewers: + jwd@chromium.org, miguelg@chromium.org
Thanks Theresa, I'm currently working on making these changes and since it looks like I'm close to landing, would Miguel and Jesse mind giving OWNERS approval? Jesse: histograms.xml Miguel: ProcessInitializationHandler.java BUILD.gn UiUtils.java OnPhotoPickerListener.java (soon to be renamed PhotoPickerListener.java)
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...
Theresa, PTAL. https://codereview.chromium.org/2737393002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/init/ProcessInitializationHandler.java (right): https://codereview.chromium.org/2737393002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/init/ProcessInitializationHandler.java:168: @Override On 2017/03/15 15:56:31, Theresa wrote: > nit: add a blank line before this one Done. https://codereview.chromium.org/2737393002/diff/140001/chrome/app/generated_r... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2737393002/diff/140001/chrome/app/generated_r... chrome/app/generated_resources.grd:5553: + Enable new Photopicker Good point. I'd say yes. https://codereview.chromium.org/2737393002/diff/140001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/OnPhotoPickerListener.java (right): https://codereview.chromium.org/2737393002/diff/140001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/OnPhotoPickerListener.java:14: public interface OnPhotoPickerListener { On 2017/03/15 15:56:32, Theresa wrote: > nit: I like "PhotoPickerListener" better than "OnPhotoPickerListener" for the > interface name. Done. https://codereview.chromium.org/2737393002/diff/140001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/OnPhotoPickerListener.java:32: * above. On 2017/03/15 15:56:32, Theresa wrote: > nit: this should fit on the line above. Done. https://codereview.chromium.org/2737393002/diff/140001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/UiUtils.java (right): https://codereview.chromium.org/2737393002/diff/140001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/UiUtils.java:83: * @param listener The listener that will be notified of the selection. On 2017/03/15 15:56:32, Theresa wrote: > nit: ... notified of the action the user took in the picker? Done. https://codereview.chromium.org/2737393002/diff/140001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/UiUtils.java:205: public static boolean showPhotoPicker( Oh, I didn't realize these were keyboard methods below. Done. https://codereview.chromium.org/2737393002/diff/140001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/UiUtils.java:212: public static void dismissPhotoPicker() { On 2017/03/15 15:56:32, Theresa wrote: > This method and the one above need JavaDocs. Done. https://codereview.chromium.org/2737393002/diff/140001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/UiUtils.java:214: sPhotoPickerDelegate.dismissPhotoPicker(); That's a good point. I'll take it into consideration. My initial reaction (without seeing how the multi-window works) is that we could make it so that only one dialog is open at a time, but I need to research this better. Shouldn't block this CL, though. https://codereview.chromium.org/2737393002/diff/140001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/base/SelectFileDialog.java (right): https://codereview.chromium.org/2737393002/diff/140001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/SelectFileDialog.java:248: if (UiUtils.showPhotoPicker(activity, this, mAllowMultiple)) { On 2017/03/15 15:56:32, Theresa wrote: > nit: combine this with the line above: > > if (activity != null && UiUtils....) { > return; > } Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm It's great to see this work coming along! https://codereview.chromium.org/2737393002/diff/140001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/UiUtils.java (right): https://codereview.chromium.org/2737393002/diff/140001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/UiUtils.java:205: public static boolean showPhotoPicker( On 2017/03/16 11:38:04, Finnur wrote: > Oh, I didn't realize these were keyboard methods below. Done. I don't think insertBefore, et al. are keyboard methods. I meant keeping setKeyboardShowingDelegate grouped with showKeyboard. https://codereview.chromium.org/2737393002/diff/160001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/UiUtils.java (right): https://codereview.chromium.org/2737393002/diff/160001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/UiUtils.java:133: public static void setPhotoPickerDelegate(PhotoPickerDelegate delegate) { nit: move this above showPhotoPicker()
histograms LGTM
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...
https://codereview.chromium.org/2737393002/diff/160001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/UiUtils.java (right): https://codereview.chromium.org/2737393002/diff/160001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/UiUtils.java:133: public static void setPhotoPickerDelegate(PhotoPickerDelegate delegate) { On 2017/03/16 16:26:17, Theresa wrote: > nit: move this above showPhotoPicker() Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm owners rubberstamp
The CQ bit was checked by finnur@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mvanouwerkerk@chromium.org, twellington@chromium.org, jwd@chromium.org Link to the patchset: https://codereview.chromium.org/2737393002/#ps180001 (title: "git cl upload")
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": 180001, "attempt_start_ts": 1489753389070230,
"parent_rev": "050301250e5a169e4cbfc4c322c5e25a3c7beaa3", "commit_rev":
"a6a921c6657a051d484306088a1d85b992f1a64f"}
Message was sent while issue was closed.
Description was changed from
==========
Implement the new Photo picker, part one.
This is the first step towards implementing a new Photo Picker for Chrome for
Android, broken down into multiple steps for easier reviewing.
First part includes simply replacing the stock Android picker with a Toast
saying "Not implemented".
Includes a Finch tie-in and a flag to turn this on manually ("Enable new
photopicker" in chrome://flags).
BUG=656015
==========
to
==========
Implement the new Photo picker, part one.
This is the first step towards implementing a new Photo Picker for Chrome for
Android, broken down into multiple steps for easier reviewing.
First part includes simply replacing the stock Android picker with a Toast
saying "Not implemented".
Includes a Finch tie-in and a flag to turn this on manually ("Enable new
photopicker" in chrome://flags).
BUG=656015
Review-Url: https://codereview.chromium.org/2737393002
Cr-Commit-Position: refs/heads/master@{#457741}
Committed:
https://chromium.googlesource.com/chromium/src/+/a6a921c6657a051d484306088a1d...
==========
Message was sent while issue was closed.
Committed patchset #8 (id:180001) as https://chromium.googlesource.com/chromium/src/+/a6a921c6657a051d484306088a1d... |
