Sync FakeServer cleanup: CHECKs and logging info

Sync FakeServer cleanup: CHECKs and logging info

This CL removes unnecessary DCHECKs, switches useful DCHECKs
to CHECKS, and adds logging statements.

BUG=NONE

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

[rlarocque, 2014-06-09 21:28:49]

I think there's a style preference written down somewhere that says we'd prefer
to not CHECK a pointer if we'd crash on it if it were NULL anyway.

ie.

CHECK(ptr);
ptr->foo();

is redundant.  The line "ptr->foo()" already implies that ptr is non-NULL.

====

I think I might actually prefer DCHECK() to CHECK().  The wiki states that
neither should be used in tests, since they can take down the test process.

In your case, I don't think this can be avoided.  Using EXPECT isn't really an
option for these classes.  DCHECK would at least not crash on the trybots,
though.

http://www.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREACHED-

https://codereview.chromium.org/327593003/diff/1/sync/test/fake_server/fake_s...
File sync/test/fake_server/fake_server_entity.cc (right):

https://codereview.chromium.org/327593003/diff/1/sync/test/fake_server/fake_s...
sync/test/fake_server/fake_server_entity.cc:90: CHECK(sync_entity);
See CL-level comment.

https://codereview.chromium.org/327593003/diff/1/sync/test/fake_server/unique...
File sync/test/fake_server/unique_client_entity.cc (right):

https://codereview.chromium.org/327593003/diff/1/sync/test/fake_server/unique...
sync/test/fake_server/unique_client_entity.cc:48: CHECK(current_server_entity !=
NULL) << "An existing server entity must be "
I think the "!= NULL" is redundant here.

[pval...(no longer on Chromium), 2014-06-09 23:29:27]

Ok, I'm onboard for no CHECKs. The style guide and past discussions on this make
sense. I've updated the CL (and description).

https://codereview.chromium.org/327593003/diff/1/sync/test/fake_server/fake_s...
File sync/test/fake_server/fake_server_entity.cc (right):

https://codereview.chromium.org/327593003/diff/1/sync/test/fake_server/fake_s...
sync/test/fake_server/fake_server_entity.cc:90: CHECK(sync_entity);
On 2014/06/09 21:28:48, rlarocque wrote:
> See CL-level comment.

sounds good. removed.

https://codereview.chromium.org/327593003/diff/1/sync/test/fake_server/unique...
File sync/test/fake_server/unique_client_entity.cc (right):

https://codereview.chromium.org/327593003/diff/1/sync/test/fake_server/unique...
sync/test/fake_server/unique_client_entity.cc:48: CHECK(current_server_entity !=
NULL) << "An existing server entity must be "
On 2014/06/09 21:28:48, rlarocque wrote:
> I think the "!= NULL" is redundant here.

removed per the top-level comment

[pval...(no longer on Chromium), 2014-06-10 00:38:15]

back to CHECKs after an offline discussion, PTAL

[rlarocque, 2014-06-10 00:54:24]

On 2014/06/10 00:38:15, pvalenzuela wrote:
> back to CHECKs after an offline discussion, PTAL

lgtm.

[pval...(no longer on Chromium), 2014-06-10 00:56:04]

The CQ bit was checked by pvalenzuela@chromium.org

[commit-bot: I haz the power, 2014-06-10 00:57:06]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/pvalenzuela@chromium.org/327593003/60001

[commit-bot: I haz the power, 2014-06-10 19:28:52]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-06-10 22:00:55]

Change committed as 276171