Merge ChromeOS and SyncFS XMPP Notification into one class

Make DriveSystemService an observer of DriveNotificationManager so that it no longer has to handle registering for Drive XMPP notifications itself. This will also allow both SyncFileSystem and the ChromeOS Filemanager (and possibly more observers) to use XMPP notifications solving the problem from before where the same XMPP Invalidation ID was being registered twice.

Also removed polling code from SyncFS it's OKed to remove by both SyncFS and ChromeOS teams.

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

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

[calvinlo, 2013-04-17 08:52:42]

kinaba, I'm not sure if you worked on DriveSystemService before, but you know if
the backup polling code was removed? I think I remember it being there before
with a long delay (4hrs) set as a backup in case an XMPP notifications were
lost.

[kinaba, 2013-04-17 09:11:27]

As chatted offline, yes, +satorux has removed the polling code in
https://codereview.chromium.org/14256007/
under the assumption "shouldn't be difficult to add this back if needed." :)

I think it's fine for us to have it in DriveNotificationManager as a back up
plan.
The main goal of above change is to make DriveFileSystem as clean as possible.

[calvinlo, 2013-04-17 11:02:12]

RE: Polling. I have added satorux to review of patch 13929013 to get his
thoughts on polling. Since there's no polling code from this patch to port, it
can be submitted independently of 13929013 now.

kinaba, can you please do a review for google_apis and chromeos/drive?
satoru, can you please do an ownership review for
chrome/browser/ui/webui/chromeos/drive_internals_ui.cc

[tzik, 2013-04-17 12:39:51]

lgtm

[kinaba, 2013-04-19 02:26:47]

On 2013/04/17 12:39:51, tzik wrote:
> lgtm

c/b/cros/drive change looks good to me but I want to defer
the go-sign to Satoru, who is working on notification related
bug right now.

[satorux1, 2013-04-19 04:12:13]

https://codereview.chromium.org/13891016/diff/10001/chrome/browser/chromeos/d...
File chrome/browser/chromeos/drive/drive_system_service.cc (left):

https://codereview.chromium.org/13891016/diff/10001/chrome/browser/chromeos/d...
chrome/browser/chromeos/drive/drive_system_service.cc:225: const
syncer::ObjectIdInvalidationMap& invalidation_map) {
Wanted to insert EventLogger logging here, but now it's moved to google_apis. I
guess I should move EventLogger to google_apis.

https://codereview.chromium.org/13891016/diff/10001/chrome/browser/google_api...
File chrome/browser/google_apis/drive_notification_manager.h (right):

https://codereview.chromium.org/13891016/diff/10001/chrome/browser/google_api...
chrome/browser/google_apis/drive_notification_manager.h:27: public
base::SupportsWeakPtr<DriveNotificationManager> {
Please use WeakPtrFactory instead in favor of less inheritance.

Besides, IIRC, there was a chromium-dev thread where SupportsWeakPtr was
considered a not good idea.

https://codereview.chromium.org/13891016/diff/10001/chrome/browser/google_api...
chrome/browser/google_apis/drive_notification_manager.h:43: bool
IsPushNotificationEnabled();
Function comment is missing.

https://codereview.chromium.org/13891016/diff/10001/chrome/browser/google_api...
chrome/browser/google_apis/drive_notification_manager.h:65:
base::RepeatingTimer<DriveNotificationManager> polling_timer_;
I thought you wanted to remove the polling code??

[calvinlo, 2013-04-19 06:15:09]

I've merged the other patch that removes polling from SyncFS into this patch.
After the rebase, I think I've addressed all your comments. PTAL, thanks.

https://codereview.chromium.org/13891016/diff/10001/chrome/browser/google_api...
File chrome/browser/google_apis/drive_notification_manager.h (right):

https://codereview.chromium.org/13891016/diff/10001/chrome/browser/google_api...
chrome/browser/google_apis/drive_notification_manager.h:27: public
base::SupportsWeakPtr<DriveNotificationManager> {
Removed. 

Thanks for the tip. Actually, after my rebase from patch
https://codereview.chromium.org/13929013/, this isn't needed anymore anyway

https://codereview.chromium.org/13891016/diff/10001/chrome/browser/google_api...
chrome/browser/google_apis/drive_notification_manager.h:43: bool
IsPushNotificationEnabled();
On 2013/04/19 04:12:13, satorux1 wrote:
> Function comment is missing.

Done.

https://codereview.chromium.org/13891016/diff/10001/chrome/browser/google_api...
chrome/browser/google_apis/drive_notification_manager.h:65:
base::RepeatingTimer<DriveNotificationManager> polling_timer_;
On 2013/04/19 04:12:13, satorux1 wrote:
> I thought you wanted to remove the polling code??

Removed. Sorry, I originally based this patch on
https://codereview.chromium.org/13929013/ but forgot to rebase this based on
changes from that patches latest review

[satorux1, 2013-04-19 07:28:25]

https://codereview.chromium.org/13891016/diff/26001/chrome/browser/chromeos/d...
File chrome/browser/chromeos/drive/drive_system_service.h (right):

https://codereview.chromium.org/13891016/diff/26001/chrome/browser/chromeos/d...
chrome/browser/chromeos/drive/drive_system_service.h:93: virtual void
CheckForUpdates() OVERRIDE;
I found it rather unusual for a function name of an observer class. I think
functions in observer classes usually look like OnBlahBlah().

In this case, something like OnNotificationArrived() would be nicer. what do you
think?

[calvinlo, 2013-04-19 07:54:06]

https://codereview.chromium.org/13891016/diff/26001/chrome/browser/chromeos/d...
File chrome/browser/chromeos/drive/drive_system_service.h (right):

https://codereview.chromium.org/13891016/diff/26001/chrome/browser/chromeos/d...
chrome/browser/chromeos/drive/drive_system_service.h:93: virtual void
CheckForUpdates() OVERRIDE;
Done. Originally I called it CheckForUpdates because it could have been called
from either getting an XMPP Notification or from the polling timer hitting 0.
Since there's only XMPP notifications now, OnNotificationReceived() would be an
appropriate name.

[satorux1, 2013-04-19 07:59:22]

LGTM

[commit-bot: I haz the power, 2013-04-19 08:04:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/calvinlo@chromium.org/13891016/25010

[commit-bot: I haz the power, 2013-04-19 08:52:11]

Retried try job too often on linux_chromeos for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[kinaba, 2013-04-19 10:05:39]

https://codereview.chromium.org/13891016/diff/25014/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/drive_file_sync_service.h (right):

https://codereview.chromium.org/13891016/diff/25014/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_file_sync_service.h:14: #include
"base/callback.h"
fyi: if you include callback.h, the callback_forward.h is not necessary,

[calvinlo, 2013-04-19 10:08:55]

Yup thanks, I sorta figured that out by trial and error. Testing the compile out
now.

[calvinlo, 2013-04-19 10:22:09]

https://codereview.chromium.org/13891016/diff/25014/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/drive_file_sync_service.h (right):

https://codereview.chromium.org/13891016/diff/25014/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_file_sync_service.h:14: #include
"base/callback.h"
On 2013/04/19 10:05:40, kinaba wrote:
> fyi: if you include callback.h, the callback_forward.h is not necessary,

Done.

[calvinlo, 2013-04-22 10:12:37]

Committed patchset #11 manually as r195482 (presubmit successful).

[kinuko, 2013-04-22 11:22:49]

On 2013/04/22 10:12:37, calvinlo wrote:
> Committed patchset #11 manually as r195482 (presubmit successful).

interactive_ui_tests failures on linux_chromeos look real:

http://build.chromium.org/p/tryserver.chromium/builders/linux_chromeos/builds...

[commit-bot: I haz the power, 2013-04-23 03:54:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/calvinlo@chromium.org/13891016/11008

[commit-bot: I haz the power, 2013-04-23 05:43:36]

Retried try job too often on win_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-04-23 08:33:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/calvinlo@chromium.org/13891016/45001

[commit-bot: I haz the power, 2013-04-23 08:33:34]

Failed to apply patch for chrome/browser/chromeos/drive/drive_system_service.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/chromeos/drive/drive_system_service.h
  Hunk #1 FAILED at 16.
  1 out of 5 hunks FAILED -- saving rejects to file
chrome/browser/chromeos/drive/drive_system_service.h.rej

Patch:       chrome/browser/chromeos/drive/drive_system_service.h
Index: chrome/browser/chromeos/drive/drive_system_service.h
diff --git a/chrome/browser/chromeos/drive/drive_system_service.h
b/chrome/browser/chromeos/drive/drive_system_service.h
index
5bb365386a11f4e77e405ea5ba2a46d93e2b562c..25dee3312038a1a05423301ef603559d2c47d83e
100644
--- a/chrome/browser/chromeos/drive/drive_system_service.h
+++ b/chrome/browser/chromeos/drive/drive_system_service.h
@@ -16,9 +16,9 @@
 #include "chrome/browser/chromeos/drive/drive_file_error.h"
 #include "chrome/browser/chromeos/drive/drive_file_system_util.h"
 #include "chrome/browser/chromeos/drive/drive_scheduler.h"
+#include "chrome/browser/google_apis/drive_notification_observer.h"
 #include "chrome/browser/profiles/profile_keyed_service.h"
 #include "chrome/browser/profiles/profile_keyed_service_factory.h"
-#include "sync/notifier/invalidation_handler.h"
 
 namespace base {
 class FilePath;
@@ -65,8 +65,9 @@ class DriveSystemServiceObserver {
 // The class is essentially a container that manages lifetime of the objects
 // that are used to run the Drive system. The DriveSystemService object is
 // created per-profile.
-class DriveSystemService : public ProfileKeyedService,
-                           public syncer::InvalidationHandler {
+class DriveSystemService
+    : public ProfileKeyedService,
+      public google_apis::DriveNotificationObserver {
  public:
   // test_drive_service, test_cache_root and test_file_system are used by tests
   // to inject customized instances.
@@ -88,6 +89,9 @@ class DriveSystemService : public ProfileKeyedService,
   void AddObserver(DriveSystemServiceObserver* observer);
   void RemoveObserver(DriveSystemServiceObserver* observer);
 
+  // google_apis::DriveNotificationObserver implementation.
+  virtual void OnNotificationReceived() OVERRIDE;
+
   google_apis::DriveServiceInterface* drive_service() {
     return drive_service_.get();
   }
@@ -110,16 +114,7 @@ class DriveSystemService : public ProfileKeyedService,
   // Reloads and remounts the file system.
   void ReloadAndRemountFileSystem();
 
-  // Returns true if the push notification is enabled.
-  bool PushNotificationEnabled();
-
  private:
-  // syncer::InvalidationHandler implementation.
-  virtual void OnInvalidatorStateChange(
-      syncer::InvalidatorState state) OVERRIDE;
-  virtual void OnIncomingInvalidation(
-      const syncer::ObjectIdInvalidationMap& invalidation_map) OVERRIDE;
-
   // Returns true if Drive is enabled.
   // Must be called on UI thread.
   bool IsDriveEnabled();
@@ -159,9 +154,6 @@ class DriveSystemService : public ProfileKeyedService,
   // True if Drive is disabled due to initialization errors.
   bool drive_disabled_;
 
-  // True once this is registered to listen to the Drive updates.
-  bool push_notification_registered_;
-
   scoped_refptr<base::SequencedTaskRunner> blocking_task_runner_;
   scoped_ptr<google_apis::EventLogger> event_logger_;
   scoped_ptr<DriveCache, util::DestroyHelper> cache_;

[commit-bot: I haz the power, 2013-04-23 11:30:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/calvinlo@chromium.org/13891016/62001

[commit-bot: I haz the power, 2013-04-23 12:57:05]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...

[commit-bot: I haz the power, 2013-04-23 21:12:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/calvinlo@chromium.org/13891016/62001

[commit-bot: I haz the power, 2013-04-23 23:16:55]

Change committed as 195935

[hshi1, 2013-04-24 00:30:44]

On 2013/04/23 23:16:55, I haz the power (commit-bot) wrote:
> Change committed as 195935

Looks like 195935 is causing crashes: crbug.com/234833
Why is this codereview URL used to land two separate revisions? It is very hard
to figure out what exactly are the diffs.

[satorux1, 2013-04-24 00:47:27]

Whoa. I wasn't aware that the codereview URL was reused.
Looks 195482 was reverted: crrev.com/195488

calvinlo, please refrain from reusing a code review URL as it's often confusing.
When you reland with a fix, a recommended way is:

1) upload the original patch as-is to a new codereview URL
2) upload the fixed version to that URL
3) in the patch description, mention that this is a reland of xxx with a fix

This way reviewers can see the diff from the original version, which was
reverted. The updated patch description would also help for investigation.

[calvinlo, 2013-04-24 02:20:59]

Ooops, sorry! This is my first reland so I didn't know the procedure was
different. Sorry about that, I will know for next time.