Enrich fileBrowserPrivate.onFileTransfersUpdated event to support displaying total number of jobs.

chrome/browser/chromeos/extensions/file_manager/event_router.cc

Index: chrome/browser/chromeos/extensions/file_manager/event_router.cc
diff --git a/chrome/browser/chromeos/extensions/file_manager/event_router.cc b/chrome/browser/chromeos/extensions/file_manager/event_router.cc
index c459ccbbff8b4417e23c7db0156a72c8aac4a85e..2667577c5d7c289b39094dad619a6f7f4fafd8ba 100644
--- a/chrome/browser/chromeos/extensions/file_manager/event_router.cc
+++ b/chrome/browser/chromeos/extensions/file_manager/event_router.cc
@@ -10,6 +10,7 @@
 #include "base/prefs/pref_change_registrar.h"
 #include "base/prefs/pref_service.h"
 #include "base/stl_util.h"
+#include "base/thread_task_runner_handle.h"
 #include "base/threading/sequenced_worker_pool.h"
 #include "base/values.h"
 #include "chrome/browser/app_mode/app_mode_utils.h"
@@ -61,6 +62,7 @@ namespace file_browser_private = extensions::api::file_browser_private;
 namespace file_manager {
 namespace {
 // Constants for the "transferState" field of onFileTransferUpdated event.
+const char kFileTransferStateAdded[] = "added";
 const char kFileTransferStateStarted[] = "started";
 const char kFileTransferStateInProgress[] = "in_progress";
 const char kFileTransferStateCompleted[] = "completed";
@@ -74,6 +76,11 @@ const int64 kProgressEventFrequencyInMilliseconds = 1000;
 // is kicked.
 const size_t kDirectoryChangeEventMaxDetailInfoSize = 1000;
 
+// This time is used for confirm following event exists.
+const int64 kFileTransferEventDelayTime = 100;

[kinaba, 2014/09/02 05:01:39]

Could you add the uint (millisecond?) of the value at least in the comment, or
preferably in the variable name?

[iseki, 2014/09/02 10:01:42]

Done.

+// This time is used for sending a event forcely.
+const int64 kFileTransferEventForceSendInterval = 500;

[kinaba, 2014/09/02 05:01:39]

ditto

[iseki, 2014/09/02 10:01:42]

Done.

+
 // Utility function to check if |job_info| is a file uploading job.
 bool IsUploadJob(drive::JobType type) {
   return (type == drive::TYPE_UPLOAD_NEW_FILE ||
@@ -98,6 +105,7 @@ void JobInfoToTransferStatus(
       IsUploadJob(job_info.job_type) ?
       file_browser_private::TRANSFER_TYPE_UPLOAD :
       file_browser_private::TRANSFER_TYPE_DOWNLOAD;
+  status->num_total_jobs = job_info.num_total_jobs;
   // JavaScript does not have 64-bit integers. Instead we use double, which
   // is in IEEE 754 formant and accurate up to 52-bits in JS, and in practice
   // in C++. Larger values are rounded.
@@ -620,7 +628,10 @@ void EventRouter::OnFileManagerPrefsChanged() {
 
 void EventRouter::OnJobAdded(const drive::JobInfo& job_info) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  OnJobUpdated(job_info);
+  if (!drive::IsActiveFileTransferJobInfo(job_info))
+    return;
+  ScheduleDriveFileTransferEvent(
+      job_info, kFileTransferStateAdded, false /* immediate */);
 }
 
 void EventRouter::OnJobUpdated(const drive::JobInfo& job_info) {
@@ -630,14 +641,13 @@ void EventRouter::OnJobUpdated(const drive::JobInfo& job_info) {
 
   bool is_new_job = (drive_jobs_.find(job_info.job_id) == drive_jobs_.end());
 
+  const std::string status =
+      is_new_job ? kFileTransferStateStarted : kFileTransferStateInProgress;
+
   // Replace with the latest job info.
-  drive_jobs_[job_info.job_id] = DriveJobInfoWithStatus(
-      job_info,
-      is_new_job ? kFileTransferStateStarted : kFileTransferStateInProgress);
+  drive_jobs_[job_info.job_id] = DriveJobInfoWithStatus(job_info, status);
 
-  // Fire event if needed.
-  bool always = is_new_job;
-  SendDriveFileTransferEvent(always);
+  ScheduleDriveFileTransferEvent(job_info, status, false /* immediate */);
 }
 
 void EventRouter::OnJobDone(const drive::JobInfo& job_info,
@@ -646,47 +656,76 @@ void EventRouter::OnJobDone(const drive::JobInfo& job_info,
   if (!drive::IsActiveFileTransferJobInfo(job_info))
     return;
 
-  // Replace with the latest job info.
-  drive_jobs_[job_info.job_id] = DriveJobInfoWithStatus(
-      job_info,
-      error == drive::FILE_ERROR_OK ? kFileTransferStateCompleted
-      : kFileTransferStateFailed);
+  const std::string status = error == drive::FILE_ERROR_OK
+                                 ? kFileTransferStateCompleted
+                                 : kFileTransferStateFailed;
 
-  // Fire event if needed.
-  bool always = true;
-  SendDriveFileTransferEvent(always);
+  ScheduleDriveFileTransferEvent(job_info, status, true /* immediate */);
 
   // Forget about the job.
   drive_jobs_.erase(job_info.job_id);
 }
 
-void EventRouter::SendDriveFileTransferEvent(bool always) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+void EventRouter::ScheduleDriveFileTransferEvent(const drive::JobInfo& job_info,
+                                                 const std::string& status,
+                                                 bool immediate) {
+  // Update next send time.
+  const base::Time now = base::Time::Now();
+  if (immediate) {
+    next_send_file_transfer_event_ = now;
+  } else if (!drive_job_info_with_status_) {
+    // The first job is delayed to confirm whether only job or many jobs.
+    next_send_file_transfer_event_ = std::max(
+        now + base::TimeDelta::FromMilliseconds(kFileTransferEventDelayTime),
+        next_send_file_transfer_event_);
+  }
+
+  // Update the latest event.
+  drive_job_info_with_status_.reset(
+      new DriveJobInfoWithStatus(job_info, status));
+
+  // Schedule event.
+  if (next_send_file_transfer_event_ < now) {
+    SendDriveFileTransferEvent();
+  } else {
+    const base::TimeDelta delta = next_send_file_transfer_event_ - now;
+    base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
+        FROM_HERE,
+        base::Bind(&EventRouter::SendDriveFileTransferEvent,
+                   weak_factory_.GetWeakPtr()),
+        delta);
+  }
+}
 
-  // When |always| flag is not set, we don't send the event until certain
-  // amount of time passes after the previous one. This is to avoid
-  // flooding the IPC between extensions by many onFileTransferUpdated events.
-  if (!ShouldSendProgressEvent(always, &last_file_transfer_event_))
+void EventRouter::SendDriveFileTransferEvent() {
+  const base::Time now = base::Time::Now();
+  if (now < next_send_file_transfer_event_ || !drive_job_info_with_status_)

[kinaba, 2014/09/02 05:01:39]

This may be fragile due to reasons.

First, according to the document in task_runner.h, PostDelayedTask() do not
always ensure the specified time to be elapsed. In that case, it's still now <
next_send_file_transfer_event_ and since no more task for
SendDriveFileTransferEvent is posted, the event does not fire forever.

Even if PostDelayedTask ensures the gap, in rare conditions (like the system
clock was synced and Time::Now() rolled back), |immediate| events may be
discarded. Is the JS side implementation fine with that?

[iseki, 2014/09/02 10:01:42]

Done.

     return;
 
-  // Convert the current |drive_jobs_| to IDL type.
+  // Convert the drive_job_info_with_status_ to IDL type.
   std::vector<linked_ptr<file_browser_private::FileTransferStatus> >
       status_list;
-  for (std::map<drive::JobID, DriveJobInfoWithStatus>::iterator
-           iter = drive_jobs_.begin(); iter != drive_jobs_.end(); ++iter) {
-    linked_ptr<file_browser_private::FileTransferStatus> status(
-        new file_browser_private::FileTransferStatus());
-    JobInfoToTransferStatus(profile_,
-                            kFileManagerAppId,
-                            iter->second.status,
-                            iter->second.job_info,
-                            status.get());
-    status_list.push_back(status);
-  }
+
+  linked_ptr<file_browser_private::FileTransferStatus> status(
+      new file_browser_private::FileTransferStatus());
+  JobInfoToTransferStatus(profile_,
+                          kFileManagerAppId,
+                          drive_job_info_with_status_->status,
+                          drive_job_info_with_status_->job_info,
+                          status.get());
+  status_list.push_back(status);

[kinaba, 2014/09/02 05:01:39]

Previously the onFileTransferUpdated event sent all the list of active events,
while now it sends only the last updated event. Is it intentional?
If it is so, the private API should also be updated.

[iseki, 2014/09/02 10:01:42]

I make the clean up issue(crbug.com/408482) and try to fix it another patch.

+
+  drive_job_info_with_status_.reset();
+
   BroadcastEvent(
       profile_,
       file_browser_private::OnFileTransfersUpdated::kEventName,
       file_browser_private::OnFileTransfersUpdated::Create(status_list));
+  // To avoid terriable delay due to dense consecutive event, set the
+  // force send time.

[kinaba, 2014/09/02 05:01:39]

I couldn't see how it works, or what is it for.

Is it (1) for avoiding to send too many events in short interval, or (2) for
avoiding to suppress too many events by always sending some event at the
frequency of ForceSendInterval?

If it is (1), the comment and the variable name is unclear, and moreover, can't
you reuse the existing ShouldSendProgressEvent?

If it is (2), it doesn't look working as intended (event without it the events
are sent in the interval of kFileTransferEventDelayTime)

[iseki, 2014/09/02 10:01:42]

Done.

+  next_send_file_transfer_event_ =
+      now +
+      base::TimeDelta::FromMilliseconds(kFileTransferEventForceSendInterval);
 }
 
 void EventRouter::OnDirectoryChanged(const base::FilePath& drive_path) {