Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
6905095:
Integrated test with the actual QuotaManager and PathManager. (Closed)
|
Created:
9 years, 8 months ago by Dai Mikurube (NOT FULLTIME) Modified:
9 years, 6 months ago Reviewers:
kinuko CC:
chromium-reviews, pam+watch_chromium.org, kinuko+watch, darin-cc_chromium.org, Paweł Hajdan Jr. Visibility:
Public. |
DescriptionIntegrated test with the actual QuotaManager and PathManager.
BUG=86016
TEST=FileSystemQuotaTest.*
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=88965
Patch Set 1 #
Total comments: 4
Patch Set 2 : Rebased. (It causes signal 11.) #
Total comments: 5
Patch Set 3 : It works with the notification patch. #Patch Set 4 : Catching-up. #
Total comments: 34
Patch Set 5 : Rebased and reflected the comments. #
Total comments: 24
Patch Set 6 : Reflected the comments, and rebased. #
Total comments: 2
Patch Set 7 : Reflected the comments. #
Messages
Total messages: 13 (0 generated)
1st cut. It doesn't work yet (because of no notification).
Drive-by with a testing nit, no need to wait for me. http://codereview.chromium.org/6905095/diff/1/webkit/fileapi/file_system_quot... File webkit/fileapi/file_system_quota_unittest.cc (right): http://codereview.chromium.org/6905095/diff/1/webkit/fileapi/file_system_quot... webkit/fileapi/file_system_quota_unittest.cc:191: NOTREACHED(); nit: Did you mean FAIL/ADD_FAILURE here and below? NOTREACHED() will work only in Debug mode and not Release mode, and will crash entire test binary. http://codereview.chromium.org/6905095/diff/1/webkit/fileapi/file_system_quot... webkit/fileapi/file_system_quota_unittest.cc:383: #if 0 A general tip (I assume you will update it before committing anyway): it is much better to use DISABLED_ instead of #if 0, because the latter usually results in super-fast rot. DISABLED_ tests may become broken at runtime, but at least they will always compile.
Hi Pawel, Thank you for the comments. I didn't fix it in the patch set 2, but will fix. Kinuko-san, This rebased version caused segmentation fault in TestMoveSuccessSrcDirRecursive2 at the line 339 (RunAllPending)... It's completely identical with TestMoveSuccessSrcDirRecursive. The patch is based on 6865008.
Ok now I think I found how it gets segv. http://codereview.chromium.org/6905095/diff/3001/webkit/fileapi/file_system_q... File webkit/fileapi/file_system_quota_unittest.cc (right): http://codereview.chromium.org/6905095/diff/3001/webkit/fileapi/file_system_q... webkit/fileapi/file_system_quota_unittest.cc:128: scoped_ptr<FileSystemPathManager> path_manager_; path_manager is owned by file_system_context, so you don't need to make it scoped_ptr here. http://codereview.chromium.org/6905095/diff/3001/webkit/fileapi/file_system_q... webkit/fileapi/file_system_quota_unittest.cc:224: path_manager_.get()); The FSContext takes over the path manager's ownership here http://codereview.chromium.org/6905095/diff/3001/webkit/fileapi/file_system_q... webkit/fileapi/file_system_quota_unittest.cc:236: path_manager_.reset(NULL); No need to reset here. Instead please call MessageLoop::current()->RunPendingAll() to make sure we run all the cleanup tasks that might have been posted by the QM / FSC.
This patch set works for Copy and Move with : * 6903067 (notify changes to QuotaManager) * 6865008 (read usage and quota from QuotaManager) * 6997008 (fix .usage location). http://codereview.chromium.org/6905095/diff/1/webkit/fileapi/file_system_quot... File webkit/fileapi/file_system_quota_unittest.cc (right): http://codereview.chromium.org/6905095/diff/1/webkit/fileapi/file_system_quot... webkit/fileapi/file_system_quota_unittest.cc:191: NOTREACHED(); On 2011/04/29 09:21:51, Paweł Hajdan Jr. wrote: > nit: Did you mean FAIL/ADD_FAILURE here and below? NOTREACHED() will work only > in Debug mode and not Release mode, and will crash entire test binary. Modified to ADD_FAILURE(). http://codereview.chromium.org/6905095/diff/1/webkit/fileapi/file_system_quot... webkit/fileapi/file_system_quota_unittest.cc:383: #if 0 On 2011/04/29 09:21:51, Paweł Hajdan Jr. wrote: > A general tip (I assume you will update it before committing anyway): it is much > better to use DISABLED_ instead of #if 0, because the latter usually results in > super-fast rot. DISABLED_ tests may become broken at runtime, but at least they > will always compile. Yes, I uploaded this patch just to share the current status, not for committing. So I kept it in the next patch, but your suggestion is very helpful. Thank you. :) http://codereview.chromium.org/6905095/diff/3001/webkit/fileapi/file_system_q... webkit/fileapi/file_system_quota_unittest.cc:128: scoped_ptr<FileSystemPathManager> path_manager_; On 2011/05/09 15:20:38, kinuko wrote: > path_manager is owned by file_system_context, so you don't need to make it > scoped_ptr here. Oh, I see. Changed it to a simple pointer. http://codereview.chromium.org/6905095/diff/3001/webkit/fileapi/file_system_q... webkit/fileapi/file_system_quota_unittest.cc:236: path_manager_.reset(NULL); On 2011/05/09 15:20:38, kinuko wrote: > No need to reset here. > > Instead please call MessageLoop::current()->RunPendingAll() to make sure we run > all the cleanup tasks that might have been posted by the QM / FSC. It works. Thanks!
Catching up the current version.
Thanks for retaking this. The test itself looks good, though the test code will need some cleanup (since you forked the test code from old FileSystemOperationTest). Most of comments below are for those cleanups. http://codereview.chromium.org/6905095/diff/15001/webkit/fileapi/file_system_... File webkit/fileapi/file_system_quota_unittest.cc (right): http://codereview.chromium.org/6905095/diff/15001/webkit/fileapi/file_system_... webkit/fileapi/file_system_quota_unittest.cc:56: } Many of them seem to be unnecessary (see the other comment for DidXxx in dispatcher) http://codereview.chromium.org/6905095/diff/15001/webkit/fileapi/file_system_... webkit/fileapi/file_system_quota_unittest.cc:82: } For these two methods (ActualSize and SizeInUsageFile) the test helper provides two helper methods: int64 GetCachedOriginUsage() const; int64 ComputeCurrentOriginUsage() const; Note that ComputeCurrentOriginUsage() always returns the size that doesn't include usage file size (for backward compat) so you'll want to drop '+ usage_file_size' from all the lines you do something like EXPECT_EQ(foo + usage_file_size, ActualSize()) http://codereview.chromium.org/6905095/diff/15001/webkit/fileapi/file_system_... webkit/fileapi/file_system_quota_unittest.cc:122: return FilePath(); For these checks (checking the return value of file_util::CreateTemporary... and returning empty path if it was false) can we simply call EXPECT_xxx for the return value of file_util::CreateTemporary? Returning empty path doesn't look very helpful to me. http://codereview.chromium.org/6905095/diff/15001/webkit/fileapi/file_system_... webkit/fileapi/file_system_quota_unittest.cc:145: FilePath filesystem_dir_path_; Looks like this (and probably two other paths below) doesn't need to be member variables. http://codereview.chromium.org/6905095/diff/15001/webkit/fileapi/file_system_... webkit/fileapi/file_system_quota_unittest.cc:188: virtual void DidGetLocalPath(const FilePath& local_path) { We no longer have this method http://codereview.chromium.org/6905095/diff/15001/webkit/fileapi/file_system_... webkit/fileapi/file_system_quota_unittest.cc:198: test_->set_status(base::PLATFORM_FILE_OK); this doesn't seem to be used ADD_FAILURE() http://codereview.chromium.org/6905095/diff/15001/webkit/fileapi/file_system_... webkit/fileapi/file_system_quota_unittest.cc:204: test_->set_entries(entries); this doesn't seem to be used ADD_FAILURE() http://codereview.chromium.org/6905095/diff/15001/webkit/fileapi/file_system_... webkit/fileapi/file_system_quota_unittest.cc:240: file_util::Delete(usage_file_path_, false); This doesn't look right to me...? If it's for making the computed directory size right, the issue should be resolved if you use test_helper_ methods to get cached usage size / actual size. Or if it's for reseting the usage cache I think we should have a method like ResetUsageCache() in the test helper and call that explicitly in the test code. (Wherever before you're checking EXPECT_EQ(-1, SizeInUsageFile())) I want to make the amount of code code that directly modifies usage cache file as minimal as possible, so that whenever we want to change the usage cache code (e.g. we may want to use levelDb to store the usage cache instead of a plain file) the changes we need to make will be small. http://codereview.chromium.org/6905095/diff/15001/webkit/fileapi/file_system_... webkit/fileapi/file_system_quota_unittest.cc:262: base::PLATFORM_FILE_ASYNC, &created, &error_code); maybe better to check error_code here? http://codereview.chromium.org/6905095/diff/15001/webkit/fileapi/file_system_... webkit/fileapi/file_system_quota_unittest.cc:282: grandchild_file2_ = OpenFile(grandchild_file2_path_); Don't we need to close the opened files in TearDown? http://codereview.chromium.org/6905095/diff/15001/webkit/fileapi/file_system_... webkit/fileapi/file_system_quota_unittest.cc:295: ASSERT_TRUE(base::TruncatePlatformFile(grandchild_file2_, 517)); nit: these days I'm trying to use simpler values like 1, 2, 3... or 10, 200, 3000, ... for test values, since if we do so we can instantly know what went wrong from the test output log. http://codereview.chromium.org/6905095/diff/15001/webkit/fileapi/file_system_... webkit/fileapi/file_system_quota_unittest.cc:296: it might be better to check we have enough quota here (e.g. ASSERT_GT(quota(), all_file_size) etc) http://codereview.chromium.org/6905095/diff/15001/webkit/fileapi/file_system_... webkit/fileapi/file_system_quota_unittest.cc:299: int64 all_file_size = child_file_size + grandchild_file_size; nit: For this test seems like you can directly calculate the all_file_size (without two mostly unsed variables (i.e. child_ and grandchild_)) http://codereview.chromium.org/6905095/diff/15001/webkit/fileapi/file_system_... webkit/fileapi/file_system_quota_unittest.cc:299: int64 all_file_size = child_file_size + grandchild_file_size; nit: const ? http://codereview.chromium.org/6905095/diff/15001/webkit/fileapi/file_system_... webkit/fileapi/file_system_quota_unittest.cc:300: int64 usage_file_size = FileSystemUsageCache::kUsageFileSize; nit: const ? http://codereview.chromium.org/6905095/diff/15001/webkit/fileapi/file_system_... webkit/fileapi/file_system_quota_unittest.cc:309: MessageLoop::current()->RunAllPending(); should check status_ here? http://codereview.chromium.org/6905095/diff/15001/webkit/fileapi/file_system_... webkit/fileapi/file_system_quota_unittest.cc:368: MessageLoop::current()->RunAllPending(); should check status_ here?
Hi, thank you for the comments. Fixed the patch after 2-week interval. http://codereview.chromium.org/6905095/diff/15001/webkit/fileapi/file_system_... File webkit/fileapi/file_system_quota_unittest.cc (right): http://codereview.chromium.org/6905095/diff/15001/webkit/fileapi/file_system_... webkit/fileapi/file_system_quota_unittest.cc:56: } On 2011/06/02 08:04:08, kinuko wrote: > Many of them seem to be unnecessary (see the other comment for DidXxx in > dispatcher) Done. http://codereview.chromium.org/6905095/diff/15001/webkit/fileapi/file_system_... webkit/fileapi/file_system_quota_unittest.cc:82: } On 2011/06/02 08:04:08, kinuko wrote: > For these two methods (ActualSize and SizeInUsageFile) the test helper provides > two helper methods: > > int64 GetCachedOriginUsage() const; > int64 ComputeCurrentOriginUsage() const; > > Note that ComputeCurrentOriginUsage() always returns the size that doesn't > include usage file size (for backward compat) so you'll want to drop '+ > usage_file_size' from all the lines you do something like > EXPECT_EQ(foo + usage_file_size, ActualSize()) Done. http://codereview.chromium.org/6905095/diff/15001/webkit/fileapi/file_system_... webkit/fileapi/file_system_quota_unittest.cc:122: return FilePath(); On 2011/06/02 08:04:08, kinuko wrote: > For these checks (checking the return value of file_util::CreateTemporary... and > returning empty path if it was false) can we simply call EXPECT_xxx for the > return value of file_util::CreateTemporary? > > Returning empty path doesn't look very helpful to me. Done. http://codereview.chromium.org/6905095/diff/15001/webkit/fileapi/file_system_... webkit/fileapi/file_system_quota_unittest.cc:145: FilePath filesystem_dir_path_; On 2011/06/02 08:04:08, kinuko wrote: > Looks like this (and probably two other paths below) doesn't need to be member > variables. Done. http://codereview.chromium.org/6905095/diff/15001/webkit/fileapi/file_system_... webkit/fileapi/file_system_quota_unittest.cc:188: virtual void DidGetLocalPath(const FilePath& local_path) { On 2011/06/02 08:04:08, kinuko wrote: > We no longer have this method Done. http://codereview.chromium.org/6905095/diff/15001/webkit/fileapi/file_system_... webkit/fileapi/file_system_quota_unittest.cc:198: test_->set_status(base::PLATFORM_FILE_OK); On 2011/06/02 08:04:08, kinuko wrote: > this doesn't seem to be used > > ADD_FAILURE() Done. http://codereview.chromium.org/6905095/diff/15001/webkit/fileapi/file_system_... webkit/fileapi/file_system_quota_unittest.cc:204: test_->set_entries(entries); On 2011/06/02 08:04:08, kinuko wrote: > this doesn't seem to be used > > ADD_FAILURE() Done. http://codereview.chromium.org/6905095/diff/15001/webkit/fileapi/file_system_... webkit/fileapi/file_system_quota_unittest.cc:240: file_util::Delete(usage_file_path_, false); On 2011/06/02 08:04:08, kinuko wrote: > This doesn't look right to me...? > > If it's for making the computed directory size right, the issue should be > resolved if you use test_helper_ methods to get cached usage size / actual size. > > Or if it's for reseting the usage cache I think we should have a method like > ResetUsageCache() in the test helper and call that explicitly in the test code. > (Wherever before you're checking EXPECT_EQ(-1, SizeInUsageFile())) > > I want to make the amount of code code that directly modifies usage cache file > as minimal as possible, so that whenever we want to change the usage cache code > (e.g. we may want to use levelDb to store the usage cache instead of a plain > file) the changes we need to make will be small. Removing .usage was required so that .usage catches up to the current disk state. Finally, changed the testing procedure as follows. * Truncating files with operation()->Truncate() without resetting. * all_file_size (w/o usage_file_size) for ActualSize(). * all_file_size (w/o usage_file_size) for SizeInUsageFile(). http://codereview.chromium.org/6905095/diff/15001/webkit/fileapi/file_system_... webkit/fileapi/file_system_quota_unittest.cc:262: base::PLATFORM_FILE_ASYNC, &created, &error_code); On 2011/06/02 08:04:08, kinuko wrote: > maybe better to check error_code here? Done. http://codereview.chromium.org/6905095/diff/15001/webkit/fileapi/file_system_... webkit/fileapi/file_system_quota_unittest.cc:282: grandchild_file2_ = OpenFile(grandchild_file2_path_); On 2011/06/02 08:04:08, kinuko wrote: > Don't we need to close the opened files in TearDown? Done. http://codereview.chromium.org/6905095/diff/15001/webkit/fileapi/file_system_... webkit/fileapi/file_system_quota_unittest.cc:295: ASSERT_TRUE(base::TruncatePlatformFile(grandchild_file2_, 517)); On 2011/06/02 08:04:08, kinuko wrote: > nit: these days I'm trying to use simpler values like 1, 2, 3... or 10, 200, > 3000, ... for test values, since if we do so we can instantly know what went > wrong from the test output log. Changed it to { 5000, 400, 30, 2 } and { 8000, 700, 60, 5 }. http://codereview.chromium.org/6905095/diff/15001/webkit/fileapi/file_system_... webkit/fileapi/file_system_quota_unittest.cc:296: On 2011/06/02 08:04:08, kinuko wrote: > it might be better to check we have enough quota here (e.g. ASSERT_GT(quota(), > all_file_size) etc) Done. http://codereview.chromium.org/6905095/diff/15001/webkit/fileapi/file_system_... webkit/fileapi/file_system_quota_unittest.cc:299: int64 all_file_size = child_file_size + grandchild_file_size; On 2011/06/02 08:04:08, kinuko wrote: > nit: For this test seems like you can directly calculate the all_file_size > (without two mostly unsed variables (i.e. child_ and grandchild_)) Done. http://codereview.chromium.org/6905095/diff/15001/webkit/fileapi/file_system_... webkit/fileapi/file_system_quota_unittest.cc:299: int64 all_file_size = child_file_size + grandchild_file_size; On 2011/06/02 08:04:08, kinuko wrote: > nit: For this test seems like you can directly calculate the all_file_size > (without two mostly unsed variables (i.e. child_ and grandchild_)) Done. http://codereview.chromium.org/6905095/diff/15001/webkit/fileapi/file_system_... webkit/fileapi/file_system_quota_unittest.cc:300: int64 usage_file_size = FileSystemUsageCache::kUsageFileSize; On 2011/06/02 08:04:08, kinuko wrote: > nit: const ? Done. http://codereview.chromium.org/6905095/diff/15001/webkit/fileapi/file_system_... webkit/fileapi/file_system_quota_unittest.cc:309: MessageLoop::current()->RunAllPending(); On 2011/06/02 08:04:08, kinuko wrote: > should check status_ here? Done in the following line. http://codereview.chromium.org/6905095/diff/15001/webkit/fileapi/file_system_... webkit/fileapi/file_system_quota_unittest.cc:368: MessageLoop::current()->RunAllPending(); On 2011/06/02 08:04:08, kinuko wrote: > should check status_ here? Done.
Thanks, some more nits. http://codereview.chromium.org/6905095/diff/11003/webkit/fileapi/file_system_... File webkit/fileapi/file_system_quota_unittest.cc (right): http://codereview.chromium.org/6905095/diff/11003/webkit/fileapi/file_system_... webkit/fileapi/file_system_quota_unittest.cc:61: FilePath PlatformPath(FilePath virtual_path) { const FilePath& ? (here and elsewhere) http://codereview.chromium.org/6905095/diff/11003/webkit/fileapi/file_system_... webkit/fileapi/file_system_quota_unittest.cc:73: int64 SizeFromQuotaManager() { I might prefer declaring this as GetUsageAndQuotaFromQM or something and accessing usage_ via the accessor usage(), mainly because in the current code it is not really obvious where quota() is filled in the test code. http://codereview.chromium.org/6905095/diff/11003/webkit/fileapi/file_system_... webkit/fileapi/file_system_quota_unittest.cc:91: FilePath CreateVirtualDirectory(const char* virtual_path_string) { this isn't used? http://codereview.chromium.org/6905095/diff/11003/webkit/fileapi/file_system_... webkit/fileapi/file_system_quota_unittest.cc:92: FilePath virtual_path(virtual_path_string); This wouldn't work on Windows would it? You can use test_helper_.GetLocalPathFromASCII() (to replace this together with PlatformPath()). http://codereview.chromium.org/6905095/diff/11003/webkit/fileapi/file_system_... webkit/fileapi/file_system_quota_unittest.cc:98: const FilePath& virtual_dir_path) { this isn't used? http://codereview.chromium.org/6905095/diff/11003/webkit/fileapi/file_system_... webkit/fileapi/file_system_quota_unittest.cc:141: base::PlatformFile grandchild_file2_; If they're defined as member vars just to get deleted at TearDown() step, could they be a vector like vector<base::PlatformFile> opened_files_? http://codereview.chromium.org/6905095/diff/11003/webkit/fileapi/file_system_... webkit/fileapi/file_system_quota_unittest.cc:153: std::vector<base::FileUtilProxy::Entry> entries_; many of these variables appear unused now. http://codereview.chromium.org/6905095/diff/11003/webkit/fileapi/file_system_... webkit/fileapi/file_system_quota_unittest.cc:223: grandchild_file2_ = base::kInvalidPlatformFileValue; I think they should be initialized at the test class's constructor-- or they could just be replaced with a vector of PlatformFile as the above comment (rather simpler). http://codereview.chromium.org/6905095/diff/11003/webkit/fileapi/file_system_... webkit/fileapi/file_system_quota_unittest.cc:239: MessageLoop::current()->RunAllPending(); We won't need this line as helper's TearDown() call this. http://codereview.chromium.org/6905095/diff/11003/webkit/fileapi/file_system_... webkit/fileapi/file_system_quota_unittest.cc:248: base::PlatformFile file; I think this line should be merged with line 251 (to avoid having an uninitialized variable). base::PlatformFile file = base::CreatePlatformFile(...); http://codereview.chromium.org/6905095/diff/11003/webkit/fileapi/file_system_... webkit/fileapi/file_system_quota_unittest.cc:250: base::PlatformFileError error_code; maybe they should be initialized in prior? (created = false, error_code = kFileOperationStatusNotSet) http://codereview.chromium.org/6905095/diff/11003/webkit/fileapi/file_system_... webkit/fileapi/file_system_quota_unittest.cc:279: FilePath src_dir_path(CreateVirtualTemporaryDir("")); I think this will need to be FILE_PATH_LITERAL(""). Also if we don't specify prefix at any callsites I think we should just get rid of this parameter. http://codereview.chromium.org/6905095/diff/11003/webkit/fileapi/file_system_... webkit/fileapi/file_system_quota_unittest.cc:291: const int64 all_file_size = 5000+400+30+2; nit: space between digits and '+'
Thanks. http://codereview.chromium.org/6905095/diff/11003/webkit/fileapi/file_system_... File webkit/fileapi/file_system_quota_unittest.cc (right): http://codereview.chromium.org/6905095/diff/11003/webkit/fileapi/file_system_... webkit/fileapi/file_system_quota_unittest.cc:61: FilePath PlatformPath(FilePath virtual_path) { On 2011/06/13 12:01:19, kinuko wrote: > const FilePath& ? (here and elsewhere) Done. http://codereview.chromium.org/6905095/diff/11003/webkit/fileapi/file_system_... webkit/fileapi/file_system_quota_unittest.cc:73: int64 SizeFromQuotaManager() { On 2011/06/13 12:01:19, kinuko wrote: > I might prefer declaring this as GetUsageAndQuotaFromQM or something and > accessing usage_ via the accessor usage(), mainly because in the current code it > is not really obvious where quota() is filled in the test code. Done. http://codereview.chromium.org/6905095/diff/11003/webkit/fileapi/file_system_... webkit/fileapi/file_system_quota_unittest.cc:98: const FilePath& virtual_dir_path) { On 2011/06/13 12:01:19, kinuko wrote: > this isn't used? Done. http://codereview.chromium.org/6905095/diff/11003/webkit/fileapi/file_system_... webkit/fileapi/file_system_quota_unittest.cc:141: base::PlatformFile grandchild_file2_; On 2011/06/13 12:01:19, kinuko wrote: > If they're defined as member vars just to get deleted at TearDown() step, could > they be a vector like vector<base::PlatformFile> opened_files_? Ah, I found that they're not required at all. They were used only for base::TruncatePlatformFile(). http://codereview.chromium.org/6905095/diff/11003/webkit/fileapi/file_system_... webkit/fileapi/file_system_quota_unittest.cc:153: std::vector<base::FileUtilProxy::Entry> entries_; On 2011/06/13 12:01:19, kinuko wrote: > many of these variables appear unused now. Done. http://codereview.chromium.org/6905095/diff/11003/webkit/fileapi/file_system_... webkit/fileapi/file_system_quota_unittest.cc:223: grandchild_file2_ = base::kInvalidPlatformFileValue; On 2011/06/13 12:01:19, kinuko wrote: > I think they should be initialized at the test class's constructor-- or they > could just be replaced with a vector of PlatformFile as the above comment > (rather simpler). They're removed as described above. http://codereview.chromium.org/6905095/diff/11003/webkit/fileapi/file_system_... webkit/fileapi/file_system_quota_unittest.cc:239: MessageLoop::current()->RunAllPending(); On 2011/06/13 12:01:19, kinuko wrote: > We won't need this line as helper's TearDown() call this. Done. http://codereview.chromium.org/6905095/diff/11003/webkit/fileapi/file_system_... webkit/fileapi/file_system_quota_unittest.cc:248: base::PlatformFile file; On 2011/06/13 12:01:19, kinuko wrote: > I think this line should be merged with line 251 (to avoid having an > uninitialized variable). > > base::PlatformFile file = base::CreatePlatformFile(...); OpenFile() is now removed as described above. http://codereview.chromium.org/6905095/diff/11003/webkit/fileapi/file_system_... webkit/fileapi/file_system_quota_unittest.cc:250: base::PlatformFileError error_code; On 2011/06/13 12:01:19, kinuko wrote: > maybe they should be initialized in prior? (created = false, error_code = > kFileOperationStatusNotSet) ditto. http://codereview.chromium.org/6905095/diff/11003/webkit/fileapi/file_system_... webkit/fileapi/file_system_quota_unittest.cc:279: FilePath src_dir_path(CreateVirtualTemporaryDir("")); On 2011/06/13 12:01:19, kinuko wrote: > I think this will need to be FILE_PATH_LITERAL(""). > Also if we don't specify prefix at any callsites I think we should just get rid > of this parameter. Removed all prefixes. http://codereview.chromium.org/6905095/diff/11003/webkit/fileapi/file_system_... webkit/fileapi/file_system_quota_unittest.cc:291: const int64 all_file_size = 5000+400+30+2; On 2011/06/13 12:01:19, kinuko wrote: > nit: space between digits and '+' Done.
lgtm http://codereview.chromium.org/6905095/diff/22001/webkit/fileapi/file_system_... File webkit/fileapi/file_system_quota_unittest.cc (right): http://codereview.chromium.org/6905095/diff/22001/webkit/fileapi/file_system_... webkit/fileapi/file_system_quota_unittest.cc:10: #include "webkit/fileapi/file_system_operation.h" stale include? (We include this file below so I think this can simply be dropped) http://codereview.chromium.org/6905095/diff/22001/webkit/fileapi/file_system_... webkit/fileapi/file_system_quota_unittest.cc:139: MockDispatcher(FileSystemQuotaTest* test) : test_(test) { } nit: explicit
On 2011/06/14 06:51:59, kinuko wrote: > lgtm > > http://codereview.chromium.org/6905095/diff/22001/webkit/fileapi/file_system_... > File webkit/fileapi/file_system_quota_unittest.cc (right): > > http://codereview.chromium.org/6905095/diff/22001/webkit/fileapi/file_system_... > webkit/fileapi/file_system_quota_unittest.cc:10: #include > "webkit/fileapi/file_system_operation.h" > stale include? (We include this file below so I think this can simply be > dropped) > > http://codereview.chromium.org/6905095/diff/22001/webkit/fileapi/file_system_... > webkit/fileapi/file_system_quota_unittest.cc:139: > MockDispatcher(FileSystemQuotaTest* test) : test_(test) { } > nit: explicit Thanks. Updated and checked "Commit".
Change committed as 88965 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
