Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2694103002:
Create OptionalShareTargetsManager (Closed)
DescriptionCreate OptionalShareTargetsManager
This change creates a class to manage the optional share activities to
be displayed in the share intent chooser. It works by taking in classes
to be displayed and enables them as componenents. It also tracks the
state of the application to disable the components.
BUG=685856
Review-Url: https://codereview.chromium.org/2694103002
Cr-Commit-Position: refs/heads/master@{#450468}
Committed: https://chromium.googlesource.com/chromium/src/+/5cbba721b07114d34c83086486bf9f4e5f69b5f2
Patch Set 1 #
Total comments: 20
Patch Set 2 : Fixing Spelling #
Total comments: 13
Patch Set 3 : Fixing Teds comments 1 #
Messages
Total messages: 18 (7 generated)
Description was changed from ========== Create OptionalShareTargetsManager This change creates a class to manage the optional share activities to be displayed in the share intent chooser. It works by taking in classes to be displayed and enables them as componenents. It also tracks the state of the application to disable the components. BUG=685856 ========== to ========== Create OptionalShareTargetsManager This change creates a class to manage the optional share activities to be displayed in the share intent chooser. It works by taking in classes to be displayed and enables them as componenents. It also tracks the state of the application to disable the components. BUG=685856 ==========
iankc@google.com changed reviewers: + mattreynolds@chromium.org
Hey Matt, Here is a refactor that Ted requested. Basically, he didn't like how big sharehelper was getting so we are making a manager class to handle the extra options.
https://codereview.chromium.org/2694103002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2694103002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1153: new ArrayList<Class<? extends Activity>>(1); Can you move classesToEnable into the if block below? https://codereview.chromium.org/2694103002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/printing/PrintShareActivity.java (right): https://codereview.chromium.org/2694103002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/printing/PrintShareActivity.java:68: public static boolean printingIsEnabled(Tab currentTab) { Add javadoc https://codereview.chromium.org/2694103002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/printing/PrintShareActivity.java:70: if (printingController != null && !currentTab.isNativePage() && !printingController.isBusy() simplify "if (condition) { return true; } return false;" to "return condition;" https://codereview.chromium.org/2694103002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/share/OptionalShareTargetsManager.java (right): https://codereview.chromium.org/2694103002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/share/OptionalShareTargetsManager.java:27: * This class is the manager for enabling optional share activities to be added Remove "This class is". Also, I think it would be clearer to shorten it to "A manager for optional share activities in the share picker intent." https://codereview.chromium.org/2694103002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/share/OptionalShareTargetsManager.java:33: /** Variables for adding activities dynamically to share picker intent. */ This comment doesn't really add any new info, let's remove it. https://codereview.chromium.org/2694103002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/share/OptionalShareTargetsManager.java:41: * Enable the sharing options. Enable -> Enables (don't use imperative mood in javadoc) Also, I think this would read better as "Enable sharing options." https://codereview.chromium.org/2694103002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/share/OptionalShareTargetsManager.java:43: * activitiy's activitiy's -> activity's Also please fix the text wrapping. https://codereview.chromium.org/2694103002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/share/OptionalShareTargetsManager.java:44: * state will be tracked to disable the optiones when the share operation optiones -> options, here and one more below https://codereview.chromium.org/2694103002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/share/OptionalShareTargetsManager.java:46: * @param enabledClasses classes to be enbaled. enbaled -> enabled https://codereview.chromium.org/2694103002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/share/OptionalShareTargetsManager.java:103: * @param triggeringActivity The actvitity that is triggering the share action. actvitity -> activity
Just want to say, many of the spelling errors that you found were copy & pasted... https://codereview.chromium.org/2694103002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2694103002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1153: new ArrayList<Class<? extends Activity>>(1); On 2017/02/13 23:08:50, mattreynolds wrote: > Can you move classesToEnable into the if block below? Done. https://codereview.chromium.org/2694103002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/printing/PrintShareActivity.java (right): https://codereview.chromium.org/2694103002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/printing/PrintShareActivity.java:68: public static boolean printingIsEnabled(Tab currentTab) { On 2017/02/13 23:08:50, mattreynolds wrote: > Add javadoc Done. https://codereview.chromium.org/2694103002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/printing/PrintShareActivity.java:70: if (printingController != null && !currentTab.isNativePage() && !printingController.isBusy() On 2017/02/13 23:08:50, mattreynolds wrote: > simplify "if (condition) { return true; } return false;" to "return condition;" Done. https://codereview.chromium.org/2694103002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/share/OptionalShareTargetsManager.java (right): https://codereview.chromium.org/2694103002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/share/OptionalShareTargetsManager.java:27: * This class is the manager for enabling optional share activities to be added On 2017/02/13 23:08:50, mattreynolds wrote: > Remove "This class is". Also, I think it would be clearer to shorten it to "A > manager for optional share activities in the share picker intent." Done. https://codereview.chromium.org/2694103002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/share/OptionalShareTargetsManager.java:33: /** Variables for adding activities dynamically to share picker intent. */ On 2017/02/13 23:08:51, mattreynolds wrote: > This comment doesn't really add any new info, let's remove it. Done. https://codereview.chromium.org/2694103002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/share/OptionalShareTargetsManager.java:41: * Enable the sharing options. On 2017/02/13 23:08:51, mattreynolds wrote: > Enable -> Enables (don't use imperative mood in javadoc) > > Also, I think this would read better as "Enable sharing options." Done. https://codereview.chromium.org/2694103002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/share/OptionalShareTargetsManager.java:43: * activitiy's On 2017/02/13 23:08:50, mattreynolds wrote: > activitiy's -> activity's > > Also please fix the text wrapping. Done. https://codereview.chromium.org/2694103002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/share/OptionalShareTargetsManager.java:44: * state will be tracked to disable the optiones when the share operation On 2017/02/13 23:08:51, mattreynolds wrote: > optiones -> options, here and one more below Done. https://codereview.chromium.org/2694103002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/share/OptionalShareTargetsManager.java:46: * @param enabledClasses classes to be enbaled. On 2017/02/13 23:08:51, mattreynolds wrote: > enbaled -> enabled Done. https://codereview.chromium.org/2694103002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/share/OptionalShareTargetsManager.java:103: * @param triggeringActivity The actvitity that is triggering the share action. On 2017/02/13 23:08:51, mattreynolds wrote: > actvitity -> activity Done.
Just want to say, many of the spelling errors that you found were copy & pasted...
lgtm
iankc@google.com changed reviewers: + tedchoc@chromium.org
Hey Ted, Here is the cl for the changes you requested in https://codereview.chromium.org/2680983002/ I decided it was a big enough change to warrant another cl. Just to let you know, I'm trying to get this in by asap for feature freeze on Friday. Thanks!
https://codereview.chromium.org/2694103002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2694103002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1154: new ArrayList<Class<? extends Activity>>(1); For this, you should be able to do: List<Class<? extends Activity>> classesToEnable = new ArrayList<>(1); https://codereview.chromium.org/2694103002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/share/OptionalShareTargetsManager.java (right): https://codereview.chromium.org/2694103002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/share/OptionalShareTargetsManager.java:1: // Copyright 2014 The Chromium Authors. All rights reserved. should be 2017 https://codereview.chromium.org/2694103002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/share/OptionalShareTargetsManager.java:66: if (wasEmpty) { FWIW if you get two calls to enableOptionalShareTargets with different enabled classes, only the first will win. I don't think this will actually ever be an issue in the future either, but something we can noodle about if it does start popping up. https://codereview.chromium.org/2694103002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/share/OptionalShareTargetsManager.java:71: sEnabledComponents = new ArrayList<ComponentName>(enabledClasses.size()); same things here, can just do <> after ArrayList https://codereview.chromium.org/2694103002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/share/OptionalShareTargetsManager.java:99: * Disables all of the previously enabled components. This is a bit misleading. Really, this is just saying that the triggering activity has finished the share operation (either with a share to a different app, or a cancelled share back to chrome). It only disables the enabled components if all pending shares have been completed. Might want to clarify slightly. https://codereview.chromium.org/2694103002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/share/OptionalShareTargetsManager.java:114: for (ComponentName component : sEnabledComponents) { should use the for (int i = 0; ...) method of iterating non-arrays. This allocates an iterator where we don't want to do any more object allocation than needed.
Hey Ted, I fixed those comments. Thanks for the quick review https://codereview.chromium.org/2694103002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2694103002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1154: new ArrayList<Class<? extends Activity>>(1); On 2017/02/14 18:17:03, Ted C wrote: > For this, you should be able to do: > > List<Class<? extends Activity>> classesToEnable = new ArrayList<>(1); Done. https://codereview.chromium.org/2694103002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/share/OptionalShareTargetsManager.java (right): https://codereview.chromium.org/2694103002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/share/OptionalShareTargetsManager.java:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2017/02/14 18:17:03, Ted C wrote: > should be 2017 Done. https://codereview.chromium.org/2694103002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/share/OptionalShareTargetsManager.java:66: if (wasEmpty) { On 2017/02/14 18:17:03, Ted C wrote: > FWIW if you get two calls to enableOptionalShareTargets with different enabled > classes, only the first will win. > > I don't think this will actually ever be an issue in the future either, but > something we can noodle about if it does start popping up. Done. https://codereview.chromium.org/2694103002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/share/OptionalShareTargetsManager.java:71: sEnabledComponents = new ArrayList<ComponentName>(enabledClasses.size()); On 2017/02/14 18:17:03, Ted C wrote: > same things here, can just do <> after ArrayList Done. https://codereview.chromium.org/2694103002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/share/OptionalShareTargetsManager.java:99: * Disables all of the previously enabled components. On 2017/02/14 18:17:03, Ted C wrote: > This is a bit misleading. Really, this is just saying that the triggering > activity has finished the share operation (either with a share to a different > app, or a cancelled share back to chrome). It only disables the enabled > components if all pending shares have been completed. > > Might want to clarify slightly. Done. https://codereview.chromium.org/2694103002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/share/OptionalShareTargetsManager.java:114: for (ComponentName component : sEnabledComponents) { On 2017/02/14 18:17:03, Ted C wrote: > should use the for (int i = 0; ...) method of iterating non-arrays. This > allocates an iterator where we don't want to do any more object allocation than > needed. Done. Out of curiousity, why? Just for memory purposes?
lgtm https://codereview.chromium.org/2694103002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/share/OptionalShareTargetsManager.java (right): https://codereview.chromium.org/2694103002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/share/OptionalShareTargetsManager.java:114: for (ComponentName component : sEnabledComponents) { On 2017/02/14 18:59:43, iankc wrote: > On 2017/02/14 18:17:03, Ted C wrote: > > should use the for (int i = 0; ...) method of iterating non-arrays. This > > allocates an iterator where we don't want to do any more object allocation > than > > needed. > > Done. Out of curiousity, why? Just for memory purposes? Yeah, just memory. It really doesn't matter in this case as it is called so infrequently. This is really important on per frame updates, but it is easier to have a blanket rule than try to piece out when to use it.
The CQ bit was checked by iankc@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from mattreynolds@chromium.org Link to the patchset: https://codereview.chromium.org/2694103002/#ps40001 (title: "Fixing Teds comments 1")
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": 1487099350939050,
"parent_rev": "0b6ac93479e27fcfeb00c9a88483df9c8f37ecda", "commit_rev":
"5cbba721b07114d34c83086486bf9f4e5f69b5f2"}
Message was sent while issue was closed.
Description was changed from ========== Create OptionalShareTargetsManager This change creates a class to manage the optional share activities to be displayed in the share intent chooser. It works by taking in classes to be displayed and enables them as componenents. It also tracks the state of the application to disable the components. BUG=685856 ========== to ========== Create OptionalShareTargetsManager This change creates a class to manage the optional share activities to be displayed in the share intent chooser. It works by taking in classes to be displayed and enables them as componenents. It also tracks the state of the application to disable the components. BUG=685856 Review-Url: https://codereview.chromium.org/2694103002 Cr-Commit-Position: refs/heads/master@{#450468} Committed: https://chromium.googlesource.com/chromium/src/+/5cbba721b07114d34c83086486bf... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/5cbba721b07114d34c83086486bf... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
