views: Fix a bunch of dchecks.

views: Fix a bunch of dchecks.

We should use DCHECK_EQ whenever possible.

R=pkasting@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=162413

[tfarina, 2012-10-13 23:14:17]

Hi Peter, can you check if I did everything right?

Thanks.

[Peter Kasting, 2012-10-14 00:54:35]

LGTM

[akalin, 2012-10-14 09:30:11]

http://codereview.chromium.org/11148007/diff/4001/chrome/browser/ui/views/boo...
File chrome/browser/ui/views/bookmarks/bookmark_context_menu_controller_views.cc
(right):

http://codereview.chromium.org/11148007/diff/4001/chrome/browser/ui/views/boo...
chrome/browser/ui/views/bookmarks/bookmark_context_menu_controller_views.cc:187:
DCHECK_EQ(IDC_BOOKMARK_BAR_ALWAYS_SHOW, id);
please preserve the order of the arguments (e.g., 'id' should go on the left)

here and everywhere else

[tfarina, 2012-10-14 22:26:17]

http://codereview.chromium.org/11148007/diff/4001/chrome/browser/ui/views/boo...
File chrome/browser/ui/views/bookmarks/bookmark_context_menu_controller_views.cc
(right):

http://codereview.chromium.org/11148007/diff/4001/chrome/browser/ui/views/boo...
chrome/browser/ui/views/bookmarks/bookmark_context_menu_controller_views.cc:187:
DCHECK_EQ(IDC_BOOKMARK_BAR_ALWAYS_SHOW, id);
On 2012/10/14 09:30:11, akalin wrote:
> please preserve the order of the arguments (e.g., 'id' should go on the left)
> 
> here and everywhere else
No, unless I'm wrong, Peter advocates that it's DCHECK_EQ(expected, actual). So
I read constants like IDC_BOOKMARK_BAR_ALWAYS_SHOW as expected, and variables as
actual.

[akalin, 2012-10-14 22:29:15]

On 2012/10/14 22:26:17, tfarina wrote:
>
http://codereview.chromium.org/11148007/diff/4001/chrome/browser/ui/views/boo...
> File
chrome/browser/ui/views/bookmarks/bookmark_context_menu_controller_views.cc
> (right):
> 
>
http://codereview.chromium.org/11148007/diff/4001/chrome/browser/ui/views/boo...
>
chrome/browser/ui/views/bookmarks/bookmark_context_menu_controller_views.cc:187:
> DCHECK_EQ(IDC_BOOKMARK_BAR_ALWAYS_SHOW, id);
> On 2012/10/14 09:30:11, akalin wrote:
> > please preserve the order of the arguments (e.g., 'id' should go on the
left)
> > 
> > here and everywhere else
> No, unless I'm wrong, Peter advocates that it's DCHECK_EQ(expected, actual).
So
> I read constants like IDC_BOOKMARK_BAR_ALWAYS_SHOW as expected, and variables
as
> actual.

Can you or Peter explain why?

Unlike gtest macros (EXPECT_EQ/ASSERT_EQ) there's no distinction between the
arguments of DCHECK_EQ, so there's no reason to put the constant first, just as
there is no reason to put the constant first for an equality comparison in an if
statement.

[Peter Kasting, 2012-10-15 17:42:47]

On 2012/10/14 22:29:15, akalin wrote:
> Unlike gtest macros (EXPECT_EQ/ASSERT_EQ) there's no distinction between the
> arguments of DCHECK_EQ, so there's no reason to put the constant first, just
as
> there is no reason to put the constant first for an equality comparison in an
if
> statement.

That's technically true.  However, asking people to do one thing for DCHECK_EQ
and a different one for ASSERT_EQ has always been considered so confusing that
we have always requested that people do (expected, actual) order in
[D]CHECK_{EQ,NE} for consistency with {ASSERT,EXPECT}_{EQ,NE}.

There may have also been historical reason for this, viz., when we started
Chrome I think the macros maybe did care about order more than they do now.

In any case, (expected,actual) is the general convention across Chrome for EQ
and NE forms.

[akalin, 2012-10-15 17:44:54]

On 2012/10/15 17:42:47, Peter Kasting wrote:
> In any case, (expected,actual) is the general convention across Chrome for EQ
> and NE forms.

News to me!  I've seen both forms in roughly equal forms.  But I guess that
shouldn't block this CL.

[Peter Kasting, 2012-10-15 17:46:37]

On 2012/10/15 17:44:54, akalin wrote:
> On 2012/10/15 17:42:47, Peter Kasting wrote:
> > In any case, (expected,actual) is the general convention across Chrome for
EQ
> > and NE forms.
> 
> News to me!  I've seen both forms in roughly equal forms.  But I guess that
> shouldn't block this CL.

In fairness, it's not important enough to be in the style guide -- even the
older, longer version -- and I certainly wouldn't bother writing changes just to
address this.