Enable debugging code for non-official Android builds.

Enable debugging code for non-official Android builds.

This enables DCHECK, NOTREACHED, and other debugging code on Android
non-official release builds. Since all Android builds are done in
release mode, we don't set NDEBUG according to whether the build is in
release or debug mode. Rather we set NDEBUG for official builds only.

This requires a change to skia.gyp to define SK_RELEASE exactly the
same when building skia and its direct dependencies:

SK_RELEASE should be defined in skia.gyp only when use_system_skia is
set to 1 because system skia is always in release mode. When building
with chromium version of skia (which is what we do now) this should
not be defined and left to the skia headers to figure out whether to
include debug code or not (they define SK_RELEASE only if NDEBUG was
defined).

BUG=http://b/6712716


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=145789

[newt (away), 2012-07-02 19:00:26]

I've added reviewers for the following areas.  Let me know if anyone else would
be more appropriate.
mark -- common.gypi
Nilesh -- gpu
gman -- gpu (rubberstamp)
senorblanco -- skia

[Mark Mentovai, 2012-07-02 19:03:03]

> Since all Android builds are done in 
> release mode, we don't set NDEBUG according to whether the build is in 
> release or debug mode. Rather we set NDEBUG for official builds only. 

Maybe you should start using debug mode for this instead of introducing another
odd permutation on build configurations. It’s fine with me if you want debug
mode on Android to mean the same thing release mode does but without NDEBUG set.

[Stephen White, 2012-07-02 19:56:10]

On 2012/07/02 19:00:26, newt wrote:
> I've added reviewers for the following areas.  Let me know if anyone else
would
> be more appropriate.
> mark -- common.gypi
> Nilesh -- gpu
> gman -- gpu (rubberstamp)
> senorblanco -- skia

I agree w/Mark about this being a confusing configuration, but LGTM for the skia
stuff.

[greggman, 2012-07-02 23:55:23]

lgtm

[newt (away), 2012-07-03 01:03:46]

> Maybe you should start using debug mode for this instead of introducing
another
> odd permutation on build configurations. It’s fine with me if you want debug
> mode on Android to mean the same thing release mode does but without NDEBUG
set.

Let me be sure I understand what you're saying: that for Android we define a
configuration named 'Debug' that inherits_from 'Release_Base' and undefines
NDEBUG?  Or that Debug inherits_from from Debug_Base but customizes the defines,
etc. to act like Release mode?

One benefit of the current method is that we still have a Debug configuration,
which has been used in the past with the emulator and might be useful in the
future.  I don't want to make it impossible to build an actual debug build with
symbols.  Also, having Debug inherit from Release_Base introduces confusion in
other places.

If your main concern is that the release_assertions variable spreads out and
obfuscates the logic behind removing NDEBUG, I could remove the variable and
simply remove NDEBUG if 'OS=="Android" and buildtype=="Official"'.  If the
concern is that this CL introduces another build level (debug,
release+assertions, release official), that seems harder to avoid as there are
uses for all of those configurations.

Let me know what you're thinking.  Thanks!

[Mark Mentovai, 2012-07-03 14:15:20]

newt@chromium.org wrote:

> If your main concern is that the release_assertions variable spreads out
> and
> obfuscates the logic behind removing NDEBUG, I could remove the variable
> and
> simply remove NDEBUG if 'OS=="Android" and buildtype=="Official"'.


Yes. Do this, then.

[newt (away), 2012-07-03 22:54:25]

I changed common.gypi per your suggestion, mark.  Everything else is the same
(though I rebased).

[Mark Mentovai, 2012-07-04 01:59:51]

LGTM

http://codereview.chromium.org/10695047/diff/1005/skia/skia.gyp
File skia/skia.gyp (right):

http://codereview.chromium.org/10695047/diff/1005/skia/skia.gyp#newcode1092
skia/skia.gyp:1092: 'SK_BUILD_FOR_ANDROID_NDK',  # Don't use non-NDK available
stuff.
I’d put this comment on its own line to keep this within 80 characters.

[nilesh, 2012-07-09 15:08:36]

On 2012/07/04 01:59:51, Mark Mentovai wrote:
> LGTM
> 
> http://codereview.chromium.org/10695047/diff/1005/skia/skia.gyp
> File skia/skia.gyp (right):
> 
> http://codereview.chromium.org/10695047/diff/1005/skia/skia.gyp#newcode1092
> skia/skia.gyp:1092: 'SK_BUILD_FOR_ANDROID_NDK',  # Don't use non-NDK available
> stuff.
> I’d put this comment on its own line to keep this within 80 characters.

Please try the CL on android_test trybot too.

[commit-bot: I haz the power, 2012-07-09 22:20:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/newt@chromium.org/10695047/19001

[newt (away), 2012-07-09 22:20:12]

http://codereview.chromium.org/10695047/diff/1005/skia/skia.gyp
File skia/skia.gyp (right):

http://codereview.chromium.org/10695047/diff/1005/skia/skia.gyp#newcode1092
skia/skia.gyp:1092: 'SK_BUILD_FOR_ANDROID_NDK',  # Don't use non-NDK available
stuff.
On 2012/07/04 01:59:51, Mark Mentovai wrote:
> I’d put this comment on its own line to keep this within 80 characters.

Done.

[commit-bot: I haz the power, 2012-07-09 23:37:14]

Change committed as 145789

[newt (away), 2012-07-09 23:49:09]

Thanks for reviewing, Mark, Stephen, Greg, and Nilesh!