components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java - Issue 2109293005: [Cronet] Fix up log TAGs to be accurate and prevent future inaccuracies.

Unified Diff: components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java

Issue 2109293005: [Cronet] Fix up log TAGs to be accurate and prevent future inaccuracies. (Closed) Base URL: https://chromium.googlesource.com/chromium/src.git@master

Patch Set: Created 4 years, 6 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

View side-by-side diff with in-line comments

Download patch

« components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequestContext.java ('K') | « components/cronet/android/java/src/org/chromium/net/CronetUploadDataStream.java ('k') | components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java » ('j') | no next file with comments »
Expand Comments ('e') | Collapse Comments ('c') | Hide Comments ('s')

Index: components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java

diff --git a/components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java b/components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java

index 4a7c8d8f519b1edb1b3d82c5f2ccd2de3a271c57..92591172cda428c1c5ec267e859dca5a08bfc5e5 100644

--- a/components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java

+++ b/components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java

@@ -43,7 +43,7 @@ class CronetUrlRequestContext extends CronetEngine {

private static final int LOG_NONE = 3; // LOG(FATAL), no VLOG.

private static final int LOG_DEBUG = -1; // LOG(FATAL...INFO), VLOG(1)

private static final int LOG_VERBOSE = -2; // LOG(FATAL...INFO), VLOG(2)

- static final String LOG_TAG = "ChromiumNetwork";

xunjieli 2016/07/01 14:11:07 There was a reason that this TAG is ChromiumNetwor

mef 2016/07/01 15:18:18 I think the reason was mostly historical (based on

pauljensen 2016/07/01 15:35:22 I don't feel too strongly as there are pros and co

mef 2016/07/13 19:15:58 Acknowledged. I don't feel strongly either, and fo

Acknowledged. I don't feel strongly either, and following common pattern sgtm.

Missing "cr_" is added by org.chromium.base.Log: https://cs.chromium.org/chromium/src/base/android/java/src/org/chromium/base/... I think we should drop explicit "cr." from our code, and use org.chromium.base.Log in implementation. WDYT?

pauljensen 2016/08/17 11:58:33 Done.

On 2016/07/13 19:15:58, mef (OOO Aug 15 - Aug 19) wrote: > On 2016/07/01 15:35:22, pauljensen wrote: > > On 2016/07/01 15:18:18, mef wrote: > > > On 2016/07/01 14:11:07, xunjieli wrote: > > > > There was a reason that this TAG is ChromiumNetwork. I am not sure if it > is > > > > still applicable. +mef@ to comment. > > > > > > I think the reason was mostly historical (based on existing implementation). > > > I don't mind the rename, but I'm not sure whether class name is the right > tag > > to > > > use. > > > > > > 2 reasons: > > > - Wouldn't it be useful to adjust all Cronet logging with one setting as > > opposed > > > to X class-specific settings? > > > > I don't feel too strongly as there are pros and cons to both arguments: > > 1. using the class name is AFAIK the pattern that everyone uses. > > 2. using the class name makes it easy to track something down, rather than > > trying to grep for a particular error message substring > > 3. using a more general tag like "Cronet" can lead to identical log messages > in > > different files that are impossible to differentiate > > 4. using a more general tag like "Cronet" makes it harder to know if you're > > adding a log message that is identical to those in another file > > 5. using a more general tag like "Cronet" helps embedders know it came from > > cronet, rather than trying to guess what "CronetUrlRequestContext" is > > Acknowledged. I don't feel strongly either, and following common pattern sgtm. > > > > > > - It seems that Chrome likes to prefix tags with 'cr.', should Cronet do the > > > same? > > > > I don't think this is common in Chrome. It appears to be less than 10%: > > 616 instances of "TAG = " in Java files, and only 45 instances of "cr." in > > those: > > > https://cs.chromium.org/search/?q=%22TAG+%3D+%22+file:java&sq=package:chromiu... > > > https://cs.chromium.org/search/?q=%22TAG+%3D+%22+file:java+%22cr.%22&sq=packa... > > Missing "cr_" is added by org.chromium.base.Log: > https://cs.chromium.org/chromium/src/base/android/java/src/org/chromium/base/... > > I think we should drop explicit "cr." from our code, and use > org.chromium.base.Log in implementation. WDYT?

Done.

+ static final String LOG_TAG = CronetUrlRequestContext.class.getSimpleName();

/**

* Synchronize access to mUrlRequestContextAdapter and shutdown routine.