Cleanup nits in ImportantFileWriter and JsonPrefStore.

base/files/important_file_writer.cc

 // Copyright (c) 2011 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 "base/files/important_file_writer.h"
 
 #include <stdio.h>
 
 #include <string>
 
@@ skipping 29 matching lines @@
   FAILED_WRITING,
   FAILED_RENAMING,
   FAILED_FLUSHING,
   TEMP_FILE_FAILURE_MAX
 };
 
 void LogFailure(const FilePath& path, TempFileFailure failure_code,
                 const std::string& message) {
   UMA_HISTOGRAM_ENUMERATION("ImportantFile.TempFileFailures", failure_code,
                             TEMP_FILE_FAILURE_MAX);
-  DPLOG(WARNING) << "temp file failure: " << path.value().c_str()
-                 << " : " << message;
+  DPLOG(WARNING) << "temp file failure: " << path.value() << " : " << message;
 }
 
 // Helper function to call WriteFileAtomically() with a scoped_ptr<std::string>.
 bool WriteScopedStringToFileAtomically(const FilePath& path,
                                        scoped_ptr<std::string> data) {
   return ImportantFileWriter::WriteFileAtomically(path, *data);
 }
 
 }  // namespace
 
 // static
 bool ImportantFileWriter::WriteFileAtomically(const FilePath& path,
                                               const std::string& data) {
 #if defined(OS_CHROMEOS)
   // On Chrome OS, chrome gets killed when it cannot finish shutdown quickly,
   // and this function seems to be one of the slowest shutdown steps.
   // Include some info to the report for investigation. crbug.com/418627
   // TODO(hashimoto): Remove this.
   struct {
     size_t data_size;
     char path[128];
   } file_info;
   file_info.data_size = data.size();
-  base::strlcpy(file_info.path, path.value().c_str(),
-                arraysize(file_info.path));
-  base::debug::Alias(&file_info);
+  strlcpy(file_info.path, path.value().c_str(), arraysize(file_info.path));
+  debug::Alias(&file_info);
 #endif
+
   // Write the data to a temp file then rename to avoid data loss if we crash
   // while writing the file. Ensure that the temp file is on the same volume
   // as target file, so it can be moved in one step, and that the temp file
   // is securely created.
   FilePath tmp_file_path;
-  if (!base::CreateTemporaryFileInDir(path.DirName(), &tmp_file_path)) {
+  if (!CreateTemporaryFileInDir(path.DirName(), &tmp_file_path)) {
     LogFailure(path, FAILED_CREATING, "could not create temporary file");
     return false;
   }
 
   File tmp_file(tmp_file_path, File::FLAG_OPEN | File::FLAG_WRITE);
   if (!tmp_file.IsValid()) {
     LogFailure(path, FAILED_OPENING, "could not open temporary file");
     return false;
   }
 
   // If this happens in the wild something really bad is going on.
-  CHECK_LE(data.length(), static_cast<size_t>(kint32max));
-  int bytes_written = tmp_file.Write(0, data.data(),
-                                     static_cast<int>(data.length()));
+  CHECK_LE(data.length(),
+           static_cast<size_t>(std::numeric_limits<int32>::max()));

[cmumford, 2015/08/05 20:45:04]

Need to #include <limits>

[Lei Zhang, 2015/08/06 07:13:41]

Went with IsValueInRangeForNumericType() instead.

+  const int data_length = static_cast<int>(data.length());
+  int bytes_written = tmp_file.Write(0, data.data(), data_length);
   bool flush_success = tmp_file.Flush();
   tmp_file.Close();
 
-  if (bytes_written < static_cast<int>(data.length())) {
+  if (bytes_written < data_length) {
     LogFailure(path, FAILED_WRITING, "error writing, bytes_written=" +
                IntToString(bytes_written));
-    base::DeleteFile(tmp_file_path, false);
+    DeleteFile(tmp_file_path, false);
     return false;
   }
 
   if (!flush_success) {
     LogFailure(path, FAILED_FLUSHING, "error flushing");
-    base::DeleteFile(tmp_file_path, false);
+    DeleteFile(tmp_file_path, false);
     return false;
   }
 
-  if (!base::ReplaceFile(tmp_file_path, path, NULL)) {
+  if (!ReplaceFile(tmp_file_path, path, nullptr)) {
     LogFailure(path, FAILED_RENAMING, "could not rename temporary file");
-    base::DeleteFile(tmp_file_path, false);
+    DeleteFile(tmp_file_path, false);
     return false;
   }
 
   return true;
 }
 
 ImportantFileWriter::ImportantFileWriter(
     const FilePath& path,
-    const scoped_refptr<base::SequencedTaskRunner>& task_runner)
+    const scoped_refptr<SequencedTaskRunner>& task_runner)
+    : ImportantFileWriter(
+        path,
+        task_runner,
+        TimeDelta::FromMilliseconds(kDefaultCommitIntervalMs)) {

[cmumford, 2015/08/05 20:45:05]

Instead of creating a new constructor why not just add a default parameter value
for interval? Sure, you need to move kDefaultCommitIntervalMs to the header, but
I think will simplify the code.

Your call - keep as is if you feel this is simpler.

[Lei Zhang, 2015/08/06 07:13:41]

https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Default_Argu...

+}
+
+ImportantFileWriter::ImportantFileWriter(
+    const FilePath& path,
+    const scoped_refptr<SequencedTaskRunner>& task_runner,
+    TimeDelta interval)
     : path_(path),
       task_runner_(task_runner),
-      serializer_(NULL),
-      commit_interval_(TimeDelta::FromMilliseconds(kDefaultCommitIntervalMs)),
+      serializer_(nullptr),

[cmumford, 2015/08/05 20:45:05]

If you're going to modify this line, why not just initialize serializer_ in the
header and delete this line?

[Lei Zhang, 2015/08/06 07:13:41]

Class member initializers haven't really caught on in base/. I don't see a
strong advantage to using it, because we'd then need to look at 2 different
places for init code. The ctor above delegates to this one, so we are not
suffering the pain of having to initialize |serializer_| in multiple places.

+      commit_interval_(interval),
       weak_factory_(this) {
   DCHECK(CalledOnValidThread());
   DCHECK(task_runner_);
 }
 
 ImportantFileWriter::~ImportantFileWriter() {
   // We're usually a member variable of some other object, which also tends
   // to be our serializer. It may not be safe to call back to the parent object
   // being destructed.
   DCHECK(!HasPendingWrite());
 }
 
 bool ImportantFileWriter::HasPendingWrite() const {
   DCHECK(CalledOnValidThread());
   return timer_.IsRunning();
 }
 
 void ImportantFileWriter::WriteNow(scoped_ptr<std::string> data) {
   DCHECK(CalledOnValidThread());
-  if (data->length() > static_cast<size_t>(kint32max)) {
+  if (data->length() > static_cast<size_t>(std::numeric_limits<int32>::max())) {
     NOTREACHED();
     return;
   }
 
   if (HasPendingWrite())
     timer_.Stop();
 
   auto task = Bind(&WriteScopedStringToFileAtomically, path_, Passed(&data));
   if (!PostWriteTask(task)) {
     // Posting the task to background message loop is not expected
@@ skipping 17 matching lines @@
   }
 }
 
 void ImportantFileWriter::DoScheduledWrite() {
   DCHECK(serializer_);
   scoped_ptr<std::string> data(new std::string);
   if (serializer_->SerializeData(data.get())) {
     WriteNow(data.Pass());
   } else {
     DLOG(WARNING) << "failed to serialize data to be saved in "
-                  << path_.value().c_str();
+                  << path_.value();
   }
-  serializer_ = NULL;
+  serializer_ = nullptr;
 }
 
 void ImportantFileWriter::RegisterOnNextSuccessfulWriteCallback(
-    const base::Closure& on_next_successful_write) {
+    const Closure& on_next_successful_write) {
   DCHECK(on_next_successful_write_.is_null());
   on_next_successful_write_ = on_next_successful_write;
 }
 
 bool ImportantFileWriter::PostWriteTask(const Callback<bool()>& task) {
   // TODO(gab): This code could always use PostTaskAndReplyWithResult and let
   // ForwardSuccessfulWrite() no-op if |on_next_successful_write_| is null, but
   // PostTaskAndReply causes memory leaks in tests (crbug.com/371974) and
   // suppressing all of those is unrealistic hence we avoid most of them by
   // using PostTask() in the typical scenario below.
   if (!on_next_successful_write_.is_null()) {
-    return base::PostTaskAndReplyWithResult(
+    return PostTaskAndReplyWithResult(
         task_runner_.get(),
         FROM_HERE,
         MakeCriticalClosure(task),
         Bind(&ImportantFileWriter::ForwardSuccessfulWrite,
              weak_factory_.GetWeakPtr()));
   }
   return task_runner_->PostTask(
       FROM_HERE,
-      MakeCriticalClosure(base::Bind(IgnoreResult(task))));
+      MakeCriticalClosure(Bind(IgnoreResult(task))));
 }
 
 void ImportantFileWriter::ForwardSuccessfulWrite(bool result) {
   DCHECK(CalledOnValidThread());
   if (result && !on_next_successful_write_.is_null()) {
     on_next_successful_write_.Run();
     on_next_successful_write_.Reset();
   }
 }
 
 }  // namespace base