Allow sync integration tests to operate on multiple datatypes: Typed Urls

Allow sync integration tests to operate on multiple datatypes: Typed Urls

The sync integration tests currently use a class hierarchy where the test classes for each datatype are subclasses of LiveSyncTest. While this design worked in the past, it allows tests to work with only one datatype at a time, and therefore doesn't allow us to test the interplay between datatypes.

This patch is another in the series of patches that will move away from an inheritance model to one where test cases can operate on more than one datatype. It updates the Typed Urls datatype to the new model, and contains the following changes:

- LiveTypedUrlsSyncTest is no longer a class that inherits from LiveSyncTest, but is now a namespace called typed_urls_helper, and contains a bunch of methods that perform various operations related to Typed Urls.
- SingleClientLiveTypedUrlsSyncTest is renamed to SingleClientTypedUrlsSyncTest, and is a subclass of LiveSyncTest. It uses the methods in namespace typed_urls_helper by including its header file and attaching itself to the helper.

In addition, bookmarks, preferences and autofill helpers and the SyncDatatypeHelper were originally implemented as static classes. Based on the code review feedback in this patch, they have been modified to the "functions-in-a-namespace" model similar to typed urls. This patch consists of those changes too.

BUG=88510
TEST=sync_integration_tests, sync_performance_tests

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

[Raghu Simha, 2011-07-27 10:48:48]

Tim, please review. This patch is based on the changes in
http://codereview.chromium.org/7259005. Thanks.

[Raghu Simha, 2011-07-28 11:26:12]

Ping. Now that http://codereview.chromium.org/7259005 has been landed, this is
ready to go.

[Raghu Simha, 2011-07-29 10:46:25]

+atwilson as backup reviewer in case tim can't get to this today.

[Raghu Simha, 2011-08-01 05:30:34]

Bumping this in the hope that someone gets to look at it on Monday :)

[Raghu Simha, 2011-08-02 21:04:35]

Drew, you're it now. Thanks for the review.

[Andrew T Wilson (Slow), 2011-08-02 23:55:43]

Consider either making TypedUrlHelper and SyncDatatypeHelper into real objects,
or else not making them classes at all (just static utility functions).

The way you're doing it currently is OK, I guess, but then you shouldn't have
things like protected-access constructors and inheritance because you don't
really have a class heirarchy or real objects.

http://codereview.chromium.org/7461109/diff/17001/chrome/test/live_sync/typed...
File chrome/test/live_sync/typed_urls_helper.cc (right):

http://codereview.chromium.org/7461109/diff/17001/chrome/test/live_sync/typed...
chrome/test/live_sync/typed_urls_helper.cc:56:
TypedUrlsHelper::TypedUrlsHelper() {}
I don't think you want to be able to instantiate this object, so I'd omit these.

http://codereview.chromium.org/7461109/diff/17001/chrome/test/live_sync/typed...
chrome/test/live_sync/typed_urls_helper.cc:86: if (timestamp_ == NULL)
This is fine, although I think the following is preferred:

if (!timestamp_)

http://codereview.chromium.org/7461109/diff/17001/chrome/test/live_sync/typed...
File chrome/test/live_sync/typed_urls_helper.h (right):

http://codereview.chromium.org/7461109/diff/17001/chrome/test/live_sync/typed...
chrome/test/live_sync/typed_urls_helper.h:20: class TypedUrlsHelper : public
SyncDatatypeHelper {
Not sure why we need to derive from SyncDatatypeHelper since it only contains
static functions/data members. If you want to have easy access to test(), you
can just have a private helper function in this class:

static LiveSyncTest* test() { return SyncDatatypeHelper::test(); }

http://codereview.chromium.org/7461109/diff/17001/chrome/test/live_sync/typed...
chrome/test/live_sync/typed_urls_helper.h:53: TypedUrlsHelper();
Why are these protected and not private, since this class is basically just a
glorified namespace (all static functions)?

http://codereview.chromium.org/7461109/diff/17001/chrome/test/live_sync/typed...
chrome/test/live_sync/typed_urls_helper.h:54: virtual ~TypedUrlsHelper();
Why is this virtual?

[Raghu Simha, 2011-08-05 00:47:53]

Thanks for the review, Drew.

I took a closer look at the style guide recommendations re: static classes vs.
helper functions in a namespace, and have fixed this patch based on your
feedback.

Unfortunately, this required fixing a few other datatypes that were using the
static helper class model, so this patch now contains a teeny number of
additional files :) (Although most changes are text deletions without any real
implementation changes)

Please take another look when you get a chance.

http://codereview.chromium.org/7461109/diff/17001/chrome/test/live_sync/typed...
File chrome/test/live_sync/typed_urls_helper.cc (right):

http://codereview.chromium.org/7461109/diff/17001/chrome/test/live_sync/typed...
chrome/test/live_sync/typed_urls_helper.cc:56:
TypedUrlsHelper::TypedUrlsHelper() {}
On 2011/08/02 23:55:43, Andrew T Wilson wrote:
> I don't think you want to be able to instantiate this object, so I'd omit
these.

Done.

http://codereview.chromium.org/7461109/diff/17001/chrome/test/live_sync/typed...
chrome/test/live_sync/typed_urls_helper.cc:86: if (timestamp_ == NULL)
On 2011/08/02 23:55:43, Andrew T Wilson wrote:
> This is fine, although I think the following is preferred:
> 
> if (!timestamp_)

Done.

http://codereview.chromium.org/7461109/diff/17001/chrome/test/live_sync/typed...
File chrome/test/live_sync/typed_urls_helper.h (right):

http://codereview.chromium.org/7461109/diff/17001/chrome/test/live_sync/typed...
chrome/test/live_sync/typed_urls_helper.h:20: class TypedUrlsHelper : public
SyncDatatypeHelper {
On 2011/08/02 23:55:43, Andrew T Wilson wrote:
> Not sure why we need to derive from SyncDatatypeHelper since it only contains
> static functions/data members. If you want to have easy access to test(), you
> can just have a private helper function in this class:
> 
> static LiveSyncTest* test() { return SyncDatatypeHelper::test(); }

Done.

http://codereview.chromium.org/7461109/diff/17001/chrome/test/live_sync/typed...
chrome/test/live_sync/typed_urls_helper.h:53: TypedUrlsHelper();
On 2011/08/02 23:55:43, Andrew T Wilson wrote:
> Why are these protected and not private, since this class is basically just a
> glorified namespace (all static functions)?

Removed.

http://codereview.chromium.org/7461109/diff/17001/chrome/test/live_sync/typed...
chrome/test/live_sync/typed_urls_helper.h:54: virtual ~TypedUrlsHelper();
On 2011/08/02 23:55:43, Andrew T Wilson wrote:
> Why is this virtual?

Removed.

[Andrew T Wilson (Slow), 2011-08-07 02:39:51]

LGTM with a few nits below.

http://codereview.chromium.org/7461109/diff/32001/chrome/test/live_sync/sync_...
File chrome/test/live_sync/sync_datatype_helper.cc (right):

http://codereview.chromium.org/7461109/diff/32001/chrome/test/live_sync/sync_...
chrome/test/live_sync/sync_datatype_helper.cc:12: LiveSyncTest* test_ = NULL;
should be static.

Also, I'm not sure about the naming of non-data-members with an underscore - can
you double-check the style guide to make sure this is correct?

http://codereview.chromium.org/7461109/diff/32001/chrome/test/live_sync/sync_...
File chrome/test/live_sync/sync_datatype_helper.h (right):

http://codereview.chromium.org/7461109/diff/32001/chrome/test/live_sync/sync_...
chrome/test/live_sync/sync_datatype_helper.h:20: // Returns |test_| after making
sure it is a valid pointer.
This comment no longer makes sense now that there's no test_ data member.

http://codereview.chromium.org/7461109/diff/32001/chrome/test/live_sync/typed...
File chrome/test/live_sync/typed_urls_helper.cc (right):

http://codereview.chromium.org/7461109/diff/32001/chrome/test/live_sync/typed...
chrome/test/live_sync/typed_urls_helper.cc:95: base::Time* timestamp_ = NULL;
Again, this should be static (so it's not accessible outside this file) and also
please check whether timestamp_ is correct for non-member variables.

http://codereview.chromium.org/7461109/diff/32001/chrome/test/live_sync/typed...
File chrome/test/live_sync/typed_urls_helper.h (right):

http://codereview.chromium.org/7461109/diff/32001/chrome/test/live_sync/typed...
chrome/test/live_sync/typed_urls_helper.h:17: class HistoryService;
I don't think we need this forward decl of HistoryService any more.

[Raghu Simha, 2011-08-08 19:04:23]

Thanks for the feedback. I'll land this after the trybots go green.

http://codereview.chromium.org/7461109/diff/32001/chrome/test/live_sync/sync_...
File chrome/test/live_sync/sync_datatype_helper.cc (right):

http://codereview.chromium.org/7461109/diff/32001/chrome/test/live_sync/sync_...
chrome/test/live_sync/sync_datatype_helper.cc:12: LiveSyncTest* test_ = NULL;
On 2011/08/07 02:39:51, Andrew T Wilson wrote:
> should be static.
> 
> Also, I'm not sure about the naming of non-data-members with an underscore -
can
> you double-check the style guide to make sure this is correct?

Done. Renamed test_ to test and made it a static.

http://codereview.chromium.org/7461109/diff/32001/chrome/test/live_sync/sync_...
File chrome/test/live_sync/sync_datatype_helper.h (right):

http://codereview.chromium.org/7461109/diff/32001/chrome/test/live_sync/sync_...
chrome/test/live_sync/sync_datatype_helper.h:20: // Returns |test_| after making
sure it is a valid pointer.
On 2011/08/07 02:39:51, Andrew T Wilson wrote:
> This comment no longer makes sense now that there's no test_ data member.

Good catch. Fixed.

http://codereview.chromium.org/7461109/diff/32001/chrome/test/live_sync/typed...
File chrome/test/live_sync/typed_urls_helper.cc (right):

http://codereview.chromium.org/7461109/diff/32001/chrome/test/live_sync/typed...
chrome/test/live_sync/typed_urls_helper.cc:95: base::Time* timestamp_ = NULL;
On 2011/08/07 02:39:51, Andrew T Wilson wrote:
> Again, this should be static (so it's not accessible outside this file) and
also
> please check whether timestamp_ is correct for non-member variables.

Done.

http://codereview.chromium.org/7461109/diff/32001/chrome/test/live_sync/typed...
File chrome/test/live_sync/typed_urls_helper.h (right):

http://codereview.chromium.org/7461109/diff/32001/chrome/test/live_sync/typed...
chrome/test/live_sync/typed_urls_helper.h:17: class HistoryService;
On 2011/08/07 02:39:51, Andrew T Wilson wrote:
> I don't think we need this forward decl of HistoryService any more.

Removed.