Control the frequency of fileBrowserPrivate.onFileTransfersUpdated events.

The update notifications were sent at every download progress and flooded the
file manager UI. This patch inserts a check on frequency so that the event
keeps the interval of certain (currently 1000) milliseconds.

BUG=138838
TEST=manually tested copying large files.

Review URL: https://chromiumcodereview.appspot.com/10834027

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@149371 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/chromeos/gdata/gdata_operation_registry.cc

@@ -9,6 +9,12 @@
 
 using content::BrowserThread;
 
+namespace {
+
+const int64 kNotificationFrequencyInMilliseconds = 1000;
+
+}  // namespace
+
 namespace gdata {
 
 // static
@@ -137,7 +143,8 @@ void GDataOperationRegistry::Operation::NotifyAuthFailed() {
   registry_->OnOperationAuthFailed();
 }
 
-GDataOperationRegistry::GDataOperationRegistry() {
+GDataOperationRegistry::GDataOperationRegistry()
+    : do_notification_frequency_control_(true) {
   in_flight_operations_.set_check_on_null_data(true);
 }
 
@@ -153,6 +160,10 @@ void GDataOperationRegistry::RemoveObserver(Observer* observer) {
   observer_list_.RemoveObserver(observer);
 }
 
+void GDataOperationRegistry::DisableNotificationFrequencyControlForTest() {
+  do_notification_frequency_control_ = false;
+}
+
 void GDataOperationRegistry::CancelAll() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
@@ -189,10 +200,8 @@ void GDataOperationRegistry::OnOperationStart(
 
   *id = in_flight_operations_.Add(operation);
   DVLOG(1) << "GDataOperation[" << *id << "] started.";
-  if (IsFileTransferOperation(operation)) {
-    FOR_EACH_OBSERVER(Observer, observer_list_,
-                      OnProgressUpdate(GetProgressStatusList()));
-  }
+  if (IsFileTransferOperation(operation))
+    NotifyStatusToObservers();
 }
 
 void GDataOperationRegistry::OnOperationProgress(OperationID id) {
@@ -203,10 +212,8 @@ void GDataOperationRegistry::OnOperationProgress(OperationID id) {
 
   DVLOG(1) << "GDataOperation[" << id << "] "
            << operation->progress_status().DebugString();
-  if (IsFileTransferOperation(operation)) {
-    FOR_EACH_OBSERVER(Observer, observer_list_,
-                      OnProgressUpdate(GetProgressStatusList()));
-  }
+  if (IsFileTransferOperation(operation))
+    NotifyStatusToObservers();
 }
 
 void GDataOperationRegistry::OnOperationFinish(OperationID id) {
@@ -216,10 +223,8 @@ void GDataOperationRegistry::OnOperationFinish(OperationID id) {
   DCHECK(operation);
 
   DVLOG(1) << "GDataOperation[" << id << "] finished.";
-  if (IsFileTransferOperation(operation)) {
-    FOR_EACH_OBSERVER(Observer, observer_list_,
-                      OnProgressUpdate(GetProgressStatusList()));
-  }
+  if (IsFileTransferOperation(operation))
+    NotifyStatusToObservers();
   in_flight_operations_.Remove(id);
 }
 
@@ -256,10 +261,8 @@ void GDataOperationRegistry::OnOperationResume(
   new_status->operation_id = in_flight_operations_.Add(operation);
   DVLOG(1) << "GDataOperation[" << old_id << " -> " <<
            new_status->operation_id << "] resumed.";
-  if (IsFileTransferOperation(operation)) {
-    FOR_EACH_OBSERVER(Observer, observer_list_,
-                      OnProgressUpdate(GetProgressStatusList()));
-  }
+  if (IsFileTransferOperation(operation))
+    NotifyStatusToObservers();
 }
 
 void GDataOperationRegistry::OnOperationSuspend(OperationID id) {
@@ -269,10 +272,8 @@ void GDataOperationRegistry::OnOperationSuspend(OperationID id) {
   DCHECK(operation);
 
   DVLOG(1) << "GDataOperation[" << id << "] suspended.";
-  if (IsFileTransferOperation(operation)) {
-    FOR_EACH_OBSERVER(Observer, observer_list_,
-                      OnProgressUpdate(GetProgressStatusList()));
-  }
+  if (IsFileTransferOperation(operation))
+    NotifyStatusToObservers();
 }
 
 void GDataOperationRegistry::OnOperationAuthFailed() {
@@ -303,4 +304,45 @@ GDataOperationRegistry::GetProgressStatusList() {
   return status_list;
 }
 
+bool GDataOperationRegistry::ShouldNotifyStatusNow(
+    const ProgressStatusList& list) {
+  if (!do_notification_frequency_control_)
+    return true;
+
+  base::Time now = base::Time::Now();
+
+  // If it is a first event, or some time abnormality is detected, we should
+  // not skip this notification.
+  if (last_notification_.is_null() || now < last_notification_) {
+    last_notification_ = now;
+    return true;
+  }
+
+  // If sufficiently long time has elapsed since the previous event, we should
+  // not skip this notification.
+  if ((now - last_notification_).InMilliseconds() >=
+      kNotificationFrequencyInMilliseconds) {
+    last_notification_ = now;
+    return true;
+  }
+
+  // If important events (OPERATION_STARTED, COMPLETED, or FAILED) are there,
+  // we should not skip this notification.
+  for (size_t i = 0; i < list.size(); ++i) {
+    if (list[i].transfer_state != OPERATION_IN_PROGRESS) {
+      last_notification_ = now;
+      return true;
+    }
+  }
+
+  // Otherwise we can skip it.
+  return false;
+}
+
+void GDataOperationRegistry::NotifyStatusToObservers() {
+  ProgressStatusList list(GetProgressStatusList());
+  if (ShouldNotifyStatusNow(list))
+    FOR_EACH_OBSERVER(Observer, observer_list_, OnProgressUpdate(list));
+}
+
 }  // namespace gdata

chrome/browser/chromeos/gdata/gdata_operation_registry.h

@@ -143,6 +143,9 @@ class GDataOperationRegistry {
   void AddObserver(Observer* observer);
   void RemoveObserver(Observer* observer);
 
+  // Disables the notification suppression for testing purpose.
+  void DisableNotificationFrequencyControlForTest();
+
  private:
   // Handlers for notifications from Operations.
   friend class Operation;
@@ -157,9 +160,18 @@ class GDataOperationRegistry {
 
   bool IsFileTransferOperation(const Operation* operation) const;
 
+  // Controls the frequency of notifications, not to flood the listeners with
+  // too many events.
+  bool ShouldNotifyStatusNow(const ProgressStatusList& list);
+  // Sends notifications to the observers after checking that the frequency is
+  // not too high by ShouldNotifyStatusNow.
+  void NotifyStatusToObservers();
+
   typedef IDMap<Operation, IDMapOwnPointer> OperationIDMap;
   OperationIDMap in_flight_operations_;
   ObserverList<Observer> observer_list_;
+  base::Time last_notification_;
+  bool do_notification_frequency_control_;
 
   DISALLOW_COPY_AND_ASSIGN(GDataOperationRegistry);
 };

chrome/browser/chromeos/gdata/gdata_operation_registry_unittest.cc

@@ -128,6 +128,7 @@ class GDataOperationRegistryTest : public testing::Test {
 TEST_F(GDataOperationRegistryTest, OneSuccess) {
   TestObserver observer;
   GDataOperationRegistry registry;
+  registry.DisableNotificationFrequencyControlForTest();
   registry.AddObserver(&observer);
 
   base::WeakPtr<MockOperation> op1 =
@@ -152,6 +153,7 @@ TEST_F(GDataOperationRegistryTest, OneSuccess) {
 TEST_F(GDataOperationRegistryTest, OneCancel) {
   TestObserver observer;
   GDataOperationRegistry registry;
+  registry.DisableNotificationFrequencyControlForTest();
   registry.AddObserver(&observer);
 
   base::WeakPtr<MockOperation> op1 =
@@ -172,6 +174,7 @@ TEST_F(GDataOperationRegistryTest, OneCancel) {
 TEST_F(GDataOperationRegistryTest, TwoSuccess) {
   TestObserver observer;
   GDataOperationRegistry registry;
+  registry.DisableNotificationFrequencyControlForTest();
   registry.AddObserver(&observer);
 
   base::WeakPtr<MockOperation> op1 =
@@ -204,6 +207,7 @@ TEST_F(GDataOperationRegistryTest, TwoSuccess) {
 TEST_F(GDataOperationRegistryTest, ThreeCancel) {
   TestObserver observer;
   GDataOperationRegistry registry;
+  registry.DisableNotificationFrequencyControlForTest();
   registry.AddObserver(&observer);
 
   base::WeakPtr<MockOperation> op1 =
@@ -234,6 +238,7 @@ TEST_F(GDataOperationRegistryTest, ThreeCancel) {
 TEST_F(GDataOperationRegistryTest, RestartOperation) {
   TestObserver observer;
   GDataOperationRegistry registry;
+  registry.DisableNotificationFrequencyControlForTest();
   registry.AddObserver(&observer);
 
   base::WeakPtr<MockOperation> op1 =