Update mtime of directory when containee changed.

Update mtime of directory when containing entry changed.

BUG=None
TEST='ObfuscatedFileUtilTest.TestDirectoryTimestamp'


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

[tzik, 2011-11-29 15:00:46]

Hi, Eric.
I wrote a patch to update modification time of directory when a entry is
created/deleted on it.

Currently, creating or deleting entry using FileSystem API don't modify
modification time of parent directory.
This behavior differs from native filesystem, and updating mtime of parent
directory let us watch directory easier with polling.

Could you review this patch?

[ericu, 2011-11-30 19:16:03]

http://codereview.chromium.org/8728029/diff/4/webkit/fileapi/obfuscated_file_...
File webkit/fileapi/obfuscated_file_util.cc (right):

http://codereview.chromium.org/8728029/diff/4/webkit/fileapi/obfuscated_file_...
webkit/fileapi/obfuscated_file_util.cc:98: // Do nothing for root directory.
Why do you special-case the root?  My Linux box doesn't.

Also, you'll want to remove the TODO at the top of obfuscated_file_util.h.

http://codereview.chromium.org/8728029/diff/4/webkit/fileapi/obfuscated_file_...
webkit/fileapi/obfuscated_file_util.cc:302: TouchDirectory(db, parent_id);
You don't want to touch the parent if the creation failed [here and elsewhere].

http://codereview.chromium.org/8728029/diff/4/webkit/fileapi/obfuscated_file_...
webkit/fileapi/obfuscated_file_util.cc:364: TouchDirectory(db, parent_id);
Perhaps it would be better to put the call inside CreateFile?

http://codereview.chromium.org/8728029/diff/4/webkit/fileapi/obfuscated_file_...
webkit/fileapi/obfuscated_file_util.cc:729: TouchDirectory(db,
src_file_info.parent_id);
You need to update the destination directory's mtime as well; even though the
names in the list haven't changed, the contents of the directory [the inode
list] have, and so there's an update.

http://codereview.chromium.org/8728029/diff/4/webkit/fileapi/obfuscated_file_...
File webkit/fileapi/obfuscated_file_util_unittest.cc (right):

http://codereview.chromium.org/8728029/diff/4/webkit/fileapi/obfuscated_file_...
webkit/fileapi/obfuscated_file_util_unittest.cc:1455:
ofu()->Touch(context.get(), dir_path, base::Time(), base::Time()));
I'm not sure the round-trip from base::Time to what's stored in the database is
always guaranteed to be lossless.  Please add a call to GetFileInfo and an
EXPECT_EQ(base::Time(), file_info.last_modified) here just to make sure.

http://codereview.chromium.org/8728029/diff/4/webkit/fileapi/obfuscated_file_...
webkit/fileapi/obfuscated_file_util_unittest.cc:1472: EXPECT_NE(base::Time(),
file_info.last_modified);
Please also add tests for move, mkdir, copy, etc., and check the source
directory of a move as well.

[tzik, 2011-12-01 16:28:46]

http://codereview.chromium.org/8728029/diff/4/webkit/fileapi/obfuscated_file_...
File webkit/fileapi/obfuscated_file_util.cc (right):

http://codereview.chromium.org/8728029/diff/4/webkit/fileapi/obfuscated_file_...
webkit/fileapi/obfuscated_file_util.cc:98: // Do nothing for root directory.
On 2011/11/30 19:16:04, ericu wrote:
> Why do you special-case the root?  My Linux box doesn't.
It is just to keep consistency to current implementation and existing tests.
In current directory database implementation, we don't have database entry for
root. Its mtime is always now.

> 
> Also, you'll want to remove the TODO at the top of obfuscated_file_util.h.
Done.

http://codereview.chromium.org/8728029/diff/4/webkit/fileapi/obfuscated_file_...
webkit/fileapi/obfuscated_file_util.cc:302: TouchDirectory(db, parent_id);
On 2011/11/30 19:16:04, ericu wrote:
> You don't want to touch the parent if the creation failed [here and
elsewhere].

Done.

http://codereview.chromium.org/8728029/diff/4/webkit/fileapi/obfuscated_file_...
webkit/fileapi/obfuscated_file_util.cc:364: TouchDirectory(db, parent_id);
On 2011/11/30 19:16:04, ericu wrote:
> Perhaps it would be better to put the call inside CreateFile?

Done.

http://codereview.chromium.org/8728029/diff/4/webkit/fileapi/obfuscated_file_...
webkit/fileapi/obfuscated_file_util.cc:729: TouchDirectory(db,
src_file_info.parent_id);
On 2011/11/30 19:16:04, ericu wrote:
> You need to update the destination directory's mtime as well; even though the
> names in the list haven't changed, the contents of the directory [the inode
> list] have, and so there's an update.

Done.

http://codereview.chromium.org/8728029/diff/4/webkit/fileapi/obfuscated_file_...
File webkit/fileapi/obfuscated_file_util_unittest.cc (right):

http://codereview.chromium.org/8728029/diff/4/webkit/fileapi/obfuscated_file_...
webkit/fileapi/obfuscated_file_util_unittest.cc:1455:
ofu()->Touch(context.get(), dir_path, base::Time(), base::Time()));
On 2011/11/30 19:16:04, ericu wrote:
> I'm not sure the round-trip from base::Time to what's stored in the database
is
> always guaranteed to be lossless.  Please add a call to GetFileInfo and an
> EXPECT_EQ(base::Time(), file_info.last_modified) here just to make sure.

Done.

http://codereview.chromium.org/8728029/diff/4/webkit/fileapi/obfuscated_file_...
webkit/fileapi/obfuscated_file_util_unittest.cc:1472: EXPECT_NE(base::Time(),
file_info.last_modified);
On 2011/11/30 19:16:04, ericu wrote:
> Please also add tests for move, mkdir, copy, etc., and check the source
> directory of a move as well.

Done.

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

http://codereview.chromium.org/8728029/diff/10001/webkit/fileapi/obfuscated_f...
webkit/fileapi/obfuscated_file_util.cc:537: if (!db->UpdateFileInfo(file_id,
file_info))
Calling UpdateFileInfo failed for updating directory with children.

[ericu, 2011-12-05 21:43:50]

LGTM with the below fixes.

http://codereview.chromium.org/8728029/diff/4/webkit/fileapi/obfuscated_file_...
File webkit/fileapi/obfuscated_file_util.cc (right):

http://codereview.chromium.org/8728029/diff/4/webkit/fileapi/obfuscated_file_...
webkit/fileapi/obfuscated_file_util.cc:98: // Do nothing for root directory.
On 2011/12/01 16:28:46, tzik wrote:
> On 2011/11/30 19:16:04, ericu wrote:
> > Why do you special-case the root?  My Linux box doesn't.
> It is just to keep consistency to current implementation and existing tests.
> In current directory database implementation, we don't have database entry for
> root. Its mtime is always now.

That is incorrect.  Look at FileSystemDirectoryDatabase::StoreDefaultValues. 
The mtime of the root is always base::Time::Now() at the time it's /created/. 
And until there's something in the database, we don't bother to create the root.
 But when there are things in the database, the root exists there as well, and
can be updated.

http://codereview.chromium.org/8728029/diff/10001/webkit/fileapi/obfuscated_f...
File webkit/fileapi/obfuscated_file_util_unittest.cc (right):

http://codereview.chromium.org/8728029/diff/10001/webkit/fileapi/obfuscated_f...
webkit/fileapi/obfuscated_file_util_unittest.cc:484: FilePath data_path;
This could use GetModifiedTime here.

http://codereview.chromium.org/8728029/diff/10001/webkit/fileapi/obfuscated_f...
webkit/fileapi/obfuscated_file_util_unittest.cc:1517: // EnsureFileExists,
create case.
Please add a fail case as well [perhaps trying to create a file where a
directory exists].

http://codereview.chromium.org/8728029/diff/10001/webkit/fileapi/obfuscated_f...
webkit/fileapi/obfuscated_file_util_unittest.cc:1595: EXPECT_EQ(base::Time(),
GetModifiedTime(dir_path));
Please also check that the timestamp on CreateDirectory_dir/1 got updated.

[tzik, 2011-12-06 05:13:50]

http://codereview.chromium.org/8728029/diff/10001/webkit/fileapi/obfuscated_f...
File webkit/fileapi/obfuscated_file_util_unittest.cc (right):

http://codereview.chromium.org/8728029/diff/10001/webkit/fileapi/obfuscated_f...
webkit/fileapi/obfuscated_file_util_unittest.cc:484: FilePath data_path;
On 2011/12/05 21:43:50, ericu wrote:
> This could use GetModifiedTime here.

Done.

http://codereview.chromium.org/8728029/diff/10001/webkit/fileapi/obfuscated_f...
webkit/fileapi/obfuscated_file_util_unittest.cc:1517: // EnsureFileExists,
create case.
On 2011/12/05 21:43:50, ericu wrote:
> Please add a fail case as well [perhaps trying to create a file where a
> directory exists].

Done.

http://codereview.chromium.org/8728029/diff/10001/webkit/fileapi/obfuscated_f...
webkit/fileapi/obfuscated_file_util_unittest.cc:1595: EXPECT_EQ(base::Time(),
GetModifiedTime(dir_path));
On 2011/12/05 21:43:50, ericu wrote:
> Please also check that the timestamp on CreateDirectory_dir/1 got updated.

Done.

[ericu, 2011-12-07 01:07:33]

Please don't forget to remove the special-casing of the root directory.

On Mon, Dec 5, 2011 at 9:13 PM,  <tzik@chromium.org> wrote:
>
>
http://codereview.chromium.org/8728029/diff/10001/webkit/fileapi/obfuscated_f...
> File webkit/fileapi/obfuscated_file_util_unittest.cc (right):
>
>
http://codereview.chromium.org/8728029/diff/10001/webkit/fileapi/obfuscated_f...
> webkit/fileapi/obfuscated_file_util_unittest.cc:484: FilePath data_path;
> On 2011/12/05 21:43:50, ericu wrote:
>>
>> This could use GetModifiedTime here.
>
> Done.
>
>
http://codereview.chromium.org/8728029/diff/10001/webkit/fileapi/obfuscated_f...
> webkit/fileapi/obfuscated_file_util_unittest.cc:1517: //
> EnsureFileExists, create case.
> On 2011/12/05 21:43:50, ericu wrote:
>>
>> Please add a fail case as well [perhaps trying to create a file where
>
> a
>>
>> directory exists].
>
> Done.
>
>
http://codereview.chromium.org/8728029/diff/10001/webkit/fileapi/obfuscated_f...
> webkit/fileapi/obfuscated_file_util_unittest.cc:1595:
> EXPECT_EQ(base::Time(), GetModifiedTime(dir_path));
> On 2011/12/05 21:43:50, ericu wrote:
>>
>> Please also check that the timestamp on CreateDirectory_dir/1 got
>
> updated.
>
> Done.
>
> http://codereview.chromium.org/8728029/
>

[ericu, 2011-12-08 01:05:02]

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

http://codereview.chromium.org/8728029/diff/14001/webkit/fileapi/obfuscated_f...
webkit/fileapi/obfuscated_file_util.cc:99: if (dir_id &&
!db->UpdateModificationTime(dir_id, base::Time::Now()))
Just remove the "dir_id && " here.

http://codereview.chromium.org/8728029/diff/14001/webkit/fileapi/obfuscated_f...
File webkit/fileapi/obfuscated_file_util_unittest.cc (right):

http://codereview.chromium.org/8728029/diff/14001/webkit/fileapi/obfuscated_f...
webkit/fileapi/obfuscated_file_util_unittest.cc:1600: ClearTimestamp(dir_path);
Missing a ClearTimestamp(path); here.

[tzik, 2011-12-08 02:11:40]

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

http://codereview.chromium.org/8728029/diff/14001/webkit/fileapi/obfuscated_f...
webkit/fileapi/obfuscated_file_util.cc:99: if (dir_id &&
!db->UpdateModificationTime(dir_id, base::Time::Now()))
On 2011/12/08 01:05:02, ericu wrote:
> Just remove the "dir_id && " here.

Done.

Thanks. I mistakenly saw previous error and thought it was not enought.
But as you say, this is all to do.

http://codereview.chromium.org/8728029/diff/14001/webkit/fileapi/obfuscated_f...
File webkit/fileapi/obfuscated_file_util_unittest.cc (right):

http://codereview.chromium.org/8728029/diff/14001/webkit/fileapi/obfuscated_f...
webkit/fileapi/obfuscated_file_util_unittest.cc:1600: ClearTimestamp(dir_path);
On 2011/12/08 01:05:02, ericu wrote:
> Missing a ClearTimestamp(path); here.

Done.

[ericu, 2011-12-08 02:42:09]

lgtm

[commit-bot: I haz the power, 2011-12-08 04:34:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tzik@chromium.org/8728029/21007

[commit-bot: I haz the power, 2011-12-08 05:38:06]

Change committed as 113565