Fixes a bug that .crx can not be installed by "Could not create...

Fixes a bug that .crx can not be installed by "Could not create
directory for unzipping." in the case that the environment variable
TMP is set to a root directory of a drive such as "C:\"

TEST=Add new tests to base_unittest
BUG=23911
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=28529

[Yuzo, 2009-10-09 05:02:49]

http://codereview.chromium.org/257061/diff/3002/3004
File base/file_util_unittest.cc (right):

http://codereview.chromium.org/257061/diff/3002/3004#newcode781
Line 781: ASSERT_EQ(0, _dupenv_s(&original_tmp, &original_tmp_size, "TMP"));
To be safer, should we use _tdupenv_s?
Same should apply to _tputenv_s.

http://codereview.chromium.org/257061/diff/3002/3004#newcode781
Line 781: ASSERT_EQ(0, _dupenv_s(&original_tmp, &original_tmp_size, "TMP"));
I think we should define and use a constant for "TMP", because
it appears many times below.

http://codereview.chromium.org/257061/diff/3002/3004#newcode809
Line 809: }
Should we remove TMP if! original_tmp?

} else {
  _tputenv_s("TMP", "");
}

[tkent, 2009-10-09 05:52:12]

http://codereview.chromium.org/257061/diff/3002/3004
File base/file_util_unittest.cc (right):

http://codereview.chromium.org/257061/diff/3002/3004#newcode781
Line 781: ASSERT_EQ(0, _dupenv_s(&original_tmp, &original_tmp_size, "TMP"));
On 2009/10/09 05:02:50, Yuzo wrote:
> To be safer, should we use _tdupenv_s?
> Same should apply to _tputenv_s.

Done.

http://codereview.chromium.org/257061/diff/3002/3004#newcode781
Line 781: ASSERT_EQ(0, _dupenv_s(&original_tmp, &original_tmp_size, "TMP"));
On 2009/10/09 05:02:50, Yuzo wrote:
> I think we should define and use a constant for "TMP", because
> it appears many times below.

Done.

http://codereview.chromium.org/257061/diff/3002/3004#newcode809
Line 809: }
On 2009/10/09 05:02:50, Yuzo wrote:
> Should we remove TMP if! original_tmp?
> 
> } else {
>   _tputenv_s("TMP", "");
> } 

That's right!  Done.

[Yuzo, 2009-10-09 05:59:04]

LGTM

[Erik does not do reviews, 2009-10-09 14:16:33]

Apologies for missing this when you first sent it my way (feel free to ping me
in the future if I'm slow to respond on a review).

I don't think this patch is right, although path issues always give me a
headache, so I'm cc'ing mentovai to double-check my thinking here.

http://codereview.chromium.org/257061/diff/8001/9002
File base/file_util_unittest.cc (right):

http://codereview.chromium.org/257061/diff/8001/9002#newcode781
Line 781: _T(""), _T("C:"), _T("C:\\"), _T("C:\\tmp"), _T("C:\\tmp\\")
Isn't this saying that "" and "C:" should be considered absolute paths?  How
could that possibly be true?

http://codereview.chromium.org/257061/diff/8001/9001
File base/file_util_win.cc (right):

http://codereview.chromium.org/257061/diff/8001/9001#newcode431
Line 431: *path = FilePath(temp_path).StripTrailingSeparators();
If this changes anything for your test case, then something else is broken. 
StripTrailingSeparator on "C:\" should still be "C:\".  IsAbsolutePath on "C:\"
should be true and on "C:" should be false.  Am I misunderstanding something
here?

[Mark Mentovai, 2009-10-09 14:31:15]

http://codereview.chromium.org/257061/diff/8001/9002
File base/file_util_unittest.cc (right):

http://codereview.chromium.org/257061/diff/8001/9002#newcode781
Line 781: _T(""), _T("C:"), _T("C:\\"), _T("C:\\tmp"), _T("C:\\tmp\\")
I guess Win32's ::GetTempPath always returns an absolute path?

http://codereview.chromium.org/257061/diff/8001/9001
File base/file_util_win.cc (right):

http://codereview.chromium.org/257061/diff/8001/9001#newcode431
Line 431: *path = FilePath(temp_path).StripTrailingSeparators();
I agree.  It looks like file_util::TrimTrailingSeparator is dangerously broken.

leiz added it, maybe we should hear what he has to say.

[tkent, 2009-10-09 15:09:34]

http://codereview.chromium.org/257061/diff/8001/9002
File base/file_util_unittest.cc (right):

http://codereview.chromium.org/257061/diff/8001/9002#newcode781
Line 781: _T(""), _T("C:"), _T("C:\\"), _T("C:\\tmp"), _T("C:\\tmp\\")
On 2009/10/09 14:31:16, Mark Mentovai wrote:
> I guess Win32's ::GetTempPath always returns an absolute path?

Right.

May EXPECT_TRUE(path.isAbsolute()) was misleading.  I should have added
  EXPECT_TRUE(the last character of path.value() is not a separator ||
path.value().length() == 3);

http://codereview.chromium.org/257061/diff/8001/9001
File base/file_util_win.cc (right):

http://codereview.chromium.org/257061/diff/8001/9001#newcode431
Line 431: *path = FilePath(temp_path).StripTrailingSeparators();
On 2009/10/09 14:16:33, Erik Kay wrote:
> If this changes anything for your test case, then something else is broken. 
> StripTrailingSeparator on "C:\" should still be "C:\".  IsAbsolutePath on
"C:\"
> should be true and on "C:" should be false.  Am I misunderstanding something
> here?

You're right.  As Mark wrote, TrimTrailingSeprator() which was used here in the
previous revision made "C:" from "C:\".  It made the bug.

We had better remove TrimTrailingSeparator().

[Erik does not do reviews, 2009-10-12 18:31:29]

ping.  Any movement on this?  Is leiz/thestig around to answer Mark's quest=
ion?

I'd rather that the original fix not stay around in the code any
longer than necessary.

Erik


On Fri, Oct 9, 2009 at 8:09 AM,  <tkent@chromium.org> wrote:
>
> http://codereview.chromium.org/257061/diff/8001/9002
> File base/file_util_unittest.cc (right):
>
> http://codereview.chromium.org/257061/diff/8001/9002#newcode781
> Line 781: _T(""), _T("C:"), _T("C:\\"), _T("C:\\tmp"), _T("C:\\tmp\\")
> On 2009/10/09 14:31:16, Mark Mentovai wrote:
>>
>> I guess Win32's ::GetTempPath always returns an absolute path?
>
> Right.
>
> May EXPECT_TRUE(path.isAbsolute()) was misleading. =A0I should have added
> =A0EXPECT_TRUE(the last character of path.value() is not a separator ||
> path.value().length() =3D=3D 3);
>
> http://codereview.chromium.org/257061/diff/8001/9001
> File base/file_util_win.cc (right):
>
> http://codereview.chromium.org/257061/diff/8001/9001#newcode431
> Line 431: *path =3D FilePath(temp_path).StripTrailingSeparators();
> On 2009/10/09 14:16:33, Erik Kay wrote:
>>
>> If this changes anything for your test case, then something else is
>
> broken.
>>
>> StripTrailingSeparator on "C:\" should still be "C:\". =A0IsAbsolutePath
>
> on "C:\"
>>
>> should be true and on "C:" should be false. =A0Am I misunderstanding
>
> something
>>
>> here?
>
> You're right. =A0As Mark wrote, TrimTrailingSeprator() which was used her=
e
> in the previous revision made "C:" from "C:\". =A0It made the bug.
>
> We had better remove TrimTrailingSeparator().
>
> http://codereview.chromium.org/257061
>

[Lei Zhang, 2009-10-12 18:44:01]

On 2009/10/12 18:31:29, Erik Kay wrote:
> ping.  Any movement on this?  Is leiz/thestig around to answer Mark's
question?

Sorry, I missed this thread earlier. I actually didn't write
TrimTrailingSeparator(). I was trying to remove it, failed, and reverted. SVN
says before I touched it, the last person to touch it was Erik. _punts_

[Erik does not do reviews, 2009-10-12 18:56:09]

On Mon, Oct 12, 2009 at 11:44 AM,  <thestig@chromium.org> wrote:
> On 2009/10/12 18:31:29, Erik Kay wrote:
>>
>> ping. =A0Any movement on this? =A0Is leiz/thestig around to answer Mark'=
s
>
> question?
>
> Sorry, I missed this thread earlier. I actually didn't write
> TrimTrailingSeparator(). I was trying to remove it, failed, and reverted.
> SVN
> says before I touched it, the last person to touch it was Erik. _punts_

Thanks for the answer.  Sigh.  My change was fixing the "/" version of
this bug for Mac & Linux (the previous version is "initial commit").
It looks like I missed the "C:\" case. :-(  As Lei said, this
deprecated version should just be removed.

However, now that I look at it, I'm confused why this came up in
Mark's reply.  My comment should have taken us back to
FilePath::StripTrailingSeparatorsInternal, which also must be broken
if the original change here actually did anything.

Erik


>
> http://codereview.chromium.org/257061
>

[thelawofueki_plus, 2010-12-10 01:38:25]

''Could not create directory for unzipping''
??