Adding option for Proguard jar to be switched.

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
Committed: https://crrev.com/6174a5f8b59fcfd3dbc7273faa27851cea6c9467
Cr-Commit-Position: refs/heads/master@{#399172}

[smaier, 2016-06-03 13:26:30]

The CQ bit was checked by smaier@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-03 13:26:42]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2031033002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2031033002/20001

[smaier, 2016-06-06 15:25:03]

The CQ bit was checked by smaier@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-06 15:25:16]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2031033002/60001

[commit-bot: I haz the power, 2016-06-06 16:39:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-06 16:39:36]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[smaier, 2016-06-06 20:12:58]

Description was changed from

==========
Adding option for Proguard jar to be switched.

BUG=616831
==========

to

==========
Adding option for Proguard jar to be switched.

BUG=616831,615082,615083
==========

[smaier, 2016-06-06 20:12:58]

smaier@chromium.org changed reviewers:
+ wnwen@chromium.org

[Peter Wen, 2016-06-06 20:28:16]

Looks nice. I'm assuming the rest of the change will be in the internal repo.
Let me know if my comment isn't clear.

https://codereview.chromium.org/2031033002/diff/80001/build/config/android/co...
File build/config/android/config.gni (right):

https://codereview.chromium.org/2031033002/diff/80001/build/config/android/co...
build/config/android/config.gni:94: is_java_debug = is_debug
Can you explain why we want is_java_debug?

Right now most developers have is_debug explicitly set to true or false, this
line seems to make the default value of is_java_debug equal to is_debug. If the
user did not explicitly set is_java_debug in gn args, does it follow the
explicitly set value of is_debug? i.e. will this cause strange behaviour when
is_debug is explicitly set to false, and is_java_debug ends up being true?

[smaier, 2016-06-06 21:04:58]

https://codereview.chromium.org/2031033002/diff/80001/build/config/android/co...
File build/config/android/config.gni (right):

https://codereview.chromium.org/2031033002/diff/80001/build/config/android/co...
build/config/android/config.gni:94: is_java_debug = is_debug
On 2016/06/06 20:28:16, Peter Wen wrote:
> Can you explain why we want is_java_debug?
> 
> Right now most developers have is_debug explicitly set to true or false, this
> line seems to make the default value of is_java_debug equal to is_debug. If
the
> user did not explicitly set is_java_debug in gn args, does it follow the
> explicitly set value of is_debug? i.e. will this cause strange behaviour when
> is_debug is explicitly set to false, and is_java_debug ends up being true?

The current behaviour would continue as normal for those that did not set
is_java_debug. I am creating this to allow us to do non-debug Java features (eg.
Proguard) while still building a typical debug build.

[Peter Wen, 2016-06-06 21:45:22]

wnwen@chromium.org changed reviewers:
+ pkotwicz@chromium.org

[Peter Wen, 2016-06-06 21:45:23]

@pkotwicz - Could you check my understanding of how setting the new gn arg
"is_java_debug = is_debug" works?

My understanding is that our custom explicit gn args only gets processed after
all these variables are set and initialized, so setting "is_java_debug =
is_debug" is the same as setting "is_java_debug = true", since is_debug is true
by default. So explicitly setting is_debug = false in gn args will not change
the value of is_java_debug.

https://codereview.chromium.org/2031033002/diff/80001/base/BUILD.gn
File base/BUILD.gn (right):

https://codereview.chromium.org/2031033002/diff/80001/base/BUILD.gn#newcode2377
base/BUILD.gn:2377: if (is_java_debug) {
Guarding this on is_debug is more intuitive, as if I set is_debug = false, I
don't think I'd ever expect BuildConfig.IS_DEBUG to be true.

https://codereview.chromium.org/2031033002/diff/80001/build/config/android/co...
File build/config/android/config.gni (right):

https://codereview.chromium.org/2031033002/diff/80001/build/config/android/co...
build/config/android/config.gni:94: is_java_debug = is_debug
On 2016/06/06 21:04:58, smaier wrote:
> On 2016/06/06 20:28:16, Peter Wen wrote:
> > Can you explain why we want is_java_debug?
> > 
> > Right now most developers have is_debug explicitly set to true or false,
this
> > line seems to make the default value of is_java_debug equal to is_debug. If
> the
> > user did not explicitly set is_java_debug in gn args, does it follow the
> > explicitly set value of is_debug? i.e. will this cause strange behaviour
when
> > is_debug is explicitly set to false, and is_java_debug ends up being true?
> 
> The current behaviour would continue as normal for those that did not set
> is_java_debug. I am creating this to allow us to do non-debug Java features
(eg.
> Proguard) while still building a typical debug build.

I see. I feel that setting is_debug to false in gn will not result in
is_java_debug being set to false. That's not a bad thing, just we'll need to
communicate to the team that when they want a release proguarded build, they
need to explicitly set this new gn arg. If that's the case, something like
"run_proguard" may be a better name.

Adding pkotwicz to confirm my understanding.

https://codereview.chromium.org/2031033002/diff/80001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ChromeStrictMode.java
(right):

https://codereview.chromium.org/2031033002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ChromeStrictMode.java:162:
@SuppressFBWarnings("UCF_USELESS_CONTROL_FLOW_NEXT_LINE")
You'll need to organize imports if you're using Eclipse, and add it yourself
otherwise.

[pkotwicz, 2016-06-06 22:01:08]

On 2016/06/06 21:45:23, Peter Wen wrote:
> @pkotwicz - Could you check my understanding of how setting the new gn arg
> "is_java_debug = is_debug" works?
> 
> My understanding is that our custom explicit gn args only gets processed after
> all these variables are set and initialized, so setting "is_java_debug =
> is_debug" is the same as setting "is_java_debug = true", since is_debug is
true
> by default. So explicitly setting is_debug = false in gn args will not change
> the value of is_java_debug.
> 
> https://codereview.chromium.org/2031033002/diff/80001/base/BUILD.gn
> File base/BUILD.gn (right):
> 
>
https://codereview.chromium.org/2031033002/diff/80001/base/BUILD.gn#newcode2377
> base/BUILD.gn:2377: if (is_java_debug) {
> Guarding this on is_debug is more intuitive, as if I set is_debug = false, I
> don't think I'd ever expect BuildConfig.IS_DEBUG to be true.
> 
>
https://codereview.chromium.org/2031033002/diff/80001/build/config/android/co...
> File build/config/android/config.gni (right):
> 
>
https://codereview.chromium.org/2031033002/diff/80001/build/config/android/co...
> build/config/android/config.gni:94: is_java_debug = is_debug
> On 2016/06/06 21:04:58, smaier wrote:
> > On 2016/06/06 20:28:16, Peter Wen wrote:
> > > Can you explain why we want is_java_debug?
> > > 
> > > Right now most developers have is_debug explicitly set to true or false,
> this
> > > line seems to make the default value of is_java_debug equal to is_debug.
If
> > the
> > > user did not explicitly set is_java_debug in gn args, does it follow the
> > > explicitly set value of is_debug? i.e. will this cause strange behaviour
> when
> > > is_debug is explicitly set to false, and is_java_debug ends up being true?
> > 
> > The current behaviour would continue as normal for those that did not set
> > is_java_debug. I am creating this to allow us to do non-debug Java features
> (eg.
> > Proguard) while still building a typical debug build.
> 
> I see. I feel that setting is_debug to false in gn will not result in
> is_java_debug being set to false. That's not a bad thing, just we'll need to
> communicate to the team that when they want a release proguarded build, they
> need to explicitly set this new gn arg. If that's the case, something like
> "run_proguard" may be a better name.
> 
> Adding pkotwicz to confirm my understanding.
> 
>
https://codereview.chromium.org/2031033002/diff/80001/chrome/android/java/src...
> File chrome/android/java/src/org/chromium/chrome/browser/ChromeStrictMode.java
> (right):
> 
>
https://codereview.chromium.org/2031033002/diff/80001/chrome/android/java/src...
> chrome/android/java/src/org/chromium/chrome/browser/ChromeStrictMode.java:162:
> @SuppressFBWarnings("UCF_USELESS_CONTROL_FLOW_NEXT_LINE")
> You'll need to organize imports if you're using Eclipse, and add it yourself
> otherwise.

[pkotwicz, 2016-06-06 22:02:42]

I tried it and setting is_debug = false also sets is_java_debug to false. It
makes sense that this would work but I do not know why it works
+agrieve for more details

[smaier, 2016-06-07 18:20:28]

The CQ bit was checked by smaier@chromium.org to run a CQ dry run

[smaier, 2016-06-07 18:20:44]

smaier@chromium.org changed reviewers:
+ agrieve@chromium.org

[smaier, 2016-06-07 18:20:45]

https://codereview.chromium.org/2031033002/diff/80001/base/BUILD.gn
File base/BUILD.gn (right):

https://codereview.chromium.org/2031033002/diff/80001/base/BUILD.gn#newcode2377
base/BUILD.gn:2377: if (is_java_debug) {
On 2016/06/06 21:45:23, Peter Wen wrote:
> Guarding this on is_debug is more intuitive, as if I set is_debug = false, I
> don't think I'd ever expect BuildConfig.IS_DEBUG to be true.

This would only be the case if you explicitly set is_java_debug to true, in
which case I think you would expect BuildConfig.IS_DEBUG (a Java constant) to be
true. Perhaps IS_DEBUG should be changed to IS_JAVA_DEBUG for more clarity in
the build files? However, that seems a bit obtuse once you're in a Java file and
having to check for IS_JAVA_DEBUG.

https://codereview.chromium.org/2031033002/diff/80001/build/config/android/co...
File build/config/android/config.gni (right):

https://codereview.chromium.org/2031033002/diff/80001/build/config/android/co...
build/config/android/config.gni:94: is_java_debug = is_debug
On 2016/06/06 21:45:23, Peter Wen wrote:
> On 2016/06/06 21:04:58, smaier wrote:
> > On 2016/06/06 20:28:16, Peter Wen wrote:
> > > Can you explain why we want is_java_debug?
> > > 
> > > Right now most developers have is_debug explicitly set to true or false,
> this
> > > line seems to make the default value of is_java_debug equal to is_debug.
If
> > the
> > > user did not explicitly set is_java_debug in gn args, does it follow the
> > > explicitly set value of is_debug? i.e. will this cause strange behaviour
> when
> > > is_debug is explicitly set to false, and is_java_debug ends up being true?
> > 
> > The current behaviour would continue as normal for those that did not set
> > is_java_debug. I am creating this to allow us to do non-debug Java features
> (eg.
> > Proguard) while still building a typical debug build.
> 
> I see. I feel that setting is_debug to false in gn will not result in
> is_java_debug being set to false. That's not a bad thing, just we'll need to
> communicate to the team that when they want a release proguarded build, they
> need to explicitly set this new gn arg. If that's the case, something like
> "run_proguard" may be a better name.
> 
> Adding pkotwicz to confirm my understanding.

This is a pretty common pattern, for example line 116 is "enable_incremental_dx
= is_debug". I did try this locally and setting is_debug in gn args sets
is_java_debug.

https://codereview.chromium.org/2031033002/diff/80001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ChromeStrictMode.java
(right):

https://codereview.chromium.org/2031033002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ChromeStrictMode.java:162:
@SuppressFBWarnings("UCF_USELESS_CONTROL_FLOW_NEXT_LINE")
On 2016/06/06 21:45:23, Peter Wen wrote:
> You'll need to organize imports if you're using Eclipse, and add it yourself
> otherwise.

Done.

[commit-bot: I haz the power, 2016-06-07 18:21:22]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2031033002/100001

[Peter Wen, 2016-06-07 18:45:19]

Code lgtm so feel free to add OWNERS.

I think that is_java_debug is better named run_proguard and that everywhere else
should still use is_debug to be in sync with native debug. But this is
functionally the case as long as is_java_debug is not explicitly defined, so
I'll leave this to the other reviewers if they feel strongly about it.

Thanks Peter and Sam for verifying the behaviour. I stand corrected that my
understanding of gn args was incorrect. :)

[commit-bot: I haz the power, 2016-06-07 19:33:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-07 19:33:09]

Dry run: This issue passed the CQ dry run.

[agrieve, 2016-06-08 18:47:32]

lgtm

https://codereview.chromium.org/2031033002/diff/100001/build/config/android/c...
File build/config/android/config.gni (right):

https://codereview.chromium.org/2031033002/diff/100001/build/config/android/c...
build/config/android/config.gni:92: # Java debug on Android. Having this on
enables multidexing, and turning it
nit: Can you also mention it being wired to BuildConfig.DEBUG

https://codereview.chromium.org/2031033002/diff/100001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/ChromeStrictMode.java
(right):

https://codereview.chromium.org/2031033002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ChromeStrictMode.java:163:
@SuppressFBWarnings("UCF_USELESS_CONTROL_FLOW_NEXT_LINE")
This is annoying :(. I think it's fine for now though. I do wonder if maybe
changing it to be a function that returns true/false would make findbugs okay
with it while still allowing proguard to eliminate it. Let's leave that for
another day.

[agrieve, 2016-06-08 18:49:16]

agrieve@chromium.org changed reviewers:
+ mef@chromium.org, thakis@chromium.org, torne@chromium.org

[agrieve, 2016-06-08 18:49:17]

mef@chromium.org: Please review changes in cronet/

thakis@chromium.org: Please review changes in base/, chrome/

torne@chromium.org: Please review changes in android_webview/

[Nico, 2016-06-08 18:56:53]

https://codereview.chromium.org/2031033002/diff/100001/base/android/java/temp...
File base/android/java/templates/BuildConfig.template (right):

https://codereview.chromium.org/2031033002/diff/100001/base/android/java/temp...
base/android/java/templates/BuildConfig.template:26: #if defined(DEBUG)
why not use NDEBUG? negated constants suck, but for better or worse that's the
standard c/c++ way, and if we must preprocess java files, this seems like the
least confusing thing?

[smaier, 2016-06-08 21:43:37]

https://codereview.chromium.org/2031033002/diff/100001/base/android/java/temp...
File base/android/java/templates/BuildConfig.template (right):

https://codereview.chromium.org/2031033002/diff/100001/base/android/java/temp...
base/android/java/templates/BuildConfig.template:26: #if defined(DEBUG)
On 2016/06/08 18:56:53, Nico wrote:
> why not use NDEBUG? negated constants suck, but for better or worse that's the
> standard c/c++ way, and if we must preprocess java files, this seems like the
> least confusing thing?

Done.

https://codereview.chromium.org/2031033002/diff/100001/build/config/android/c...
File build/config/android/config.gni (right):

https://codereview.chromium.org/2031033002/diff/100001/build/config/android/c...
build/config/android/config.gni:92: # Java debug on Android. Having this on
enables multidexing, and turning it
On 2016/06/08 18:47:32, agrieve wrote:
> nit: Can you also mention it being wired to BuildConfig.DEBUG

Done.

[Torne, 2016-06-09 12:55:02]

android_webview LGTM

[Nico, 2016-06-09 14:36:22]

base, chrome lgtm

[mef, 2016-06-09 19:12:19]

components/cronet lgtm

Could you mention switch from is_debug to is_java_debug in CL description?

[smaier, 2016-06-09 19:44:59]

Description was changed from

==========
Adding option for Proguard jar to be switched.

BUG=616831,615082,615083
==========

to

==========
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
==========

[smaier, 2016-06-09 19:45:54]

The CQ bit was checked by smaier@chromium.org

[smaier, 2016-06-09 19:45:54]

The patchset sent to the CQ was uploaded after l-g-t-m from wnwen@chromium.org,
agrieve@chromium.org, torne@chromium.org, thakis@chromium.org
Link to the patchset: https://codereview.chromium.org/2031033002/#ps140001
(title: "Changing incremental dexing to use is_java_debug")

[commit-bot: I haz the power, 2016-06-09 19:46:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2031033002/140001

[commit-bot: I haz the power, 2016-06-09 19:49:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-09 19:49:33]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[smaier, 2016-06-09 22:50:35]

The CQ bit was checked by smaier@chromium.org

[smaier, 2016-06-09 22:50:36]

The patchset sent to the CQ was uploaded after l-g-t-m from wnwen@chromium.org,
torne@chromium.org, agrieve@chromium.org, thakis@chromium.org, mef@chromium.org
Link to the patchset: https://codereview.chromium.org/2031033002/#ps180001
(title: "Fixed single newline")

[commit-bot: I haz the power, 2016-06-09 22:51:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2031033002/180001

[commit-bot: I haz the power, 2016-06-10 00:53:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-10 00:53:06]

Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_TIMED_OUT, no build
URL)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL)

[smaier, 2016-06-10 13:03:45]

The CQ bit was checked by smaier@chromium.org

[commit-bot: I haz the power, 2016-06-10 13:04:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2031033002/180001

[commit-bot: I haz the power, 2016-06-10 13:33:13]

Description was changed from

==========
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
==========

to

==========
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
==========

[commit-bot: I haz the power, 2016-06-10 13:33:15]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-06-10 13:33:20]

CQ bit was unchecked

[commit-bot: I haz the power, 2016-06-10 13:34:45]

Description was changed from

==========
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
==========

to

==========
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

Committed: https://crrev.com/6174a5f8b59fcfd3dbc7273faa27851cea6c9467
Cr-Commit-Position: refs/heads/master@{#399172}
==========

[commit-bot: I haz the power, 2016-06-10 13:34:47]

Patchset 10 (id:??) landed as
https://crrev.com/6174a5f8b59fcfd3dbc7273faa27851cea6c9467
Cr-Commit-Position: refs/heads/master@{#399172}