Add App Notification Sync Integration Test

Add App Notification Sync Integration Test

[Munjal (Google), 2011-11-30 19:22:26]

Actually Lingesh and Raghu should review this.

[elvin, 2011-12-03 01:16:55]

On 2011/11/30 19:22:26, munjal wrote:
> Actually Lingesh and Raghu should review this.

Ping

[lipalani1, 2011-12-03 01:25:39]

LGTM on the test case.

I am sorry I should have told this earlier. The helper methods are beyond my
knowledge scope. Somebody from your team should review that.

http://codereview.chromium.org/8735019/diff/10001/chrome/browser/sync/test/in...
File
chrome/browser/sync/test/integration/single_client_app_notification_sync_test.cc
(right):

http://codereview.chromium.org/8735019/diff/10001/chrome/browser/sync/test/in...
chrome/browser/sync/test/integration/single_client_app_notification_sync_test.cc:36:

Should this be done before setup sync?

http://codereview.chromium.org/8735019/diff/10001/chrome/browser/sync/test/in...
chrome/browser/sync/test/integration/single_client_app_notification_sync_test.cc:44:
ASSERT_TRUE(GetClient(0)->AwaitFullSyncCompletion(
Change this from search engine.

[Munjal (Google), 2011-12-03 05:03:48]

LGTM. Again, thanks for doing this.

http://codereview.chromium.org/8735019/diff/10001/chrome/browser/sync/test/in...
File chrome/browser/sync/test/integration/app_notification_helper.cc (right):

http://codereview.chromium.org/8735019/diff/10001/chrome/browser/sync/test/in...
chrome/browser/sync/test/integration/app_notification_helper.cc:61: if
(verifier_list->size() != other_list->size()) {
You are checking verifier_list == NULL && other_list == NULL above. So both
cannot be NULL here, but one of them can be.

http://codereview.chromium.org/8735019/diff/10001/chrome/browser/sync/test/in...
chrome/browser/sync/test/integration/app_notification_helper.cc:70: const
AppNotification* verifier_notification = verifier_list->at(i).get();
Nit: you can do verifier_list[i].get()

http://codereview.chromium.org/8735019/diff/10001/chrome/browser/sync/test/in...
chrome/browser/sync/test/integration/app_notification_helper.cc:94: 
Nit: remove this empty line.

http://codereview.chromium.org/8735019/diff/10001/chrome/browser/sync/test/in...
File chrome/browser/sync/test/integration/app_notification_helper.h (right):

http://codereview.chromium.org/8735019/diff/10001/chrome/browser/sync/test/in...
chrome/browser/sync/test/integration/app_notification_helper.h:21: // Used to
access the search engines within a particular sync profile.
Remove reference to "search engine" in comments. Here and other places in this
file.

http://codereview.chromium.org/8735019/diff/10001/chrome/browser/sync/test/in...
chrome/browser/sync/test/integration/app_notification_helper.h:27: // Compared a
single AppNotificationManager for a given profile to the verifier.
Nit: Compares

http://codereview.chromium.org/8735019/diff/10001/chrome/browser/sync/test/in...
chrome/browser/sync/test/integration/app_notification_helper.h:28: // Retrns
true iff their user-visible fields match.
Nit: Returns

http://codereview.chromium.org/8735019/diff/10001/chrome/browser/sync/test/in...
chrome/browser/sync/test/integration/app_notification_helper.h:40: // The caller
owns the returned AppNotification*.
Nit: AppNotification* -> AppNotification.

[asargent_no_longer_on_chrome, 2011-12-03 05:34:30]

lgtm

http://codereview.chromium.org/8735019/diff/10001/chrome/browser/sync/test/in...
File chrome/browser/sync/test/integration/app_notification_helper.h (right):

http://codereview.chromium.org/8735019/diff/10001/chrome/browser/sync/test/in...
chrome/browser/sync/test/integration/app_notification_helper.h:17: typedef
std::map<std::string, const AppNotification*> GUIDToTURLMap;
nit: What's a TURL? =)

Should this be named "GUIDToNotificationMap" or something like that?

[Raghu Simha, 2011-12-05 18:33:55]

Sorry -- this skipped my attention until today. I've made some comments below.

http://codereview.chromium.org/8735019/diff/10001/chrome/browser/sync/test/in...
File chrome/browser/sync/test/integration/app_notification_helper.h (right):

http://codereview.chromium.org/8735019/diff/10001/chrome/browser/sync/test/in...
chrome/browser/sync/test/integration/app_notification_helper.h:22:
AppNotificationManager* GetServiceForProfile(int index);
Rename to GetAppNotificationManager().

http://codereview.chromium.org/8735019/diff/10001/chrome/browser/sync/test/in...
chrome/browser/sync/test/integration/app_notification_helper.h:25:
AppNotificationManager* GetVerifierService();
GetVerifierAppNotificationManager().

http://codereview.chromium.org/8735019/diff/10001/chrome/browser/sync/test/in...
chrome/browser/sync/test/integration/app_notification_helper.h:32: bool
AppNotificationMatch(const AppNotification* not1,
nit: Rename to AppNotificationsMatch().

http://codereview.chromium.org/8735019/diff/10001/chrome/browser/sync/test/in...
chrome/browser/sync/test/integration/app_notification_helper.h:37: void
AddNotification(int profile, int seed);
nit: Rename to AddAppNotification().

http://codereview.chromium.org/8735019/diff/10001/chrome/browser/sync/test/in...
chrome/browser/sync/test/integration/app_notification_helper.h:41:
AppNotification* CreateTestAppNotification(int seed);
Nit: Add a blank line here.

http://codereview.chromium.org/8735019/diff/10001/chrome/browser/sync/test/in...
chrome/browser/sync/test/integration/app_notification_helper.h:45: 
nit: Delete extra blank line.

http://codereview.chromium.org/8735019/diff/10001/chrome/browser/sync/test/in...
File
chrome/browser/sync/test/integration/single_client_app_notification_sync_test.cc
(right):

http://codereview.chromium.org/8735019/diff/10001/chrome/browser/sync/test/in...
chrome/browser/sync/test/integration/single_client_app_notification_sync_test.cc:39:
FROM_HERE,
nit: Fix indentation.