Remove ExtensionFunction::SetResult(T*) overload.

chrome/browser/extensions/api/file_system/file_system_api.cc

 // Copyright (c) 2012 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/extensions/api/file_system/file_system_api.h"
 
 #include <stddef.h>
 
+#include <memory>
 #include <set>
+#include <utility>
 #include <vector>
 
 #include "apps/saved_files_service.h"
 #include "base/bind.h"
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
 #include "base/logging.h"
 #include "base/macros.h"
 #include "base/memory/linked_ptr.h"
 #include "base/memory/ptr_util.h"
@@ skipping 460 matching lines @@
   EXTENSION_FUNCTION_VALIDATE(args_->GetString(0, &filesystem_name));
   EXTENSION_FUNCTION_VALIDATE(args_->GetString(1, &filesystem_path));
 
   base::FilePath file_path;
   if (!app_file_handler_util::ValidateFileEntryAndGetPath(
           filesystem_name, filesystem_path,
           render_frame_host()->GetProcess()->GetID(), &file_path, &error_))
     return false;
 
   file_path = path_util::PrettifyPath(file_path);
-  SetResult(new base::StringValue(file_path.value()));
+  SetResult(base::MakeUnique<base::StringValue>(file_path.value()));
   return true;
 }
 
 FileSystemEntryFunction::FileSystemEntryFunction()
-    : multiple_(false),
-      is_directory_(false),
-      response_(NULL) {}
+    : multiple_(false), is_directory_(false) {}
 
 void FileSystemEntryFunction::PrepareFilesForWritableApp(
     const std::vector<base::FilePath>& paths) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   // TODO(cmihail): Path directory set should be initialized only with the
   // paths that are actually directories, but for now we will consider
   // all paths directories in case is_directory_ is true, otherwise
   // all paths files, as this was the previous logic.
   std::set<base::FilePath> path_directory_set_ =
       is_directory_ ? std::set<base::FilePath>(paths.begin(), paths.end())
                     : std::set<base::FilePath>{};
   app_file_handler_util::PrepareFilesForWritableApp(
       paths, GetProfile(), path_directory_set_,
       base::Bind(&FileSystemEntryFunction::RegisterFileSystemsAndSendResponse,
                  this, paths),
       base::Bind(&FileSystemEntryFunction::HandleWritableFileError, this));
 }
 
 void FileSystemEntryFunction::RegisterFileSystemsAndSendResponse(
     const std::vector<base::FilePath>& paths) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   if (!render_frame_host())
     return;
 
-  CreateResponse();
-  for (std::vector<base::FilePath>::const_iterator it = paths.begin();
-       it != paths.end(); ++it) {
-    AddEntryToResponse(*it, "");
-  }
+  std::unique_ptr<base::DictionaryValue> result = CreateResult();
+  for (const auto& path : paths)
+    AddEntryToResult(path, std::string(), result.get());
+  SetResult(std::move(result));
   SendResponse(true);
 }
 
-void FileSystemEntryFunction::CreateResponse() {
-  DCHECK(!response_);
-  response_ = new base::DictionaryValue();
-  base::ListValue* list = new base::ListValue();
-  response_->Set("entries", list);
-  response_->SetBoolean("multiple", multiple_);
-  SetResult(response_);
+std::unique_ptr<base::DictionaryValue> FileSystemEntryFunction::CreateResult() {
+  std::unique_ptr<base::DictionaryValue> result(new base::DictionaryValue());
+  result->Set("entries", base::MakeUnique<base::ListValue>());
+  result->SetBoolean("multiple", multiple_);
+  return result;
 }
 
-void FileSystemEntryFunction::AddEntryToResponse(
-    const base::FilePath& path,
-    const std::string& id_override) {
-  DCHECK(response_);
+void FileSystemEntryFunction::AddEntryToResult(const base::FilePath& path,
+                                               const std::string& id_override,
+                                               base::DictionaryValue* result) {
   GrantedFileEntry file_entry = app_file_handler_util::CreateFileEntry(
       GetProfile(),
       extension(),
       render_frame_host()->GetProcess()->GetID(),
       path,
       is_directory_);
   base::ListValue* entries;
-  bool success = response_->GetList("entries", &entries);
+  bool success = result->GetList("entries", &entries);

[Devlin, 2016/05/20 17:56:54]

nit: this is silly, since it means we repeat this look up each time we call into
AddEntryToResult.  Since you're changing the signature anyway, can you just pass
in the base::ListValue* entries?

[dcheng, 2016/05/20 18:11:57]

It's a map internally, so lookup is reasonably timed. And casting from Value to
ListValue is constant time.

The alternatives are:
- copy and paste these two lines into two different places
- have CreateResult() return a tuple with an owned DictionaryValue and an
unowned ListValue

Both seem worse than the current version.

[Devlin, 2016/05/20 20:56:18]

Ah, I didn't notice it's called from two places.  Nevermind.

   DCHECK(success);
 
   base::DictionaryValue* entry = new base::DictionaryValue();
   entry->SetString("fileSystemId", file_entry.filesystem_id);
   entry->SetString("baseName", file_entry.registered_name);
   if (id_override.empty())
     entry->SetString("id", file_entry.id);
   else
     entry->SetString("id", id_override);
   entry->SetBoolean("isDirectory", is_directory_);
@@ skipping 67 matching lines @@
     error_ = app_file_handler_util::kInvalidParameters;
     return false;
   }
 
   content::ChildProcessSecurityPolicy* policy =
       content::ChildProcessSecurityPolicy::GetInstance();
   int renderer_id = render_frame_host()->GetProcess()->GetID();
   bool is_writable = policy->CanReadWriteFileSystem(renderer_id,
                                                     filesystem_id);
 
-  SetResult(new base::FundamentalValue(is_writable));
+  SetResult(base::MakeUnique<base::FundamentalValue>(is_writable));
   return true;
 }
 
 // Handles showing a dialog to the user to ask for the filename for a file to
 // save or open.
 class FileSystemChooseEntryFunction::FilePicker
     : public ui::SelectFileDialog::Listener {
  public:
   FilePicker(FileSystemChooseEntryFunction* function,
              content::WebContents* web_contents,
@@ skipping 545 matching lines @@
   SavedFilesService* saved_files_service = SavedFilesService::Get(GetProfile());
   saved_files_service->RegisterFileEntry(
       extension_->id(), entry_id, path, file_info->is_directory);
   saved_files_service->EnqueueFileEntry(extension_->id(), entry_id);
   SendResponse(true);
 }
 
 bool FileSystemIsRestorableFunction::RunSync() {
   std::string entry_id;
   EXTENSION_FUNCTION_VALIDATE(args_->GetString(0, &entry_id));
-  SetResult(new base::FundamentalValue(SavedFilesService::Get(
-      GetProfile())->IsRegistered(extension_->id(), entry_id)));
+  SetResult(base::MakeUnique<base::FundamentalValue>(
+      SavedFilesService::Get(GetProfile())
+          ->IsRegistered(extension_->id(), entry_id)));
   return true;
 }
 
 bool FileSystemRestoreEntryFunction::RunAsync() {
   std::string entry_id;
   bool needs_new_entry;
   EXTENSION_FUNCTION_VALIDATE(args_->GetString(0, &entry_id));
   EXTENSION_FUNCTION_VALIDATE(args_->GetBoolean(1, &needs_new_entry));
   const SavedFileEntry* file_entry = SavedFilesService::Get(
       GetProfile())->GetFileEntry(extension_->id(), entry_id);
   if (!file_entry) {
     error_ = kUnknownIdError;
     return false;
   }
 
   SavedFilesService::Get(GetProfile())
       ->EnqueueFileEntry(extension_->id(), entry_id);
 
   // Only create a new file entry if the renderer requests one.
   // |needs_new_entry| will be false if the renderer already has an Entry for
   // |entry_id|.
   if (needs_new_entry) {
     is_directory_ = file_entry->is_directory;
-    CreateResponse();
-    AddEntryToResponse(file_entry->path, file_entry->id);
+    std::unique_ptr<base::DictionaryValue> result = CreateResult();
+    AddEntryToResult(file_entry->path, file_entry->id, result.get());
+    SetResult(std::move(result));
   }
   SendResponse(true);
   return true;
 }
 
 bool FileSystemObserveDirectoryFunction::RunSync() {
   NOTIMPLEMENTED();
   error_ = kUnknownIdError;
   return false;
 }
@@ skipping 176 matching lines @@
     policy->GrantCopyInto(render_frame_host()->GetProcess()->GetID(),
                           volume->mount_path());
     policy->GrantWriteFileSystem(render_frame_host()->GetProcess()->GetID(),
                                  file_system_id);
     policy->GrantDeleteFromFileSystem(
         render_frame_host()->GetProcess()->GetID(), file_system_id);
     policy->GrantCreateFileForFileSystem(
         render_frame_host()->GetProcess()->GetID(), file_system_id);
   }
 
-  base::DictionaryValue* const dict = new base::DictionaryValue();
+  std::unique_ptr<base::DictionaryValue> dict(new base::DictionaryValue());
   dict->SetString("file_system_id", file_system_id);
   dict->SetString("file_system_path", register_name);
 
-  SetResult(dict);
+  SetResult(std::move(dict));
   SendResponse(true);
 }
 
 FileSystemGetVolumeListFunction::FileSystemGetVolumeListFunction()
     : chrome_details_(this) {
 }
 
 FileSystemGetVolumeListFunction::~FileSystemGetVolumeListFunction() {
 }
 
 ExtensionFunction::ResponseAction FileSystemGetVolumeListFunction::Run() {
   // Only kiosk apps in kiosk sessions can use this API.
   // Additionally it is enabled for whitelisted component extensions and apps.
   file_system_api::ConsentProviderDelegate consent_provider_delegate(
       chrome_details_.GetProfile(), render_frame_host());
   file_system_api::ConsentProvider consent_provider(&consent_provider_delegate);
 
   if (!consent_provider.IsGrantable(*extension()))
     return RespondNow(Error(kNotSupportedOnNonKioskSessionError));
   std::vector<api::file_system::Volume> result_volume_list;
   FillVolumeList(chrome_details_.GetProfile(), &result_volume_list);
 
   return RespondNow(ArgumentList(
       api::file_system::GetVolumeList::Results::Create(result_volume_list)));
 }
 #endif
 
 }  // namespace extensions