C++ Readability for Raghu Simha (rsimha@chromium.org).

I would like to request a C++ readability review.

The code in this issue was committed to the chromium code base as part of a larger effort to refactor all the test code for the chrome sync feature.

See http://codereview.chromium.org/2863008/show.

Buganizer issue: http://b/issue?id=2786642

Thanks.

[radix, 2010-06-30 18:01:18]

How about tests for this library?

http://codereview.appspot.com/1737041/diff/1/2
File chrome/test/live_sync/live_sync_test.cc (right):

http://codereview.appspot.com/1737041/diff/1/2#newcode52
chrome/test/live_sync/live_sync_test.cc:52: const FilePath::StringType name) {
Can the above two lines be merged?

http://codereview.appspot.com/1737041/diff/1/3
File chrome/test/live_sync/live_sync_test.h (right):

http://codereview.appspot.com/1737041/diff/1/3#newcode15
chrome/test/live_sync/live_sync_test.h:15: class Profile;
I think it would be nice to have a comment pointing to where these classes are
declared.

http://codereview.appspot.com/1737041/diff/1/3#newcode24
chrome/test/live_sync/live_sync_test.h:24: extern const wchar_t
kSyncPasswordForTest[];
Why are these extern? Where are they defined? Why are they not declared in some
header file?

http://codereview.appspot.com/1737041/diff/1/3#newcode42
chrome/test/live_sync/live_sync_test.h:42: // Tests where three client profiles
are synced with the server. Typically,
shouldn't it be three or more? if not, this should be named THREE_CLIENT, no?

http://codereview.appspot.com/1737041/diff/1/3#newcode48
chrome/test/live_sync/live_sync_test.h:48: MANY_CLIENT
What is the difference between MULTIPLE and MANY?

http://codereview.appspot.com/1737041/diff/1/3#newcode86
chrome/test/live_sync/live_sync_test.h:86: // Append command line flag to enable
sync.
Should start with "Appends" rather than "Append"

http://codereview.appspot.com/1737041/diff/1/3#newcode90
chrome/test/live_sync/live_sync_test.h:90: static Profile* MakeProfile(const
FilePath::StringType name);
If it's a helper, should it be public? If it needs to be public, then it should
be documented in its own right.

http://codereview.appspot.com/1737041/diff/1/3#newcode92
chrome/test/live_sync/live_sync_test.h:92: // Used to access a particular sync
profile.
Should start with "Returns".

Also, please document ownership/lifetime semantics for the returned argument.

http://codereview.appspot.com/1737041/diff/1/3#newcode121
chrome/test/live_sync/live_sync_test.h:121: std::string username_;
#include <string>

http://codereview.appspot.com/1737041/diff/1/3#newcode154
chrome/test/live_sync/live_sync_test.h:154: scoped_ptr<Profile> verifier_;
Where does the declaration for scoped_ptr come from?

http://codereview.appspot.com/1737041/diff/1/3#newcode167
chrome/test/live_sync/live_sync_test.h:167:
DISALLOW_COPY_AND_ASSIGN(LiveSyncTest);
Where does the declaration for DISALLOW_COPY_AND_ASSIGN come from?

[rsimha, 2010-07-01 21:25:53]

Thanks for the readability review. I have uploaded a new patch set that has
fixes for all the issues you pointed out.

http://codereview.appspot.com/1737041/diff/1/2
File chrome/test/live_sync/live_sync_test.cc (right):

http://codereview.appspot.com/1737041/diff/1/2#newcode52
chrome/test/live_sync/live_sync_test.cc:52: const FilePath::StringType name) {
On 2010/06/30 18:01:19, radix wrote:
> Can the above two lines be merged?

Done.

http://codereview.appspot.com/1737041/diff/1/3
File chrome/test/live_sync/live_sync_test.h (right):

http://codereview.appspot.com/1737041/diff/1/3#newcode15
chrome/test/live_sync/live_sync_test.h:15: class Profile;
On 2010/06/30 18:01:19, radix wrote:
> I think it would be nice to have a comment pointing to where these classes are
> declared.

Done.

http://codereview.appspot.com/1737041/diff/1/3#newcode24
chrome/test/live_sync/live_sync_test.h:24: extern const wchar_t
kSyncPasswordForTest[];
On 2010/06/30 18:01:19, radix wrote:
> Why are these extern? Where are they defined? Why are they not declared in
some
> header file?

Good catch.

In the chrome codebase, each test is allowed to define any command line switches
it might use so long as they are in namespace switches. The LiveSyncTest class
needs these two switches.

The switches are defined and used in live_sync_test.cc. I have removed the
declarations from here since they are unnecessary.

Done.

http://codereview.appspot.com/1737041/diff/1/3#newcode42
chrome/test/live_sync/live_sync_test.h:42: // Tests where three client profiles
are synced with the server. Typically,
On 2010/06/30 18:01:19, radix wrote:
> shouldn't it be three or more? if not, this should be named THREE_CLIENT, no?

You're right -- this should be three or more.

Done.

http://codereview.appspot.com/1737041/diff/1/3#newcode48
chrome/test/live_sync/live_sync_test.h:48: MANY_CLIENT
On 2010/06/30 18:01:19, radix wrote:
> What is the difference between MULTIPLE and MANY?

This borrows from the difference between the computing concept of a "multi-core
processor" vs. a "many-core processor", where there is a difference in the order
of magnitude of the number of processors.

In this case, the difference lies in the order of magnitude of the number of
messages that need to be passed when each sync client makes one change to sync
data. The number of messages is O(N^2) where N is the number of clients.

The values currently in use result in MULTIPLE_CLIENT => 9 messages and
MANY_CLIENT => 100 messages. These numbers accurately capture the message
complexity desired for our tests.

http://codereview.appspot.com/1737041/diff/1/3#newcode86
chrome/test/live_sync/live_sync_test.h:86: // Append command line flag to enable
sync.
On 2010/06/30 18:01:19, radix wrote:
> Should start with "Appends" rather than "Append"

Done.

http://codereview.appspot.com/1737041/diff/1/3#newcode90
chrome/test/live_sync/live_sync_test.h:90: static Profile* MakeProfile(const
FilePath::StringType name);
On 2010/06/30 18:01:19, radix wrote:
> If it's a helper, should it be public? If it needs to be public, then it
should
> be documented in its own right.

Good catch -- this should be private.

Done.

http://codereview.appspot.com/1737041/diff/1/3#newcode92
chrome/test/live_sync/live_sync_test.h:92: // Used to access a particular sync
profile.
On 2010/06/30 18:01:19, radix wrote:
> Should start with "Returns".
> 
> Also, please document ownership/lifetime semantics for the returned argument.

Done.

http://codereview.appspot.com/1737041/diff/1/3#newcode121
chrome/test/live_sync/live_sync_test.h:121: std::string username_;
On 2010/06/30 18:01:19, radix wrote:
> #include <string>

Done.

http://codereview.appspot.com/1737041/diff/1/3#newcode154
chrome/test/live_sync/live_sync_test.h:154: scoped_ptr<Profile> verifier_;
On 2010/06/30 18:01:19, radix wrote:
> Where does the declaration for scoped_ptr come from?

Added an include for "base/scoped_ptr.h".

Done.

http://codereview.appspot.com/1737041/diff/1/3#newcode167
chrome/test/live_sync/live_sync_test.h:167:
DISALLOW_COPY_AND_ASSIGN(LiveSyncTest);
On 2010/06/30 18:01:19, radix wrote:
> Where does the declaration for DISALLOW_COPY_AND_ASSIGN come from?

Added an include for "base/basictypes.h"

Done.

[rsimha, 2010-07-01 21:30:03]

On 2010/06/30 18:01:18, radix wrote:
> How about tests for this library?

There are several integration test cases that use the code in this library.
(They aren't part of this changelist). There is no plan of adding specific unit
tests for this library.

[rsimha, 2010-07-02 23:15:16]

This is good to go -- see comment from radix@ at http://b/issue?id=2786642.