cros: Flush profile files at critical moments

chrome/browser/chromeos/base/file_flusher.cc

Index: chrome/browser/chromeos/base/file_flusher.cc
diff --git a/chrome/browser/chromeos/base/file_flusher.cc b/chrome/browser/chromeos/base/file_flusher.cc
new file mode 100644
index 0000000000000000000000000000000000000000..aad2f039bed086cf92b1824cff28c9100bb5b32c
--- /dev/null
+++ b/chrome/browser/chromeos/base/file_flusher.cc
@@ -0,0 +1,183 @@
+// Copyright 2016 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/base/file_flusher.h"
+
+#include <set>
+
+#include "base/bind.h"
+#include "base/files/file.h"
+#include "base/files/file_enumerator.h"
+#include "base/logging.h"
+#include "base/synchronization/cancellation_flag.h"
+#include "content/public/browser/browser_thread.h"
+
+namespace chromeos {
+
+namespace {
+
+std::set<base::FilePath> MakeAbsoutePathSet(
+    const base::FilePath& base,
+    const std::vector<base::FilePath>& paths) {
+  std::set<base::FilePath> result;
+  for (const auto& path : paths) {
+    result.insert(path.IsAbsolute() ? path : base.Append(path));
+  }
+  return result;
+}
+
+}  // namespace
+
+////////////////////////////////////////////////////////////////////////////////
+// FileFlusher::Job
+
+class FileFlusher::Job {
+ public:
+  Job(const base::WeakPtr<FileFlusher>& master,
+      const base::FilePath& path,
+      const std::vector<base::FilePath>& excludes);
+  ~Job() = default;
+
+  void Start();
+  void Cancel();
+
+  const base::FilePath& path() const { return path_; }
+  bool started() const { return started_; }
+
+ private:
+  void FlushOnBlockingPool();
+  bool ShouldExclude(const base::FilePath& path) const;
+
+  void ScheduleFinishOnUIThread();

[achuithb, 2016/03/21 17:45:41]

Maybe add a comment to say that the Schedule call posts to the UI thread?

[xiyuan, 2016/03/21 22:14:54]

Done.

+  void FinishOnUIThread();
+
+  base::WeakPtr<FileFlusher> master_;
+  const base::FilePath path_;
+  const std::set<base::FilePath> excludes_;
+
+  bool started_ = false;
+  base::CancellationFlag cancel_flag_;
+
+  DISALLOW_COPY_AND_ASSIGN(Job);
+};
+
+FileFlusher::Job::Job(const base::WeakPtr<FileFlusher>& master,
+                      const base::FilePath& path,
+                      const std::vector<base::FilePath>& excludes)
+    : master_(master),
+      path_(path),
+      excludes_(MakeAbsoutePathSet(path, excludes)) {}
+
+void FileFlusher::Job::Start() {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  DCHECK(!started());
+
+  started_ = true;
+
+  if (cancel_flag_.IsSet()) {
+    ScheduleFinishOnUIThread();
+    return;
+  }
+
+  content::BrowserThread::PostBlockingPoolTaskAndReply(
+      FROM_HERE, base::Bind(&FileFlusher::Job::FlushOnBlockingPool,
+                            base::Unretained(this)),
+      base::Bind(&FileFlusher::Job::FinishOnUIThread, base::Unretained(this)));
+}
+
+void FileFlusher::Job::Cancel() {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+
+  cancel_flag_.Set();
+
+  // Cancel() could be called in an iterator/range loop in master thus don't
+  // invoke FinishOnUIThread in-place.
+  if (!started())
+    ScheduleFinishOnUIThread();
+}
+
+void FileFlusher::Job::FlushOnBlockingPool() {
+  DCHECK(content::BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread());
+

[achuithb, 2016/03/21 17:45:41]

Would a VLOG(1) logging statement be useful here?

[xiyuan, 2016/03/21 22:14:54]

Done.

+  const bool kRecursive = true;

[achuithb, 2016/03/21 17:45:41]

Is kRecursive appropriate for const stack variables? shouldn't this just be
recursive? Why not just inlign the arg with true /* recursive */

[xiyuan, 2016/03/21 22:14:54]

Personally prefer to use a const bool instead of using a comment. The compiler
should optimize the const stack variables out. Not feel strongly about it
through. Changed as suggested.

+  base::FileEnumerator traversal(path_, kRecursive,
+                                 base::FileEnumerator::FILES);
+  for (base::FilePath current = traversal.Next();
+       !current.empty() && !cancel_flag_.IsSet(); current = traversal.Next()) {
+    if (ShouldExclude(current))
+      continue;
+
+    base::File currentFile(current,
+                           base::File::FLAG_OPEN | base::File::FLAG_WRITE);
+    if (!currentFile.IsValid()) {
+      VLOG(1) << "Unable to flush file:" << current.value();
+      continue;
+    }
+
+    currentFile.Flush();
+    currentFile.Close();
+  }
+}
+
+bool FileFlusher::Job::ShouldExclude(const base::FilePath& path) const {
+  return excludes_.find(path) != excludes_.end();
+}
+
+void FileFlusher::Job::ScheduleFinishOnUIThread() {
+  content::BrowserThread::PostTask(

[achuithb, 2016/03/21 17:45:41]

DCHECK_CURRENTLY_ON(content::BrowserThread::UI)?

[xiyuan, 2016/03/21 22:14:54]

The name is misleading. It schedules "FinishOnUITherad" but itself can be called
on any threads. Renamed to ScheduleFinish to avoid confusions.

+      content::BrowserThread::UI, FROM_HERE,
+      base::Bind(&FileFlusher::Job::FinishOnUIThread, base::Unretained(this)));
+}
+
+void FileFlusher::Job::FinishOnUIThread() {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+
+  if (master_)
+    master_->OnJobDone(this);
+
+  delete this;
+}
+
+////////////////////////////////////////////////////////////////////////////////
+// FileFlusher
+
+FileFlusher::FileFlusher() : weak_factory_(this) {}
+
+FileFlusher::~FileFlusher() {
+  for (const auto& job : jobs_)

[achuithb, 2016/03/21 17:45:41]

are we allowed to drop {} here?

[xiyuan, 2016/03/21 22:14:54]

Cancel() does not remove it from the vector. It schedules a removal. So we
should be good here.

[achuithb, 2016/03/21 22:32:56]

I didn't realize that braces were optional for single statement for loops (I
thought it was only for if statements)

[xiyuan, 2016/03/22 18:41:33]

I completely mis-understood to comment. :p

And yes, "Braces are optional for single-statement loops", see
https://google.github.io/styleguide/cppguide.html#Loops_and_Switch_Statements

+    job->Cancel();
+}
+
+void FileFlusher::RequestFlush(const base::FilePath& path,
+                               const std::vector<base::FilePath>& excludes) {
+  for (const auto& job : jobs_) {
+    if (path == job->path() || path.IsParent(job->path()))
+      job->Cancel();
+  }
+
+  jobs_.push_back(new Job(weak_factory_.GetWeakPtr(), path, excludes));
+  ScheduleJob();
+}
+
+void FileFlusher::ScheduleJob() {
+  if (jobs_.empty())
+    return;
+
+  auto job = jobs_.front();
+  if (!job->started())
+    job->Start();
+}
+
+void FileFlusher::OnJobDone(FileFlusher::Job* job) {
+  for (auto it = jobs_.begin(); it != jobs_.end(); ++it) {

[achuithb, 2016/03/21 17:45:41]

for (auto job : jobs_)?

[xiyuan, 2016/03/21 22:14:54]

Need the iterator to remove |job| from |jobs_| so cannot use range loop here.

[achuithb, 2016/03/21 22:32:56]

Acknowledged.

+    if (*it == job) {
+      jobs_.erase(it);
+      break;
+    }
+  }
+
+  ScheduleJob();
+}
+
+}  // namespace chromeos