BuildConfig.java uses C++ style constants
Reported by
dolo...@amazon.com,
May 26 2016
|
||||
Issue descriptionThis bug is concerned with //base/android/java/templates/BuildConfig.template which gets compiled into org.chromium.base.BuildConfig.java using the C++ preprocessor There is a function `isMultidexEnabled` and a constant `sIsDebug` in this file that exposes C++ build flags to Java. There are two problems with this file. 1) There is an inconsistent way of exposing these flags to Java. They should either both be functions or both by constants. 2) `sIsDebug` is a C++ style static variable and should be `IS_DEBUG` to match Java conventions
,
Jun 3 2016
,
Jun 6 2016
Assigning to Sam as he's working on BuildConfig.
,
Jun 8 2016
Note: sIsDebug should probably just be "DEBUG", to align with Gradle-genearted BuildConfig.java files.
,
Jun 10 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/6174a5f8b59fcfd3dbc7273faa27851cea6c9467 commit 6174a5f8b59fcfd3dbc7273faa27851cea6c9467 Author: smaier <smaier@chromium.org> Date: Fri Jun 10 13:33:02 2016 Adding option for Proguard jar to be switched. Additionally, adds the is_java_debug gn arg, which can now be set separately from is_debug, which will control Java specific debug features, such as Proguard, multidexing, and incremental dexing. BUG= 616831 , 615082 , 615083 Review-Url: https://codereview.chromium.org/2031033002 Cr-Commit-Position: refs/heads/master@{#399172} [modify] https://crrev.com/6174a5f8b59fcfd3dbc7273faa27851cea6c9467/android_webview/system_webview_apk_tmpl.gni [modify] https://crrev.com/6174a5f8b59fcfd3dbc7273faa27851cea6c9467/base/BUILD.gn [modify] https://crrev.com/6174a5f8b59fcfd3dbc7273faa27851cea6c9467/base/android/java/templates/BuildConfig.template [modify] https://crrev.com/6174a5f8b59fcfd3dbc7273faa27851cea6c9467/build/config/android/config.gni [modify] https://crrev.com/6174a5f8b59fcfd3dbc7273faa27851cea6c9467/build/config/android/internal_rules.gni [modify] https://crrev.com/6174a5f8b59fcfd3dbc7273faa27851cea6c9467/build/config/android/rules.gni [modify] https://crrev.com/6174a5f8b59fcfd3dbc7273faa27851cea6c9467/chrome/android/BUILD.gn [modify] https://crrev.com/6174a5f8b59fcfd3dbc7273faa27851cea6c9467/chrome/android/chrome_public_apk_tmpl.gni [modify] https://crrev.com/6174a5f8b59fcfd3dbc7273faa27851cea6c9467/chrome/android/java/src/org/chromium/chrome/browser/ChromeStrictMode.java [modify] https://crrev.com/6174a5f8b59fcfd3dbc7273faa27851cea6c9467/components/cronet/android/BUILD.gn
,
Jun 13 2016
I don't think that 1) is a huge deal, especially since in BuildConfig.template we have pretty explicit comments describing why we are not doing 1). If it starts to cause issues, we can look at it again. If we do decide to do 1), I think it should probably be switching them both to functions, since there are some issues with constant variables (namely, FindBugs taking issue with them cutting off branches, and the fact that they get precomputed into the code before we can change the value by another step in the build process). However, if we do switch the constant over to a function, we need to find a way to get the function optimized out. As it stands right now, it appears it does NOT get optimized out with our current build setup. 2) is fixed.
,
Jun 15 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/6174a5f8b59fcfd3dbc7273faa27851cea6c9467 commit 6174a5f8b59fcfd3dbc7273faa27851cea6c9467 Author: smaier <smaier@chromium.org> Date: Fri Jun 10 13:33:02 2016 Adding option for Proguard jar to be switched. Additionally, adds the is_java_debug gn arg, which can now be set separately from is_debug, which will control Java specific debug features, such as Proguard, multidexing, and incremental dexing. BUG= 616831 , 615082 , 615083 Review-Url: https://codereview.chromium.org/2031033002 Cr-Commit-Position: refs/heads/master@{#399172} [modify] https://crrev.com/6174a5f8b59fcfd3dbc7273faa27851cea6c9467/android_webview/system_webview_apk_tmpl.gni [modify] https://crrev.com/6174a5f8b59fcfd3dbc7273faa27851cea6c9467/base/BUILD.gn [modify] https://crrev.com/6174a5f8b59fcfd3dbc7273faa27851cea6c9467/base/android/java/templates/BuildConfig.template [modify] https://crrev.com/6174a5f8b59fcfd3dbc7273faa27851cea6c9467/build/config/android/config.gni [modify] https://crrev.com/6174a5f8b59fcfd3dbc7273faa27851cea6c9467/build/config/android/internal_rules.gni [modify] https://crrev.com/6174a5f8b59fcfd3dbc7273faa27851cea6c9467/build/config/android/rules.gni [modify] https://crrev.com/6174a5f8b59fcfd3dbc7273faa27851cea6c9467/chrome/android/BUILD.gn [modify] https://crrev.com/6174a5f8b59fcfd3dbc7273faa27851cea6c9467/chrome/android/chrome_public_apk_tmpl.gni [modify] https://crrev.com/6174a5f8b59fcfd3dbc7273faa27851cea6c9467/chrome/android/java/src/org/chromium/chrome/browser/ChromeStrictMode.java [modify] https://crrev.com/6174a5f8b59fcfd3dbc7273faa27851cea6c9467/components/cronet/android/BUILD.gn |
||||
►
Sign in to add a comment |
||||
Comment 1 by rsgav...@chromium.org
, Jun 3 2016