Improving logging in /app, /base, /crypto and /ipc. Updating plain DCHECK() usages for DCHECK_EQ/LE/

Improving logging in /app, /base, /crypto and /ipc. Updating plain DCHECK() usages for DCHECK_EQ/LE/GE() equivalents.

BUG=58409
TEST=None

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

[KushalP, 2011-04-24 11:48:37]

Hey guys,

I've compiled and tested these changes on my system (Mac 10.6) but would
appreciate a tryserver run for the Windows changes. Especially the NO_ERROR
stuff as I couldn't work out what type (DWORD?) it is.

Thanks,

Kush

[Peter Kasting, 2011-04-25 18:33:06]

Your entire patch needs to use "(expected, actual)" arg ordering.

[KushalP, 2011-04-25 19:12:54]

On 2011/04/25 18:33:06, Peter Kasting wrote:
> Your entire patch needs to use "(expected, actual)" arg ordering.

Okay, I'll update with a new patch set shortly.

[KushalP, 2011-04-25 19:29:50]

On 2011/04/25 19:12:54, KushalP wrote:
> On 2011/04/25 18:33:06, Peter Kasting wrote:
> > Your entire patch needs to use "(expected, actual)" arg ordering.
> 
> Okay, I'll update with a new patch set shortly.

Patch set updated. Let me know if there's anything else that I've missed.

[Peter Kasting, 2011-04-25 19:59:15]

LGTM

http://codereview.chromium.org/6880166/diff/8001/ipc/ipc_sync_message_unittes...
File ipc/ipc_sync_message_unittest.cc (right):

http://codereview.chromium.org/6880166/diff/8001/ipc/ipc_sync_message_unittes...
ipc/ipc_sync_message_unittest.cc:59: DCHECK(in1 == 1 && !in2);
Nit: While you're here, you might want to break these sorts of statements into
multiple DCHECKs, one per condition.

[KushalP, 2011-04-25 20:10:06]

http://codereview.chromium.org/6880166/diff/8001/ipc/ipc_sync_message_unittes...
File ipc/ipc_sync_message_unittest.cc (right):

http://codereview.chromium.org/6880166/diff/8001/ipc/ipc_sync_message_unittes...
ipc/ipc_sync_message_unittest.cc:59: DCHECK(in1 == 1 && !in2);
On 2011/04/25 19:59:15, Peter Kasting wrote:
> Nit: While you're here, you might want to break these sorts of statements into
> multiple DCHECKs, one per condition.

Should I am to do that wherever I see it? This is a pretty common usage pattern
for DCHECK that I've spotted and I wasn't sure whether the combination was
specific or not.

[KushalP, 2011-04-25 20:36:00]

I've pushed a new patch set. I've separated out the DCHECK() usages into better
versions for each case.

I've also changed the order slightly to improve readability of the code. This,
however, decreases the readability of the diff for you guys (sorry).

[Peter Kasting, 2011-04-26 01:31:01]

http://codereview.chromium.org/6880166/diff/14/ipc/ipc_sync_message_unittest.cc
File ipc/ipc_sync_message_unittest.cc (right):

http://codereview.chromium.org/6880166/diff/14/ipc/ipc_sync_message_unittest....
ipc/ipc_sync_message_unittest.cc:60: DCHECK_EQ(1, in1);
I don't understand why you changed the order of the conditions.  It's not more
readable this way.  Please check them in order.

[KushalP, 2011-04-26 09:28:04]

Reverted the order of the conditions. It's now back to the same order.

http://codereview.chromium.org/6880166/diff/14/ipc/ipc_sync_message_unittest.cc
File ipc/ipc_sync_message_unittest.cc (right):

http://codereview.chromium.org/6880166/diff/14/ipc/ipc_sync_message_unittest....
ipc/ipc_sync_message_unittest.cc:60: DCHECK_EQ(1, in1);
On 2011/04/26 01:31:01, Peter Kasting wrote:
> I don't understand why you changed the order of the conditions.  It's not more
> readable this way.  Please check them in order.

Done.

[Peter Kasting, 2011-04-26 18:23:33]

LGTM

[KushalP, 2011-04-26 18:58:14]

On 2011/04/26 18:23:33, Peter Kasting wrote:
> LGTM

Great!

Quick question: Are my descriptions/messages enough for a commit? If not, how
should I improve them?

[tfarina, 2011-04-26 19:00:54]

On 2011/04/26 18:58:14, KushalP wrote:
> On 2011/04/26 18:23:33, Peter Kasting wrote:
> > LGTM
> 
> Great!
> 
> Quick question: Are my descriptions/messages enough for a commit? If not, how
> should I improve them?

I think you should try to be descriptive. Explaining what you are trying to fix
and why.

[KushalP, 2011-04-26 19:07:29]

Would a better description be something like:

Improving logging in /app, /base, /crypto and /ipc. Updating plain DCHECK()
usages for more expressive macro equivalents.

[Peter Kasting, 2011-04-26 19:08:23]

On 2011/04/26 18:58:14, KushalP wrote:
> Quick question: Are my descriptions/messages enough for a commit?

The patch is trivial enough that this is fine.  "Use DHCECK_EQ/NE() where
possible" might be better.

[tfarina, 2011-04-26 19:09:04]

On 2011/04/26 19:07:29, KushalP wrote:
> Would a better description be something like:
> 
> Improving logging in /app, /base, /crypto and /ipc. Updating plain DCHECK()
> usages for more expressive macro equivalents.

Yes, looks much better.

[tfarina, 2011-04-26 19:10:30]

On 2011/04/26 19:09:04, tfarina wrote:
> On 2011/04/26 19:07:29, KushalP wrote:
> > Would a better description be something like:
> > 
> > Improving logging in /app, /base, /crypto and /ipc. Updating plain DCHECK()
> > usages for more expressive macro equivalents.
> 
> Yes, looks much better.

Or what Peter suggested. I'm fine either way :)

[KushalP, 2011-04-26 19:16:43]

Okay. I've updated the description here accordingly.

[commit-bot: I haz the power, 2011-04-26 19:25:21]

Presubmit check for 6880166-6014 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
crypto/cssm_init.cc,crypto/signature_verifier_mac.cc,crypto/signature_verifier_win.cc,crypto/signature_creator_mac.cc,base/at_exit.cc,crypto/signature_verifier_nss.cc

Presubmit checks took 2.0s to calculate.

[KushalP, 2011-04-26 19:26:48]

Should I CC one of these individuals accordingly?

[Peter Kasting, 2011-04-26 19:28:58]

On 2011/04/26 19:26:48, KushalP wrote:
> Should I CC one of these individuals accordingly?

Add an appropriate owner as a direct reviewer.

[willchan no longer on Chromium, 2011-04-26 19:31:47]

LGTM for base/, you will need someone for crypto/

On Tue, Apr 26, 2011 at 9:26 PM, <kushi.p@gmail.com> wrote:

> Should I CC one of these individuals accordingly?
>
>
> http://codereview.chromium.org/6880166/
>

[Ryan Sleevi, 2011-04-27 03:23:22]

LGTM for crypto. Note that when you add a reviewer, you may wish to "Publish +
Mail Comments" (as opposed to "Edit Issue"), or to send a message after editing,
as otherwise no e-mail will get sent out. I made the same mistake my first few
reviews and wondered why it took so long to hear back from people.

[KushalP, 2011-04-27 09:04:27]

Didn't realise that. Thanks for letting me know!

[commit-bot: I haz the power, 2011-04-28 14:38:35]

Try job failure for 6880166-6014 on win:
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[KushalP, 2011-04-28 15:01:54]

On 2011/04/28 14:38:35, commit-bot wrote:
> Try job failure for 6880166-6014 on win:
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

There was a signed/unsigned type mismatch. Thakis suggested it was a MS header
bug and to static cast to ALG_ID.

[commit-bot: I haz the power, 2011-04-28 15:12:22]

Try job failure for 6880166-10003 on win:
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[KushalP, 2011-04-28 15:28:54]

On 2011/04/28 15:12:22, commit-bot wrote:
> Try job failure for 6880166-10003 on win:
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

Static casting to DWORD to fix type mismatch

[commit-bot: I haz the power, 2011-04-28 17:13:14]

Try job failure for 6880166-8004 on win:
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[KushalP, 2011-04-28 17:20:39]

On 2011/04/28 17:13:14, commit-bot wrote:
> Try job failure for 6880166-8004 on win:
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

Submitted a patchset for the type mismatch on windows.

[commit-bot: I haz the power, 2011-04-28 18:20:10]

Change committed as 83362