Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2917703004:
[Android] Wrap all share parameters into the ShareParams class (Closed)
|
Created:
3 years, 6 months ago by ltian Modified:
3 years, 6 months ago CC:
chromium-reviews, dewittj+watch_chromium.org, fgorski+watch_chromium.org, romax+watch_chromium.org, petewil+watch_chromium.org, chili+watch_chromium.org, agrieve+watch_chromium.org, dimich+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[Android] Wrap all share parameters into the ShareParams class
Functions in ShareHelper take too many parameters for sharing. Wrap them
into a new ShareParams class to redunce the number of parameters passed
to the functions.
BUG=None
Review-Url: https://codereview.chromium.org/2917703004
Cr-Commit-Position: refs/heads/master@{#478878}
Committed: https://chromium.googlesource.com/chromium/src/+/0776ca0be68ad5a262d9b1b386eadcdb05706379
Patch Set 1 #
Total comments: 33
Patch Set 2 : Update based on Ted and Yusuf's comments. #
Total comments: 10
Patch Set 3 : Update based on Yusuf's comments. #
Total comments: 2
Patch Set 4 : Update based on Ted's nit. #Patch Set 5 : Rebase and fix test failure. #Patch Set 6 : Fix WebShareTest failure. #Patch Set 7 : Improve code. #
Total comments: 2
Patch Set 8 : Update based on Matt's comments. #Dependent Patchsets: Messages
Total messages: 37 (17 generated)
ltian@chromium.org changed reviewers: + tedchoc@chromium.org
Can you take a look of the changes in this CL? Thanks!
ltian@chromium.org changed reviewers: + yusufo@chromium.org
woot! this will be a great cleanup https://codereview.chromium.org/2917703004/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java (right): https://codereview.chromium.org/2917703004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:450: File shareableDir = getDirectoryForOfflineSharing(builder.getActivity()); this method should just be able to use the application context. Let's remove the param entirely and have it is ContextUtils inside that method. https://codereview.chromium.org/2917703004/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java (right): https://codereview.chromium.org/2917703004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:432: ShareParams simpleShareParams = 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. https://codereview.chromium.org/2917703004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:494: private static void shareWithLastUsed(ShareParams params) { I think this is only used in share, let's just remove this function and inline it above. https://codereview.chromium.org/2917703004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:500: private static void shareIntent(ShareParams params, Intent sharingIntent) { Is this only used from within makeIntentAndShare? If so, let's inline that as well. https://codereview.chromium.org/2917703004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:512: private static void makeIntentAndShare(ShareParams params, ComponentName component) { let's mark ComponentName with @Nullable https://codereview.chromium.org/2917703004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:619: public static Intent getShareIntent(ShareParams params) { 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). https://codereview.chromium.org/2917703004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:639: if (screenshotUri != null) { tangential cleanup, but it seems like we should be able to combine this if with the following one https://codereview.chromium.org/2917703004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:672: private static Intent getDirectShareIntentForComponent( 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. https://codereview.chromium.org/2917703004/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/share/ShareParams.java (right): https://codereview.chromium.org/2917703004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/share/ShareParams.java:52: String title, String text, String url, @Nullable Uri offlineUri, Uri screenshotUri, screenshotUri can be nullable as well. Let's mark all the accessors as Nullable too https://codereview.chromium.org/2917703004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/share/ShareParams.java:207: public Activity getActivity() { We shouldn't have getters on the builder. How many places need this? https://codereview.chromium.org/2917703004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/share/ShareParams.java:214: mSaveLastUsed = false; I don't think we should overwrite values. If this isn't a valid combination, we should assert !mSaveLastUsed here instead.
https://codereview.chromium.org/2917703004/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java (right): https://codereview.chromium.org/2917703004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:431: final TargetChosenCallback callback = params.getCallback(); 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. https://codereview.chromium.org/2917703004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:508: sharingIntent, params.getCallback()); why not also pass params here? https://codereview.chromium.org/2917703004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:512: private static void makeIntentAndShare(ShareParams params, ComponentName component) { why is componentName not a part of the shareParam? https://codereview.chromium.org/2917703004/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/share/ShareParams.java (right): https://codereview.chromium.org/2917703004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/share/ShareParams.java:160: mShareDirectly = shareDirectly; Can we not deduce these two from each other? At least we should do: assert shareDirectly ? savelastused != true : true; https://codereview.chromium.org/2917703004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/share/ShareParams.java:213: if (mShareDirectly) { one line.
https://codereview.chromium.org/2917703004/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java (right): https://codereview.chromium.org/2917703004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:450: File shareableDir = getDirectoryForOfflineSharing(builder.getActivity()); On 2017/06/01 20:39:02, Ted C wrote: > this method should just be able to use the application context. Let's remove > the param entirely and have it is ContextUtils inside that method. Done. https://codereview.chromium.org/2917703004/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java (right): https://codereview.chromium.org/2917703004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:431: final TargetChosenCallback callback = params.getCallback(); On 2017/06/01 20:41:45, Yusuf wrote: > 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. No, I think the callback problem has been solved so this should not be the problem. https://codereview.chromium.org/2917703004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:432: ShareParams simpleShareParams = On 2017/06/01 20:39:03, Ted C wrote: > 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. 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. https://codereview.chromium.org/2917703004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:494: private static void shareWithLastUsed(ShareParams params) { On 2017/06/01 20:39:03, Ted C wrote: > I think this is only used in share, let's just remove this function and inline > it above. Done. https://codereview.chromium.org/2917703004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:500: private static void shareIntent(ShareParams params, Intent sharingIntent) { On 2017/06/01 20:39:03, Ted C wrote: > Is this only used from within makeIntentAndShare? If so, let's inline that as > well. Done. https://codereview.chromium.org/2917703004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:508: sharingIntent, params.getCallback()); On 2017/06/01 20:41:45, Yusuf wrote: > why not also pass params here? 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. https://codereview.chromium.org/2917703004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:512: private static void makeIntentAndShare(ShareParams params, ComponentName component) { On 2017/06/01 20:41:45, Yusuf wrote: > why is componentName not a part of the shareParam? 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. https://codereview.chromium.org/2917703004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:512: private static void makeIntentAndShare(ShareParams params, ComponentName component) { On 2017/06/01 20:39:03, Ted C wrote: > let's mark ComponentName with @Nullable Done. https://codereview.chromium.org/2917703004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:619: public static Intent getShareIntent(ShareParams params) { On 2017/06/01 20:39:03, Ted C wrote: > 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). 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. https://codereview.chromium.org/2917703004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:639: if (screenshotUri != null) { On 2017/06/01 20:39:03, Ted C wrote: > tangential cleanup, but it seems like we should be able to combine this if with > the following one Done. https://codereview.chromium.org/2917703004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:672: private static Intent getDirectShareIntentForComponent( On 2017/06/01 20:39:03, Ted C wrote: > 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. Done. https://codereview.chromium.org/2917703004/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/share/ShareParams.java (right): https://codereview.chromium.org/2917703004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/share/ShareParams.java:52: String title, String text, String url, @Nullable Uri offlineUri, Uri screenshotUri, On 2017/06/01 20:39:03, Ted C wrote: > screenshotUri can be nullable as well. > > Let's mark all the accessors as Nullable too Done. https://codereview.chromium.org/2917703004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/share/ShareParams.java:160: mShareDirectly = shareDirectly; On 2017/06/01 20:41:45, Yusuf wrote: > Can we not deduce these two from each other? > > At least we should do: > > assert shareDirectly ? savelastused != true : true; The reason I add the deduction is because in ShareHelper if mShareDirectly is true, it forces the mSaveLastUsed to false. Assert is definitely better to deal with this. https://codereview.chromium.org/2917703004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/share/ShareParams.java:207: public Activity getActivity() { On 2017/06/01 20:39:03, Ted C wrote: > We shouldn't have getters on the builder. How many places need this? It is only used in one place in OfflinePageUtils and seems that one can use Application context. So this is not needed. https://codereview.chromium.org/2917703004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/share/ShareParams.java:213: if (mShareDirectly) { On 2017/06/01 20:41:46, Yusuf wrote: > one line. Done. https://codereview.chromium.org/2917703004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/share/ShareParams.java:214: mSaveLastUsed = false; On 2017/06/01 20:39:03, Ted C wrote: > I don't think we should overwrite values. If this isn't a valid combination, we > should assert !mSaveLastUsed here instead. Done.
https://codereview.chromium.org/2917703004/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java (right): https://codereview.chromium.org/2917703004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:512: private static void makeIntentAndShare(ShareParams params, ComponentName component) { On 2017/06/01 23:27:11, ltian wrote: > On 2017/06/01 20:41:45, Yusuf wrote: > > why is componentName not a part of the shareParam? > > 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. Acknowledged. https://codereview.chromium.org/2917703004/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2917703004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1296: boolean saveLastUsed = shareDirectly ? false : true; ! https://codereview.chromium.org/2917703004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1300: .setSaveLastUsed(saveLastUsed) how about !shareDirectly here? But the question still stands: Do we need these both? Are they always negative of each other? If yes, then we should have a more enveloping name and keep just one imo. https://codereview.chromium.org/2917703004/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java (right): https://codereview.chromium.org/2917703004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:655: * Convenient method to create an Intent to retrieve all the apps support sharing text. Convenience https://codereview.chromium.org/2917703004/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/share/ShareParams.java (right): https://codereview.chromium.org/2917703004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/share/ShareParams.java:52: String title, String text, String url, @Nullable Uri offlineUri, These Nullables would actually help in the builder as well in the individual setters, so that we avoid setting them to begin with. And if we will put Nullable, lets put NonNulls as well to the builder setters. https://codereview.chromium.org/2917703004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/share/ShareParams.java:210: if (mShareDirectly) { one line!
https://codereview.chromium.org/2917703004/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2917703004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1296: boolean saveLastUsed = shareDirectly ? false : true; On 2017/06/02 20:07:46, Yusuf wrote: > ! Done. https://codereview.chromium.org/2917703004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1300: .setSaveLastUsed(saveLastUsed) On 2017/06/02 20:07:46, Yusuf wrote: > how about !shareDirectly here? > > But the question still stands: Do we need these both? Are they always negative > of each other? If yes, then we should have a more enveloping name and keep just > one imo. For offline discussion, we will merge them into one parameter. https://codereview.chromium.org/2917703004/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java (right): https://codereview.chromium.org/2917703004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:655: * Convenient method to create an Intent to retrieve all the apps support sharing text. On 2017/06/02 20:07:46, Yusuf wrote: > Convenience Done. https://codereview.chromium.org/2917703004/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/share/ShareParams.java (right): https://codereview.chromium.org/2917703004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/share/ShareParams.java:52: String title, String text, String url, @Nullable Uri offlineUri, On 2017/06/02 20:07:47, Yusuf wrote: > These Nullables would actually help in the builder as well in the individual > setters, so that we avoid setting them to begin with. And if we will put > Nullable, lets put NonNulls as well to the builder setters. Done. https://codereview.chromium.org/2917703004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/share/ShareParams.java:210: if (mShareDirectly) { On 2017/06/02 20:07:46, Yusuf wrote: > one line! Done.
lgtm
lgtm https://codereview.chromium.org/2917703004/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/share/ShareParams.java (right): https://codereview.chromium.org/2917703004/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/share/ShareParams.java:4: package org.chromium.chrome.browser.share; add a space above this
https://codereview.chromium.org/2917703004/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/share/ShareParams.java (right): https://codereview.chromium.org/2917703004/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/share/ShareParams.java:4: package org.chromium.chrome.browser.share; On 2017/06/07 18:03:06, Ted C wrote: > add a space above this Done.
The CQ bit was checked by ltian@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org, yusufo@chromium.org Link to the patchset: https://codereview.chromium.org/2917703004/#ps60001 (title: "Update based on Ted's nit.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by ltian@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org, yusufo@chromium.org Link to the patchset: https://codereview.chromium.org/2917703004/#ps80001 (title: "Rebase and fix test failure.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
ltian@chromium.org changed reviewers: + mgiuca@chromium.org
mgiuca@chromium.org: can you take a look of this CL that the code refactor makes sense? Thanks!
The CQ bit was checked by ltian@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2917703004/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/share/ShareParams.java (right): https://codereview.chromium.org/2917703004/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/share/ShareParams.java:72: public boolean isShareDirectly() { This name (and the following, isSaveLastUsed) is awkward grammatically. Can they just be called "shareDirectly" and "saveLastUsed"? (Or are you trying to satisfy some style guide rule about getter names?)
The CQ bit was checked by ltian@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org, yusufo@chromium.org, mgiuca@chromium.org Link to the patchset: https://codereview.chromium.org/2917703004/#ps140001 (title: "Update based on Matt's comments.")
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": 140001, "attempt_start_ts": 1497317002201340,
"parent_rev": "35e4fd70b755009eb78195ed9dfe0e8a3a21ba0f", "commit_rev":
"0776ca0be68ad5a262d9b1b386eadcdb05706379"}
Message was sent while issue was closed.
Description was changed from ========== [Android] Wrap all share parameters into the ShareParams class Functions in ShareHelper take too many parameters for sharing. Wrap them into a new ShareParams class to redunce the number of parameters passed to the functions. BUG=None ========== to ========== [Android] Wrap all share parameters into the ShareParams class Functions in ShareHelper take too many parameters for sharing. Wrap them into a new ShareParams class to redunce the number of parameters passed to the functions. BUG=None Review-Url: https://codereview.chromium.org/2917703004 Cr-Commit-Position: refs/heads/master@{#478878} Committed: https://chromium.googlesource.com/chromium/src/+/0776ca0be68ad5a262d9b1b386ea... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/0776ca0be68ad5a262d9b1b386ea...
Message was sent while issue was closed.
https://codereview.chromium.org/2917703004/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/share/ShareParams.java (right): https://codereview.chromium.org/2917703004/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/share/ShareParams.java:72: public boolean isShareDirectly() { On 2017/06/13 00:53:31, Matt Giuca wrote: > This name (and the following, isSaveLastUsed) is awkward grammatically. Can they > just be called "shareDirectly" and "saveLastUsed"? (Or are you trying to satisfy > some style guide rule about getter names?) Done.
Message was sent while issue was closed.
On 2017/06/13 17:40:53, ltian wrote: > https://codereview.chromium.org/2917703004/diff/120001/chrome/android/java/sr... > File chrome/android/java/src/org/chromium/chrome/browser/share/ShareParams.java > (right): > > https://codereview.chromium.org/2917703004/diff/120001/chrome/android/java/sr... > chrome/android/java/src/org/chromium/chrome/browser/share/ShareParams.java:72: > public boolean isShareDirectly() { > On 2017/06/13 00:53:31, Matt Giuca wrote: > > This name (and the following, isSaveLastUsed) is awkward grammatically. Can > they > > just be called "shareDirectly" and "saveLastUsed"? (Or are you trying to > satisfy > > some style guide rule about getter names?) > > Done. Thanks! |
