Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2716293003:
Abstract ShareActivity Class (Closed)
DescriptionAbstract ShareActivity Class
This change abstracts the general requirements for activities that are
dynamically added to the share chooser intent. Namely, when the feature
should be enabled, and what to do when the option is choosen.
BUG=685856
Review-Url: https://codereview.chromium.org/2716293003
Cr-Commit-Position: refs/heads/master@{#454420}
Committed: https://chromium.googlesource.com/chromium/src/+/2b37f25bb3abb2bd85109b89dd6a4122f0d2ebf8
Patch Set 1 #
Total comments: 10
Patch Set 2 : Add Inner Availability Class #
Total comments: 14
Patch Set 3 : Removing unneeded imports #Patch Set 4 : Converting Inner Class to Abstract #
Total comments: 1
Patch Set 5 : Removed Availability #
Total comments: 4
Patch Set 6 : Privatizing getTriggeringActivity #Patch Set 7 : rebasing #
Messages
Total messages: 26 (6 generated)
iankc@google.com changed reviewers: + cco3@chromium.org, mattreynolds@chromium.org
Here is the Cl to create an Abstract ShareActivity for the PrintShareActivity and the PhysicalWebShareActivity to inherit from.
https://codereview.chromium.org/2716293003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2716293003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:205: { usually these brackets are placed next to each other I think private static final mything = new MyThing() {{ blah(); }}; https://codereview.chromium.org/2716293003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:206: add(new PrintShareActivity()); Oh, hm. I didn't realize you were creating instances of these activities. That's not exactly typical. Let's create a ShareActivity.Availability interface to use instead. It would have two methods: .isAvailable(Tab) and .getActivityClass(). https://codereview.chromium.org/2716293003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1195: for (int i = 0; i < SHARE_ACTIVITIES.size(); i++) { Given above advice: for (ShareActivity.Availability shareAcativityAvailability : SHARE_ACTIVITY_AVAILABILITIES) { if (shareActivityAvailability.isAvailable()) { classesToEnable.add(shareActivity); } } https://codereview.chromium.org/2716293003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/share/ShareActivity.java (right): https://codereview.chromium.org/2716293003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/share/ShareActivity.java:63: * Override to handle the desired action of the activity. Describe the method before describing why to override it: Completes the share action. Override this activity to implement desired share functionality. The activity will be destroyed immediately after this method is called. https://codereview.chromium.org/2716293003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/share/ShareActivity.java:68: * Override this method to describe the conditions this share option should be present same here.
https://codereview.chromium.org/2716293003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2716293003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:205: { On 2017/02/27 23:28:09, cco3 wrote: > usually these brackets are placed next to each other I think > > private static final mything = new MyThing() {{ > blah(); > }}; The linter throws errors when I upload code that has two '{' next to each other like '{{'. Also, it throws warnings when it's not in the format as above. So I ran 'git cl format' and this is the format the linter put it into... I thought it was weird too. https://codereview.chromium.org/2716293003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:206: add(new PrintShareActivity()); On 2017/02/27 23:28:09, cco3 wrote: > Oh, hm. I didn't realize you were creating instances of these activities. > That's not exactly typical. > > Let's create a ShareActivity.Availability interface to use instead. It would > have two methods: .isAvailable(Tab) and .getActivityClass(). Done. https://codereview.chromium.org/2716293003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1195: for (int i = 0; i < SHARE_ACTIVITIES.size(); i++) { On 2017/02/27 23:28:09, cco3 wrote: > Given above advice: > > for (ShareActivity.Availability shareAcativityAvailability : > SHARE_ACTIVITY_AVAILABILITIES) { > if (shareActivityAvailability.isAvailable()) { > classesToEnable.add(shareActivity); > } > } Done. https://codereview.chromium.org/2716293003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/share/ShareActivity.java (right): https://codereview.chromium.org/2716293003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/share/ShareActivity.java:63: * Override to handle the desired action of the activity. On 2017/02/27 23:28:09, cco3 wrote: > Describe the method before describing why to override it: > > Completes the share action. > Override this activity to implement desired share functionality. The activity > will be destroyed immediately after this method is called. Done. https://codereview.chromium.org/2716293003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/share/ShareActivity.java:68: * Override this method to describe the conditions this share option should be present On 2017/02/27 23:28:09, cco3 wrote: > same here. Done.
lgtm -imports Matt, could you take a look? https://codereview.chromium.org/2716293003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebShareActivity.java (right): https://codereview.chromium.org/2716293003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebShareActivity.java:10: import org.chromium.chrome.browser.share.ShareActivity.Availability; is this needed? https://codereview.chromium.org/2716293003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/printing/PrintShareActivity.java (right): https://codereview.chromium.org/2716293003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/printing/PrintShareActivity.java:10: import org.chromium.chrome.browser.share.ShareActivity.Availability; is this needed?
https://codereview.chromium.org/2716293003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebShareActivity.java (right): https://codereview.chromium.org/2716293003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebShareActivity.java:10: import org.chromium.chrome.browser.share.ShareActivity.Availability; On 2017/02/28 01:13:05, cco3 wrote: > is this needed? Done. https://codereview.chromium.org/2716293003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/printing/PrintShareActivity.java (right): https://codereview.chromium.org/2716293003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/printing/PrintShareActivity.java:10: import org.chromium.chrome.browser.share.ShareActivity.Availability; On 2017/02/28 01:13:05, cco3 wrote: > is this needed? Done.
https://codereview.chromium.org/2716293003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2716293003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:201: * Potential activities to add to the share picker intent. Could add a note to this comment to briefly describe how the ShareActivity.Availability objects are used? https://codereview.chromium.org/2716293003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebShareActivity.java (right): https://codereview.chromium.org/2716293003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebShareActivity.java:35: public boolean featureIsAvailable(Tab currentTab) { @Override https://codereview.chromium.org/2716293003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/printing/PrintShareActivity.java (right): https://codereview.chromium.org/2716293003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/printing/PrintShareActivity.java:37: public boolean featureIsAvailable(Tab currentTab) { @Override https://codereview.chromium.org/2716293003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/share/ShareActivity.java (right): https://codereview.chromium.org/2716293003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/share/ShareActivity.java:1: // Copyright 2016 The Chromium Authors. All rights reserved. 2017 https://codereview.chromium.org/2716293003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/share/ShareActivity.java:92: * @param currentTab The current Tab that chrome has open. This gives access to capitalize Chrome https://codereview.chromium.org/2716293003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/share/ShareActivity.java:96: public boolean featureIsAvailable(Tab currentTab) { Are you planning on using ShareActivity for always-available share options? If not, I think featureIsAvailable should be abstract since the default always-available implementation will never be useful.
https://codereview.chromium.org/2716293003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2716293003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:201: * Potential activities to add to the share picker intent. On 2017/02/28 01:33:28, mattreynolds wrote: > Could add a note to this comment to briefly describe how the > ShareActivity.Availability objects are used? Done. https://codereview.chromium.org/2716293003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebShareActivity.java (right): https://codereview.chromium.org/2716293003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebShareActivity.java:35: public boolean featureIsAvailable(Tab currentTab) { On 2017/02/28 01:33:28, mattreynolds wrote: > @Override Done. https://codereview.chromium.org/2716293003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/share/ShareActivity.java (right): https://codereview.chromium.org/2716293003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/share/ShareActivity.java:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2017/02/28 01:33:28, mattreynolds wrote: > 2017 Done. https://codereview.chromium.org/2716293003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/share/ShareActivity.java:92: * @param currentTab The current Tab that chrome has open. This gives access to On 2017/02/28 01:33:28, mattreynolds wrote: > capitalize Chrome Done.
cco3@chromium.org changed reviewers: + tedchoc@chromium.org
lgtm Hi Ted. Would you be able to take a look at this change that abstracts out common code between components used in the share sheet?
https://codereview.chromium.org/2716293003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2716293003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:208: add(new PrintShareActivity.Availability()); I'm not convinced this availability is worth it. This level of abstraction results in more code bloat than I think is is worth it for now. On Android (and especially in java), we try to not to introduce classes if not needed (i.e. it is usually better to implement an interface at the class level than have anonymous classes throughout, we're not very consistent there though). I think the split of ShareActivity makes sense, but unless there is a very strong need for this, I'd prefer to just leave the static methods for now.
On 2017/03/02 16:51:26, Ted C wrote: > https://codereview.chromium.org/2716293003/diff/60001/chrome/android/java/src... > File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java > (right): > > https://codereview.chromium.org/2716293003/diff/60001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:208: > add(new PrintShareActivity.Availability()); > I'm not convinced this availability is worth it. This level of abstraction > results in more code bloat than I think is is worth it for now. On Android (and > especially in java), we try to not to introduce classes if not needed (i.e. it > is usually better to implement an interface at the class level than have > anonymous classes throughout, we're not very consistent there though). > > I think the split of ShareActivity makes sense, but unless there is a very > strong need for this, I'd prefer to just leave the static methods for now. OK, we can put this on hold. I think after some of the other changes to PhysicalWebShareActivity are made, it might become clear that an abstract class will be more useful.
On 2017/03/02 18:19:07, cco3 wrote: > On 2017/03/02 16:51:26, Ted C wrote: > > > https://codereview.chromium.org/2716293003/diff/60001/chrome/android/java/src... > > File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java > > (right): > > > > > https://codereview.chromium.org/2716293003/diff/60001/chrome/android/java/src... > > chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:208: > > add(new PrintShareActivity.Availability()); > > I'm not convinced this availability is worth it. This level of abstraction > > results in more code bloat than I think is is worth it for now. On Android > (and > > especially in java), we try to not to introduce classes if not needed (i.e. it > > is usually better to implement an interface at the class level than have > > anonymous classes throughout, we're not very consistent there though). > > > > I think the split of ShareActivity makes sense, but unless there is a very > > strong need for this, I'd prefer to just leave the static methods for now. > > OK, we can put this on hold. I think after some of the other changes to > PhysicalWebShareActivity are made, it might become clear that an abstract class > will be more useful. I think the ShareActivity abstract class is something that we can and should land. I think it is just the inner class of Availability that I think is overkill for now.
On 2017/03/02 18:58:36, Ted C wrote: > On 2017/03/02 18:19:07, cco3 wrote: > > On 2017/03/02 16:51:26, Ted C wrote: > > > > > > https://codereview.chromium.org/2716293003/diff/60001/chrome/android/java/src... > > > File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java > > > (right): > > > > > > > > > https://codereview.chromium.org/2716293003/diff/60001/chrome/android/java/src... > > > chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:208: > > > add(new PrintShareActivity.Availability()); > > > I'm not convinced this availability is worth it. This level of abstraction > > > results in more code bloat than I think is is worth it for now. On Android > > (and > > > especially in java), we try to not to introduce classes if not needed (i.e. > it > > > is usually better to implement an interface at the class level than have > > > anonymous classes throughout, we're not very consistent there though). > > > > > > I think the split of ShareActivity makes sense, but unless there is a very > > > strong need for this, I'd prefer to just leave the static methods for now. > > > > OK, we can put this on hold. I think after some of the other changes to > > PhysicalWebShareActivity are made, it might become clear that an abstract > class > > will be more useful. > > I think the ShareActivity abstract class is something that we can and > should land. I think it is just the inner class of Availability that > I think is overkill for now. A gotchya. We can just make it an interface.
On 2017/03/02 19:02:28, cco3 wrote: > On 2017/03/02 18:58:36, Ted C wrote: > > On 2017/03/02 18:19:07, cco3 wrote: > > > On 2017/03/02 16:51:26, Ted C wrote: > > > > > > > > > > https://codereview.chromium.org/2716293003/diff/60001/chrome/android/java/src... > > > > File > chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java > > > > (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2716293003/diff/60001/chrome/android/java/src... > > > > > chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:208: > > > > add(new PrintShareActivity.Availability()); > > > > I'm not convinced this availability is worth it. This level of > abstraction > > > > results in more code bloat than I think is is worth it for now. On > Android > > > (and > > > > especially in java), we try to not to introduce classes if not needed > (i.e. > > it > > > > is usually better to implement an interface at the class level than have > > > > anonymous classes throughout, we're not very consistent there though). > > > > > > > > I think the split of ShareActivity makes sense, but unless there is a very > > > > strong need for this, I'd prefer to just leave the static methods for now. > > > > > > OK, we can put this on hold. I think after some of the other changes to > > > PhysicalWebShareActivity are made, it might become clear that an abstract > > class > > > will be more useful. > > > > I think the ShareActivity abstract class is something that we can and > > should land. I think it is just the inner class of Availability that > > I think is overkill for now. > > A gotchya. We can just make it an interface. eeek...I think i'm sending you on a wild goose chase here. For this change, I'm saying keep ShareActivity as is (as an abstract class), but get rid of the concept of Availability. Reintroduce the static methods in both classes and undo the change in ChromeActivity (it just builds the two item list as before). If we find the need for a ton of these share activities, we can revisit introducing a way of more dynamically registering these, but I think let's just introduce ShareActivity that helps with reducing the duplicate code shared between the two classes for now.
On 2017/03/02 19:54:07, Ted C wrote: > On 2017/03/02 19:02:28, cco3 wrote: > > On 2017/03/02 18:58:36, Ted C wrote: > > > On 2017/03/02 18:19:07, cco3 wrote: > > > > On 2017/03/02 16:51:26, Ted C wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/2716293003/diff/60001/chrome/android/java/src... > > > > > File > > chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java > > > > > (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2716293003/diff/60001/chrome/android/java/src... > > > > > > > chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:208: > > > > > add(new PrintShareActivity.Availability()); > > > > > I'm not convinced this availability is worth it. This level of > > abstraction > > > > > results in more code bloat than I think is is worth it for now. On > > Android > > > > (and > > > > > especially in java), we try to not to introduce classes if not needed > > (i.e. > > > it > > > > > is usually better to implement an interface at the class level than have > > > > > anonymous classes throughout, we're not very consistent there though). > > > > > > > > > > I think the split of ShareActivity makes sense, but unless there is a > very > > > > > strong need for this, I'd prefer to just leave the static methods for > now. > > > > > > > > OK, we can put this on hold. I think after some of the other changes to > > > > PhysicalWebShareActivity are made, it might become clear that an abstract > > > class > > > > will be more useful. > > > > > > I think the ShareActivity abstract class is something that we can and > > > should land. I think it is just the inner class of Availability that > > > I think is overkill for now. > > > > A gotchya. We can just make it an interface. > > eeek...I think i'm sending you on a wild goose chase here. > > For this change, I'm saying keep ShareActivity as is (as an > abstract class), but get rid of the concept of Availability. > > Reintroduce the static methods in both classes and undo the > change in ChromeActivity (it just builds the two item list > as before). > > If we find the need for a ton of these share activities, > we can revisit introducing a way of more dynamically registering > these, but I think let's just introduce ShareActivity that > helps with reducing the duplicate code shared between the two > classes for now. OK, thanks.
Hey Ted, I removed the availability code. Let me know what you think. Thanks
https://codereview.chromium.org/2716293003/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/share/ShareActivity.java (right): https://codereview.chromium.org/2716293003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/share/ShareActivity.java:52: // Since the share intent is triggered without NEW_TASK or NEW_DOCUMENT, the task ID I made this comment invalid in one of my earlier patches in the area, can you remove it now that you're moving this so its lies don't continue :-) https://codereview.chromium.org/2716293003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/share/ShareActivity.java:66: protected abstract void handleShareAction(); Should we pass the triggering activity here? That can return null so I think it would make sense to get the triggering activity in onCreate, if null, return and only in the non-null case call into handleShareAction. Then the method can be private as well.
Hey Ted, Passing in the triggering activity makes it look a lot better. https://codereview.chromium.org/2716293003/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/share/ShareActivity.java (right): https://codereview.chromium.org/2716293003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/share/ShareActivity.java:52: // Since the share intent is triggered without NEW_TASK or NEW_DOCUMENT, the task ID On 2017/03/02 21:21:26, Ted C wrote: > I made this comment invalid in one of my earlier patches in the area, can you > remove it now that you're moving this so its lies don't continue :-) Done. https://codereview.chromium.org/2716293003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/share/ShareActivity.java:66: protected abstract void handleShareAction(); On 2017/03/02 21:21:26, Ted C wrote: > Should we pass the triggering activity here? That can return null so I think it > would make sense to get the triggering activity in onCreate, if null, return and > only in the non-null case call into handleShareAction. Then the method can be > private as well. Done.
lgtm
The CQ bit was checked by iankc@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from cco3@chromium.org, tedchoc@chromium.org Link to the patchset: https://codereview.chromium.org/2716293003/#ps110001 (title: "rebasing")
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": 110001, "attempt_start_ts": 1488491973518400,
"parent_rev": "e52518a3a6faa15b49d85a4eeb278300f4bcdaa1", "commit_rev":
"2b37f25bb3abb2bd85109b89dd6a4122f0d2ebf8"}
Message was sent while issue was closed.
Description was changed from ========== Abstract ShareActivity Class This change abstracts the general requirements for activities that are dynamically added to the share chooser intent. Namely, when the feature should be enabled, and what to do when the option is choosen. BUG=685856 ========== to ========== Abstract ShareActivity Class This change abstracts the general requirements for activities that are dynamically added to the share chooser intent. Namely, when the feature should be enabled, and what to do when the option is choosen. BUG=685856 Review-Url: https://codereview.chromium.org/2716293003 Cr-Commit-Position: refs/heads/master@{#454420} Committed: https://chromium.googlesource.com/chromium/src/+/2b37f25bb3abb2bd85109b89dd6a... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:110001) as https://chromium.googlesource.com/chromium/src/+/2b37f25bb3abb2bd85109b89dd6a... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
