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
This commit is contained in:
olly 2019-05-01 20:50:44 +01:00 committed by Oliver Woodman
parent 214a372e06
commit 9f9cf316bd
8 changed files with 10 additions and 100 deletions

View file

@ -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";

View file

@ -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. */

View file

@ -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);
}
}

View file

@ -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";

View file

@ -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) {

View file

@ -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();
}
});

View file

@ -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.

View file

@ -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<Integer> 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);
}
}
}