Clean up Cronet native logging level code - Make it clear this sets the *native* Chromium code log level, not the Android or Java log level - Native Chromium LOG() statements are logged with the "chromium" Android log tag, so use that tag to enable VLOG() instead of using the CronetUrlRequestContext log tag (which is incredibly confusing) - Move the code to CronetLibraryLoader - this is Cronet library init code and has no business living in CronetUrlRequestContext (also, that's racy). - Don't send the reader on a Fedex quest fetching random constants at the other end of the source file - Add a TODO to revisit the whole approach as it's basically a hack Change-Id: I52d150e87c9042ac86bfede356d800540bcedf05 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4907020 Reviewed-by: Stefano Duo <stefanoduo@google.com> Auto-Submit: Etienne Dechamps <edechamps@google.com> Commit-Queue: Stefano Duo <stefanoduo@google.com> Cr-Commit-Position: refs/heads/main@{#1204056}
diff --git a/components/cronet/android/cronet_context_adapter.cc b/components/cronet/android/cronet_context_adapter.cc index 81386c87..83f296e 100644 --- a/components/cronet/android/cronet_context_adapter.cc +++ b/components/cronet/android/cronet_context_adapter.cc
@@ -318,14 +318,6 @@ return reinterpret_cast<jlong>(context_adapter); } -static jint JNI_CronetUrlRequestContext_SetMinLogLevel(JNIEnv* env, - jint jlog_level) { - jint old_log_level = static_cast<jint>(logging::GetMinLogLevel()); - // MinLogLevel is global, shared by all URLRequestContexts. - logging::SetMinLogLevel(static_cast<int>(jlog_level)); - return old_log_level; -} - static ScopedJavaLocalRef<jbyteArray> JNI_CronetUrlRequestContext_GetHistogramDeltas(JNIEnv* env) { std::vector<uint8_t> data;
diff --git a/components/cronet/android/cronet_library_loader.cc b/components/cronet/android/cronet_library_loader.cc index 7a07e6d..6b4a702 100644 --- a/components/cronet/android/cronet_library_loader.cc +++ b/components/cronet/android/cronet_library_loader.cc
@@ -157,6 +157,10 @@ return base::android::ConvertUTF8ToJavaString(env, CRONET_VERSION); } +void JNI_CronetLibraryLoader_SetMinLogLevel(JNIEnv* env, jint jlog_level) { + logging::SetMinLogLevel(jlog_level); +} + void PostTaskToInitThread(const base::Location& posted_from, base::OnceClosure task) { g_init_thread_init_done.Wait();
diff --git a/components/cronet/android/java/src/org/chromium/net/impl/CronetLibraryLoader.java b/components/cronet/android/java/src/org/chromium/net/impl/CronetLibraryLoader.java index 0e61804..928f5a6 100644 --- a/components/cronet/android/java/src/org/chromium/net/impl/CronetLibraryLoader.java +++ b/components/cronet/android/java/src/org/chromium/net/impl/CronetLibraryLoader.java
@@ -86,12 +86,31 @@ } Log.i(TAG, "Cronet version: %s, arch: %s", implVersion, System.getProperty("os.arch")); + setNativeLoggingLevel(); sLibraryLoaded = true; sWaitForLibLoad.open(); } } } + private static void setNativeLoggingLevel() { + // The constants used here should be kept in sync with logging::LogMessage::~LogMessage(). + final String nativeLogTag = "chromium"; + int loggingLevel; + // TODO: this way of enabling VLOG is a hack - it doesn't make a ton of sense because + // logging::LogMessage() will still log VLOG() at the Android INFO log level, not DEBUG or + // VERBOSE; also this doesn't make it possible to use advanced filters like --vmodule. See + // https://crbug.com/1488393 for a proposed alternative. + if (Log.isLoggable(nativeLogTag, Log.VERBOSE)) { + loggingLevel = -2; // VLOG(2) + } else if (Log.isLoggable(nativeLogTag, Log.DEBUG)) { + loggingLevel = -1; // VLOG(1) + } else { + loggingLevel = 3; // LOG(FATAL) only + } + CronetLibraryLoaderJni.get().setMinLogLevel(loggingLevel); + } + /** * Returns {@code true} if running on the initialization thread. */ @@ -228,5 +247,7 @@ void cronetInitOnInitThread(); String getCronetVersion(); + + void setMinLogLevel(int loggingLevel); } }
diff --git a/components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.java b/components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.java index 9a6b7a61..e34bc0a 100644 --- a/components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.java +++ b/components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.java
@@ -56,9 +56,6 @@ @UsedByReflection("CronetEngine.java") @VisibleForTesting public class CronetUrlRequestContext extends CronetEngineBase { - 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 = CronetUrlRequestContext.class.getSimpleName(); /** @@ -205,7 +202,6 @@ mThroughputListenerList.disableThreadAsserts(); mNetworkQualityEstimatorEnabled = builder.networkQualityEstimatorEnabled(); CronetLibraryLoader.ensureInitialized(builder.getContext(), builder); - CronetUrlRequestContextJni.get().setMinLogLevel(getLoggingLevel()); if (builder.httpCacheMode() == HttpCacheType.DISK) { mInUseStoragePath = builder.storagePath(); synchronized (sInUseStoragePaths) { @@ -715,21 +711,6 @@ return mUrlRequestContextAdapter != 0; } - /** - * @return loggingLevel see {@link #LOG_NONE}, {@link #LOG_DEBUG} and {@link #LOG_VERBOSE}. - */ - private int getLoggingLevel() { - int loggingLevel; - if (Log.isLoggable(LOG_TAG, Log.VERBOSE)) { - loggingLevel = LOG_VERBOSE; - } else if (Log.isLoggable(LOG_TAG, Log.DEBUG)) { - loggingLevel = LOG_DEBUG; - } else { - loggingLevel = LOG_NONE; - } - return loggingLevel; - } - private static int convertConnectionTypeToApiValue(@EffectiveConnectionType int type) { switch (type) { case EffectiveConnectionType.TYPE_OFFLINE: @@ -870,7 +851,6 @@ void addPkp(long urlRequestContextConfig, String host, byte[][] hashes, boolean includeSubdomains, long expirationTime); long createRequestContextAdapter(long urlRequestContextConfig); - int setMinLogLevel(int loggingLevel); byte[] getHistogramDeltas(); @NativeClassQualifiedName("CronetContextAdapter") void destroy(long nativePtr, CronetUrlRequestContext caller);
diff --git a/components/cronet/android/test_instructions.md b/components/cronet/android/test_instructions.md index 74f4323..896eddf61 100644 --- a/components/cronet/android/test_instructions.md +++ b/components/cronet/android/test_instructions.md
@@ -77,19 +77,19 @@ #### See VLOG(1) and VLOG(2) logging: ```shell -$ adb shell setprop log.tag.CronetUrlRequestContext VERBOSE +$ adb shell setprop log.tag.chromium VERBOSE ``` #### See VLOG(1) logging: ```shell -$ adb shell setprop log.tag.CronetUrlRequestContext DEBUG +$ adb shell setprop log.tag.chromium DEBUG ``` #### See NO (only FATAL) logging: ```shell -$ adb shell setprop log.tag.CronetUrlRequestContext NONE +$ adb shell setprop log.tag.chromium NONE ``` ### Network Log