[Android] Wrap all share parameters into the ShareParams class

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

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 package org.chromium.chrome.browser.share;
 
 import android.annotation.TargetApi;
 import android.app.Activity;
 import android.app.PendingIntent;
 import android.content.BroadcastReceiver;
@@ skipping 267 matching lines @@
 
     /**
      * Creates and shows a share intent picker dialog or starts a share intent directly with the
      * activity that was most recently used to share based on shareDirectly value.
      *
      * This function will save |screenshot| under {app's root}/files/images/screenshot (or
      * /sdcard/DCIM/browser-images/screenshot if ADK is lower than JB MR2).
      * 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);
         }
     }
 
     /**
      * Trigger the share action for the given image data.
      * @param activity The activity used to trigger the share action.
      * @param jpegImageData The image data to be shared in jpeg format.
      * @param name When this is not null, it will share the image directly with the
      *             {@link ComponentName}
      */
@@ skipping 110 matching lines @@
                     fileUri = ApiCompatibilityUtils.getUriForImageCaptureFile(savedFile);
                 }
                 callback.onResult(fileUri);
             }
         }.executeOnExecutor(AsyncTask.THREAD_POOL_EXECUTOR);
     }
 
     /**
      * 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;
         if (resolveInfoList.size() == 0) return;
         Collections.sort(resolveInfoList, new ResolveInfo.DisplayNameComparator(manager));
 
         final ShareDialogAdapter adapter =
                 new ShareDialogAdapter(activity, manager, resolveInfoList);
         AlertDialog.Builder builder = new AlertDialog.Builder(activity, R.style.AlertDialogTheme);
         builder.setTitle(activity.getString(R.string.share_link_chooser_title));
         builder.setAdapter(adapter, null);
 
         // Need a mutable object to record whether the callback has been fired.
         final boolean[] callbackCalled = new boolean[1];
 
         final AlertDialog dialog = builder.create();
         dialog.show();
         dialog.getListView().setOnItemClickListener(new OnItemClickListener() {
             @Override
             public void onItemClick(AdapterView<?> parent, View view, int position, long id) {
                 ResolveInfo info = adapter.getItem(position);
                 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();
             }
         });
 
         if (callback != null) {
             dialog.setOnDismissListener(new OnDismissListener() {
                 @Override
                 public void onDismiss(DialogInterface dialog) {
                     if (!callbackCalled[0]) {
                         callback.onCancel();
                         callbackCalled[0] = true;
                     }
                 }
             });
         }
 
         if (sFakeIntentReceiverForTesting != null) {
             sFakeIntentReceiverForTesting.onCustomChooserShown(dialog);
         }
     }
 
     /**
      * 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);
     }
 
     /**
      * Set the icon and the title for the menu item used for direct share.
      *
      * @param activity Activity that is used to access the package manager.
      * @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;
 
         item.setIcon(directShareIcon);
         if (directShareTitle != null) {
             item.setTitle(
                     activity.getString(R.string.accessibility_menu_share_via, directShareTitle));
         }
     }
@@ skipping 73 matching lines @@
     @VisibleForTesting
     public static void setLastShareComponentName(ComponentName component) {
         SharedPreferences preferences = ContextUtils.getAppSharedPreferences();
         SharedPreferences.Editor editor = preferences.edit();
         editor.putString(PACKAGE_NAME_KEY, component.getPackageName());
         editor.putString(CLASS_NAME_KEY, component.getClassName());
         editor.apply();
     }
 
     @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)) {
                 // Concatenate text and URL with a space.
                 text = text + " " + url;
             } else {
                 text = url;
             }
         }
 
         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);
         }
         if (screenshotUri != null) {
             // To give read access to an Intent target, we need to put |screenshotUri| in clipData
             // because adding Intent.FLAG_GRANT_READ_URI_PERMISSION doesn't work for
             // EXTRA_SHARE_SCREENSHOT_AS_STREAM.
             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;
     }
 
     /**
      * Creates an Intent to share an image.
      * @param imageUri The Uri of the image.
      * @return The Intent used to share the image.
      */
     public static Intent getShareImageIntent(Uri imageUri) {
         Intent intent = new Intent(Intent.ACTION_SEND);
         intent.addFlags(ApiCompatibilityUtils.getActivityNewDocumentFlag());
         intent.setType("image/jpeg");
         intent.addFlags(Intent.FLAG_GRANT_READ_URI_PERMISSION);
         intent.putExtra(Intent.EXTRA_STREAM, imageUri);
         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);
         return intent;
     }
 
     /**
      * Gets the {@link ComponentName} of the app that was used to last share.
      */
     @Nullable
     public static ComponentName getLastShareComponentName() {
         SharedPreferences preferences = ContextUtils.getAppSharedPreferences();
         String packageName = preferences.getString(PACKAGE_NAME_KEY, null);
         String className = preferences.getString(CLASS_NAME_KEY, null);
         if (packageName == null || className == null) return null;
         return new ComponentName(packageName, className);
     }
 }