Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7608018:
Handle inconsistency between DB and Files (Closed)
|
Created:
9 years, 4 months ago by tzik Modified:
9 years, 3 months ago CC:
chromium-reviews, kinuko+watch, darin-cc_chromium.org, Paweł Hajdan Jr., Dai Mikurube (NOT FULLTIME) Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionHandle inconsistency between DB and Files
(retrying http://codereview.chromium.org/7540022)
BUG=91328
TEST='PPAPITest.FileRef,ObfuscatedFileSystemFileUtilTest.*'
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=98943
Patch Set 1 : previous one #Patch Set 2 : '' #
Total comments: 4
Patch Set 3 : '' #Patch Set 4 : '' #
Total comments: 8
Patch Set 5 : '' #
Total comments: 14
Patch Set 6 : '' #
Total comments: 8
Patch Set 7 : '' #Patch Set 8 : '' #Patch Set 9 : Rebased, added comment #Patch Set 10 : Fixed regression #
Messages
Total messages: 19 (0 generated)
Hi. I landed http://codereview.chromium.org/7540022/ yesterday, but a ui_test, PPAPITest.FileRef, failed with the patch and the patch is reverted. This is a fixed version of previous patch. Could you take another look?
lgtm On 2011/08/10 05:06:16, tzik wrote: > Hi. > > I landed http://codereview.chromium.org/7540022/ yesterday, but a ui_test, > PPAPITest.FileRef, failed with the patch and the patch is reverted. > This is a fixed version of previous patch. > > Could you take another look?
http://codereview.chromium.org/7608018/diff/2001/webkit/fileapi/obfuscated_fi... File webkit/fileapi/obfuscated_file_system_file_util.cc (right): http://codereview.chromium.org/7608018/diff/2001/webkit/fileapi/obfuscated_fi... webkit/fileapi/obfuscated_file_system_file_util.cc:99: base::PLATFORM_FILE_OPEN_ALWAYS; From platform_file.h: // PLATFORM_FILE_(OPEN|CREATE).* are mutually exclusive. You should specify // exactly one of the five (possibly combining with other flags) when opening // or creating a file. If the comment is wrong, it should be fixed. If it's not wrong, then this code is. http://codereview.chromium.org/7608018/diff/2001/webkit/fileapi/obfuscated_fi... File webkit/fileapi/obfuscated_file_system_file_util_unittest.cc (right): http://codereview.chromium.org/7608018/diff/2001/webkit/fileapi/obfuscated_fi... webkit/fileapi/obfuscated_file_system_file_util_unittest.cc:1069: base::PLATFORM_FILE_READ | base::PLATFORM_FILE_CREATE, I see that you changed this from WRITE to READ; does that have any effect on the test?
My previous patch was too complicated. Just deleting garbage file in CreateFile is sufficient to error recovery. Could you let me change the patch like this? http://codereview.chromium.org/7608018/diff/2001/webkit/fileapi/obfuscated_fi... File webkit/fileapi/obfuscated_file_system_file_util.cc (right): http://codereview.chromium.org/7608018/diff/2001/webkit/fileapi/obfuscated_fi... webkit/fileapi/obfuscated_file_system_file_util.cc:99: base::PLATFORM_FILE_OPEN_ALWAYS; On 2011/08/10 17:28:01, ericu wrote: > From platform_file.h: > > > // PLATFORM_FILE_(OPEN|CREATE).* are mutually exclusive. You should specify > // exactly one of the five (possibly combining with other flags) when opening > // or creating a file. > > > If the comment is wrong, it should be fixed. If it's not wrong, then this code > is. Yes, the patch went wrong way. It caused tests fail explicitly in windows. http://codereview.chromium.org/7608018/diff/2001/webkit/fileapi/obfuscated_fi... File webkit/fileapi/obfuscated_file_system_file_util_unittest.cc (right): http://codereview.chromium.org/7608018/diff/2001/webkit/fileapi/obfuscated_fi... webkit/fileapi/obfuscated_file_system_file_util_unittest.cc:1069: base::PLATFORM_FILE_READ | base::PLATFORM_FILE_CREATE, On 2011/08/10 17:28:01, ericu wrote: > I see that you changed this from WRITE to READ; does that have any effect on the > test? READ | CREATE_ALWAYS was fail-case in previous patch, so I want to check it specially. Also, as open(..., O_RDONLY | O_CREAT) is valid in linux, READ | CREATE_ALWAYS looks easy to be misused.
http://codereview.chromium.org/7608018/diff/19003/webkit/fileapi/obfuscated_f... File webkit/fileapi/obfuscated_file_system_file_util.cc (right): http://codereview.chromium.org/7608018/diff/19003/webkit/fileapi/obfuscated_f... webkit/fileapi/obfuscated_file_system_file_util.cc:795: context->file_system_context()->RecalculateUsage(origin_url, type); What's the purpose of this RecalculateUsage call? If you're trying to update the quota usage, it's not necessary; the quota doesn't include these stray files at all. http://codereview.chromium.org/7608018/diff/19003/webkit/fileapi/obfuscated_f... webkit/fileapi/obfuscated_file_system_file_util.cc:1051: FilePath path = file_system_directory_.Append(directory_name); This part looks like a good idea. http://codereview.chromium.org/7608018/diff/19003/webkit/fileapi/sandbox_moun... File webkit/fileapi/sandbox_mount_point_provider.cc (right): http://codereview.chromium.org/7608018/diff/19003/webkit/fileapi/sandbox_moun... webkit/fileapi/sandbox_mount_point_provider.cc:625: StartUpdateOriginOnFileThread(origin_url, type); This doesn't actually start an update--it just marks the file to say there's one in progress.
http://codereview.chromium.org/7608018/diff/19003/webkit/fileapi/obfuscated_f... File webkit/fileapi/obfuscated_file_system_file_util.cc (right): http://codereview.chromium.org/7608018/diff/19003/webkit/fileapi/obfuscated_f... webkit/fileapi/obfuscated_file_system_file_util.cc:795: context->file_system_context()->RecalculateUsage(origin_url, type); On 2011/08/17 00:54:08, ericu wrote: > What's the purpose of this RecalculateUsage call? > If you're trying to update the quota usage, it's not necessary; the quota > doesn't include these stray files at all. Eric, this is one of the place we wondered if we'd better mark the usage cache dirty before making any operations. This inconsistency could happen in a situation like: 1. an app calls DeleteFile() to delete a file 2. first we delete the file entry from the database in OFSFU 3. then we mark the usage cache dirty, delete file, update the usage cache and clear the dirty flag in QFU. There's a slight chance chrome could crash between 2 and 3. The FS inconsistency could be easily and lazily fixed later (and this patch is doing the fix), but in this case we cannot know if the usage cache has been already updated or not, so we need to force recalculating it (that's why he calls RecalculateUsage there). But if we always mark the usage cache dirty before making any operations we will no longer have such problems-- we can just recalculate the usage whenever the cache is marked dirty at start-up time.
http://codereview.chromium.org/7608018/diff/19003/webkit/fileapi/obfuscated_f... File webkit/fileapi/obfuscated_file_system_file_util.cc (right): http://codereview.chromium.org/7608018/diff/19003/webkit/fileapi/obfuscated_f... webkit/fileapi/obfuscated_file_system_file_util.cc:795: context->file_system_context()->RecalculateUsage(origin_url, type); On 2011/08/18 01:26:36, kinuko wrote: > On 2011/08/17 00:54:08, ericu wrote: > > What's the purpose of this RecalculateUsage call? > > If you're trying to update the quota usage, it's not necessary; the quota > > doesn't include these stray files at all. > > Eric, this is one of the place we wondered if we'd better mark the usage cache > dirty before making any operations. > > This inconsistency could happen in a situation like: > 1. an app calls DeleteFile() to delete a file > 2. first we delete the file entry from the database in OFSFU > 3. then we mark the usage cache dirty, delete file, update the usage cache and > clear the dirty flag in QFU. > > There's a slight chance chrome could crash between 2 and 3. The FS > inconsistency could be easily and lazily fixed later (and this patch is doing > the fix), but in this case we cannot know if the usage cache has been already > updated or not, so we need to force recalculating it (that's why he calls > RecalculateUsage there). > > But if we always mark the usage cache dirty before making any operations we will > no longer have such problems-- we can just recalculate the usage whenever the > cache is marked dirty at start-up time. Thanks for explaining kinuko-san, it is what I wanted to say. http://codereview.chromium.org/7608018/diff/19003/webkit/fileapi/sandbox_moun... File webkit/fileapi/sandbox_mount_point_provider.cc (right): http://codereview.chromium.org/7608018/diff/19003/webkit/fileapi/sandbox_moun... webkit/fileapi/sandbox_mount_point_provider.cc:625: StartUpdateOriginOnFileThread(origin_url, type); On 2011/08/17 00:54:08, ericu wrote: > This doesn't actually start an update--it just marks the file to say there's one > in progress. Actually, what I want to do in here is to increment the dirty count for the origin and the type. It'll force SMPP to recalculate usage at next initialization time.
LGTM if Kinuko OKs the MarkDirty vs. remove-the-usage-file; you don't need to wait for me. http://codereview.chromium.org/7608018/diff/19003/webkit/fileapi/sandbox_moun... File webkit/fileapi/sandbox_mount_point_provider.cc (right): http://codereview.chromium.org/7608018/diff/19003/webkit/fileapi/sandbox_moun... webkit/fileapi/sandbox_mount_point_provider.cc:625: StartUpdateOriginOnFileThread(origin_url, type); On 2011/08/18 08:02:26, tzik wrote: > On 2011/08/17 00:54:08, ericu wrote: > > This doesn't actually start an update--it just marks the file to say there's > one > > in progress. > > Actually, what I want to do in here is to increment the dirty count for the > origin and the type. > It'll force SMPP to recalculate usage at next initialization time. > What about just deleting the usage cache file? Or would that cause other problems? http://codereview.chromium.org/7608018/diff/28002/webkit/fileapi/file_system_... File webkit/fileapi/file_system_quota_util.h (right): http://codereview.chromium.org/7608018/diff/28002/webkit/fileapi/file_system_... webkit/fileapi/file_system_quota_util.h:95: virtual void MarkDirtyOriginOnFileThread(const GURL& origin_url, s/MarkDirtyOrigin/MarkOriginDirty/ ? http://codereview.chromium.org/7608018/diff/28002/webkit/fileapi/obfuscated_f... File webkit/fileapi/obfuscated_file_system_file_util.cc (right): http://codereview.chromium.org/7608018/diff/28002/webkit/fileapi/obfuscated_f... webkit/fileapi/obfuscated_file_system_file_util.cc:114: MarkDirtyOriginOnFileThread(context->src_origin_url(), Marking the cache dirty won't help entirely solve the problem; we also need code to remove the database entry at some point. Please add a TODO. http://codereview.chromium.org/7608018/diff/28002/webkit/fileapi/obfuscated_f... File webkit/fileapi/obfuscated_file_system_file_util.h (right): http://codereview.chromium.org/7608018/diff/28002/webkit/fileapi/obfuscated_f... webkit/fileapi/obfuscated_file_system_file_util.h:240: void MarkDirty(const GURL& origin, FileSystemType type); Unimplemented/Unused? http://codereview.chromium.org/7608018/diff/28002/webkit/fileapi/sandbox_moun... File webkit/fileapi/sandbox_mount_point_provider.h (right): http://codereview.chromium.org/7608018/diff/28002/webkit/fileapi/sandbox_moun... webkit/fileapi/sandbox_mount_point_provider.h:147: virtual void MarkDirtyOriginOnFileThread(const GURL& origin_url, Ditto.
http://codereview.chromium.org/7608018/diff/19003/webkit/fileapi/sandbox_moun... File webkit/fileapi/sandbox_mount_point_provider.cc (right): http://codereview.chromium.org/7608018/diff/19003/webkit/fileapi/sandbox_moun... webkit/fileapi/sandbox_mount_point_provider.cc:625: StartUpdateOriginOnFileThread(origin_url, type); On 2011/08/19 04:05:56, ericu wrote: > On 2011/08/18 08:02:26, tzik wrote: > > On 2011/08/17 00:54:08, ericu wrote: > > > This doesn't actually start an update--it just marks the file to say there's > > one > > > in progress. > > > > Actually, what I want to do in here is to increment the dirty count for the > > origin and the type. > > It'll force SMPP to recalculate usage at next initialization time. > > > > What about just deleting the usage cache file? Or would that cause other > problems? If some write-operation are running, they breaks dirty count at completion without .usage file. How about adding validity flag to .usage to mark it invalid, like patchset 5? http://codereview.chromium.org/7608018/diff/28002/webkit/fileapi/file_system_... File webkit/fileapi/file_system_quota_util.h (right): http://codereview.chromium.org/7608018/diff/28002/webkit/fileapi/file_system_... webkit/fileapi/file_system_quota_util.h:95: virtual void MarkDirtyOriginOnFileThread(const GURL& origin_url, On 2011/08/19 04:05:56, ericu wrote: > s/MarkDirtyOrigin/MarkOriginDirty/ ? More directly, can I use InvalidateUsageCache? http://codereview.chromium.org/7608018/diff/28002/webkit/fileapi/obfuscated_f... File webkit/fileapi/obfuscated_file_system_file_util.cc (right): http://codereview.chromium.org/7608018/diff/28002/webkit/fileapi/obfuscated_f... webkit/fileapi/obfuscated_file_system_file_util.cc:114: MarkDirtyOriginOnFileThread(context->src_origin_url(), On 2011/08/19 04:05:56, ericu wrote: > Marking the cache dirty won't help entirely solve the problem; we also need code > to remove the database entry at some point. Please add a TODO. Done. http://codereview.chromium.org/7608018/diff/28002/webkit/fileapi/obfuscated_f... File webkit/fileapi/obfuscated_file_system_file_util.h (right): http://codereview.chromium.org/7608018/diff/28002/webkit/fileapi/obfuscated_f... webkit/fileapi/obfuscated_file_system_file_util.h:240: void MarkDirty(const GURL& origin, FileSystemType type); On 2011/08/19 04:05:56, ericu wrote: > Unimplemented/Unused? Done.
http://codereview.chromium.org/7608018/diff/28002/webkit/fileapi/file_system_... File webkit/fileapi/file_system_quota_util.h (right): http://codereview.chromium.org/7608018/diff/28002/webkit/fileapi/file_system_... webkit/fileapi/file_system_quota_util.h:95: virtual void MarkDirtyOriginOnFileThread(const GURL& origin_url, On 2011/08/23 04:13:13, tzik wrote: > On 2011/08/19 04:05:56, ericu wrote: > > s/MarkDirtyOrigin/MarkOriginDirty/ ? > > More directly, can I use InvalidateUsageCache? That looks even better. http://codereview.chromium.org/7608018/diff/28002/webkit/fileapi/obfuscated_f... File webkit/fileapi/obfuscated_file_system_file_util.cc (right): http://codereview.chromium.org/7608018/diff/28002/webkit/fileapi/obfuscated_f... webkit/fileapi/obfuscated_file_system_file_util.cc:114: MarkDirtyOriginOnFileThread(context->src_origin_url(), On 2011/08/23 04:13:13, tzik wrote: > On 2011/08/19 04:05:56, ericu wrote: > > Marking the cache dirty won't help entirely solve the problem; we also need > code > > to remove the database entry at some point. Please add a TODO. > > Done. I was talking about the entry in the FileSystemDirectoryDatabase, although yes, we also need to tell the UsageTracker if it's caching this info. Please add another TODO. http://codereview.chromium.org/7608018/diff/32001/webkit/fileapi/file_system_... File webkit/fileapi/file_system_usage_cache.h (right): http://codereview.chromium.org/7608018/diff/32001/webkit/fileapi/file_system_... webkit/fileapi/file_system_usage_cache.h:29: // Notifies quota system need to recalculate their usage cache of the origin. s/need/that it needs/ s/their/the Sorry, I know it won't fit on one line now ;'>. http://codereview.chromium.org/7608018/diff/32001/webkit/fileapi/obfuscated_f... File webkit/fileapi/obfuscated_file_system_file_util.cc (right): http://codereview.chromium.org/7608018/diff/32001/webkit/fileapi/obfuscated_f... webkit/fileapi/obfuscated_file_system_file_util.cc:886: if (file_util::PathExists(path)) { This should probably use underlying_file_util_, not file_util. However, in the case of quota_file_util, which is generally what's under there, it might falsely lower the quota. Dai, Kinuko: what do you think about this?
http://codereview.chromium.org/7608018/diff/28002/webkit/fileapi/obfuscated_f... File webkit/fileapi/obfuscated_file_system_file_util.cc (right): http://codereview.chromium.org/7608018/diff/28002/webkit/fileapi/obfuscated_f... webkit/fileapi/obfuscated_file_system_file_util.cc:114: MarkDirtyOriginOnFileThread(context->src_origin_url(), On 2011/08/24 03:43:50, ericu wrote: > On 2011/08/23 04:13:13, tzik wrote: > > On 2011/08/19 04:05:56, ericu wrote: > > > Marking the cache dirty won't help entirely solve the problem; we also need > > code > > > to remove the database entry at some point. Please add a TODO. > > > > Done. > > I was talking about the entry in the FileSystemDirectoryDatabase, although yes, > we also need to tell the UsageTracker if it's caching this info. > > Please add another TODO. I see. But, should we delete the database entry silently, when we lost the backing file? When the backing file was lost due to a error on native file system, the user looks less unhappy if he can detect the fact. Anyway, I'll add some comment.
On Tue, Aug 23, 2011 at 11:31 PM, <tzik@chromium.org> wrote: > > http://codereview.chromium.org/7608018/diff/28002/webkit/fileapi/obfuscated_f... > File webkit/fileapi/obfuscated_file_system_file_util.cc (right): > > http://codereview.chromium.org/7608018/diff/28002/webkit/fileapi/obfuscated_f... > webkit/fileapi/obfuscated_file_system_file_util.cc:114: > MarkDirtyOriginOnFileThread(context->src_origin_url(), > On 2011/08/24 03:43:50, ericu wrote: >> >> On 2011/08/23 04:13:13, tzik wrote: >> > On 2011/08/19 04:05:56, ericu wrote: >> > > Marking the cache dirty won't help entirely solve the problem; we > > also need >> >> > code >> > > to remove the database entry at some point. Please add a TODO. >> > >> > Done. > >> I was talking about the entry in the FileSystemDirectoryDatabase, > > although yes, >> >> we also need to tell the UsageTracker if it's caching this info. > >> Please add another TODO. > > I see. > But, should we delete the database entry silently, when we lost the > backing file? > When the backing file was lost due to a error on native file system, the > user looks less unhappy if he can detect the fact. We have no choice; there's nothing better that we can do. > Anyway, I'll add some comment. > > http://codereview.chromium.org/7608018/ >
http://codereview.chromium.org/7608018/diff/28002/webkit/fileapi/obfuscated_f... File webkit/fileapi/obfuscated_file_system_file_util.cc (right): http://codereview.chromium.org/7608018/diff/28002/webkit/fileapi/obfuscated_f... webkit/fileapi/obfuscated_file_system_file_util.cc:114: MarkDirtyOriginOnFileThread(context->src_origin_url(), On 2011/08/24 06:31:59, tzik wrote: > On 2011/08/24 03:43:50, ericu wrote: > > On 2011/08/23 04:13:13, tzik wrote: > > > On 2011/08/19 04:05:56, ericu wrote: > > > > Marking the cache dirty won't help entirely solve the problem; we also > need > > > code > > > > to remove the database entry at some point. Please add a TODO. > > > > > > Done. > > > > I was talking about the entry in the FileSystemDirectoryDatabase, although > yes, > > we also need to tell the UsageTracker if it's caching this info. > > > > Please add another TODO. > > I see. > But, should we delete the database entry silently, when we lost the backing > file? > When the backing file was lost due to a error on native file system, the user > looks less unhappy if he can detect the fact. > > Anyway, I'll add some comment. Done. http://codereview.chromium.org/7608018/diff/32001/webkit/fileapi/file_system_... File webkit/fileapi/file_system_usage_cache.h (right): http://codereview.chromium.org/7608018/diff/32001/webkit/fileapi/file_system_... webkit/fileapi/file_system_usage_cache.h:29: // Notifies quota system need to recalculate their usage cache of the origin. On 2011/08/24 03:43:50, ericu wrote: > s/need/that it needs/ > s/their/the > > Sorry, I know it won't fit on one line now ;'>. Done. http://codereview.chromium.org/7608018/diff/32001/webkit/fileapi/obfuscated_f... File webkit/fileapi/obfuscated_file_system_file_util.cc (right): http://codereview.chromium.org/7608018/diff/32001/webkit/fileapi/obfuscated_f... webkit/fileapi/obfuscated_file_system_file_util.cc:886: if (file_util::PathExists(path)) { On 2011/08/24 03:43:50, ericu wrote: > This should probably use underlying_file_util_, not file_util. However, in the > case of quota_file_util, which is generally what's under there, it might falsely > lower the quota. Dai, Kinuko: what do you think about this? We use underlying_file_util_ in the next patch. It makes the usage lower than actual usage until next recalculation, maybe until next boot.
http://codereview.chromium.org/7608018/diff/32001/webkit/fileapi/obfuscated_f... File webkit/fileapi/obfuscated_file_system_file_util.cc (right): http://codereview.chromium.org/7608018/diff/32001/webkit/fileapi/obfuscated_f... webkit/fileapi/obfuscated_file_system_file_util.cc:886: if (file_util::PathExists(path)) { On 2011/08/29 06:54:13, tzik wrote: > On 2011/08/24 03:43:50, ericu wrote: > > This should probably use underlying_file_util_, not file_util. However, in > the > > case of quota_file_util, which is generally what's under there, it might > falsely > > lower the quota. Dai, Kinuko: what do you think about this? > > We use underlying_file_util_ in the next patch. > It makes the usage lower than actual usage until next recalculation, maybe until > next boot. Maybe we can explicitly call underlying_file_util_->GetLocalFilePath() to get a platform path and call file_util methods?
http://codereview.chromium.org/7608018/diff/32001/webkit/fileapi/obfuscated_f... File webkit/fileapi/obfuscated_file_system_file_util.cc (right): http://codereview.chromium.org/7608018/diff/32001/webkit/fileapi/obfuscated_f... webkit/fileapi/obfuscated_file_system_file_util.cc:886: if (file_util::PathExists(path)) { On 2011/08/29 07:45:37, kinuko wrote: > On 2011/08/29 06:54:13, tzik wrote: > > On 2011/08/24 03:43:50, ericu wrote: > > > This should probably use underlying_file_util_, not file_util. However, in > > the > > > case of quota_file_util, which is generally what's under there, it might > > falsely > > > lower the quota. Dai, Kinuko: what do you think about this? > > > > We use underlying_file_util_ in the next patch. > > It makes the usage lower than actual usage until next recalculation, maybe > until > > next boot. > > Maybe we can explicitly call underlying_file_util_->GetLocalFilePath() to get a > platform path and call file_util methods? Done.
LGTM again ;'>. http://codereview.chromium.org/7608018/diff/28002/webkit/fileapi/obfuscated_f... File webkit/fileapi/obfuscated_file_system_file_util.cc (right): http://codereview.chromium.org/7608018/diff/28002/webkit/fileapi/obfuscated_f... webkit/fileapi/obfuscated_file_system_file_util.cc:114: MarkDirtyOriginOnFileThread(context->src_origin_url(), On 2011/08/24 06:31:59, tzik wrote: > On 2011/08/24 03:43:50, ericu wrote: > > On 2011/08/23 04:13:13, tzik wrote: > > > On 2011/08/19 04:05:56, ericu wrote: > > > > Marking the cache dirty won't help entirely solve the problem; we also > need > > > code > > > > to remove the database entry at some point. Please add a TODO. > > > > > > Done. > > > > I was talking about the entry in the FileSystemDirectoryDatabase, although > yes, > > we also need to tell the UsageTracker if it's caching this info. > > > > Please add another TODO. > > I see. > But, should we delete the database entry silently, when we lost the backing > file? > When the backing file was lost due to a error on native file system, the user > looks less unhappy if he can detect the fact. It's really hard to say what will make the user happier. In many cases these files aren't even really visible to the user; they're silently used by some web app as part of its manual cache management. In others, the user might want to know about the data loss, but isn't really technically savvy enough to understand what's happened. In any case, there's generally nothing the user can do about it, so we're probably just better off cleaning up in almost all cases. > Anyway, I'll add some comment. Thanks. http://codereview.chromium.org/7608018/diff/32001/webkit/fileapi/obfuscated_f... File webkit/fileapi/obfuscated_file_system_file_util.cc (right): http://codereview.chromium.org/7608018/diff/32001/webkit/fileapi/obfuscated_f... webkit/fileapi/obfuscated_file_system_file_util.cc:886: if (file_util::PathExists(path)) { On 2011/08/29 08:34:15, tzik wrote: > On 2011/08/29 07:45:37, kinuko wrote: > > On 2011/08/29 06:54:13, tzik wrote: > > > On 2011/08/24 03:43:50, ericu wrote: > > > > This should probably use underlying_file_util_, not file_util. However, > in > > > the > > > > case of quota_file_util, which is generally what's under there, it might > > > falsely > > > > lower the quota. Dai, Kinuko: what do you think about this? > > > > > > We use underlying_file_util_ in the next patch. > > > It makes the usage lower than actual usage until next recalculation, maybe > > until > > > next boot. > > > > Maybe we can explicitly call underlying_file_util_->GetLocalFilePath() to get > a > > platform path and call file_util methods? > > Done. Thanks, and Kinuko, that was a good idea. I've just noticed another inconsistency, though, which dates from when I wrote this. GetDirectoryForOriginAndType always returns a real OS path, and I've mixed that in with calls to underlying_file_util as if the paths are in the same namespace. I don't see an obvious fix for that right now, but then nothing's actually broken yet, just brittle. Taiju, would you mind putting in this comment for me? [at line 864] TODO(ericu): local_path is an OS path; underlying_file_util_ isn't guaranteed to understand OS paths. Thanks.
Thanks for reviewing. If there's no problem, I'll check commit box after try completion. http://codereview.chromium.org/7608018/diff/28002/webkit/fileapi/obfuscated_f... File webkit/fileapi/obfuscated_file_system_file_util.cc (right): http://codereview.chromium.org/7608018/diff/28002/webkit/fileapi/obfuscated_f... webkit/fileapi/obfuscated_file_system_file_util.cc:114: MarkDirtyOriginOnFileThread(context->src_origin_url(), On 2011/08/29 22:48:20, ericu wrote: > On 2011/08/24 06:31:59, tzik wrote: > > On 2011/08/24 03:43:50, ericu wrote: > > > On 2011/08/23 04:13:13, tzik wrote: > > > > On 2011/08/19 04:05:56, ericu wrote: > > > > > Marking the cache dirty won't help entirely solve the problem; we also > > need > > > > code > > > > > to remove the database entry at some point. Please add a TODO. > > > > > > > > Done. > > > > > > I was talking about the entry in the FileSystemDirectoryDatabase, although > > yes, > > > we also need to tell the UsageTracker if it's caching this info. > > > > > > Please add another TODO. > > > > I see. > > But, should we delete the database entry silently, when we lost the backing > > file? > > When the backing file was lost due to a error on native file system, the user > > looks less unhappy if he can detect the fact. > > It's really hard to say what will make the user happier. In many cases these > files aren't even really visible to the user; they're silently used by some web > app as part of its manual cache management. In others, the user might want to > know about the data loss, but isn't really technically savvy enough to > understand what's happened. In any case, there's generally nothing the user can > do about it, so we're probably just better off cleaning up in almost all cases. > I agree that deleting files will work better in most case. But,,, could we discuss some more in other place? > > Anyway, I'll add some comment. > > Thanks. http://codereview.chromium.org/7608018/diff/32001/webkit/fileapi/obfuscated_f... File webkit/fileapi/obfuscated_file_system_file_util.cc (right): http://codereview.chromium.org/7608018/diff/32001/webkit/fileapi/obfuscated_f... webkit/fileapi/obfuscated_file_system_file_util.cc:886: if (file_util::PathExists(path)) { On 2011/08/29 22:48:20, ericu wrote: > On 2011/08/29 08:34:15, tzik wrote: > > On 2011/08/29 07:45:37, kinuko wrote: > > > On 2011/08/29 06:54:13, tzik wrote: > > > > On 2011/08/24 03:43:50, ericu wrote: > > > > > This should probably use underlying_file_util_, not file_util. However, > > in > > > > the > > > > > case of quota_file_util, which is generally what's under there, it might > > > > falsely > > > > > lower the quota. Dai, Kinuko: what do you think about this? > > > > > > > > We use underlying_file_util_ in the next patch. > > > > It makes the usage lower than actual usage until next recalculation, maybe > > > until > > > > next boot. > > > > > > Maybe we can explicitly call underlying_file_util_->GetLocalFilePath() to > get > > a > > > platform path and call file_util methods? > > > > Done. > > Thanks, and Kinuko, that was a good idea. > I've just noticed another inconsistency, though, which dates from when I wrote > this. GetDirectoryForOriginAndType always returns a real OS path, and I've > mixed that in with calls to underlying_file_util as if the paths are in the same > namespace. I don't see an obvious fix for that right now, but then nothing's > actually broken yet, just brittle. Taiju, would you mind putting in this > comment for me? > > [at line 864] TODO(ericu): local_path is an OS path; underlying_file_util_ isn't > guaranteed to understand OS paths. > > Thanks. Done.
http://codereview.chromium.org/7608018/diff/28002/webkit/fileapi/obfuscated_f... File webkit/fileapi/obfuscated_file_system_file_util.cc (right): http://codereview.chromium.org/7608018/diff/28002/webkit/fileapi/obfuscated_f... webkit/fileapi/obfuscated_file_system_file_util.cc:114: MarkDirtyOriginOnFileThread(context->src_origin_url(), On 2011/08/31 02:40:05, tzik wrote: > On 2011/08/29 22:48:20, ericu wrote: > > On 2011/08/24 06:31:59, tzik wrote: > > > On 2011/08/24 03:43:50, ericu wrote: > > > > On 2011/08/23 04:13:13, tzik wrote: > > > > > On 2011/08/19 04:05:56, ericu wrote: > > > > > > Marking the cache dirty won't help entirely solve the problem; we also > > > need > > > > > code > > > > > > to remove the database entry at some point. Please add a TODO. > > > > > > > > > > Done. > > > > > > > > I was talking about the entry in the FileSystemDirectoryDatabase, although > > > yes, > > > > we also need to tell the UsageTracker if it's caching this info. > > > > > > > > Please add another TODO. > > > > > > I see. > > > But, should we delete the database entry silently, when we lost the backing > > > file? > > > When the backing file was lost due to a error on native file system, the > user > > > looks less unhappy if he can detect the fact. > > > > It's really hard to say what will make the user happier. In many cases these > > files aren't even really visible to the user; they're silently used by some > web > > app as part of its manual cache management. In others, the user might want to > > know about the data loss, but isn't really technically savvy enough to > > understand what's happened. In any case, there's generally nothing the user > can > > do about it, so we're probably just better off cleaning up in almost all > cases. > > > > I agree that deleting files will work better in most case. > But,,, could we discuss some more in other place? Yes, of course.
Change committed as 98943 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
