Add a gyp flag to enable dcheck by default in release without

Add a gyp flag to enable dcheck by default in release without
having the pass a flag. This will be used on the try bots.

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

[M-A Ruel, 2011-08-23 20:04:51]

lgtm, let's run that on all the try builder configurations.

[nsylvain, 2011-08-23 20:32:49]

also adding rick

[rvargas (doing something else), 2011-08-23 20:48:22]

http://codereview.chromium.org/7719007/diff/2001/chrome/common/logging_chrome.cc
File chrome/common/logging_chrome.cc (right):

http://codereview.chromium.org/7719007/diff/2001/chrome/common/logging_chrome...
chrome/common/logging_chrome.cc:385:
command_line.HasSwitch(switches::kEnableDCHECK)) {
Looks like we need another condition here :(

[nsylvain, 2011-08-23 21:15:54]

i moved the check to a better place. ptal

[rvargas (doing something else), 2011-08-23 22:03:59]

http://codereview.chromium.org/7719007/diff/3005/base/logging.h
File base/logging.h (right):

http://codereview.chromium.org/7719007/diff/3005/base/logging.h#newcode630
base/logging.h:630: #else // defined(DCHECK_ALWAYS_ON)
nit: I think the comment adds more noise than value.

http://codereview.chromium.org/7719007/diff/3005/base/logging.h#newcode634
base/logging.h:634: LOG_IS_ON(DCHECK))
this line looks broken to me (before this CL). If it works, it should be part of
the #if.

http://codereview.chromium.org/7719007/diff/3005/base/logging.h#newcode635
base/logging.h:635: #endif // defined(DCHECK_ALWAYS_ON)
nit: two spaces before comments.

[nsylvain, 2011-08-23 22:21:42]

updated

[rvargas (doing something else), 2011-08-23 22:28:04]

LGTM, but we should figure out what is LOG_IS_ON about.

[nsylvain, 2011-08-24 00:04:00]

I submitted a try job on the release try bots with the gyp file hardcoded to
have this code to true. One test failed. Fixed has been uploaded here.

[rvargas (doing something else), 2011-08-24 00:49:37]

LGTM

http://codereview.chromium.org/7719007/diff/6004/base/logging_unittest.cc
File base/logging_unittest.cc (right):

http://codereview.chromium.org/7719007/diff/6004/base/logging_unittest.cc#new...
base/logging_unittest.cc:209: #endif  // !defined(LOGGING_IS_OFFICIAL_BUILD) &&
defined(NDEBUG) && \
nit: personally, I'd remove this comment.

[nsylvain, 2011-08-27 22:42:29]

Another update for this never ending patch. Now all tests running on the try
bots are passing on all 3 platforms.

[nsylvain, 2011-08-27 22:53:23]

On 2011/08/27 22:42:29, nsylvain wrote:
> Another update for this never ending patch. Now all tests running on the try
> bots are passing on all 3 platforms.

except mac, because mac hates me.  I'm getting 
FATAL:pickle.cc(105)] Check failed: 0 == tmp || 1 == tmp. 

And I have no clue how to debug. This seems easy to fix though.  Tempted to just
disable this DCHECK ... but it's not likely the right thing to do.

[M-A Ruel, 2011-08-29 14:44:57]

I'd prefer a DCHECK_ENABLED define. It'd require a lower cognitive overhead and
would be unambiguous.

wdyt?

http://codereview.chromium.org/7719007/diff/9001/base/logging.h
File base/logging.h (right):

http://codereview.chromium.org/7719007/diff/9001/base/logging.h#newcode622
base/logging.h:622: BASE_EXPORT extern DcheckState g_dcheck_state;
Oh, that's sad. :( I think it shouldn't exist in official builds.

[nsylvain, 2011-08-29 17:12:40]

there is already a ENABLE_DCHECK define, which means the DCHECK are compiled in.
 If i have a DCHECK_ENABLED this will just confuse things.

http://codereview.chromium.org/7719007/diff/9001/base/logging.h
File base/logging.h (right):

http://codereview.chromium.org/7719007/diff/9001/base/logging.h#newcode622
base/logging.h:622: BASE_EXPORT extern DcheckState g_dcheck_state;
On 2011/08/29 14:44:57, Marc-Antoine Ruel wrote:
> Oh, that's sad. :( I think it shouldn't exist in official builds.
i doubt it's used in official builds.

[rvargas (doing something else), 2011-08-29 18:25:22]

Each time I look at that file it seems messier than before :(.

But again, not your fault. Still LGTM.

[M-A Ruel, 2011-08-29 18:36:52]

lgtm

[commit-bot: I haz the power, 2011-09-20 16:37:21]

Presubmit check for 7719007-32001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
If this change has an associated bug, add BUG=[bug number].

If this change requires manual test instructions to QA team, add
TEST=[instructions].

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
content/browser/renderer_host/render_view_host_unittest.cc,base/file_util_unittest.cc,chrome/browser/sync/syncable/directory_backing_store_unittest.cc,base/logging_unittest.cc,base/logging.h

Presubmit checks took 2.5s to calculate.

[nsylvain, 2011-09-20 17:02:14]

Adding more reviewers (avi) to make the OWNERS file in content/browser happy.

Diff for that file :
http://codereview.chromium.org/7719007/diff/32001/content/browser/renderer_ho...

[nsylvain, 2011-09-20 17:20:14]

Yes one more reviewer (willchan) for Base/.

[willchan no longer on Chromium, 2011-09-20 18:09:08]

On 2011/09/20 17:20:14, nsylvain wrote:
> Yes one more reviewer (willchan) for Base/.

LGTM