[Android log] Promote using hardcoded string tags
This is to avoid static initializers that would make the code slower.
See the linked bug for discussion.
This patch deprecates Log#makeTag(String) and adds presubmit checks
to enforce at submit time what makeTag was trying to do: length and
naming rules
BUG=485772
Review URL: https://codereview.chromium.org/1131903007
Cr-Commit-Position: refs/heads/master@{#333710}
diff --git a/PRESUBMIT.py b/PRESUBMIT.py
index 4d5c853..515da1d 100644
--- a/PRESUBMIT.py
+++ b/PRESUBMIT.py
@@ -1304,6 +1304,77 @@
black_list=_EXCLUDED_PATHS + input_api.DEFAULT_BLACK_LIST)
+def _CheckAndroidCrLogUsage(input_api, output_api):
+ """Checks that new logs using org.chromium.base.Log:
+ - Are using 'TAG' as variable name for the tags (warn)
+ - Are using the suggested name format for the tags: "cr.<PackageTag>" (warn)
+ - Are using a tag that is shorter than 23 characters (error)
+ """
+ cr_log_import_pattern = input_api.re.compile(
+ r'^import org\.chromium\.base\.Log;$', input_api.re.MULTILINE);
+ # Extract the tag from lines like `Log.d(TAG, "*");` or `Log.d("TAG", "*");`
+ cr_log_pattern = input_api.re.compile(r'^\s*Log\.\w\((?P<tag>\"?\w+\"?)\,')
+ log_decl_pattern = input_api.re.compile(
+ r'^\s*private static final String TAG = "(?P<name>(.*)")',
+ input_api.re.MULTILINE)
+ log_name_pattern = input_api.re.compile(r'^cr[.\w]*')
+
+ REF_MSG = ('See base/android/java/src/org/chromium/base/README_logging.md '
+ 'or contact [email protected] for more info.')
+ sources = lambda x: input_api.FilterSourceFile(x, white_list=(r'.*\.java$',))
+ tag_errors = []
+ tag_decl_errors = []
+ tag_length_errors = []
+
+ for f in input_api.AffectedSourceFiles(sources):
+ file_content = input_api.ReadFile(f)
+ has_modified_logs = False
+
+ # Per line checks
+ if cr_log_import_pattern.search(file_content):
+ for line_num, line in f.ChangedContents():
+
+ # Check if the new line is doing some logging
+ match = cr_log_pattern.search(line)
+ if match:
+ has_modified_logs = True
+
+ # Make sure it uses "TAG"
+ if not match.group('tag') == 'TAG':
+ tag_errors.append("%s:%d" % (f.LocalPath(), line_num))
+
+ # Per file checks
+ if has_modified_logs:
+ # Make sure the tag is using the "cr" prefix and is not too long
+ match = log_decl_pattern.search(file_content)
+ tag_name = match.group('name') if match else ''
+ if not log_name_pattern.search(tag_name ):
+ tag_decl_errors.append(f.LocalPath())
+ if len(tag_name) > 23:
+ tag_length_errors.append(f.LocalPath())
+
+ results = []
+ if tag_decl_errors:
+ results.append(output_api.PresubmitPromptWarning(
+ 'Please define your tags using the suggested format: .\n'
+ '"private static final String TAG = "cr.<package tag>".\n' + REF_MSG,
+ tag_decl_errors))
+
+ if tag_length_errors:
+ results.append(output_api.PresubmitError(
+ 'The tag length is restricted by the system to be at most '
+ '23 characters.\n' + REF_MSG,
+ tag_length_errors))
+
+ if tag_errors:
+ results.append(output_api.PresubmitPromptWarning(
+ 'Please use a variable named "TAG" for your log tags.\n' + REF_MSG,
+ tag_errors))
+
+ return results
+
+
+# TODO(dgn): refactor with _CheckAndroidCrLogUsage
def _CheckNoNewUtilLogUsage(input_api, output_api):
"""Checks that new logs are using org.chromium.base.Log."""
@@ -1456,6 +1527,14 @@
return results
+def _AndroidSpecificOnUploadChecks(input_api, output_api):
+ """Groups checks that target android code."""
+ results = []
+ results.extend(_CheckNoNewUtilLogUsage(input_api, output_api))
+ results.extend(_CheckAndroidCrLogUsage(input_api, output_api))
+ return results
+
+
def _CommonChecks(input_api, output_api):
"""Checks common to both upload and commit."""
results = []
@@ -1728,7 +1807,7 @@
results.extend(
input_api.canned_checks.CheckGNFormatted(input_api, output_api))
results.extend(_CheckUmaHistogramChanges(input_api, output_api))
- results.extend(_CheckNoNewUtilLogUsage(input_api, output_api))
+ results.extend(_AndroidSpecificOnUploadChecks(input_api, output_api))
return results
diff --git a/PRESUBMIT_test.py b/PRESUBMIT_test.py
index 195fe4f..613fd41c 100755
--- a/PRESUBMIT_test.py
+++ b/PRESUBMIT_test.py
@@ -859,6 +859,77 @@
self.assertTrue('HasAndroidLog.java' in warnings[0].items[0])
self.assertTrue('HasExplicitLog.java' in warnings[0].items[1])
+ def testCheckAndroidCrLogUsage(self):
+ mock_input_api = MockInputApi()
+ mock_output_api = MockOutputApi()
+
+ mock_input_api.files = [
+ MockAffectedFile('RandomStuff.java', [
+ 'random stuff'
+ ]),
+ MockAffectedFile('HasCorrectTag.java', [
+ 'import org.chromium.base.Log;',
+ 'some random stuff',
+ 'private static final String TAG = "cr.Foo";',
+ 'Log.d(TAG, "foo");',
+ ]),
+ MockAffectedFile('HasShortCorrectTag.java', [
+ 'import org.chromium.base.Log;',
+ 'some random stuff',
+ 'private static final String TAG = "cr";',
+ 'Log.d(TAG, "foo");',
+ ]),
+ MockAffectedFile('HasNoTagDecl.java', [
+ 'import org.chromium.base.Log;',
+ 'some random stuff',
+ 'Log.d(TAG, "foo");',
+ ]),
+ MockAffectedFile('HasIncorrectTagDecl.java', [
+ 'import org.chromium.base.Log;',
+ 'private static final String TAHG = "cr.Foo";',
+ 'some random stuff',
+ 'Log.d(TAG, "foo");',
+ ]),
+ MockAffectedFile('HasInlineTag.java', [
+ 'import org.chromium.base.Log;',
+ 'some random stuff',
+ 'private static final String TAG = "cr.Foo";',
+ 'Log.d("TAG", "foo");',
+ ]),
+ MockAffectedFile('HasIncorrectTag.java', [
+ 'import org.chromium.base.Log;',
+ 'some random stuff',
+ 'private static final String TAG = "rubbish";',
+ 'Log.d(TAG, "foo");',
+ ]),
+ MockAffectedFile('HasTooLongTag.java', [
+ 'import org.chromium.base.Log;',
+ 'some random stuff',
+ 'private static final String TAG = "cr.24_charachers_long___";',
+ 'Log.d(TAG, "foo");',
+ ]),
+ ]
+
+ msgs = PRESUBMIT._CheckAndroidCrLogUsage(
+ mock_input_api, mock_output_api)
+
+ self.assertEqual(3, len(msgs))
+
+ # Declaration format
+ self.assertEqual(3, len(msgs[0].items))
+ self.assertTrue('HasNoTagDecl.java' in msgs[0].items)
+ self.assertTrue('HasIncorrectTagDecl.java' in msgs[0].items)
+ self.assertTrue('HasIncorrectTag.java' in msgs[0].items)
+
+ # Tag length
+ self.assertEqual(1, len(msgs[1].items))
+ self.assertTrue('HasTooLongTag.java' in msgs[1].items)
+
+ # Tag must be a variable named TAG
+ self.assertEqual(1, len(msgs[2].items))
+ self.assertTrue('HasInlineTag.java:4' in msgs[2].items)
+
+
if __name__ == '__main__':
unittest.main()
diff --git a/base/android/java/src/org/chromium/base/Log.java b/base/android/java/src/org/chromium/base/Log.java
index b7fd774..c83cfe79 100644
--- a/base/android/java/src/org/chromium/base/Log.java
+++ b/base/android/java/src/org/chromium/base/Log.java
@@ -23,8 +23,6 @@
* </p>
*/
public class Log {
- private static final String BASE_TAG = "cr";
-
/** Convenience property, same as {@link android.util.Log#ASSERT}. */
public static final int ASSERT = android.util.Log.ASSERT;
@@ -72,10 +70,12 @@
*
* @see android.util.Log#isLoggable(String, int)
* @throws IllegalArgumentException if the tag is too long.
+ * @deprecated Directly use a string (e.g. "cr.Tag") in your class. See http://crbug.com/485772
*/
+ @Deprecated
public static String makeTag(String groupTag) {
- if (TextUtils.isEmpty(groupTag)) return BASE_TAG;
- String tag = BASE_TAG + "." + groupTag;
+ if (TextUtils.isEmpty(groupTag)) return "cr";
+ String tag = "cr." + groupTag;
if (tag.length() > 23) {
throw new IllegalArgumentException(
"The full tag (" + tag + ") is longer than 23 characters.");
@@ -95,9 +95,7 @@
* than 7 parameters, consider building your log message using a function annotated with
* {@link NoSideEffects}.
*
- * @param tag Used to identify the source of a log message. Should be created using
- * {@link #makeTag(String)}.
- *
+ * @param tag Used to identify the source of a log message.
* @param messageTemplate The message you would like logged. It is to be specified as a format
* string.
* @param args Arguments referenced by the format specifiers in the format string. If the last
@@ -167,9 +165,7 @@
* than 7 parameters, consider building your log message using a function annotated with
* {@link NoSideEffects}.
*
- * @param tag Used to identify the source of a log message. Should be created using
- * {@link #makeTag(String)}.
- *
+ * @param tag Used to identify the source of a log message.
* @param messageTemplate The message you would like logged. It is to be specified as a format
* string.
* @param args Arguments referenced by the format specifiers in the format string. If the last
@@ -233,8 +229,7 @@
/**
* Sends an {@link android.util.Log#INFO} log message.
*
- * @param tag Used to identify the source of a log message. Should be created using
- * {@link #makeTag(String)}.
+ * @param tag Used to identify the source of a log message.
* @param messageTemplate The message you would like logged. It is to be specified as a format
* string.
* @param args Arguments referenced by the format specifiers in the format string. If the last
@@ -255,8 +250,7 @@
/**
* Sends a {@link android.util.Log#WARN} log message.
*
- * @param tag Used to identify the source of a log message. Should be created using
- * {@link #makeTag(String)}.
+ * @param tag Used to identify the source of a log message.
* @param messageTemplate The message you would like logged. It is to be specified as a format
* string.
* @param args Arguments referenced by the format specifiers in the format string. If the last
@@ -277,8 +271,7 @@
/**
* Sends an {@link android.util.Log#ERROR} log message.
*
- * @param tag Used to identify the source of a log message. Should be created using
- * {@link #makeTag(String)}.
+ * @param tag Used to identify the source of a log message.
* @param messageTemplate The message you would like logged. It is to be specified as a format
* string.
* @param args Arguments referenced by the format specifiers in the format string. If the last
@@ -303,8 +296,7 @@
*
* @see android.util.Log#wtf(String, String, Throwable)
*
- * @param tag Used to identify the source of a log message. Should be created using
- * {@link #makeTag(String)}.
+ * @param tag Used to identify the source of a log message.
* @param messageTemplate The message you would like logged. It is to be specified as a format
* string.
* @param args Arguments referenced by the format specifiers in the format string. If the last
diff --git a/base/android/java/src/org/chromium/base/README_logging.md b/base/android/java/src/org/chromium/base/README_logging.md
index d7fbcb4..a795b6b 100644
--- a/base/android/java/src/org/chromium/base/README_logging.md
+++ b/base/android/java/src/org/chromium/base/README_logging.md
@@ -9,7 +9,7 @@
Usage:
- private static final String TAG = Log.makeTag("YourModuleTag");
+ private static final String TAG = "cr.YourModuleTag";
...
Log.i(TAG, "Logged INFO message.");
Log.d(TAG, "Some DEBUG info: %s", data);
