Ports XMPP Invalidation code from drive_file_sync_service in /sync_file_system to /google_apis/driv…

Ports XMPP Invalidation code from drive_file_sync_service in /sync_file_system to /google_apis/drive_notification_manager so it can be shared between ChromeOS file manager and SyncFS.

Please note that to keep this patch streamlined I've only ported over the XMPP invalidation code for now and will port the polling logic in the next patch.

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

[calvinlo, 2013-04-11 09:33:48]

Hi Tzik san, can you please take an initial look.

[tzik, 2013-04-12 02:00:15]

Looks good!

https://codereview.chromium.org/13866037/diff/1/chrome/browser/google_apis/dr...
File chrome/browser/google_apis/drive_notification_manager.cc (right):

https://codereview.chromium.org/13866037/diff/1/chrome/browser/google_apis/dr...
chrome/browser/google_apis/drive_notification_manager.cc:122: LOG(WARNING) <<
"SetPushNotificationEnabled" << state;
Can it be DVLOG?

https://codereview.chromium.org/13866037/diff/1/chrome/browser/sync_file_syst...
File chrome/browser/sync_file_system/drive_file_sync_service.cc (left):

https://codereview.chromium.org/13866037/diff/1/chrome/browser/sync_file_syst...
chrome/browser/sync_file_system/drive_file_sync_service.cc:1121:
may_have_unfetched_changes_ = true;
Any alternate for this?

[calvinlo, 2013-04-12 04:41:59]

https://codereview.chromium.org/13866037/diff/1/chrome/browser/google_apis/dr...
File chrome/browser/google_apis/drive_notification_manager.cc (right):

https://codereview.chromium.org/13866037/diff/1/chrome/browser/google_apis/dr...
chrome/browser/google_apis/drive_notification_manager.cc:122: LOG(WARNING) <<
"SetPushNotificationEnabled" << state;
On 2013/04/12 02:00:15, tzik wrote:
> Can it be DVLOG?

Done. Sorry, forgot I added this originally just for quick testing locally.

https://codereview.chromium.org/13866037/diff/1/chrome/browser/sync_file_syst...
File chrome/browser/sync_file_system/drive_file_sync_service.cc (left):

https://codereview.chromium.org/13866037/diff/1/chrome/browser/sync_file_syst...
chrome/browser/sync_file_system/drive_file_sync_service.cc:1121:
may_have_unfetched_changes_ = true;
On 2013/04/12 02:00:15, tzik wrote:
> Any alternate for this?

I think in general I want to get rid of the "may_have_unfetched_changes"
variable so only DriveNotificationManager manages when new changes should be
checked. Everytime CheckForUpdates() is called, may_have_unfetched_changes would
implicitly be true. If we can make that assertion true, then we can remove all
references to may_have_unfetched_changes in this file and thus this block would
no longer be needed.

[tzik, 2013-04-12 05:12:46]

https://codereview.chromium.org/13866037/diff/1/chrome/browser/sync_file_syst...
File chrome/browser/sync_file_system/drive_file_sync_service.cc (left):

https://codereview.chromium.org/13866037/diff/1/chrome/browser/sync_file_syst...
chrome/browser/sync_file_system/drive_file_sync_service.cc:1121:
may_have_unfetched_changes_ = true;
On 2013/04/12 04:42:00, calvinlo wrote:
> On 2013/04/12 02:00:15, tzik wrote:
> > Any alternate for this?
> 
> I think in general I want to get rid of the "may_have_unfetched_changes"
> variable so only DriveNotificationManager manages when new changes should be
> checked. Everytime CheckForUpdates() is called, may_have_unfetched_changes
would
> implicitly be true. If we can make that assertion true, then we can remove all
> references to may_have_unfetched_changes in this file and thus this block
would
> no longer be needed.

I think we still need to kick CheckForUpdates around here, since the
notification has been ignored before this point.
So, how about just call CheckForUpdate here instead?

[calvinlo, 2013-04-12 06:44:38]

https://codereview.chromium.org/13866037/diff/1/chrome/browser/sync_file_syst...
File chrome/browser/sync_file_system/drive_file_sync_service.cc (left):

https://codereview.chromium.org/13866037/diff/1/chrome/browser/sync_file_syst...
chrome/browser/sync_file_system/drive_file_sync_service.cc:1121:
may_have_unfetched_changes_ = true;
Done. Yeah, I agree, calling CheckForUpdates here is probably the safest thing
to do even thought it might not be the most optimized.

[calvinlo, 2013-04-12 06:49:35]

+kinaba for google_apis review

[tzik, 2013-04-12 07:23:02]

lgtm

[kinaba, 2013-04-12 07:32:38]

google_apis lgtm

[tzik, 2013-04-16 09:14:10]

lgtm

https://codereview.chromium.org/13866037/diff/97001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/drive_file_sync_service_mock_unittest.cc
(right):

https://codereview.chromium.org/13866037/diff/97001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_file_sync_service_mock_unittest.cc:601:
.WillOnce(InvokeGetChangeListWithEmptyChange());
Could you add .RetiresOnSaturation(); since now we use WillOnce?

[calvinlo, 2013-04-16 09:27:51]

https://codereview.chromium.org/13866037/diff/97001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/drive_file_sync_service_mock_unittest.cc
(right):

https://codereview.chromium.org/13866037/diff/97001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_file_sync_service_mock_unittest.cc:601:
.WillOnce(InvokeGetChangeListWithEmptyChange());
On 2013/04/16 09:14:10, tzik wrote:
> Could you add .RetiresOnSaturation(); since now we use WillOnce?

Done.

[calvinlo, 2013-04-17 03:26:20]

Committed patchset #10 manually as r194526 (presubmit successful).