Don't keep file system directories around if they contain unrelated files.

Don't keep file system directories around if they contain unrelated files.

Finder on Mac OS has a habit of putting .DS_Store files into directories, which would (silently) prevent us from deleting them.

Also, don't treat trying to delete a directory that doesn't exist on disk anymore as an error.
Add URL to quota managed origin deletion callback.


BUG=134825
TEST=Clear Browsing Data does that.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=146089

[Bernhard Bauer, 2012-07-04 17:33:07]

Please review.

[kinuko, 2012-07-09 07:23:33]

http://codereview.chromium.org/10701094/diff/1/webkit/fileapi/obfuscated_file...
File webkit/fileapi/obfuscated_file_util.cc (right):

http://codereview.chromium.org/10701094/diff/1/webkit/fileapi/obfuscated_file...
webkit/fileapi/obfuscated_file_util.cc:965:
origin_path.Append(GetDirectoryNameForType(other_type)))) {
Could we instead check if the type directory is the only directory in the origin
directory?

[Bernhard Bauer, 2012-07-09 09:03:29]

http://codereview.chromium.org/10701094/diff/1/webkit/fileapi/obfuscated_file...
File webkit/fileapi/obfuscated_file_util.cc (right):

http://codereview.chromium.org/10701094/diff/1/webkit/fileapi/obfuscated_file...
webkit/fileapi/obfuscated_file_util.cc:965:
origin_path.Append(GetDirectoryNameForType(other_type)))) {
On 2012/07/09 07:23:33, kinuko wrote:
> Could we instead check if the type directory is the only directory in the
origin
> directory?

Hmm, I think there can also be garbage directories (for example, ZIP files of
directories on HFS volumes created by Mac OS contain a __MACOSX directory when
extracted on other file systems).

[kinuko, 2012-07-09 11:47:50]

http://codereview.chromium.org/10701094/diff/1/webkit/fileapi/obfuscated_file...
File webkit/fileapi/obfuscated_file_util.cc (right):

http://codereview.chromium.org/10701094/diff/1/webkit/fileapi/obfuscated_file...
webkit/fileapi/obfuscated_file_util.cc:920: *error_code = error;
I'm afraid some of the callsites of this method rely on the fact that the
returned path is empty if the error code is not OK.

http://codereview.chromium.org/10701094/diff/1/webkit/fileapi/obfuscated_file...
webkit/fileapi/obfuscated_file_util.cc:965:
origin_path.Append(GetDirectoryNameForType(other_type)))) {
On 2012/07/09 09:03:29, Bernhard Bauer wrote:
> On 2012/07/09 07:23:33, kinuko wrote:
> > Could we instead check if the type directory is the only directory in the
> origin
> > directory?
> 
> Hmm, I think there can also be garbage directories (for example, ZIP files of
> directories on HFS volumes created by Mac OS contain a __MACOSX directory when
> extracted on other file systems).

Ok...

[Bernhard Bauer, 2012-07-09 15:20:27]

http://codereview.chromium.org/10701094/diff/1/webkit/fileapi/obfuscated_file...
File webkit/fileapi/obfuscated_file_util.cc (right):

http://codereview.chromium.org/10701094/diff/1/webkit/fileapi/obfuscated_file...
webkit/fileapi/obfuscated_file_util.cc:920: *error_code = error;
On 2012/07/09 11:47:50, kinuko wrote:
> I'm afraid some of the callsites of this method rely on the fact that the
> returned path is empty if the error code is not OK.

Good point. Would you prefer updating all callsites to look at the error code
instead, or adding a convenience method that returns an empty path if the error
code is not OK?

[kinuko, 2012-07-10 08:09:52]

On 2012/07/09 15:20:27, Bernhard Bauer wrote:
>
http://codereview.chromium.org/10701094/diff/1/webkit/fileapi/obfuscated_file...
> File webkit/fileapi/obfuscated_file_util.cc (right):
> 
>
http://codereview.chromium.org/10701094/diff/1/webkit/fileapi/obfuscated_file...
> webkit/fileapi/obfuscated_file_util.cc:920: *error_code = error;
> On 2012/07/09 11:47:50, kinuko wrote:
> > I'm afraid some of the callsites of this method rely on the fact that the
> > returned path is empty if the error code is not OK.
> 
> Good point. Would you prefer updating all callsites to look at the error code
> instead, or adding a convenience method that returns an empty path if the
error
> code is not OK?

If you're ok making the changes I prefer changing all the callsites to check the
error code.  Or you can just make a small change in the glue convenient function
which is defined as an inline function in the .h file to always return an empty
path if there has been an error.
Thanks!

[Bernhard Bauer, 2012-07-10 09:00:55]

On 2012/07/10 08:09:52, kinuko wrote:
> On 2012/07/09 15:20:27, Bernhard Bauer wrote:
> >
>
http://codereview.chromium.org/10701094/diff/1/webkit/fileapi/obfuscated_file...
> > File webkit/fileapi/obfuscated_file_util.cc (right):
> > 
> >
>
http://codereview.chromium.org/10701094/diff/1/webkit/fileapi/obfuscated_file...
> > webkit/fileapi/obfuscated_file_util.cc:920: *error_code = error;
> > On 2012/07/09 11:47:50, kinuko wrote:
> > > I'm afraid some of the callsites of this method rely on the fact that the
> > > returned path is empty if the error code is not OK.
> > 
> > Good point. Would you prefer updating all callsites to look at the error
code
> > instead, or adding a convenience method that returns an empty path if the
> error
> > code is not OK?
> 
> If you're ok making the changes I prefer changing all the callsites to check
the
> error code.  Or you can just make a small change in the glue convenient
function
> which is defined as an inline function in the .h file to always return an
empty
> path if there has been an error.
> Thanks!

Yeah, I think I prefer changing the callsites. Otherwise we'd have two
equally-named methods that behave slightly different, and if I start
disambiguating them, it wouldn't be any less work than directly updating the
callsites.

[kinuko, 2012-07-10 11:12:23]

Thanks for fixing the callsites!  It looks a lot cleaner now.

(Adding ericu/tzik to cc as they're more familiar with the code than I)

https://chromiumcodereview.appspot.com/10701094/diff/9003/webkit/fileapi/obfu...
File webkit/fileapi/obfuscated_file_util.cc (left):

https://chromiumcodereview.appspot.com/10701094/diff/9003/webkit/fileapi/obfu...
webkit/fileapi/obfuscated_file_util.cc:1238: LOG(WARNING) << "Failed to
origin+type directory: " << path.value();
Can we preserve this WARNING when error != OK case?

https://chromiumcodereview.appspot.com/10701094/diff/9003/webkit/fileapi/obfu...
File webkit/fileapi/obfuscated_file_util.cc (right):

https://chromiumcodereview.appspot.com/10701094/diff/9003/webkit/fileapi/obfu...
webkit/fileapi/obfuscated_file_util.cc:1166: return
base::PLATFORM_FILE_ERROR_FAILED;
Maybe we should just return the error?

[Bernhard Bauer, 2012-07-10 11:56:38]

http://codereview.chromium.org/10701094/diff/9003/webkit/fileapi/obfuscated_f...
File webkit/fileapi/obfuscated_file_util.cc (left):

http://codereview.chromium.org/10701094/diff/9003/webkit/fileapi/obfuscated_f...
webkit/fileapi/obfuscated_file_util.cc:1238: LOG(WARNING) << "Failed to
origin+type directory: " << path.value();
On 2012/07/10 11:12:24, kinuko wrote:
> Can we preserve this WARNING when error != OK case?

Done.

http://codereview.chromium.org/10701094/diff/9003/webkit/fileapi/obfuscated_f...
File webkit/fileapi/obfuscated_file_util.cc (right):

http://codereview.chromium.org/10701094/diff/9003/webkit/fileapi/obfuscated_f...
webkit/fileapi/obfuscated_file_util.cc:1166: return
base::PLATFORM_FILE_ERROR_FAILED;
On 2012/07/10 11:12:24, kinuko wrote:
> Maybe we should just return the error?

Done.

[ericu, 2012-07-10 20:20:07]

LGTM

http://codereview.chromium.org/10701094/diff/6018/webkit/fileapi/obfuscated_f...
File webkit/fileapi/obfuscated_file_util.cc (right):

http://codereview.chromium.org/10701094/diff/6018/webkit/fileapi/obfuscated_f...
webkit/fileapi/obfuscated_file_util.cc:1261: LOG(WARNING) << "Failed to
origin+type directory: " << path.value();
Missing "get " before "origin", from an old CL.  Mind fixing that while you're
in here?

[kinuko, 2012-07-11 05:06:45]

lgtm2

On 2012/07/10 20:20:07, ericu wrote:
> LGTM
> 
>
http://codereview.chromium.org/10701094/diff/6018/webkit/fileapi/obfuscated_f...
> File webkit/fileapi/obfuscated_file_util.cc (right):
> 
>
http://codereview.chromium.org/10701094/diff/6018/webkit/fileapi/obfuscated_f...
> webkit/fileapi/obfuscated_file_util.cc:1261: LOG(WARNING) << "Failed to
> origin+type directory: " << path.value();
> Missing "get " before "origin", from an old CL.  Mind fixing that while you're
> in here?

+1

[Bernhard Bauer, 2012-07-11 08:33:31]

http://codereview.chromium.org/10701094/diff/6018/webkit/fileapi/obfuscated_f...
File webkit/fileapi/obfuscated_file_util.cc (right):

http://codereview.chromium.org/10701094/diff/6018/webkit/fileapi/obfuscated_f...
webkit/fileapi/obfuscated_file_util.cc:1261: LOG(WARNING) << "Failed to
origin+type directory: " << path.value();
On 2012/07/10 20:20:07, ericu wrote:
> Missing "get " before "origin", from an old CL.  Mind fixing that while you're
> in here?

Sure, done.

[commit-bot: I haz the power, 2012-07-11 08:33:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bauerb@chromium.org/10701094/2004

[commit-bot: I haz the power, 2012-07-11 09:35:04]

Change committed as 146089