Preserve modification timestamp when zipping files.

Preserve modification timestamp when zipping files.

Files packaged in a zip archive now have their last modification date
and time set. When unzipping the files, the output file's last
modification time is also set (if the time in the zip file is not valid,
the output's file time is not set to prevent the original bug causing
files to have dates set to 1980).

Also fixed some cpplint issues.

BUG=326988
TEST=ZipTest.ZipTimeStamp

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=241278

[satorux1, 2013-12-12 08:03:57]

https://codereview.chromium.org/99333019/diff/1/third_party/zlib/google/zip.cc
File third_party/zlib/google/zip.cc (right):

https://codereview.chromium.org/99333019/diff/1/third_party/zlib/google/zip.c...
third_party/zlib/google/zip.cc:30: zip_fileinfo TimeToZipFileInfo(const
base::Time& file_time) {
function comment is missing.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Comments

https://codereview.chromium.org/99333019/diff/1/third_party/zlib/google/zip.c...
third_party/zlib/google/zip.cc:52: zip_fileinfo GetFileInfoForZipping(const
base::FilePath& path) {
ditto

https://codereview.chromium.org/99333019/diff/1/third_party/zlib/google/zip_u...
File third_party/zlib/google/zip_unittest.cc (right):

https://codereview.chromium.org/99333019/diff/1/third_party/zlib/google/zip_u...
third_party/zlib/google/zip_unittest.cc:102: void TestTimeStamp(const char*
date_time, bool valid_year) {
This function was a bit hard to follow.
Could you describe what it does as a function comment? something like:

// 1) Create out.zip that includes test.txt of the specified timestamp
// 2) Extract out.zip into a directory
// 3) Confirm that test.txt in the output directory has the specified timestamp
//    unless |valid_year| is false

https://codereview.chromium.org/99333019/diff/1/third_party/zlib/google/zip_u...
third_party/zlib/google/zip_unittest.cc:134: // Invalid date means the
modification time will default to 'now'.
now? I thought it's Unix Epoch. If so, please make it EXPECT_EQ of the unix
epoch.

https://codereview.chromium.org/99333019/diff/1/third_party/zlib/google/zip_u...
third_party/zlib/google/zip_unittest.cc:260: if (false)
The #if-defs are complicated. I'd suggest remove them and not to test the case
of year 2107. It's unlikely for the code to be used in 2107 :)

[João Eiras, 2013-12-12 14:14:56]

>
https://codereview.chromium.org/99333019/diff/1/third_party/zlib/google/zip_u...
> third_party/zlib/google/zip_unittest.cc:134: // Invalid date means the
> modification time will default to 'now'.
> now? I thought it's Unix Epoch. If so, please make it EXPECT_EQ of the unix
> epoch.

The epoch is only used internally when the date stored in the zip file is not
valid
See ZipReader::EntryInfo::EntryInfo()
Setting the time to the epoch 

> 
>
https://codereview.chromium.org/99333019/diff/1/third_party/zlib/google/zip_u...
> third_party/zlib/google/zip_unittest.cc:260: if (false)
> The #if-defs are complicated. I'd suggest remove them and not to test the case
> of year 2107. It's unlikely for the code to be used in 2107 :)

Well, it's always a good practice to test boundary conditions. Not that the code
will be used then, just to just make sure it does not misbehave if it gets bad
input. But ok, in this case it's a bit far fetched.

[João Eiras, 2013-12-12 14:16:16]

> The epoch is only used internally when the date stored in the zip file is not
> valid
> See ZipReader::EntryInfo::EntryInfo()
> Setting the time to the epoch 
>

Doh truncated sentence, sorry.

Setting the time to the epoch when extracting the file is wrong obviously, so
it's just preferable to use the current time.

[satorux1, 2013-12-13 04:20:02]

On 2013/12/12 14:14:56, João Eiras wrote:
> >
>
https://codereview.chromium.org/99333019/diff/1/third_party/zlib/google/zip_u...
> > third_party/zlib/google/zip_unittest.cc:134: // Invalid date means the
> > modification time will default to 'now'.
> > now? I thought it's Unix Epoch. If so, please make it EXPECT_EQ of the unix
> > epoch.
> 
> The epoch is only used internally when the date stored in the zip file is not
> valid
> See ZipReader::EntryInfo::EntryInfo()
> Setting the time to the epoch 

When you reply, please reply to individual comments from
codereview.chromium.org. That way, our conversation is inlined in the code.

> 
> > 
> >
>
https://codereview.chromium.org/99333019/diff/1/third_party/zlib/google/zip_u...
> > third_party/zlib/google/zip_unittest.cc:260: if (false)
> > The #if-defs are complicated. I'd suggest remove them and not to test the
case
> > of year 2107. It's unlikely for the code to be used in 2107 :)
> 
> Well, it's always a good practice to test boundary conditions. Not that the
code
> will be used then, just to just make sure it does not misbehave if it gets bad
> input. But ok, in this case it's a bit far fetched.

My concern was about complexity of the #if-defs. Maybe you can keep this only
for windows.

#if defined(OS_WIN)
  TestTimeStamp("31 Oct 2107 23:59:58", true);
#endif

[satorux1, 2013-12-13 04:52:50]

https://codereview.chromium.org/99333019/diff/20001/third_party/zlib/google/z...
File third_party/zlib/google/zip.cc (right):

https://codereview.chromium.org/99333019/diff/20001/third_party/zlib/google/z...
third_party/zlib/google/zip.cc:32: */
Please use // for comments

https://codereview.chromium.org/99333019/diff/20001/third_party/zlib/google/z...
third_party/zlib/google/zip.cc:57: */
ditto

https://codereview.chromium.org/99333019/diff/20001/third_party/zlib/google/z...
File third_party/zlib/google/zip_unittest.cc (right):

https://codereview.chromium.org/99333019/diff/20001/third_party/zlib/google/z...
third_party/zlib/google/zip_unittest.cc:104: //    set to the valud or
|date_time|.
valud? this line seems to be unnecessary.

https://codereview.chromium.org/99333019/diff/20001/third_party/zlib/google/z...
third_party/zlib/google/zip_unittest.cc:109: //    modification defaults to the
current time.
I think we should have documentation like this in zip_reader.h, describing the
policy on timestamp preservation. Could you add it?

https://codereview.chromium.org/99333019/diff/20001/third_party/zlib/google/z...
third_party/zlib/google/zip_unittest.cc:127: base::Time now_time =
base::Time::Now() - base::TimeDelta::FromSeconds(1);
|now_time| actually points to a time in the past (because of -1 second), which
is somewhat awkward.  I guess -1 was added because clock can slightly go
backward. However, here, we are doing a lot of file IO, so I guess we don't have
to worry about it. maybe:

base::Time start_time = base::Time::Now();

https://codereview.chromium.org/99333019/diff/20001/third_party/zlib/google/z...
third_party/zlib/google/zip_unittest.cc:247: TestTimeStamp("23 Oct 1997
23:22:20", true);
true/false looks cryptic. It'd be nice to define an enum like:

enum {
  VALID_YEAR,
  INVALID_YEAR
}

Then it becomes clearer:

   TestTimeStamp("23 Oct 1997 23:22:20", VALID_YEAR);

[João Eiras, 2013-12-13 17:01:56]

https://codereview.chromium.org/99333019/diff/20001/third_party/zlib/google/z...
File third_party/zlib/google/zip_unittest.cc (right):

https://codereview.chromium.org/99333019/diff/20001/third_party/zlib/google/z...
third_party/zlib/google/zip_unittest.cc:127: base::Time now_time =
base::Time::Now() - base::TimeDelta::FromSeconds(1);
On 2013/12/13 04:52:50, satorux1 wrote:
> |now_time| actually points to a time in the past (because of -1 second), which
> is somewhat awkward.  I guess -1 was added because clock can slightly go
> backward. However, here, we are doing a lot of file IO, so I guess we don't
have
> to worry about it. maybe:
> 
> base::Time start_time = base::Time::Now();

The seconds in the file's timestamp  are floored when zipping, so -1 is a quick
way to work around that, because Now(), although it should actually be -2
because "3.900" seconss will be stored as "2" in the zip file. I've made it more
explicit.

[satorux1, 2013-12-17 02:32:25]

LGTM with requests:

https://codereview.chromium.org/99333019/diff/20001/third_party/zlib/google/z...
File third_party/zlib/google/zip_unittest.cc (right):

https://codereview.chromium.org/99333019/diff/20001/third_party/zlib/google/z...
third_party/zlib/google/zip_unittest.cc:104: //    set to the valud or
|date_time|.
On 2013/12/13 04:52:50, satorux1 wrote:
> valud? this line seems to be unnecessary.

Please mark comments you addressed with 'Done'. That makes it easier to track
what's done and what's not.

https://codereview.chromium.org/99333019/diff/20001/third_party/zlib/google/z...
third_party/zlib/google/zip_unittest.cc:127: base::Time now_time =
base::Time::Now() - base::TimeDelta::FromSeconds(1);
On 2013/12/13 17:01:57, João Eiras wrote:
> On 2013/12/13 04:52:50, satorux1 wrote:
> > |now_time| actually points to a time in the past (because of -1 second),
which
> > is somewhat awkward.  I guess -1 was added because clock can slightly go
> > backward. However, here, we are doing a lot of file IO, so I guess we don't
> have
> > to worry about it. maybe:
> > 
> > base::Time start_time = base::Time::Now();
> 
> The seconds in the file's timestamp  are floored when zipping, so -1 is a
quick
> way to work around that, because Now(), although it should actually be -2
> because "3.900" seconss will be stored as "2" in the zip file. I've made it
more
> explicit.

Didn't know that the timestamp resolution in zip files is two seconds. Please
document it explicitly here.

https://codereview.chromium.org/99333019/diff/40001/third_party/zlib/google/z...
File third_party/zlib/google/zip_reader.h (right):

https://codereview.chromium.org/99333019/diff/40001/third_party/zlib/google/z...
third_party/zlib/google/zip_reader.h:61: // not valid, the unix epoch will be
returned.
Please document that the timestamp resolution is two seconds. A reference to the
spec would be nice.

[João Eiras, 2013-12-17 10:15:37]

https://codereview.chromium.org/99333019/diff/20001/third_party/zlib/google/z...
File third_party/zlib/google/zip.cc (right):

https://codereview.chromium.org/99333019/diff/20001/third_party/zlib/google/z...
third_party/zlib/google/zip.cc:32: */
On 2013/12/13 04:52:50, satorux1 wrote:
> Please use // for comments

Done.

https://codereview.chromium.org/99333019/diff/20001/third_party/zlib/google/z...
third_party/zlib/google/zip.cc:57: */
On 2013/12/13 04:52:50, satorux1 wrote:
> ditto

Done.

https://codereview.chromium.org/99333019/diff/20001/third_party/zlib/google/z...
File third_party/zlib/google/zip_unittest.cc (right):

https://codereview.chromium.org/99333019/diff/20001/third_party/zlib/google/z...
third_party/zlib/google/zip_unittest.cc:104: //    set to the valud or
|date_time|.
On 2013/12/13 04:52:50, satorux1 wrote:
> valud? this line seems to be unnecessary.

Done.

https://codereview.chromium.org/99333019/diff/20001/third_party/zlib/google/z...
third_party/zlib/google/zip_unittest.cc:104: //    set to the valud or
|date_time|.
On 2013/12/17 02:32:25, satorux1 wrote:
> On 2013/12/13 04:52:50, satorux1 wrote:
> > valud? this line seems to be unnecessary.
> 
> Please mark comments you addressed with 'Done'. That makes it easier to track
> what's done and what's not.

Done.

https://codereview.chromium.org/99333019/diff/20001/third_party/zlib/google/z...
third_party/zlib/google/zip_unittest.cc:109: //    modification defaults to the
current time.
On 2013/12/13 04:52:50, satorux1 wrote:
> I think we should have documentation like this in zip_reader.h, describing the
> policy on timestamp preservation. Could you add it?

Done.

https://codereview.chromium.org/99333019/diff/20001/third_party/zlib/google/z...
third_party/zlib/google/zip_unittest.cc:127: base::Time now_time =
base::Time::Now() - base::TimeDelta::FromSeconds(1);
On 2013/12/13 04:52:50, satorux1 wrote:
> |now_time| actually points to a time in the past (because of -1 second), which
> is somewhat awkward.  I guess -1 was added because clock can slightly go
> backward. However, here, we are doing a lot of file IO, so I guess we don't
have
> to worry about it. maybe:
> 
> base::Time start_time = base::Time::Now();

Done.

https://codereview.chromium.org/99333019/diff/20001/third_party/zlib/google/z...
third_party/zlib/google/zip_unittest.cc:127: base::Time now_time =
base::Time::Now() - base::TimeDelta::FromSeconds(1);
On 2013/12/13 17:01:57, João Eiras wrote:
> On 2013/12/13 04:52:50, satorux1 wrote:
> > |now_time| actually points to a time in the past (because of -1 second),
which
> > is somewhat awkward.  I guess -1 was added because clock can slightly go
> > backward. However, here, we are doing a lot of file IO, so I guess we don't
> have
> > to worry about it. maybe:
> > 
> > base::Time start_time = base::Time::Now();
> 
> The seconds in the file's timestamp  are floored when zipping, so -1 is a
quick
> way to work around that, because Now(), although it should actually be -2
> because "3.900" seconss will be stored as "2" in the zip file. I've made it
more
> explicit.

Done.

https://codereview.chromium.org/99333019/diff/20001/third_party/zlib/google/z...
third_party/zlib/google/zip_unittest.cc:127: base::Time now_time =
base::Time::Now() - base::TimeDelta::FromSeconds(1);
On 2013/12/17 02:32:25, satorux1 wrote:
> On 2013/12/13 17:01:57, João Eiras wrote:
> > On 2013/12/13 04:52:50, satorux1 wrote:
> > > |now_time| actually points to a time in the past (because of -1 second),
> which
> > > is somewhat awkward.  I guess -1 was added because clock can slightly go
> > > backward. However, here, we are doing a lot of file IO, so I guess we
don't
> > have
> > > to worry about it. maybe:
> > > 
> > > base::Time start_time = base::Time::Now();
> > 
> > The seconds in the file's timestamp  are floored when zipping, so -1 is a
> quick
> > way to work around that, because Now(), although it should actually be -2
> > because "3.900" seconss will be stored as "2" in the zip file. I've made it
> more
> > explicit.
> 
> Didn't know that the timestamp resolution in zip files is two seconds. Please
> document it explicitly here.

Done.

https://codereview.chromium.org/99333019/diff/20001/third_party/zlib/google/z...
third_party/zlib/google/zip_unittest.cc:247: TestTimeStamp("23 Oct 1997
23:22:20", true);
On 2013/12/13 04:52:50, satorux1 wrote:
> true/false looks cryptic. It'd be nice to define an enum like:
> 
> enum {
>   VALID_YEAR,
>   INVALID_YEAR
> }
> 
> Then it becomes clearer:
> 
>    TestTimeStamp("23 Oct 1997 23:22:20", VALID_YEAR);

Done.

https://codereview.chromium.org/99333019/diff/40001/third_party/zlib/google/z...
File third_party/zlib/google/zip_reader.h (right):

https://codereview.chromium.org/99333019/diff/40001/third_party/zlib/google/z...
third_party/zlib/google/zip_reader.h:61: // not valid, the unix epoch will be
returned.
On 2013/12/17 02:32:25, satorux1 wrote:
> Please document that the timestamp resolution is two seconds. A reference to
the
> spec would be nice.

Done.

[commit-bot: I haz the power, 2013-12-17 10:19:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joaoe@opera.com/99333019/60001

[commit-bot: I haz the power, 2013-12-17 14:40:33]

Change committed as 241278