|
|
Created:
6 years, 10 months ago by rvargas (doing something else) Modified:
6 years, 8 months ago CC:
chromium-reviews, erikwright+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionBase: Introduce a new version of FileUtilProxy that works with File.
BUG=322664
TEST=net_unittests
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=257290
Patch Set 1 : #
Total comments: 10
Patch Set 2 : Add comment #Patch Set 3 : Add DCHECKs #
Total comments: 4
Patch Set 4 : Review comments #Patch Set 5 : Rebase #Patch Set 6 : Changed FileInfo test #Patch Set 7 : Add some times to GetInfo test #
Total comments: 1
Messages
Total messages: 23 (0 generated)
PTAL
GetFileInfo_File seems broken on linux_chromeos? https://codereview.chromium.org/180243015/diff/100001/base/files/file_proxy.cc File base/files/file_proxy.cc (right): https://codereview.chromium.org/180243015/diff/100001/base/files/file_proxy.c... base/files/file_proxy.cc:217: FileProxy::~FileProxy() { This may lead to close a file on non-blocking thread? Should we call Close() if file_ is valid? https://codereview.chromium.org/180243015/diff/100001/base/files/file_proxy.h File base/files/file_proxy.h (right): https://codereview.chromium.org/180243015/diff/100001/base/files/file_proxy.h... base/files/file_proxy.h:50: void set_task_runner(TaskRunner* task_runner) { Why do we want to have this separate method? https://codereview.chromium.org/180243015/diff/100001/base/files/file_proxy.h... base/files/file_proxy.h:75: bool IsValid() const; This'd also need a comment (as proxy's validity == file's validity may not be obvious to everyone)
Thanks https://codereview.chromium.org/180243015/diff/100001/base/files/file_proxy.cc File base/files/file_proxy.cc (right): https://codereview.chromium.org/180243015/diff/100001/base/files/file_proxy.c... base/files/file_proxy.cc:217: FileProxy::~FileProxy() { On 2014/03/03 05:58:58, kinuko wrote: > This may lead to close a file on non-blocking thread? Should we call Close() if > file_ is valid? Correct. I think that would be an error that would be caught by the thread restriction code. If it is not, it would mean that the operation was valid on the current thread (maybe at shutdown, when the IO restrictions may be lifted). The unit tests is an example of code that actually relies on the cleanup happening when this object is destroyed, without proxying Close to another thread (of course, could be changed to proxy the call). Another option would be to assume automatic cleanup at destruction and remove Close from the API, but that is a little complicated given that all the API returns false when posting a task fails... BTW, another complication is that it is valid to do proxy.Write(); delete proxy; and even if we check if the object is valid at destruction we will miss the operation in progress and closing the file will happen on the main thread. https://codereview.chromium.org/180243015/diff/100001/base/files/file_proxy.h File base/files/file_proxy.h (right): https://codereview.chromium.org/180243015/diff/100001/base/files/file_proxy.h... base/files/file_proxy.h:50: void set_task_runner(TaskRunner* task_runner) { On 2014/03/03 05:58:58, kinuko wrote: > Why do we want to have this separate method? Maybe we don't :). I'm assuming that someone will have a FileProxy member variable without knowing at construction time what thread to use. Happy to remove it if you want me to. https://codereview.chromium.org/180243015/diff/100001/base/files/file_proxy.h... base/files/file_proxy.h:75: bool IsValid() const; On 2014/03/03 05:58:58, kinuko wrote: > This'd also need a comment (as proxy's validity == file's validity may not be > obvious to everyone) Done.
lgtm https://codereview.chromium.org/180243015/diff/100001/base/files/file_proxy.cc File base/files/file_proxy.cc (right): https://codereview.chromium.org/180243015/diff/100001/base/files/file_proxy.c... base/files/file_proxy.cc:217: FileProxy::~FileProxy() { Ok... not performing Close automatically in dtor also makes sense. Could we have a comment about the behavior? Calling proxy.Write() and forgetting about proxy looks like the easiest mistake we could make. https://codereview.chromium.org/180243015/diff/100001/base/files/file_proxy.h File base/files/file_proxy.h (right): https://codereview.chromium.org/180243015/diff/100001/base/files/file_proxy.h... base/files/file_proxy.h:50: void set_task_runner(TaskRunner* task_runner) { On 2014/03/03 20:38:34, rvargas wrote: > On 2014/03/03 05:58:58, kinuko wrote: > > Why do we want to have this separate method? > > Maybe we don't :). I'm assuming that someone will have a FileProxy member > variable without knowing at construction time what thread to use. > > Happy to remove it if you want me to. If we don't have the case yet I think not having this is simpler/cleaner. (I'm totally happy with having/re-adding it if necessary) https://codereview.chromium.org/180243015/diff/220001/base/files/file_proxy_u... File base/files/file_proxy_unittest.cc (right): https://codereview.chromium.org/180243015/diff/220001/base/files/file_proxy_u... base/files/file_proxy_unittest.cc:95: FileProxy proxy_; Not used? https://codereview.chromium.org/180243015/diff/220001/base/files/file_proxy_u... base/files/file_proxy_unittest.cc:271: File::FLAG_CREATE | File::FLAG_WRITE | File::FLAG_WRITE_ATTRIBUTES, &proxy); nit: indent
Thanks! +ajwong for owners https://codereview.chromium.org/180243015/diff/100001/base/files/file_proxy.cc File base/files/file_proxy.cc (right): https://codereview.chromium.org/180243015/diff/100001/base/files/file_proxy.c... base/files/file_proxy.cc:217: FileProxy::~FileProxy() { On 2014/03/04 04:07:45, kinuko wrote: > Ok... not performing Close automatically in dtor also makes sense. Could we have > a comment about the behavior? Calling proxy.Write() and forgetting about proxy > looks like the easiest mistake we could make. I added a comment to the class description. https://codereview.chromium.org/180243015/diff/100001/base/files/file_proxy.h File base/files/file_proxy.h (right): https://codereview.chromium.org/180243015/diff/100001/base/files/file_proxy.h... base/files/file_proxy.h:50: void set_task_runner(TaskRunner* task_runner) { On 2014/03/04 04:07:45, kinuko wrote: > On 2014/03/03 20:38:34, rvargas wrote: > > On 2014/03/03 05:58:58, kinuko wrote: > > > Why do we want to have this separate method? > > > > Maybe we don't :). I'm assuming that someone will have a FileProxy member > > variable without knowing at construction time what thread to use. > > > > Happy to remove it if you want me to. > > If we don't have the case yet I think not having this is simpler/cleaner. (I'm > totally happy with having/re-adding it if necessary) Done. https://codereview.chromium.org/180243015/diff/220001/base/files/file_proxy_u... File base/files/file_proxy_unittest.cc (right): https://codereview.chromium.org/180243015/diff/220001/base/files/file_proxy_u... base/files/file_proxy_unittest.cc:95: FileProxy proxy_; On 2014/03/04 04:07:45, kinuko wrote: > Not used? removed https://codereview.chromium.org/180243015/diff/220001/base/files/file_proxy_u... base/files/file_proxy_unittest.cc:271: File::FLAG_CREATE | File::FLAG_WRITE | File::FLAG_WRITE_ATTRIBUTES, &proxy); On 2014/03/04 04:07:45, kinuko wrote: > nit: indent Done.
ping (awong)
Brett, do you mind taking a look for owners?
Finally got around to this. LGTM and removed Brett from owners file. Sorry for slow response. On 2014/03/11 00:37:44, rvargas wrote: > Brett, do you mind taking a look for owners?
Thanks!
The CQ bit was checked by rvargas@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rvargas@chromium.org/180243015/240001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for base/files/file_proxy.cc: While running patch -p1 --forward --force --no-backup-if-mismatch; A base/files/file_proxy.cc Copied base/files/file_util_proxy.cc -> base/files/file_proxy.cc patching file base/files/file_proxy.cc Hunk #2 FAILED at 15. Hunk #3 succeeded at 174 (offset 9 lines). Hunk #4 succeeded at 204 (offset 9 lines). 1 out of 4 hunks FAILED -- saving rejects to file base/files/file_proxy.cc.rej Patch: NR base/files/file_util_proxy.cc->base/files/file_proxy.cc Index: base/files/file_proxy.cc diff --git a/base/files/file_util_proxy.cc b/base/files/file_proxy.cc similarity index 16% copy from base/files/file_util_proxy.cc copy to base/files/file_proxy.cc index 141e4e10606189969d8c27e5eac6d915d8e1e0fc..af27c9d4a5a4d74088ef46df0bbf1a0d6b173739 100644 --- a/base/files/file_util_proxy.cc +++ b/base/files/file_proxy.cc @@ -2,7 +2,7 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. -#include "base/files/file_util_proxy.h" +#include "base/files/file_proxy.h" #include "base/bind.h" #include "base/bind_helpers.h" @@ -15,147 +15,158 @@ namespace base { +class FileHelper { + public: + FileHelper(FileProxy* proxy, File file) + : file_(file.Pass()), + proxy_(AsWeakPtr(proxy)), + error_(File::FILE_ERROR_FAILED) { + } + + void PassFile() { + if (proxy_) + proxy_->SetFile(file_.Pass()); + } + + protected: + File file_; + WeakPtr<FileProxy> proxy_; + File::Error error_; + + private: + DISALLOW_COPY_AND_ASSIGN(FileHelper); +}; + namespace { -void CallWithTranslatedParameter(const FileUtilProxy::StatusCallback& callback, - bool value) { - DCHECK(!callback.is_null()); - callback.Run(value ? File::FILE_OK : File::FILE_ERROR_FAILED); -} +class GenericFileHelper : public FileHelper { + public: + GenericFileHelper(FileProxy* proxy, File file) + : FileHelper(proxy, file.Pass()) { + } + + void Close() { + file_.Close(); + error_ = File::FILE_OK; + } + + void SetTimes(Time last_access_time, Time last_modified_time) { + bool rv = file_.SetTimes(last_access_time, last_modified_time); + error_ = rv ? File::FILE_OK : File::FILE_ERROR_FAILED; + } -// Helper classes or routines for individual methods. -class CreateOrOpenHelper { + void SetLength(int64 length) { + if (file_.SetLength(length)) + error_ = File::FILE_OK; + } + + void Flush() { + if (file_.Flush()) + error_ = File::FILE_OK; + } + + void Reply(const FileProxy::StatusCallback& callback) { + PassFile(); + if (!callback.is_null()) + callback.Run(error_); + } + + private: + DISALLOW_COPY_AND_ASSIGN(GenericFileHelper); +}; + +class CreateOrOpenHelper : public FileHelper { public: - CreateOrOpenHelper(TaskRunner* task_runner, - const FileUtilProxy::CloseTask& close_task) - : task_runner_(task_runner), - close_task_(close_task), - file_handle_(kInvalidPlatformFileValue), - created_(false), - error_(File::FILE_OK) {} - - ~CreateOrOpenHelper() { - if (file_handle_ != kInvalidPlatformFileValue) { - task_runner_->PostTask( - FROM_HERE, - base::Bind(base::IgnoreResult(close_task_), file_handle_)); - } + CreateOrOpenHelper(FileProxy* proxy, File file) + : FileHelper(proxy, file.Pass()) { } - void RunWork(const FileUtilProxy::CreateOrOpenTask& task) { - error_ = task.Run(&file_handle_, &created_); + void RunWork(const FilePath& file_path, int file_flags) { + file_.Initialize(file_path, file_flags); + error_ = file_.IsValid() ? File::FILE_OK : file_.error_details(); } - void Reply(const FileUtilProxy::CreateOrOpenCallback& callback) { + void Reply(const FileProxy::StatusCallback& callback) { DCHECK(!callback.is_null()); - callback.Run(error_, PassPlatformFile(&file_handle_), created_); + PassFile(); + callback.Run(error_); } private: - scoped_refptr<TaskRunner> task_runner_; - FileUtilProxy::CloseTask close_task_; - PlatformFile file_handle_; - bool created_; - File::Error error_; DISALLOW_COPY_AND_ASSIGN(CreateOrOpenHelper); }; -class CreateTemporaryHelper { +class CreateTemporaryHelper : public FileHelper { public: - explicit CreateTemporaryHelper(TaskRunner* task_runner) - : task_runner_(task_runner), - file_handle_(kInvalidPlatformFileValue), - error_(File::FILE_OK) {} - - ~CreateTemporaryHelper() { - if (file_handle_ != kInvalidPlatformFileValue) { - FileUtilProxy::Close( - task_runner_.get(), file_handle_, FileUtilProxy::StatusCallback()); - } + CreateTemporaryHelper(FileProxy* proxy, File file) + : FileHelper(proxy, file.Pass()) { } - void RunWork(int additional_file_flags) { + void RunWork(uint32 additional_file_flags) { // TODO(darin): file_util should have a variant of CreateTemporaryFile - // that returns a FilePath and a PlatformFile. - base::CreateTemporaryFile(&file_path_); + // that returns a FilePath and a File. + CreateTemporaryFile(&file_path_); - int file_flags = - PLATFORM_FILE_WRITE | - PLATFORM_FILE_TEMPORARY | - PLATFORM_FILE_CREATE_ALWAYS | - additional_file_flags; + uint32 file_flags = File::FLAG_WRITE | + File::FLAG_TEMPORARY | + File::FLAG_CREATE_ALWAYS | + additional_file_flags; - error_ = File::FILE_OK; - // TODO(rvargas): Convert this code to use File. - file_handle_ = - CreatePlatformFile(file_path_, file_flags, NULL, - reinterpret_cast<PlatformFileError*>(&error_)); + file_.Initialize(file_path_, file_flags); + error_ = file_.IsValid() ? File::FILE_OK : file_.error_details(); } - void Reply(const FileUtilProxy::CreateTemporaryCallback& callback) { + void Reply(const FileProxy::CreateTemporaryCallback& callback) { DCHECK(!callback.is_null()); - callback.Run(error_, PassPlatformFile(&file_handle_), file_path_); + PassFile(); + callback.Run(error_, file_path_); } private: - scoped_refptr<TaskRunner> task_runner_; - PlatformFile file_handle_; FilePath file_path_; - File::Error error_; DISALLOW_COPY_AND_ASSIGN(CreateTemporaryHelper); }; -class GetFileInfoHelper { +class GetInfoHelper : public FileHelper { public: - GetFileInfoHelper() - : error_(File::FILE_OK) {} - - void RunWorkForFilePath(const FilePath& file_path) { - if (!PathExists(file_path)) { - error_ = File::FILE_ERROR_NOT_FOUND; - return; - } - // TODO(rvargas): switch this file to base::File. - if (!GetFileInfo(file_path, reinterpret_cast<File::Info*>(&file_info_))) - error_ = File::FILE_ERROR_FAILED; + GetInfoHelper(FileProxy* proxy, File file) + : FileHelper(proxy, file.Pass()) { } - void RunWorkForPlatformFile(PlatformFile file) { - if (!GetPlatformFileInfo( - file, reinterpret_cast<PlatformFileInfo*>(&file_info_))) { - error_ = File::FILE_ERROR_FAILED; - } + void RunWork() { + if (file_.GetInfo(&file_info_)) + error_ = File::FILE_OK; } - void Reply(const FileUtilProxy::GetFileInfoCallback& callback) { - if (!callback.is_null()) { - callback.Run(error_, file_info_); - } + void Reply(const FileProxy::GetFileInfoCallback& callback) { + PassFile(); + DCHECK(!callback.is_null()); + callback.Run(error_, file_info_); } private: - File::Error error_; File::Info file_info_; - DISALLOW_COPY_AND_ASSIGN(GetFileInfoHelper); + DISALLOW_COPY_AND_ASSIGN(GetInfoHelper); }; -class ReadHelper { +class ReadHelper : public FileHelper { public: - explicit ReadHelper(int bytes_to_read) - : buffer_(new char[bytes_to_read]), + ReadHelper(FileProxy* proxy, File file, int bytes_to_read) + : FileHelper(proxy, file.Pass()), + buffer_(new char[bytes_to_read]), bytes_to_read_(bytes_to_read), - bytes_read_(0) {} + bytes_read_(0) { + } - void RunWork(PlatformFile file, int64 offset) { - bytes_read_ = ReadPlatformFile(file, offset, buffer_.get(), bytes_to_read_); + void RunWork(int64 offset) { + bytes_read_ = file_.Read(offset, buffer_.get(), bytes_to_read_); + error_ = (bytes_read_ < 0) ? File::FILE_ERROR_FAILED : File::FILE_OK; } - void Reply(const FileUtilProxy::ReadCallback& callback) { - if (!callback.is_null()) { - File::Error error = - (bytes_read_ < 0) ? File::FILE_ERROR_FAILED : File::FILE_OK; - callback.Run(error, buffer_.get(), bytes_read_); - } + void Reply(const FileProxy::ReadCallback& callback) { + PassFile(); + DCHECK(!callback.is_null()); + callback.Run(error_, buffer_.get(), bytes_read_); } private: @@ -165,26 +176,27 @@ class ReadHelper { DISALLOW_COPY_AND_ASSIGN(ReadHelper); }; -class WriteHelper { +class WriteHelper : public FileHelper { public: - WriteHelper(const char* buffer, int bytes_to_write) - : buffer_(new char[bytes_to_write]), + WriteHelper(FileProxy* proxy, + File file, + const char* buffer, int bytes_to_write) + : FileHelper(proxy, file.Pass()), + buffer_(new char[bytes_to_write]), bytes_to_write_(bytes_to_write), bytes_written_(0) { memcpy(buffer_.get(), buffer, bytes_to_write); } - void RunWork(PlatformFile file, int64 offset) { - bytes_written_ = WritePlatformFile(file, offset, buffer_.get(), - bytes_to_write_); + void RunWork(int64 offset) { + bytes_written_ = file_.Write(offset, buffer_.get(), bytes_to_write_); + error_ = (bytes_written_ < 0) ? File::FILE_ERROR_FAILED : File::FILE_OK; } - void Reply(const FileUtilProxy::WriteCallback& callback) { - if (!callback.is_null()) { - File::Error error = - (bytes_written_ < 0) ? File::FILE_… (message too large)
Kinuko: I modified the unit test for GetInfo because one of the bots was failing with mismatches on the timestamp. While I don't fully see why there was a mismatch on all times, the test was not really doing what the original test was doing, as it goes through the equivalent of GetFileInfoFromPlatformFile while the old test uses GetFileInfo. So I removed the offending lines. Do you mind taking a quick look?
On 2014/03/12 01:17:17, rvargas wrote: > Kinuko: I modified the unit test for GetInfo because one of the bots was failing > with mismatches on the timestamp. > > While I don't fully see why there was a mismatch on all times, the test was not > really doing what the original test was doing, as it goes through the equivalent > of GetFileInfoFromPlatformFile while the old test uses GetFileInfo. > > So I removed the offending lines. Do you mind taking a quick look? GetFileInfo and File::GetInfo() seems to be doing slightly different conversion to get the file time info. Assuming it's failing on linux_chromium_rel, GetFileInfo (in file_util_posix.cc) calculates it using Time::FromTimeSpec (which is nano-sec precision), while File.GetInfo (in file_posix.cc) calculates it using TimeDelta (which seems to be microsec precision if I read the code right). Wasn't the diff coming from there? If so can we have a test with rounded time values, or more preferably can we match the impl for File and FileUtil?
On 2014/03/12 04:16:12, kinuko wrote: > On 2014/03/12 01:17:17, rvargas wrote: > > Kinuko: I modified the unit test for GetInfo because one of the bots was > failing > > with mismatches on the timestamp. > > > > While I don't fully see why there was a mismatch on all times, the test was > not > > really doing what the original test was doing, as it goes through the > equivalent > > of GetFileInfoFromPlatformFile while the old test uses GetFileInfo. > > > > So I removed the offending lines. Do you mind taking a quick look? > > GetFileInfo and File::GetInfo() seems to be doing slightly different conversion > to get the file time info. Assuming it's failing on linux_chromium_rel, > GetFileInfo (in file_util_posix.cc) calculates it using Time::FromTimeSpec > (which is nano-sec precision), while File.GetInfo (in file_posix.cc) calculates > it using TimeDelta (which seems to be microsec precision if I read the code > right). Wasn't the diff coming from there? If so can we have a test with > rounded time values, or more preferably can we match the impl for File and > FileUtil? That's it, thanks!. I can round the time values, but that would be a test trying to match up the implementation of two parts of code that are not under test (File and file_util). I think FileProxy should just proxy File and do whatever File does... so I added back some times by querying the file directly again. It also feels a little weird so let me know if that's ok or if you prefer something else. Ideally all implementations of GetFileInfo should match (in fact there should be only one!). The problem is that when GetPlatformFileInfo was upgraded to use nano-second values, file_util::GetFileInfo was left behind... but file_util.h depends on file.h, so it's not nice to require file_util to implement file.cc. I'll open a bug about that.
Kinuko: I'm assuming that you're OK with the latest version. If that was not the case, let me know and I'll fix it in another CL (assuming that the CQ is happy as things are).
The CQ bit was checked by rvargas@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rvargas@chromium.org/180243015/370001
Message was sent while issue was closed.
Change committed as 257290
Message was sent while issue was closed.
https://codereview.chromium.org/180243015/diff/370001/base/files/file_proxy_u... File base/files/file_proxy_unittest.cc (right): https://codereview.chromium.org/180243015/diff/370001/base/files/file_proxy_u... base/files/file_proxy_unittest.cc:214: EXPECT_TRUE(file.GetInfo(&expected_info)); Sorry I hadn't responded quickly... I'm ok with this change, but this line'd likely confuse readers in a future. Would it be possible to make a small follow-up change and add a comment why we do here, along with the bug number you're opening for the issue that we use different impl (which leads to have different precision)? Thanks.
Message was sent while issue was closed.
On 2014/03/17 03:50:27, kinuko wrote: > https://codereview.chromium.org/180243015/diff/370001/base/files/file_proxy_u... > File base/files/file_proxy_unittest.cc (right): > > https://codereview.chromium.org/180243015/diff/370001/base/files/file_proxy_u... > base/files/file_proxy_unittest.cc:214: > EXPECT_TRUE(file.GetInfo(&expected_info)); > Sorry I hadn't responded quickly... I'm ok with this change, but this line'd > likely confuse readers in a future. Would it be possible to make a small > follow-up change and add a comment why we do here, along with the bug number > you're opening for the issue that we use different impl (which leads to have > different precision)? Thanks. Sorry for taking so long... I was hoping the fix would take just a couple of days. Anyway, File::GetInfo and file_util::GetFileInfo are unified as of r261956.
Message was sent while issue was closed.
On 2014/04/07 14:58:20, rvargas wrote: > On 2014/03/17 03:50:27, kinuko wrote: > > > https://codereview.chromium.org/180243015/diff/370001/base/files/file_proxy_u... > > File base/files/file_proxy_unittest.cc (right): > > > > > https://codereview.chromium.org/180243015/diff/370001/base/files/file_proxy_u... > > base/files/file_proxy_unittest.cc:214: > > EXPECT_TRUE(file.GetInfo(&expected_info)); > > Sorry I hadn't responded quickly... I'm ok with this change, but this line'd > > likely confuse readers in a future. Would it be possible to make a small > > follow-up change and add a comment why we do here, along with the bug number > > you're opening for the issue that we use different impl (which leads to have > > different precision)? Thanks. > > Sorry for taking so long... I was hoping the fix would take just a couple of > days. Anyway, File::GetInfo and file_util::GetFileInfo are unified as of > r261956. Cooool! Thanks. |