Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(318)

Issue 2680983002: Migrate share activity registration code to ShareHelper (Closed)

Created:
3 years, 10 months ago by iankc
Modified:
3 years, 10 months ago
Reviewers:
Ted C, cco3, mattreynolds
CC:
chromium-reviews, agrieve+watch_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Migrate share activity registration code to ShareHelper This change migrates some static methods from PrintShareActivity to ShareHelper. This is the code the PrintShareActivity relies on to insert itself into the share sheet. Offloading this responsibility to the ShareHelper will allow us to create a generalized ShareActivity so that we can add multiple activities to the share sheet. BUG=685856

Patch Set 1 #

Patch Set 2 : Updating Commit Message #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+120 lines, -123 lines) Patch
M chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java View 2 chunks +1 line, -2 lines 0 comments Download
M chrome/android/java/src/org/chromium/chrome/browser/printing/PrintShareActivity.java View 2 chunks +1 line, -121 lines 0 comments Download
M chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java View 5 chunks +118 lines, -0 lines 2 comments Download

Messages

Total messages: 8 (3 generated)
iankc
3 years, 10 months ago (2017-02-07 23:38:59 UTC) #2
cco3
I'd reword the commit message, but other than that, lgtm Suggestion: """ Migrate share activity ...
3 years, 10 months ago (2017-02-07 23:50:39 UTC) #3
iankc
Hello, We're breaking up a large CL here: https://codereview.chromium.org/2666833002/ to make it more easy to ...
3 years, 10 months ago (2017-02-08 00:02:39 UTC) #6
cco3
Hi Ted, this should be ready to review. Thanks!
3 years, 10 months ago (2017-02-08 00:20:33 UTC) #7
Ted C
3 years, 10 months ago (2017-02-11 00:22:27 UTC) #8
Please create a new class and not put this in ShareHelper.  That class is
already too big and I'd rather not make it any bigger.

Something like:
OptionalShareTargetsManager <or something>

https://codereview.chromium.org/2680983002/diff/20001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java
(right):

https://codereview.chromium.org/2680983002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:708:
public static void enablePrintShareOption(final Activity activity, final
Runnable callback) {
Don't make this specific to print.  In general, this class shouldn't know
anything about print.

Call it something like:
enableOptionalShareActivities(...)

Then we'll need to pass something like List<Class<? extends Activity>>
shareActivities as a param since we shouldn't hardcode PrintShareActivity below.

And if we have to pass two activity-like params here, let's change the current
activity param to do something like triggeringActivity

https://codereview.chromium.org/2680983002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:756:
public static void unregisterActivity(final Activity activity) {
This name doesn't really make sense IMO.  What activity are you unregistering? 
When it was private (and in a much smaller class), I think it was easier to
grok.

Maybe something like:
disableOptionalShareActivities(final Activity triggeringActivity);

Then you'll want javadoc describing the input and that it is the activity that
initially triggered the share (to help differentiate from the share activities
being enabled).

And since you shouldn't know about print, you'll need to keep a List/Set of the
enabled components to disable here.

Powered by Google App Engine
This is Rietveld 408576698