[Android] Wrap all share parameters into the ShareParams class

chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java

Index: chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java b/chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java
index 7566d2e5ba354c43d3b9b0d87dcb67daa09731fc..e892f4dedfbc7b287bce9e29f08b9d474768af2d 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java
@@ -285,31 +285,15 @@ public class ShareHelper {
      * Cleaning up doesn't happen automatically, and so an app should call clearSharedScreenshots()
      * explicitly when needed.
      *
-     * @param shareDirectly Whether it should share directly with the activity that was most
-     *                      recently used to share.
-     * @param saveLastUsed Whether to save the chosen activity for future direct sharing.
-     * @param activity Activity that is used to access package manager.
-     * @param title Title of the page to be shared.
-     * @param text Text to be shared. If both |text| and |url| are supplied, they are concatenated
-     *             with a space.
-     * @param url URL of the page to be shared.
-     * @param offlineUri URI to the offline MHTML file to be shared.
-     * @param screenshotUri Uri of the screenshot of the page to be shared.
-     * @param callback Optional callback to be called when user makes a choice. Will not be called
-     *                 if receiving a response when the user makes a choice is not supported (on
-     *                 older Android versions).
+     * @param params The container holding the share parameters.
      */
-    public static void share(boolean shareDirectly, boolean saveLastUsed, Activity activity,
-            String title, String text, String url, @Nullable Uri offlineUri, Uri screenshotUri,
-            @Nullable TargetChosenCallback callback) {
-        if (shareDirectly) {
-            shareWithLastUsed(activity, title, text, url, offlineUri, screenshotUri);
+    public static void share(ShareParams params) {
+        if (params.isShareDirectly()) {
+            shareWithLastUsed(params);
         } else if (TargetChosenReceiver.isSupported()) {
-            makeIntentAndShare(saveLastUsed, activity, title, text, url, offlineUri, screenshotUri,
-                    null, callback);
+            makeIntentAndShare(params, null);
         } else {
-            showShareDialog(
-                    saveLastUsed, activity, title, text, url, offlineUri, screenshotUri, callback);
+            showShareDialog(params);
         }
     }
 
@@ -440,22 +424,16 @@ public class ShareHelper {
     /**
      * Creates and shows a share intent picker dialog.
      *
-     * @param saveLastUsed Whether to save the chosen activity for future direct sharing.
-     * @param activity Activity that is used to access package manager.
-     * @param title Title of the page to be shared.
-     * @param text Text to be shared. If both |text| and |url| are supplied, they are concatenated
-     *             with a space.
-     * @param url URL of the page to be shared.
-     * @oaram offlineUri URI of the offline page to be shared.
-     * @param screenshotUri Uri of the screenshot of the page to be shared.
-     * @param callback Optional callback to be called when user makes a choice. Will not be called
-     *                 if receiving a response when the user makes a choice is not supported (on
-     *                 older Android versions).
+     * @param params The container holding the share parameters.
      */
-    private static void showShareDialog(final boolean saveLastUsed, final Activity activity,
-            final String title, final String text, final String url, final Uri offlineUri,
-            final Uri screenshotUri, @Nullable final TargetChosenCallback callback) {
-        Intent intent = getShareIntent(activity, title, text, url, null, null);
+    private static void showShareDialog(final ShareParams params) {
+        Activity activity = params.getActivity();
+        final TargetChosenCallback callback = params.getCallback();

[Yusuf, 2017/06/01 20:41:45]

is it the callback that is creating the problem here? I would really git blame
this and try to understand if we can just pass the same param.

[ltian, 2017/06/01 23:27:11]

No, I think the callback problem has been solved so this should not be the
problem.

+        ShareParams simpleShareParams =

[Ted C, 2017/06/01 20:39:03]

does this need to be any different from the default options created in
configureDirectShareMenuItem?

Maybe that should be a static helper function to create a simple link intent for
determining the share capable apps on the system? 
createLinkShareAppCompatibilityIntent or something less horrendous.

[ltian, 2017/06/01 23:27:07]

Yes, after digging on these I find both of them only use the intent to get list
of apps supporting text share. So I think all Activity, title and text are not
really needed? So a static helper function without any parameter seems ok for
both.

+                new ShareParams.Builder(params.getActivity(), params.getTitle(), params.getUrl())
+                        .setText(params.getText())
+                        .build();
+        Intent intent = getShareIntent(simpleShareParams);
         PackageManager manager = activity.getPackageManager();
         List<ResolveInfo> resolveInfoList = manager.queryIntentActivities(intent, 0);
         assert resolveInfoList.size() > 0;
@@ -480,13 +458,13 @@ public class ShareHelper {
                 ActivityInfo ai = info.activityInfo;
                 ComponentName component =
                         new ComponentName(ai.applicationInfo.packageName, ai.name);
+
                 if (callback != null && !callbackCalled[0]) {
                     callback.onTargetChosen(component);
                     callbackCalled[0] = true;
                 }
-                if (saveLastUsed) setLastShareComponentName(component);
-                makeIntentAndShare(false, activity, title, text, url, offlineUri, screenshotUri,
-                        component, null);
+                if (params.isSaveLastUsed()) setLastShareComponentName(component);
+                makeIntentAndShare(params, component);
                 dialog.dismiss();
             }
         });
@@ -511,41 +489,29 @@ public class ShareHelper {
     /**
      * Starts a share intent with the activity that was most recently used to share.
      * If there is no most recently used activity, it does nothing.
-     * @param activity Activity that is used to start the share intent.
-     * @param title Title of the page to be shared.
-     * @param text Text to be shared. If both |text| and |url| are supplied, they are concatenated
-     *             with a space.
-     * @param url URL of the page to be shared.
-     * @oaram offlineUri URI of the offline page to be shared.
-     * @param screenshotUri Uri of the screenshot of the page to be shared.
+     * @param params The container holding the share parameters.
      */
-    private static void shareWithLastUsed(Activity activity, String title, String text, String url,
-            Uri offlineUri, Uri screenshotUri) {
+    private static void shareWithLastUsed(ShareParams params) {

[Ted C, 2017/06/01 20:39:03]

I think this is only used in share, let's just remove this function and inline
it above.

[ltian, 2017/06/01 23:27:09]

Done.

         ComponentName component = getLastShareComponentName();
         if (component == null) return;
-        makeIntentAndShare(
-                false, activity, title, text, url, offlineUri, screenshotUri, component, null);
+        makeIntentAndShare(params, component);
     }
 
-    private static void shareIntent(boolean saveLastUsed, Activity activity, Intent sharingIntent,
-                                    @Nullable TargetChosenCallback callback) {
+    private static void shareIntent(ShareParams params, Intent sharingIntent) {

[Ted C, 2017/06/01 20:39:03]

Is this only used from within makeIntentAndShare?  If so, let's inline that as
well.

[ltian, 2017/06/01 23:27:08]

Done.

         if (sharingIntent.getComponent() != null) {
             // If a component was specified, there should not also be a callback.
-            assert callback == null;
-            fireIntent(activity, sharingIntent);
+            assert params.getCallback() == null;
+            fireIntent(params.getActivity(), sharingIntent);
         } else {
             assert TargetChosenReceiver.isSupported();
-            TargetChosenReceiver.sendChooserIntent(saveLastUsed, activity, sharingIntent, callback);
+            TargetChosenReceiver.sendChooserIntent(params.isSaveLastUsed(), params.getActivity(),
+                    sharingIntent, params.getCallback());

[Yusuf, 2017/06/01 20:41:45]

why not also pass params here?

[ltian, 2017/06/01 23:27:07]

The only reason is because in ShareImage the onPostExecute calls
TargetChosenReceiver.sendChooserIntent(true, activity, shareIntent, null) which
I think it is not worth to create a ShareParam in it because it doesn't even
need text and url.

         }
     }
 
-    private static void makeIntentAndShare(final boolean saveLastUsed, final Activity activity,
-            final String title, final String text, final String url, final Uri offlineUri,
-            final Uri screenshotUri, final ComponentName component,
-            @Nullable final TargetChosenCallback callback) {
-        Intent intent = getDirectShareIntentForComponent(
-                activity, title, text, url, offlineUri, screenshotUri, component);
-        shareIntent(saveLastUsed, activity, intent, callback);
+    private static void makeIntentAndShare(ShareParams params, ComponentName component) {

[Ted C, 2017/06/01 20:39:03]

let's mark ComponentName with @Nullable

[Yusuf, 2017/06/01 20:41:45]

why is componentName not a part of the shareParam?

[ltian, 2017/06/01 23:27:07]

Done.

[ltian, 2017/06/01 23:27:11]

One reason is in showShareDialog's onItemClick, it gets the componentName based
on user selection where the ShareParam has been constructed. Another reason is
the ComponentName is the component of direct share app which I feel is not
really a parameter for share.

[Yusuf, 2017/06/02 20:07:46]

Acknowledged.

+        Intent intent = getDirectShareIntentForComponent(params, component);
+        shareIntent(params, intent);
     }
 
     /**
@@ -555,7 +521,8 @@ public class ShareHelper {
      * @param item The menu item that is used for direct share
      */
     public static void configureDirectShareMenuItem(Activity activity, MenuItem item) {
-        Intent shareIntent = getShareIntent(activity, "", "", "", null, null);
+        ShareParams params = new ShareParams.Builder(activity, "", "").setText("").build();
+        Intent shareIntent = getShareIntent(params);
         Pair<Drawable, CharSequence> directShare = getShareableIconAndName(activity, shareIntent);
         Drawable directShareIcon = directShare.first;
         CharSequence directShareTitle = directShare.second;
@@ -649,8 +616,9 @@ public class ShareHelper {
     }
 
     @VisibleForTesting
-    public static Intent getShareIntent(Activity activity, String title, String text, String url,
-            Uri offlineUri, Uri screenshotUri) {
+    public static Intent getShareIntent(ShareParams params) {

[Ted C, 2017/06/01 20:39:03]

let's rename this getShareLinkIntent to differentiate from the
getShareImageIntent below.

A followup change should look at whether we can make image and link paths both
use ShareParams.

I could image ShareParams having a method like generateIntent that would do the
right thing and make this class less aware of all of that.  Ideally, this class
would only be aware of the interactions with the Android share components and
have less direct awareness of chrome internal quirks (I think the rename is
reasonable within this change, but not anything about combining the paths).

[ltian, 2017/06/01 23:27:10]

The problem I meet to combine these two is the Intents for link and image
requires totally different fields (images seems only need Uri and byte[] for
image data), combining them in ShareParams could makes every field nullable.

+        String url = params.getUrl();
+        String text = params.getText();
         if (!TextUtils.isEmpty(url)) {
             url = DomDistillerUrlUtils.getOriginalUrlFromDistillerUrl(url);
             if (!TextUtils.isEmpty(text)) {
@@ -663,10 +631,11 @@ public class ShareHelper {
 
         Intent intent = new Intent(Intent.ACTION_SEND);
         intent.addFlags(ApiCompatibilityUtils.getActivityNewDocumentFlag());
-        intent.putExtra(Intent.EXTRA_SUBJECT, title);
+        intent.putExtra(Intent.EXTRA_SUBJECT, params.getTitle());
         intent.putExtra(Intent.EXTRA_TEXT, text);
-        intent.putExtra(EXTRA_TASK_ID, activity.getTaskId());
+        intent.putExtra(EXTRA_TASK_ID, params.getActivity().getTaskId());
 
+        Uri screenshotUri = params.getScreenshotUri();
         if (screenshotUri != null) {

[Ted C, 2017/06/01 20:39:03]

tangential cleanup, but it seems like we should be able to combine this if with
the following one

[ltian, 2017/06/01 23:27:07]

Done.

             intent.addFlags(Intent.FLAG_GRANT_READ_URI_PERMISSION);
         }
@@ -677,11 +646,11 @@ public class ShareHelper {
             intent.setClipData(ClipData.newRawUri("", screenshotUri));
             intent.putExtra(EXTRA_SHARE_SCREENSHOT_AS_STREAM, screenshotUri);
         }
-        if (offlineUri == null) {
+        if (params.getOfflineUri() == null) {
             intent.setType("text/plain");
         } else {
             intent.setType("multipart/related");
-            intent.putExtra(Intent.EXTRA_STREAM, offlineUri);
+            intent.putExtra(Intent.EXTRA_STREAM, params.getOfflineUri());
         }
         return intent;
     }
@@ -700,9 +669,9 @@ public class ShareHelper {
         return intent;
     }
 
-    private static Intent getDirectShareIntentForComponent(Activity activity, String title,
-            String text, String url, Uri offlineUri, Uri screenshotUri, ComponentName component) {
-        Intent intent = getShareIntent(activity, title, text, url, offlineUri, screenshotUri);
+    private static Intent getDirectShareIntentForComponent(

[Ted C, 2017/06/01 20:39:03]

tangential cleanup, this method is used for all share types and not just direct
share, and it seems to only be used in one place, makeIntentAndShare, so let's
just remove this function and inline it there.

[ltian, 2017/06/01 23:27:13]

Done.

+            ShareParams params, ComponentName component) {
+        Intent intent = getShareIntent(params);
         intent.addFlags(Intent.FLAG_ACTIVITY_FORWARD_RESULT
                 | Intent.FLAG_ACTIVITY_PREVIOUS_IS_TOP);
         intent.setComponent(component);