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

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

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/chromeos/extensions/file_manager/event_router.h"
 
 #include "base/bind.h"
 #include "base/files/file_util.h"
 #include "base/message_loop/message_loop.h"
 #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"
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/chromeos/drive/drive_integration_service.h"
 #include "chrome/browser/chromeos/drive/file_change.h"
 #include "chrome/browser/chromeos/drive/file_system_interface.h"
 #include "chrome/browser/chromeos/drive/file_system_util.h"
 #include "chrome/browser/chromeos/extensions/file_manager/device_event_router.h"
 #include "chrome/browser/chromeos/extensions/file_manager/private_api_util.h"
@@ skipping 31 matching lines @@
 using drive::DriveIntegrationService;
 using drive::DriveIntegrationServiceFactory;
 using file_manager::util::EntryDefinition;
 using file_manager::util::FileDefinition;
 
 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";
 const char kFileTransferStateFailed[] = "failed";
 
 // Frequency of sending onFileTransferUpdated.
 const int64 kProgressEventFrequencyInMilliseconds = 1000;
 
 // Maximim size of detailed change info on directory change event. If the size
 // exceeds the maximum size, the detailed info is omitted and the force refresh
 // 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 ||
           type == drive::TYPE_UPLOAD_EXISTING_FILE);
 }
 
 // Converts the job info to a IDL generated type.
 void JobInfoToTransferStatus(
     Profile* profile,
     const std::string& extension_id,
     const std::string& job_status,
     const drive::JobInfo& job_info,
     file_browser_private::FileTransferStatus* status) {
   DCHECK(IsActiveFileTransferJobInfo(job_info));
 
   scoped_ptr<base::DictionaryValue> result(new base::DictionaryValue);
   GURL url = util::ConvertDrivePathToFileSystemUrl(
       profile, job_info.file_path, extension_id);
   status->file_url = url.spec();
   status->transfer_state = file_browser_private::ParseTransferState(job_status);
   status->transfer_type =
       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.
   status->processed.reset(
       new double(static_cast<double>(job_info.num_completed_bytes)));
   status->total.reset(
       new double(static_cast<double>(job_info.num_total_bytes)));
 }
 
 // Checks if the Recovery Tool is running. This is a temporary solution.
@@ skipping 502 matching lines @@
   }
 
   BroadcastEvent(
       profile_,
       file_browser_private::OnPreferencesChanged::kEventName,
       file_browser_private::OnPreferencesChanged::Create());
 }
 
 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) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   if (!drive::IsActiveFileTransferJobInfo(job_info))
     return;
 
   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,
                             drive::FileError error) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   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_);
+  }
 
-  // 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_))
+  // 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);
+  }
+}
+
+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) {
   HandleFileWatchNotification(NULL, drive_path, false);
 }
 
 void EventRouter::OnFileChanged(const drive::FileChange& changed_files) {
   typedef std::map<base::FilePath, drive::FileChange> FileChangeMap;
 
   FileChangeMap map;
@@ skipping 265 matching lines @@
   }
 }
 
 void EventRouter::OnOwnerEntryChanged(aura::Window* window) {
   BroadcastEvent(profile_,
                  file_browser_private::OnDesktopChanged::kEventName,
                  file_browser_private::OnDesktopChanged::Create());
 }
 
 }  // namespace file_manager