From 9f9cf316bd3c7740c3fa1fa5ccbfa0d5dd3c417b Mon Sep 17 00:00:00 2001 From: olly Date: Wed, 1 May 2019 20:50:44 +0100 Subject: [PATCH] Remove unnecessary logging As justification for why we should not have this type of logging, it would scale up to about 13K LOC, 1800 Strings, and 36K (after pro-guarding - in the case of the demo app) if we did it through the whole code base*. It makes the code messier to read, and in most cases doesn't add significant value. Note: I left the Scheduler logging because it logs interactions with some awkward library components outside of ExoPlayer, so is perhaps a bit more justified. * This is a bit unfair since realistically we wouldn't ever add lots of logging into trivial classes. But I think it is fair to say that the deltas would be non-negligible. PiperOrigin-RevId: 246181421 --- .../jobdispatcher/JobDispatcherScheduler.java | 1 + .../android/exoplayer2/offline/Download.java | 22 --------------- .../exoplayer2/offline/DownloadService.java | 27 +++++-------------- .../scheduler/PlatformScheduler.java | 1 + .../exoplayer2/scheduler/Requirements.java | 12 --------- .../scheduler/RequirementsWatcher.java | 23 ---------------- .../exoplayer2/scheduler/Scheduler.java | 2 -- .../testutil/TestDownloadManagerListener.java | 22 +-------------- 8 files changed, 10 insertions(+), 100 deletions(-) diff --git a/extensions/jobdispatcher/src/main/java/com/google/android/exoplayer2/ext/jobdispatcher/JobDispatcherScheduler.java b/extensions/jobdispatcher/src/main/java/com/google/android/exoplayer2/ext/jobdispatcher/JobDispatcherScheduler.java index 790f5ca4e5..d79dead0d7 100644 --- a/extensions/jobdispatcher/src/main/java/com/google/android/exoplayer2/ext/jobdispatcher/JobDispatcherScheduler.java +++ b/extensions/jobdispatcher/src/main/java/com/google/android/exoplayer2/ext/jobdispatcher/JobDispatcherScheduler.java @@ -57,6 +57,7 @@ import com.google.android.exoplayer2.util.Util; */ public final class JobDispatcherScheduler implements Scheduler { + private static final boolean DEBUG = false; private static final String TAG = "JobDispatcherScheduler"; private static final String KEY_SERVICE_ACTION = "service_action"; private static final String KEY_SERVICE_PACKAGE = "service_package"; diff --git a/library/core/src/main/java/com/google/android/exoplayer2/offline/Download.java b/library/core/src/main/java/com/google/android/exoplayer2/offline/Download.java index 00d81b392c..97dff8394e 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/offline/Download.java +++ b/library/core/src/main/java/com/google/android/exoplayer2/offline/Download.java @@ -81,28 +81,6 @@ public final class Download { /** The download isn't stopped. */ public static final int STOP_REASON_NONE = 0; - /** Returns the state string for the given state value. */ - public static String getStateString(@State int state) { - switch (state) { - case STATE_QUEUED: - return "QUEUED"; - case STATE_STOPPED: - return "STOPPED"; - case STATE_DOWNLOADING: - return "DOWNLOADING"; - case STATE_COMPLETED: - return "COMPLETED"; - case STATE_FAILED: - return "FAILED"; - case STATE_REMOVING: - return "REMOVING"; - case STATE_RESTARTING: - return "RESTARTING"; - default: - throw new IllegalStateException(); - } - } - /** The download request. */ public final DownloadRequest request; /** The state of the download. */ diff --git a/library/core/src/main/java/com/google/android/exoplayer2/offline/DownloadService.java b/library/core/src/main/java/com/google/android/exoplayer2/offline/DownloadService.java index ce9087c6c8..fdd7163a2c 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/offline/DownloadService.java +++ b/library/core/src/main/java/com/google/android/exoplayer2/offline/DownloadService.java @@ -127,18 +127,18 @@ public abstract class DownloadService extends Service { public static final String KEY_DOWNLOAD_REQUEST = "download_request"; /** - * Key for the content id in {@link #ACTION_SET_STOP_REASON} and {@link #ACTION_REMOVE_DOWNLOAD} - * intents. + * Key for the {@link String} content id in {@link #ACTION_SET_STOP_REASON} and {@link + * #ACTION_REMOVE_DOWNLOAD} intents. */ public static final String KEY_CONTENT_ID = "content_id"; /** - * Key for the stop reason in {@link #ACTION_SET_STOP_REASON} and {@link #ACTION_ADD_DOWNLOAD} - * intents. + * Key for the integer stop reason in {@link #ACTION_SET_STOP_REASON} and {@link + * #ACTION_ADD_DOWNLOAD} intents. */ public static final String KEY_STOP_REASON = "stop_reason"; - /** Key for the requirements in {@link #ACTION_SET_REQUIREMENTS} intents. */ + /** Key for the {@link Requirements} in {@link #ACTION_SET_REQUIREMENTS} intents. */ public static final String KEY_REQUIREMENTS = "requirements"; /** @@ -155,7 +155,6 @@ public abstract class DownloadService extends Service { public static final long DEFAULT_FOREGROUND_NOTIFICATION_UPDATE_INTERVAL = 1000; private static final String TAG = "DownloadService"; - private static final boolean DEBUG = false; // Keep DownloadManagerListeners for each DownloadService as long as there are downloads (and the // process is running). This allows DownloadService to restart when there's no scheduler. @@ -506,7 +505,6 @@ public abstract class DownloadService extends Service { @Override public void onCreate() { - logd("onCreate"); if (channelId != null) { NotificationUtil.createNotificationChannel( this, channelId, channelNameResourceId, NotificationUtil.IMPORTANCE_LOW); @@ -541,7 +539,6 @@ public abstract class DownloadService extends Service { if (intentAction == null) { intentAction = ACTION_INIT; } - logd("onStartCommand action: " + intentAction + " startId: " + startId); switch (intentAction) { case ACTION_INIT: case ACTION_RESTART: @@ -573,7 +570,7 @@ public abstract class DownloadService extends Service { if (!intent.hasExtra(KEY_STOP_REASON)) { Log.e(TAG, "Ignored SET_STOP_REASON: Missing " + KEY_STOP_REASON + " extra"); } else { - int stopReason = intent.getIntExtra(KEY_STOP_REASON, Download.STOP_REASON_NONE); + int stopReason = intent.getIntExtra(KEY_STOP_REASON, /* defaultValue= */ 0); downloadManager.setStopReason(contentId, stopReason); } break; @@ -598,13 +595,11 @@ public abstract class DownloadService extends Service { @Override public void onTaskRemoved(Intent rootIntent) { - logd("onTaskRemoved rootIntent: " + rootIntent); taskRemoved = true; } @Override public void onDestroy() { - logd("onDestroy"); isDestroyed = true; DownloadManagerHelper downloadManagerHelper = downloadManagerListeners.get(getClass()); boolean unschedule = !downloadManager.isWaitingForRequirements(); @@ -713,16 +708,8 @@ public abstract class DownloadService extends Service { } if (Util.SDK_INT < 28 && taskRemoved) { // See [Internal: b/74248644]. stopSelf(); - logd("stopSelf()"); } else { - boolean stopSelfResult = stopSelfResult(lastStartId); - logd("stopSelf(" + lastStartId + ") result: " + stopSelfResult); - } - } - - private void logd(String message) { - if (DEBUG) { - Log.d(TAG, message); + stopSelfResult(lastStartId); } } diff --git a/library/core/src/main/java/com/google/android/exoplayer2/scheduler/PlatformScheduler.java b/library/core/src/main/java/com/google/android/exoplayer2/scheduler/PlatformScheduler.java index fc8e8b61a5..8572c9c7ca 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/scheduler/PlatformScheduler.java +++ b/library/core/src/main/java/com/google/android/exoplayer2/scheduler/PlatformScheduler.java @@ -44,6 +44,7 @@ import com.google.android.exoplayer2.util.Util; @TargetApi(21) public final class PlatformScheduler implements Scheduler { + private static final boolean DEBUG = false; private static final String TAG = "PlatformScheduler"; private static final String KEY_SERVICE_ACTION = "service_action"; private static final String KEY_SERVICE_PACKAGE = "service_package"; diff --git a/library/core/src/main/java/com/google/android/exoplayer2/scheduler/Requirements.java b/library/core/src/main/java/com/google/android/exoplayer2/scheduler/Requirements.java index babc4e49fb..30cf452572 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/scheduler/Requirements.java +++ b/library/core/src/main/java/com/google/android/exoplayer2/scheduler/Requirements.java @@ -27,7 +27,6 @@ import android.os.Parcel; import android.os.Parcelable; import android.os.PowerManager; import androidx.annotation.IntDef; -import com.google.android.exoplayer2.util.Log; import com.google.android.exoplayer2.util.Util; import java.lang.annotation.Documented; import java.lang.annotation.Retention; @@ -56,8 +55,6 @@ public final class Requirements implements Parcelable { /** Requirement that the device is charging. */ public static final int DEVICE_CHARGING = 1 << 3; - private static final String TAG = "Requirements"; - @RequirementFlags private final int requirements; /** @param requirements A combination of requirement flags. */ @@ -135,7 +132,6 @@ public final class Requirements implements Parcelable { if (networkInfo == null || !networkInfo.isConnected() || !isInternetConnectivityValidated(connectivityManager)) { - logd("No network info, connection or connectivity."); return requirements & (NETWORK | NETWORK_UNMETERED); } @@ -172,7 +168,6 @@ public final class Requirements implements Parcelable { } Network activeNetwork = connectivityManager.getActiveNetwork(); if (activeNetwork == null) { - logd("No active network."); return false; } NetworkCapabilities networkCapabilities = @@ -180,16 +175,9 @@ public final class Requirements implements Parcelable { boolean validated = networkCapabilities == null || !networkCapabilities.hasCapability(NetworkCapabilities.NET_CAPABILITY_VALIDATED); - logd("Network capability validated: " + validated); return !validated; } - private static void logd(String message) { - if (Scheduler.DEBUG) { - Log.d(TAG, message); - } - } - @Override public boolean equals(Object o) { if (this == o) { diff --git a/library/core/src/main/java/com/google/android/exoplayer2/scheduler/RequirementsWatcher.java b/library/core/src/main/java/com/google/android/exoplayer2/scheduler/RequirementsWatcher.java index d2ad357ff6..f0d0f37cdf 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/scheduler/RequirementsWatcher.java +++ b/library/core/src/main/java/com/google/android/exoplayer2/scheduler/RequirementsWatcher.java @@ -28,7 +28,6 @@ import android.os.Handler; import android.os.Looper; import android.os.PowerManager; import androidx.annotation.RequiresApi; -import com.google.android.exoplayer2.util.Log; import com.google.android.exoplayer2.util.Util; /** @@ -53,8 +52,6 @@ public final class RequirementsWatcher { @Requirements.RequirementFlags int notMetRequirements); } - private static final String TAG = "RequirementsWatcher"; - private final Context context; private final Listener listener; private final Requirements requirements; @@ -75,7 +72,6 @@ public final class RequirementsWatcher { this.listener = listener; this.requirements = requirements; handler = new Handler(Util.getLooper()); - logd(this + " created"); } /** @@ -110,7 +106,6 @@ public final class RequirementsWatcher { } receiver = new DeviceStatusChangeReceiver(); context.registerReceiver(receiver, filter, null, handler); - logd(this + " started"); return notMetRequirements; } @@ -121,7 +116,6 @@ public final class RequirementsWatcher { if (networkCallback != null) { unregisterNetworkCallback(); } - logd(this + " stopped"); } /** Returns watched {@link Requirements}. */ @@ -129,14 +123,6 @@ public final class RequirementsWatcher { return requirements; } - @Override - public String toString() { - if (!Scheduler.DEBUG) { - return super.toString(); - } - return "RequirementsWatcher{" + requirements + '}'; - } - @TargetApi(23) private void registerNetworkCallbackV23() { ConnectivityManager connectivityManager = @@ -163,22 +149,14 @@ public final class RequirementsWatcher { int notMetRequirements = requirements.getNotMetRequirements(context); if (this.notMetRequirements != notMetRequirements) { this.notMetRequirements = notMetRequirements; - logd("notMetRequirements has changed: " + notMetRequirements); listener.onRequirementsStateChanged(this, notMetRequirements); } } - private static void logd(String message) { - if (Scheduler.DEBUG) { - Log.d(TAG, message); - } - } - private class DeviceStatusChangeReceiver extends BroadcastReceiver { @Override public void onReceive(Context context, Intent intent) { if (!isInitialStickyBroadcast()) { - logd(RequirementsWatcher.this + " received " + intent.getAction()); checkRequirements(); } } @@ -200,7 +178,6 @@ public final class RequirementsWatcher { handler.post( () -> { if (networkCallback != null) { - logd(RequirementsWatcher.this + " NetworkCallback"); checkRequirements(); } }); diff --git a/library/core/src/main/java/com/google/android/exoplayer2/scheduler/Scheduler.java b/library/core/src/main/java/com/google/android/exoplayer2/scheduler/Scheduler.java index 1b225d9a4d..b5a6f40424 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/scheduler/Scheduler.java +++ b/library/core/src/main/java/com/google/android/exoplayer2/scheduler/Scheduler.java @@ -22,8 +22,6 @@ import android.content.Intent; /** Schedules a service to be started in the foreground when some {@link Requirements} are met. */ public interface Scheduler { - /* package */ boolean DEBUG = false; - /** * Schedules a service to be started in the foreground when some {@link Requirements} are met. * Anything that was previously scheduled will be canceled. diff --git a/testutils_robolectric/src/main/java/com/google/android/exoplayer2/testutil/TestDownloadManagerListener.java b/testutils_robolectric/src/main/java/com/google/android/exoplayer2/testutil/TestDownloadManagerListener.java index 9d6223b8b1..4c334992b5 100644 --- a/testutils_robolectric/src/main/java/com/google/android/exoplayer2/testutil/TestDownloadManagerListener.java +++ b/testutils_robolectric/src/main/java/com/google/android/exoplayer2/testutil/TestDownloadManagerListener.java @@ -22,9 +22,7 @@ import android.os.ConditionVariable; import com.google.android.exoplayer2.offline.Download; import com.google.android.exoplayer2.offline.Download.State; import com.google.android.exoplayer2.offline.DownloadManager; -import java.util.ArrayList; import java.util.HashMap; -import java.util.Locale; import java.util.concurrent.ArrayBlockingQueue; import java.util.concurrent.CountDownLatch; import java.util.concurrent.TimeUnit; @@ -138,7 +136,6 @@ public final class TestDownloadManagerListener implements DownloadManager.Listen } private void assertStateInternal(String taskId, int expectedState, int timeoutMs) { - ArrayList receivedStates = new ArrayList<>(); while (true) { Integer state = null; try { @@ -150,25 +147,8 @@ public final class TestDownloadManagerListener implements DownloadManager.Listen if (expectedState == state) { return; } - receivedStates.add(state); } else { - StringBuilder sb = new StringBuilder(); - for (int i = 0; i < receivedStates.size(); i++) { - if (i > 0) { - sb.append(','); - } - int receivedState = receivedStates.get(i); - String receivedStateString = - receivedState == STATE_REMOVED ? "REMOVED" : Download.getStateString(receivedState); - sb.append(receivedStateString); - } - fail( - String.format( - Locale.US, - "for download (%s) expected:<%s> but was:<%s>", - taskId, - Download.getStateString(expectedState), - sb)); + fail("Didn't receive expected state: " + expectedState); } } }