Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2666833002:
Create entry to PWSharing through ShareHelper (Closed)
DescriptionCreate entry to PWSharing Through ShareHelper
This change creates makes it easier to dynamically add chrome
activities to the share-chooser display. New activities will extend
ShareOption, and override the handleShareAction and featureIsEnabled
methods. The classes also need to add themselves to the
getPotentialShareOptionsList method in ChromeActivity.
This change also adds an implementation of the above instructions in
the form of the PhysicalWebShareActivity. This will be the main entry
way into the the PhysicalWebSharingFeature, which is hidden behind a
feature flag.
BUG=685856
Patch Set 1 #
Total comments: 10
Patch Set 2 : Fixing Comments #
Total comments: 39
Patch Set 3 : Fixing more Comments #Patch Set 4 : Unregister now disables component #
Total comments: 6
Patch Set 5 : Fixing Unregister Logic #
Total comments: 2
Patch Set 6 : Abstracting ShareOption #
Total comments: 10
Patch Set 7 : Reduce ChromeActivity Logic #
Total comments: 4
Patch Set 8 : syncing with master #
Total comments: 4
Patch Set 9 : Cleaning up Chrome Activity more #
Total comments: 28
Patch Set 10 : Reducing Share Helper Redudancy #
Total comments: 6
Patch Set 11 : Fixing nits #
Total comments: 2
Messages
Total messages: 31 (4 generated)
Description was changed from ========== Entry to Physical Web Sharing Through ShareHelper This change creates an icon and clickable button in the share app- chooser, modeled in the same way as the printShareActivity. Because the share app-chooser is can be launched by only one activity, we had to refactor to allow for the print & physicalwebsharing activity to show. It is important make sure that we're not going to see unexpected behavior from the PrintShareActivity. BUG=685856 ========== to ========== Entry to Physical Web Sharing Through ShareHelper This change creates an icon and clickable button in the share app- chooser, modeled in the same way as the printShareActivity. Because the share app-chooser is can be launched by only one activity, we had to refactor to allow for the print & physicalwebsharing activity to show. It is important make sure that we're not going to see unexpected behavior from the PrintShareActivity. BUG=685856 ==========
iankc@google.com changed reviewers: + cco3@chromium.org, mattreynolds@chromium.org
https://codereview.chromium.org/2666833002/diff/1/chrome/android/java/Android... File chrome/android/java/AndroidManifest.xml (right): https://codereview.chromium.org/2666833002/diff/1/chrome/android/java/Android... chrome/android/java/AndroidManifest.xml:626: android:exported="true" exported? Do we need this? Other apps shouldn't be using it, at least not yet. https://codereview.chromium.org/2666833002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2666833002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1147: if (!PhysicalWeb.isPhysicalWebSharingEnabled()) { //TODO(check if Bluetooth Enabled) {} You've probably seen: https://codereview.chromium.org/2664053002/ Use that assuming it goes in. https://codereview.chromium.org/2666833002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWeb.java (right): https://codereview.chromium.org/2666833002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWeb.java:169: public static boolean isPhysicalWebSharingEnabled() { isSharingEnabled or isSharingFeatureEnabled seems sufficient https://codereview.chromium.org/2666833002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/printing/PrintShareActivity.java (right): https://codereview.chromium.org/2666833002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/printing/PrintShareActivity.java:41: private static void unregisterActivity(final Activity activity) { This should probably also be moved. https://codereview.chromium.org/2666833002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java (right): https://codereview.chromium.org/2666833002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:67: remove https://codereview.chromium.org/2666833002/diff/1/chrome/android/java/strings... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/2666833002/diff/1/chrome/android/java/strings... chrome/android/java/strings/android_chrome_strings.grd:2612: Beacon Share "Physical Web" seems more appropriate.
https://codereview.chromium.org/2666833002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/printing/PrintShareActivity.java (left): https://codereview.chromium.org/2666833002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/printing/PrintShareActivity.java:180: unregisterActivity(triggeringActivity); ShareHelper.unregisterActivity? https://codereview.chromium.org/2666833002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java (right): https://codereview.chromium.org/2666833002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:702: * @param callback The callback to be triggered after the options have been enabled. This javadoc (@param options) https://codereview.chromium.org/2666833002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:770: activity.getPackageManager().setComponentEnabledSetting( fix indent https://codereview.chromium.org/2666833002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:799: Log.e(TAG, "Physical Web Share state change task did not complete as expected"); "Share Helper state change task..."
Fixed conley and matts comments
https://codereview.chromium.org/2666833002/diff/20001/chrome/android/java/And... File chrome/android/java/AndroidManifest.xml (right): https://codereview.chromium.org/2666833002/diff/20001/chrome/android/java/And... chrome/android/java/AndroidManifest.xml:619: <!-- This activity is to expose the PhysicalWeb Share option via the generic Android share action. --> I think it'd be preferable to put this with the other Physical Web activities. https://codereview.chromium.org/2666833002/diff/20001/chrome/android/java/And... chrome/android/java/AndroidManifest.xml:629: <intent-filter> Is any of this intent filter needed anymore? https://codereview.chromium.org/2666833002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2666833002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1146: public static List<Class> getAdditionalShareOptionsList(Tab currentTab) { Why is this public? Also, it's not clear to me that this is helpful as its own method, but that's just a nit. https://codereview.chromium.org/2666833002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1149: // Check and handle Printing option Full stop after sentence comments. Same below. https://codereview.chromium.org/2666833002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1158: if (!PhysicalWeb.isSharingEnabled()) { This isn't a sufficient check yet. https://codereview.chromium.org/2666833002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1194: ContentBitmapCallback callback = new ContentBitmapCallback() { Is this screenshot stuff specific to printing? If so, we won't want to run it when we do a Physical Web broadcast. https://codereview.chromium.org/2666833002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWeb.java (right): https://codereview.chromium.org/2666833002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWeb.java:164: /** I think it'd make more sense to put this method directly under featureIsEnabled. Also, please make the grammar match that function (sharingIsEnabled()) https://codereview.chromium.org/2666833002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebShareActivity.java (right): https://codereview.chromium.org/2666833002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebShareActivity.java:17: * A simple activity that allows Chrome to expose print as an option in the share menu. No printing https://codereview.chromium.org/2666833002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebShareActivity.java:21: private static final String TAG = "pw_share"; PhysicalWeb https://codereview.chromium.org/2666833002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebShareActivity.java:31: if (!IntentUtils.safeHasExtra(getIntent(), ShareHelper.EXTRA_TASK_ID)) return; Just use intent (not getIntent()) https://codereview.chromium.org/2666833002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebShareActivity.java:32: Log.v(TAG, "Starting Service here"); I'd drop this. https://codereview.chromium.org/2666833002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebShareActivity.java:32: Log.v(TAG, "Starting Service here"); Also, I'd create a new private handleBroadcastAction() and call it here. In its body write `TODO(iankc): Implement this.`
https://codereview.chromium.org/2666833002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2666833002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1151: if (printingController != null && (currentTab == null || !currentTab.isNativePage()) If currentTab is null is it still possible for Physical Web sharing to work? https://codereview.chromium.org/2666833002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1158: if (!PhysicalWeb.isSharingEnabled()) { remove '!' I'm not sure what permissions we should check here. Since we aren't scanning and aren't uploading any location-sensitive data to a Google server, we probably don't need location. So far we aren't checking for OS version, but I think this feature will only work on Android M+ due to the Bluetooth permission. If that's true, we should check OS version here and only add the menu item for M+. https://codereview.chromium.org/2666833002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1194: ContentBitmapCallback callback = new ContentBitmapCallback() { On 2017/01/31 19:42:55, cco3 wrote: > Is this screenshot stuff specific to printing? If so, we won't want to run it > when we do a Physical Web broadcast. I think at this point the user hasn't made a selection yet. https://codereview.chromium.org/2666833002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWeb.java (right): https://codereview.chromium.org/2666833002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWeb.java:164: /** On 2017/01/31 19:42:55, cco3 wrote: > I think it'd make more sense to put this method directly under featureIsEnabled. > Also, please make the grammar match that function (sharingIsEnabled()) +1 https://codereview.chromium.org/2666833002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWeb.java:165: * Checks wheter the Physical Web Sharing feature is enabled. wheter -> whether https://codereview.chromium.org/2666833002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/printing/PrintShareActivity.java (right): https://codereview.chromium.org/2666833002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/printing/PrintShareActivity.java:62: ChromeActivity.getAdditionalShareOptionsList(null)); Is this correct? Before, we'd call activity.getPackageManager().setComponentEnabledSetting(...) for PrintShareActivity.class. Now we'll skip that call due to the empty options list. https://codereview.chromium.org/2666833002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java (right): https://codereview.chromium.org/2666833002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:113: /** Variables for adding options dynamically to app chooser*/ nit: add period and space before */ https://codereview.chromium.org/2666833002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:769: activity.getPackageManager().setComponentEnabledSetting( The indent level still looks wrong in the diff view. https://codereview.chromium.org/2666833002/diff/20001/chrome/browser/android/... File chrome/browser/android/chrome_feature_list.cc (right): https://codereview.chromium.org/2666833002/diff/20001/chrome/browser/android/... chrome/browser/android/chrome_feature_list.cc:136: remove newline (keep one blank line between features)
iankc@google.com changed reviewers: + tedchoc@chromium.org
Hey Ted! I'm working on refactoring the PrintShareActivity to make it easier to add chrome-based activities to the Share chooser. I've migrated a lot of the code to the ShareHelper, and it now accepts a list of class types to include in the chooser. We just want to make sure that there shouldn't be any unexpected behavior from the refactor so it would be awesome if you could take a look. Thanks! https://codereview.chromium.org/2666833002/diff/20001/chrome/android/java/And... File chrome/android/java/AndroidManifest.xml (right): https://codereview.chromium.org/2666833002/diff/20001/chrome/android/java/And... chrome/android/java/AndroidManifest.xml:619: <!-- This activity is to expose the PhysicalWeb Share option via the generic Android share action. --> On 2017/01/31 19:42:55, cco3 wrote: > I think it'd be preferable to put this with the other Physical Web activities. Done. https://codereview.chromium.org/2666833002/diff/20001/chrome/android/java/And... chrome/android/java/AndroidManifest.xml:629: <intent-filter> On 2017/01/31 19:42:55, cco3 wrote: > Is any of this intent filter needed anymore? Done. https://codereview.chromium.org/2666833002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2666833002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1146: public static List<Class> getAdditionalShareOptionsList(Tab currentTab) { On 2017/01/31 19:42:55, cco3 wrote: > Why is this public? > > Also, it's not clear to me that this is helpful as its own method, but that's > just a nit. Acknowledged. https://codereview.chromium.org/2666833002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1149: // Check and handle Printing option On 2017/01/31 19:42:55, cco3 wrote: > Full stop after sentence comments. Same below. Done. https://codereview.chromium.org/2666833002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1151: if (printingController != null && (currentTab == null || !currentTab.isNativePage()) On 2017/01/31 20:30:33, mattreynolds wrote: > If currentTab is null is it still possible for Physical Web sharing to work? Acknowledged. https://codereview.chromium.org/2666833002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1158: if (!PhysicalWeb.isSharingEnabled()) { On 2017/01/31 19:42:55, cco3 wrote: > This isn't a sufficient check yet. Done. https://codereview.chromium.org/2666833002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1158: if (!PhysicalWeb.isSharingEnabled()) { On 2017/01/31 19:42:55, cco3 wrote: > This isn't a sufficient check yet. Acknowledged. https://codereview.chromium.org/2666833002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWeb.java (right): https://codereview.chromium.org/2666833002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWeb.java:164: /** On 2017/01/31 19:42:55, cco3 wrote: > I think it'd make more sense to put this method directly under featureIsEnabled. > Also, please make the grammar match that function (sharingIsEnabled()) Done. https://codereview.chromium.org/2666833002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWeb.java:165: * Checks wheter the Physical Web Sharing feature is enabled. On 2017/01/31 20:30:33, mattreynolds wrote: > wheter -> whether Done. https://codereview.chromium.org/2666833002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebShareActivity.java (right): https://codereview.chromium.org/2666833002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebShareActivity.java:17: * A simple activity that allows Chrome to expose print as an option in the share menu. On 2017/01/31 19:42:55, cco3 wrote: > No printing Done. https://codereview.chromium.org/2666833002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebShareActivity.java:21: private static final String TAG = "pw_share"; On 2017/01/31 19:42:55, cco3 wrote: > PhysicalWeb Done. https://codereview.chromium.org/2666833002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebShareActivity.java:31: if (!IntentUtils.safeHasExtra(getIntent(), ShareHelper.EXTRA_TASK_ID)) return; On 2017/01/31 19:42:56, cco3 wrote: > Just use intent (not getIntent()) Done. https://codereview.chromium.org/2666833002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebShareActivity.java:32: Log.v(TAG, "Starting Service here"); On 2017/01/31 19:42:55, cco3 wrote: > Also, I'd create a new private handleBroadcastAction() and call it here. In its > body write `TODO(iankc): Implement this.` Done. https://codereview.chromium.org/2666833002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebShareActivity.java:32: Log.v(TAG, "Starting Service here"); On 2017/01/31 19:42:55, cco3 wrote: > I'd drop this. Done. https://codereview.chromium.org/2666833002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/printing/PrintShareActivity.java (right): https://codereview.chromium.org/2666833002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/printing/PrintShareActivity.java:62: ChromeActivity.getAdditionalShareOptionsList(null)); On 2017/01/31 20:30:33, mattreynolds wrote: > Is this correct? Before, we'd call > activity.getPackageManager().setComponentEnabledSetting(...) for > PrintShareActivity.class. Now we'll skip that call due to the empty options > list. Acknowledged. https://codereview.chromium.org/2666833002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java (right): https://codereview.chromium.org/2666833002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:113: /** Variables for adding options dynamically to app chooser*/ On 2017/01/31 20:30:33, mattreynolds wrote: > nit: add period and space before */ Done. https://codereview.chromium.org/2666833002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:769: activity.getPackageManager().setComponentEnabledSetting( On 2017/01/31 20:30:33, mattreynolds wrote: > The indent level still looks wrong in the diff view. OK, do you have a suggestion on what to do. Both lines are using spaces and they're showing up correctly on my sublime editor. https://codereview.chromium.org/2666833002/diff/20001/chrome/browser/android/... File chrome/browser/android/chrome_feature_list.cc (right): https://codereview.chromium.org/2666833002/diff/20001/chrome/browser/android/... chrome/browser/android/chrome_feature_list.cc:136: On 2017/01/31 20:30:33, mattreynolds wrote: > remove newline (keep one blank line between features) Done.
Ok, this is now ready for review.
https://codereview.chromium.org/2666833002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2666833002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1158: public static List<Class> getAdditionalShareOptionsList(Tab currentTab) { How do you know that this will give you the same values when you call it to register and when you call it to unregister? Also, it seems a little dirty for the specific share activities to call this method...let's try to find a way around that. https://codereview.chromium.org/2666833002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebShareActivity.java (right): https://codereview.chromium.org/2666833002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebShareActivity.java:38: //TODO(iankc): implement this. Don't you need to call unregister here? https://codereview.chromium.org/2666833002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java (right): https://codereview.chromium.org/2666833002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:732: Log.v(TAG, "enableOptions: " + options.get(i).getName()); remove extra logging.
https://codereview.chromium.org/2666833002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2666833002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1158: public static List<Class> getAdditionalShareOptionsList(Tab currentTab) { On 2017/02/02 00:58:57, cco3 wrote: > How do you know that this will give you the same values when you call it to > register and when you call it to unregister? Also, it seems a little dirty for > the specific share activities to call this method...let's try to find a way > around that. Done. https://codereview.chromium.org/2666833002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebShareActivity.java (right): https://codereview.chromium.org/2666833002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebShareActivity.java:38: //TODO(iankc): implement this. On 2017/02/02 00:58:58, cco3 wrote: > Don't you need to call unregister here? Done. https://codereview.chromium.org/2666833002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java (right): https://codereview.chromium.org/2666833002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:732: Log.v(TAG, "enableOptions: " + options.get(i).getName()); On 2017/02/02 00:58:58, cco3 wrote: > remove extra logging. Done.
https://codereview.chromium.org/2666833002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2666833002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1163: if (printingController != null && !currentTab.isNativePage() If any of these can change over the course of a share, we can't just statically call this in unregisterShareActivities(). For example I could imagine printingController.isBusy() changing. Instead, why don't you just unregister (i.e., mark as disabled) all possible options indiscriminately? https://codereview.chromium.org/2666833002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1227: public void unRegisterShareActivities() { `unregisterShareActivities()`
Also, please update the commit subject/description as discussed.
Description was changed from ========== Entry to Physical Web Sharing Through ShareHelper This change creates an icon and clickable button in the share app- chooser, modeled in the same way as the printShareActivity. Because the share app-chooser is can be launched by only one activity, we had to refactor to allow for the print & physicalwebsharing activity to show. It is important make sure that we're not going to see unexpected behavior from the PrintShareActivity. BUG=685856 ========== to ========== Create entry to PWSharing Through ShareHelper This change creates makes it easier to dynamically add chrome activities to the share-chooser display. New activities will extend ShareOption, and override the handleShareAction and featureIsEnabled methods. The classes also need to add themselves to the getPotentialShareOptionsList method in ChromeActivity. This change also adds an implementation of the above instructions in the form of the PhysicalWebShareActivity. This will be the main entry way into the the PhysicalWebSharingFeature, which is hidden behind a feature flag. BUG=685856 ==========
Fixed comments and abstracted the similiarities between PrintShareActivity and PhysicalWebSharingActivity into AbstractShareOption. Now we can easily add chrome based activities to the share chooser.
https://codereview.chromium.org/2666833002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2666833002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1213: List<AbstractShareOption> options = new ArrayList<AbstractShareOption>(2); can't this just be a private static final field instead of a method? https://codereview.chromium.org/2666833002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1239: ShareHelper.unregisterActivity(this, getPotentialShareOptionsList()); Why don't you move the potential share options to the share helper. Then you won't need this method at all. The activities that call this can call the ShareHelper static method directly. https://codereview.chromium.org/2666833002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/share/AbstractShareOption.java (right): https://codereview.chromium.org/2666833002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/share/AbstractShareOption.java:25: public abstract class AbstractShareOption extends AppCompatActivity { I think this might be better as `ShareActivity` https://codereview.chromium.org/2666833002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/share/AbstractShareOption.java:27: private static final String TAG = "AbstractShareOption"; no TAG needed https://codereview.chromium.org/2666833002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/share/AbstractShareOption.java:45: protected void unRegisterActivities() { `unregisterActivities` Can this be private?
Migrated code from chromeactivity to sharehelper. https://codereview.chromium.org/2666833002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2666833002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1213: List<AbstractShareOption> options = new ArrayList<AbstractShareOption>(2); On 2017/02/03 01:46:32, cco3 wrote: > can't this just be a private static final field instead of a method? Done. https://codereview.chromium.org/2666833002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1239: ShareHelper.unregisterActivity(this, getPotentialShareOptionsList()); On 2017/02/03 01:46:32, cco3 wrote: > Why don't you move the potential share options to the share helper. Then you > won't need this method at all. The activities that call this can call the > ShareHelper static method directly. Done. https://codereview.chromium.org/2666833002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/share/AbstractShareOption.java (right): https://codereview.chromium.org/2666833002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/share/AbstractShareOption.java:25: public abstract class AbstractShareOption extends AppCompatActivity { On 2017/02/03 01:46:32, cco3 wrote: > I think this might be better as `ShareActivity` Done. https://codereview.chromium.org/2666833002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/share/AbstractShareOption.java:27: private static final String TAG = "AbstractShareOption"; On 2017/02/03 01:46:32, cco3 wrote: > no TAG needed Done. https://codereview.chromium.org/2666833002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/share/AbstractShareOption.java:45: protected void unRegisterActivities() { On 2017/02/03 01:46:32, cco3 wrote: > `unregisterActivities` > > Can this be private? Done.
https://codereview.chromium.org/2666833002/diff/120001/chrome/android/java/An... File chrome/android/java/AndroidManifest.xml (right): https://codereview.chromium.org/2666833002/diff/120001/chrome/android/java/An... chrome/android/java/AndroidManifest.xml:651: android:exported="true" Was it intentional to re-add android:exported and the intent-filters? https://codereview.chromium.org/2666833002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/share/ShareActivity.java (right): https://codereview.chromium.org/2666833002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/share/ShareActivity.java:43: private void unRegisterActivities() { unregisterActivities
https://codereview.chromium.org/2666833002/diff/120001/chrome/android/java/An... File chrome/android/java/AndroidManifest.xml (right): https://codereview.chromium.org/2666833002/diff/120001/chrome/android/java/An... chrome/android/java/AndroidManifest.xml:651: android:exported="true" On 2017/02/03 20:08:16, mattreynolds wrote: > Was it intentional to re-add android:exported and the intent-filters? It was intentional to add the the intent filter back. But not the exported. I took out the exported. https://codereview.chromium.org/2666833002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/share/ShareActivity.java (right): https://codereview.chromium.org/2666833002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/share/ShareActivity.java:43: private void unRegisterActivities() { On 2017/02/03 20:08:16, mattreynolds wrote: > unregisterActivities Done.
https://codereview.chromium.org/2666833002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java (right): https://codereview.chromium.org/2666833002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:775: private static List<ShareActivity> getPotentialShareActivitiesList() { Can we just make this a private static final field?
https://codereview.chromium.org/2666833002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2666833002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1206: public void unregisterShareActivities() { Is this needed?
https://codereview.chromium.org/2666833002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2666833002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1206: public void unregisterShareActivities() { On 2017/02/03 21:57:44, cco3 wrote: > Is this needed? Done. https://codereview.chromium.org/2666833002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java (right): https://codereview.chromium.org/2666833002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:775: private static List<ShareActivity> getPotentialShareActivitiesList() { On 2017/02/03 21:55:49, cco3 wrote: > Can we just make this a private static final field? Done.
https://codereview.chromium.org/2666833002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebShareActivity.java (right): https://codereview.chromium.org/2666833002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebShareActivity.java:21: //TODO(iankc): implement sharing space https://codereview.chromium.org/2666833002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java (right): https://codereview.chromium.org/2666833002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:123: new ArrayList<ShareActivity>() { { no extra space here https://codereview.chromium.org/2666833002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:710: * @param options List of ShareActivity to add to the chooser. currentTab https://codereview.chromium.org/2666833002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:734: callback.run(); return after this, then decrease the indent afterward. https://codereview.chromium.org/2666833002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:737: if (wasEmpty) { do if (!wasEmpty) and then run the callback and return. https://codereview.chromium.org/2666833002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:769: * @param currentTab The current tab of the chrome activity. Add a summary line too, please. https://codereview.chromium.org/2666833002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:770: * @return list of all share activities to be enabled. List of all enabled ShareActivities. https://codereview.chromium.org/2666833002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:782: List<ShareActivity> potentialOptions, final Tab currentTab) { Why is the first parameter needed? Why does the tab need to be final? https://codereview.chromium.org/2666833002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:784: for (ShareActivity p: potentialOptions) { Use something like `activity` or `option` instead of p https://codereview.chromium.org/2666833002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:803: sStateChangeTask = new AsyncTask<Void, Void, Void>() { Could we split this off into an internal class so that you can share code with enable? https://codereview.chromium.org/2666833002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:814: } is this lined up? https://codereview.chromium.org/2666833002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:830: public static void waitForPendingStateChangeTask() { Does this need to be public?
https://codereview.chromium.org/2666833002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebShareActivity.java (right): https://codereview.chromium.org/2666833002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebShareActivity.java:21: //TODO(iankc): implement sharing On 2017/02/04 00:11:23, cco3 wrote: > space Done. https://codereview.chromium.org/2666833002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java (right): https://codereview.chromium.org/2666833002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:123: new ArrayList<ShareActivity>() { { On 2017/02/04 00:11:23, cco3 wrote: > no extra space here Presubmit won't pass unless there is a space between them. https://codereview.chromium.org/2666833002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:710: * @param options List of ShareActivity to add to the chooser. On 2017/02/04 00:11:23, cco3 wrote: > currentTab Done. https://codereview.chromium.org/2666833002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:734: callback.run(); On 2017/02/04 00:11:23, cco3 wrote: > return after this, then decrease the indent afterward. Done. https://codereview.chromium.org/2666833002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:737: if (wasEmpty) { On 2017/02/04 00:11:23, cco3 wrote: > do if (!wasEmpty) and then run the callback and return. Done. https://codereview.chromium.org/2666833002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:769: * @param currentTab The current tab of the chrome activity. On 2017/02/04 00:11:23, cco3 wrote: > Add a summary line too, please. Done. https://codereview.chromium.org/2666833002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:770: * @return list of all share activities to be enabled. On 2017/02/04 00:11:23, cco3 wrote: > List of all enabled ShareActivities. Done. https://codereview.chromium.org/2666833002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:782: List<ShareActivity> potentialOptions, final Tab currentTab) { On 2017/02/04 00:11:23, cco3 wrote: > Why is the first parameter needed? > > Why does the tab need to be final? Done. https://codereview.chromium.org/2666833002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:784: for (ShareActivity p: potentialOptions) { On 2017/02/04 00:11:23, cco3 wrote: > Use something like `activity` or `option` instead of p Done. https://codereview.chromium.org/2666833002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:803: sStateChangeTask = new AsyncTask<Void, Void, Void>() { On 2017/02/04 00:11:23, cco3 wrote: > Could we split this off into an internal class so that you can share code with > enable? Done. https://codereview.chromium.org/2666833002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:814: } On 2017/02/04 00:11:23, cco3 wrote: > is this lined up? Done. https://codereview.chromium.org/2666833002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:830: public static void waitForPendingStateChangeTask() { On 2017/02/04 00:11:23, cco3 wrote: > Does this need to be public? Done.
Matt, could you take a look? https://codereview.chromium.org/2666833002/diff/160001/chrome/android/java/An... File chrome/android/java/AndroidManifest.xml (right): https://codereview.chromium.org/2666833002/diff/160001/chrome/android/java/An... chrome/android/java/AndroidManifest.xml:628: <!-- Activity to list Physical Web Urls --> This shouldn't be in this change. https://codereview.chromium.org/2666833002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/share/ShareActivity.java (right): https://codereview.chromium.org/2666833002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/share/ShareActivity.java:36: ShareHelper.unregisterShareActivities(this); Why did you remove this? https://codereview.chromium.org/2666833002/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java (right): https://codereview.chromium.org/2666833002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:121: private static AsyncTask<Void, Void, Void> sStateChangeTask; private static StateChangeTask
Just some nits, otherwise LGTM https://codereview.chromium.org/2666833002/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/printing/PrintShareActivity.java (left): https://codereview.chromium.org/2666833002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/printing/PrintShareActivity.java:38: private static final String TAG = "cr_printing"; Remove https://codereview.chromium.org/2666833002/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java (right): https://codereview.chromium.org/2666833002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:122: public static final List<ShareActivity> sShareActivities = Collections.unmodifiableList( Constants should use CONSTANT_CASE (rename to SHARE_ACTIVITIES and move this up with the other private static final members)
https://codereview.chromium.org/2666833002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/share/ShareActivity.java (right): https://codereview.chromium.org/2666833002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/share/ShareActivity.java:36: ShareHelper.unregisterShareActivities(this); On 2017/02/07 00:00:54, cco3 wrote: > Why did you remove this? I noticed that unregister was being called twice. It turns out that when we enable the options we also register an ActivityStateListener that will unregister all activities when the activity state become paused. Meaning when the chromeactivity goes in the background. This call is redundant. https://codereview.chromium.org/2666833002/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/printing/PrintShareActivity.java (left): https://codereview.chromium.org/2666833002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/printing/PrintShareActivity.java:38: private static final String TAG = "cr_printing"; On 2017/02/07 00:34:52, mattreynolds wrote: > Remove Done. https://codereview.chromium.org/2666833002/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java (right): https://codereview.chromium.org/2666833002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:121: private static AsyncTask<Void, Void, Void> sStateChangeTask; On 2017/02/07 00:00:54, cco3 wrote: > private static StateChangeTask It's being picky with this change. When I make this change it throws this error: error: incompatible types: AsyncTask<Void,Void,Void> cannot be converted to StateChangeTask }.executeOnExecutor(AsyncTask.THREAD_POOL_EXECUTOR); I've also tried this: }.executeOnExecutor(StateChangeTask.THREAD_POOL_EXECUTOR); and it produces the same error. I don't see the value in making sure it's declared as a StateChangeTask rather than an AsyncTask. https://codereview.chromium.org/2666833002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:122: public static final List<ShareActivity> sShareActivities = Collections.unmodifiableList( On 2017/02/07 00:34:52, mattreynolds wrote: > Constants should use CONSTANT_CASE (rename to SHARE_ACTIVITIES and move this up > with the other private static final members) Done.
https://codereview.chromium.org/2666833002/diff/160001/chrome/android/java/An... File chrome/android/java/AndroidManifest.xml (right): https://codereview.chromium.org/2666833002/diff/160001/chrome/android/java/An... chrome/android/java/AndroidManifest.xml:628: <!-- Activity to list Physical Web Urls --> On 2017/02/07 00:00:54, cco3 wrote: > This shouldn't be in this change. Done.
https://codereview.chromium.org/2666833002/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java (right): https://codereview.chromium.org/2666833002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:127: private static Set<Activity> sPendingShareActivities = Should these be ShareActivities? https://codereview.chromium.org/2666833002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:759: public static void unregisterShareActivities(final Activity activity) { this parameter needs a more helpful name...is it the callingActivity? Is it actually a ShareActivity that could be called completedActivity? |
