Remove @RemovableInRelease usage in //base

This CL is a step towards deleting @RemovableInRelease. (See
linked crbug for rationale)

This CL:
- replaces @RemovableInRelease usage
  in org.chromium.base.Log with dedicated proguard rules.
- Removes stale comments in org.chromium.base.Log and
  org.chromium.base.library_loader.LibraryLoader
- Removes references to @RemovableInRelease in //docs/android_logging.md
- Adds Logging#isLoggingEnabled() and adds recommendation in
  //docs/android_logging.md to guard logging-related code with it.

BUG=1180332

Change-Id: I84a58496dce593259e92bd5b4e30b45e4a9a93c9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3043622
Commit-Queue: Peter Kotwicz <pkotwicz@chromium.org>
Reviewed-by: Andrew Grieve <agrieve@chromium.org>
Cr-Commit-Position: refs/heads/master@{#904502}

base/android/java/src/org/chromium/base/Log.java

@@ -4,7 +4,7 @@
 
 package org.chromium.base;
 
-import org.chromium.base.annotations.RemovableInRelease;
+import org.chromium.base.annotations.CheckDiscard;
 
 import java.util.Locale;
 
@@ -82,15 +82,14 @@ public class Log {
         return "[" + getCallOrigin() + "] " + formatLog(messageTemplate, tr, params);
     }
 
-    @RemovableInRelease
     private static boolean isDebug() {
-        // @RemovableInRelease causes this to return false in release builds.
+        // Proguard sets value to false in release builds.
         return true;
     }
 
     /**
-     * In debug: Forwards to {@link android.util.Log#isLoggable(String, int)}, but alway
-     * In release: Always returns false (via @RemovableInRelease).
+     * In debug: Forwards to {@link android.util.Log#isLoggable(String, int)}, but always
+     * In release: Always returns false (via proguard rule).
      */
     public static boolean isLoggable(String tag, int level) {
         // Early return helps optimizer eliminate calls to isLoggable().
@@ -103,10 +102,6 @@ public class Log {
     /**
      * Sends a {@link android.util.Log#VERBOSE} log message.
      *
-     * For optimization purposes, only the fixed parameters versions are visible. If you need more
-     * than 7 parameters, consider building your log message using a function annotated with
-     * {@link RemovableInRelease}.
-     *
      * @param tag Used to identify the source of a log message. Might be modified in the output
      *            (see {@link #normalizeTag(String)})
      * @param messageTemplate The message you would like logged. It is to be specified as a format
@@ -114,8 +109,10 @@ public class Log {
      * @param args Arguments referenced by the format specifiers in the format string. If the last
      *             one is a {@link Throwable}, its trace will be printed.
      */
-    @RemovableInRelease
+    @CheckDiscard("crbug.com/1231625")
     public static void v(String tag, String messageTemplate, Object... args) {
+        if (!isDebug()) return;
+
         Throwable tr = getThrowableToLog(args);
         String message = formatLogWithStack(messageTemplate, tr, args);
         if (tr != null) {
@@ -128,10 +125,6 @@ public class Log {
     /**
      * Sends a {@link android.util.Log#DEBUG} log message.
      *
-     * For optimization purposes, only the fixed parameters versions are visible. If you need more
-     * than 7 parameters, consider building your log message using a function annotated with
-     * {@link RemovableInRelease}.
-     *
      * @param tag Used to identify the source of a log message. Might be modified in the output
      *            (see {@link #normalizeTag(String)})
      * @param messageTemplate The message you would like logged. It is to be specified as a format
@@ -139,8 +132,10 @@ public class Log {
      * @param args Arguments referenced by the format specifiers in the format string. If the last
      *             one is a {@link Throwable}, its trace will be printed.
      */
-    @RemovableInRelease
+    @CheckDiscard("crbug.com/1231625")
     public static void d(String tag, String messageTemplate, Object... args) {
+        if (!isDebug()) return;
+
         Throwable tr = getThrowableToLog(args);
         String message = formatLogWithStack(messageTemplate, tr, args);
         if (tr != null) {
@@ -249,7 +244,7 @@ public class Log {
     }
 
     /** Returns a string form of the origin of the log call, to be used as secondary tag.*/
-    @RemovableInRelease
+    @CheckDiscard("crbug.com/1231625")
     private static String getCallOrigin() {
         StackTraceElement[] st = Thread.currentThread().getStackTrace();
 

base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java

@@ -408,8 +408,7 @@ public class LibraryLoader {
         }
     }
 
-    @CheckDiscard(
-            "Can't use @RemovableInRelease because Release build with ENABLE_ASSERTS needs it")
+    @CheckDiscard("")
     public void enableJniChecks() {
         if (!BuildConfig.ENABLE_ASSERTS) return;
 

base/android/proguard/chromium_code.flags

@@ -69,6 +69,10 @@
   @org.chromium.base.annotations.RemovableInRelease boolean *(...) return false;
 }
 
+-assumenosideeffects class org.chromium.base.Log {
+  static boolean isDebug() return false;
+}
+
 # Never inline classes, methods, or fields  with this annotation, but allow
 # shrinking and obfuscation.
 # Relevant to fields when they are needed to store strong refrences to objects

chrome/android/expectations/monochrome_public_bundle.proguard_flags.expected

@@ -210,6 +210,10 @@
   @org.chromium.base.annotations.RemovableInRelease boolean *(...) return false;
 }
 
+-assumenosideeffects class org.chromium.base.Log {
+  static boolean isDebug() return false;
+}
+
 # Never inline classes, methods, or fields  with this annotation, but allow
 # shrinking and obfuscation.
 # Relevant to fields when they are needed to store strong refrences to objects

chrome/android/java/src/org/chromium/chrome/browser/gcore/ConnectedTask.java

@@ -10,7 +10,7 @@ import androidx.annotation.VisibleForTesting;
 
 import org.chromium.base.Log;
 import org.chromium.base.TraceEvent;
-import org.chromium.base.annotations.RemovableInRelease;
+import org.chromium.base.annotations.CheckDiscard;
 
 /**
  * Base class for tasks which connects to Google Play Services using given GoogleApiClient,
@@ -76,7 +76,7 @@ public abstract class ConnectedTask<T extends ChromeGoogleApiClient> implements
     /**
      * Returns a name of a task (for debug logging).
      */
-    @RemovableInRelease
+    @CheckDiscard("getName() is only for debug logging")
     protected abstract String getName();
 
     /**

docs/android_logging.md

@@ -107,9 +107,8 @@ Sometimes the values to log aren't readily available and need to be computed
 specially. This should be avoided when logging is disabled.
 
 ```java
-static private final boolean DEBUG = false;  // debug toggle.
 ...
-if (DEBUG) {
+if (Log.isLoggable(TAG, Log.INFO)) {
   Log.i(TAG, createThatExpensiveLogMessage(activity))
 }
 ```
@@ -119,42 +118,6 @@ time, the Java compiler will optimize out all guarded calls from the
 generated `.class` file. Changing it however requires editing each of the
 files for which debug should be enabled and recompiling.
 
-#### Annotate debug functions with the `@RemovableInRelease` annotation.
-
-That annotation tells Proguard to assume that a given function has no side
-effects, and is called only for its returned value. If this value is unused,
-the call will be removed. If the function is not called at all, it will also
-be removed. Since Proguard is already used to strip debug and verbose calls
-out of release builds, this annotation allows it to have a deeper action by
-removing also function calls used to generate the log call's arguments.
-
-```java
-/* If that function is only used in Log.d calls, proguard should
- * completely remove it from the release builds. */
-@RemovableInRelease
-private static String getSomeDebugLogString(Thing[] things) {
-  StringBuilder sb = new StringBuilder(
-      "Reporting " + thing.length + " things: ");
-  for (Thing thing : things) {
-    sb.append('\n').append(thing.id).append(' ').append(report.foo);
-  }
-  return sb.toString();
-}
-
-public void bar() {
-  ...
-  Log.d(TAG, getSomeDebugLogString(things)); /* The line is removed in
-                                              *  release builds. */
-}
-```
-
-Again, this is useful only if the input to that function are variables
-already available in the scope. The idea is to move computations,
-concatenations, etc. to a place where that can be removed when not needed,
-without invading the main function's logic. It can then have a similar
-effect as guarding with a static final property that would be enabled in
-Debug and disabled in Release.
-
 ### Rule #3: Favor small log messages
 
 This is still related to the global fixed-sized kernel buffer used to keep all