Unpack extensions inside chrome's directory.

Unpack extensions inside chrome's profile directory.

Other users of the temp directory will be altered in a subsequent CL.

BUG=13044
TEST=SandboxedExtensionUnpackerTest.*, ScopedTempDir.UniqueTempDirUnderPath, FileUtilTest.CreateNewTempDirInDirTest, manual testing on win, linux, mac.

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

[Sam Kerner (Chrome), 2010-04-19 18:23:27]

Erik,
   This change adds a Temp directory under the chrome config directory.  I
wonder if that directory should be under Extensions instead, but I don't see a
sensible place to put it.  Suggestions welcome.

Sam

[Erik does not do reviews, 2010-04-20 00:29:53]

http://codereview.chromium.org/1582022/diff/14001/15001
File base/file_util.h (right):

http://codereview.chromium.org/1582022/diff/14001/15001#newcode251
base/file_util.h:251: bool CreateTempDirInDir(const FilePath& base_dir,
we have a naming mismatch here.  The functions above use "Temporary" and "Dir". 
The one below uses "Temp" and "Directory".  You're using one from each. :-) 
Seems like "Temporary" and "Dir" is the more common pattern.

http://codereview.chromium.org/1582022/diff/14001/15002
File base/file_util_posix.cc (right):

http://codereview.chromium.org/1582022/diff/14001/15002#newcode406
base/file_util_posix.cc:406: // this should be OK since mkdtemp just replaces
characters in place
nit: newline above comment

http://codereview.chromium.org/1582022/diff/14001/15002#newcode418
base/file_util_posix.cc:418: CHECK(new_dir_tmpl.find("%ls") !=
FilePath::StringType::npos)
This seems a bit fragile.  Why not just always append before the extension?  See
FilePath::InsertBeforeExtension.

http://codereview.chromium.org/1582022/diff/14001/15003
File base/file_util_unittest.cc (right):

http://codereview.chromium.org/1582022/diff/14001/15003#newcode1147
base/file_util_unittest.cc:1147: test_dir_,
nit: indent is off

http://codereview.chromium.org/1582022/diff/14001/15003#newcode1150
base/file_util_unittest.cc:1150: EXPECT_TRUE(file_util::PathExists(new_dir));
verify that test_dir_ is a parent of new_dir (FilePath::IsParent)

http://codereview.chromium.org/1582022/diff/14001/15004
File base/file_util_win.cc (right):

http://codereview.chromium.org/1582022/diff/14001/15004#newcode517
base/file_util_win.cc:517: //new_dir_name.append(IntToWString(rand() %
kint16max));
remove unused code

http://codereview.chromium.org/1582022/diff/14001/15004#newcode519
base/file_util_win.cc:519: new_dir_name = StringPrintf(new_dir_name.c_str(),
num.c_str());
same comment here as the posix code.  use concatenation.

http://codereview.chromium.org/1582022/diff/14001/15005
File base/scoped_temp_dir.cc (right):

http://codereview.chromium.org/1582022/diff/14001/15005#newcode34
base/scoped_temp_dir.cc:34: // Create a new, uniquly named directory under .
under |base_path|

http://codereview.chromium.org/1582022/diff/14001/15007
File base/scoped_temp_dir_unittest.cc (right):

http://codereview.chromium.org/1582022/diff/14001/15007#newcode70
base/scoped_temp_dir_unittest.cc:70:
EXPECT_TRUE(file_util::DirectoryExists(test_path));
verify IsParent

http://codereview.chromium.org/1582022/diff/14001/15008
File chrome/browser/extensions/crx_installer.cc (right):

http://codereview.chromium.org/1582022/diff/14001/15008#newcode29
chrome/browser/extensions/crx_installer.cc:29: 
nit: remove extra newlines

http://codereview.chromium.org/1582022/diff/14001/15008#newcode78
chrome/browser/extensions/crx_installer.cc:78:
CHECK(PathService::Get(chrome::DIR_USER_DATA, &user_data_dir));
Add a new directory to chrome_paths called something like DIR_USER_DATA_TEMP
rather than constructing this by hand at each callsite.

http://codereview.chromium.org/1582022/diff/14001/15009
File chrome/browser/extensions/sandboxed_extension_unpacker.cc (right):

http://codereview.chromium.org/1582022/diff/14001/15009#newcode52
chrome/browser/extensions/sandboxed_extension_unpacker.cc:52: extension_root_ =
temp_dir_.path().AppendASCII("TEMP_INSTALL");
maybe rename this to CRX_INSTALL since the user data temp dir may wind up being
shared.

[Sam Kerner (Chrome), 2010-04-20 20:46:37]

Ready for another look.

http://codereview.chromium.org/1582022/diff/14001/15001
File base/file_util.h (right):

http://codereview.chromium.org/1582022/diff/14001/15001#newcode251
base/file_util.h:251: bool CreateTempDirInDir(const FilePath& base_dir,
On 2010/04/20 00:29:53, Erik Kay wrote:
> we have a naming mismatch here.  The functions above use "Temporary" and
"Dir". 
> The one below uses "Temp" and "Directory".  You're using one from each. :-) 
> Seems like "Temporary" and "Dir" is the more common pattern.

Changed to CreateTemporaryDirInDir().

http://codereview.chromium.org/1582022/diff/14001/15002
File base/file_util_posix.cc (right):

http://codereview.chromium.org/1582022/diff/14001/15002#newcode406
base/file_util_posix.cc:406: // this should be OK since mkdtemp just replaces
characters in place
On 2010/04/20 00:29:53, Erik Kay wrote:
> nit: newline above comment

Done.

http://codereview.chromium.org/1582022/diff/14001/15002#newcode418
base/file_util_posix.cc:418: CHECK(new_dir_tmpl.find("%ls") !=
FilePath::StringType::npos)
On 2010/04/20 00:29:53, Erik Kay wrote:
> This seems a bit fragile.  Why not just always append before the extension? 
See
> FilePath::InsertBeforeExtension.

The item being created is a directory, so I am not sure an extension makes
sense.  I want to avoid changing the behavior of CreateNewTempDirectory().  I
changed the code to append the characters that make the name unique.

http://codereview.chromium.org/1582022/diff/14001/15003
File base/file_util_unittest.cc (right):

http://codereview.chromium.org/1582022/diff/14001/15003#newcode1147
base/file_util_unittest.cc:1147: test_dir_,
On 2010/04/20 00:29:53, Erik Kay wrote:
> nit: indent is off

Done.

http://codereview.chromium.org/1582022/diff/14001/15003#newcode1150
base/file_util_unittest.cc:1150: EXPECT_TRUE(file_util::PathExists(new_dir));
On 2010/04/20 00:29:53, Erik Kay wrote:
> verify that test_dir_ is a parent of new_dir (FilePath::IsParent)

Done.

http://codereview.chromium.org/1582022/diff/14001/15004
File base/file_util_win.cc (right):

http://codereview.chromium.org/1582022/diff/14001/15004#newcode517
base/file_util_win.cc:517: //new_dir_name.append(IntToWString(rand() %
kint16max));
On 2010/04/20 00:29:53, Erik Kay wrote:
> remove unused code

Done.

http://codereview.chromium.org/1582022/diff/14001/15004#newcode519
base/file_util_win.cc:519: new_dir_name = StringPrintf(new_dir_name.c_str(),
num.c_str());
On 2010/04/20 00:29:53, Erik Kay wrote:
> same comment here as the posix code.  use concatenation.

Done.

http://codereview.chromium.org/1582022/diff/14001/15005
File base/scoped_temp_dir.cc (right):

http://codereview.chromium.org/1582022/diff/14001/15005#newcode34
base/scoped_temp_dir.cc:34: // Create a new, uniquly named directory under .
On 2010/04/20 00:29:53, Erik Kay wrote:
> under |base_path|

Done.

http://codereview.chromium.org/1582022/diff/14001/15007
File base/scoped_temp_dir_unittest.cc (right):

http://codereview.chromium.org/1582022/diff/14001/15007#newcode70
base/scoped_temp_dir_unittest.cc:70:
EXPECT_TRUE(file_util::DirectoryExists(test_path));
On 2010/04/20 00:29:53, Erik Kay wrote:
> verify IsParent

Done.

http://codereview.chromium.org/1582022/diff/14001/15008
File chrome/browser/extensions/crx_installer.cc (right):

http://codereview.chromium.org/1582022/diff/14001/15008#newcode29
chrome/browser/extensions/crx_installer.cc:29: 
On 2010/04/20 00:29:53, Erik Kay wrote:
> nit: remove extra newlines

Done.

http://codereview.chromium.org/1582022/diff/14001/15008#newcode78
chrome/browser/extensions/crx_installer.cc:78:
CHECK(PathService::Get(chrome::DIR_USER_DATA, &user_data_dir));
On 2010/04/20 00:29:53, Erik Kay wrote:
> Add a new directory to chrome_paths called something like DIR_USER_DATA_TEMP
> rather than constructing this by hand at each callsite.

Done.

http://codereview.chromium.org/1582022/diff/14001/15009
File chrome/browser/extensions/sandboxed_extension_unpacker.cc (right):

http://codereview.chromium.org/1582022/diff/14001/15009#newcode52
chrome/browser/extensions/sandboxed_extension_unpacker.cc:52: extension_root_ =
temp_dir_.path().AppendASCII("TEMP_INSTALL");
On 2010/04/20 00:29:53, Erik Kay wrote:
> maybe rename this to CRX_INSTALL since the user data temp dir may wind up
being
> shared.

Done.

[Sam Kerner (Chrome), 2010-04-29 04:15:51]

Erik,
   I got the message below from a forum moderator regarding this issue.  I
wonder if I should push to get this into version 5.0.  

Sam


Comment #76 on issue 35198 by google-chrome-guide5@google.com: Extension Install
Failure for all extensions: "could not create directory for unzipping"

http://code.google.com/p/chromium/issues/detail?id=35198

@skerner - Is there an estimate on the timeline? I don't mean to bug again, but
this is
one of our top issues in the forum right now. The growing traffic to these
threads is
concerning.

Thanks!

On 2010/04/20 20:46:37, Sam Kerner (Chrome) wrote:
> Ready for another look.
> 
> http://codereview.chromium.org/1582022/diff/14001/15001
> File base/file_util.h (right):
> 
> http://codereview.chromium.org/1582022/diff/14001/15001#newcode251
> base/file_util.h:251: bool CreateTempDirInDir(const FilePath& base_dir,
> On 2010/04/20 00:29:53, Erik Kay wrote:
> > we have a naming mismatch here.  The functions above use "Temporary" and
> "Dir". 
> > The one below uses "Temp" and "Directory".  You're using one from each. :-) 
> > Seems like "Temporary" and "Dir" is the more common pattern.
> 
> Changed to CreateTemporaryDirInDir().
> 
> http://codereview.chromium.org/1582022/diff/14001/15002
> File base/file_util_posix.cc (right):
> 
> http://codereview.chromium.org/1582022/diff/14001/15002#newcode406
> base/file_util_posix.cc:406: // this should be OK since mkdtemp just replaces
> characters in place
> On 2010/04/20 00:29:53, Erik Kay wrote:
> > nit: newline above comment
> 
> Done.
> 
> http://codereview.chromium.org/1582022/diff/14001/15002#newcode418
> base/file_util_posix.cc:418: CHECK(new_dir_tmpl.find("%ls") !=
> FilePath::StringType::npos)
> On 2010/04/20 00:29:53, Erik Kay wrote:
> > This seems a bit fragile.  Why not just always append before the extension? 
> See
> > FilePath::InsertBeforeExtension.
> 
> The item being created is a directory, so I am not sure an extension makes
> sense.  I want to avoid changing the behavior of CreateNewTempDirectory().  I
> changed the code to append the characters that make the name unique.
> 
> http://codereview.chromium.org/1582022/diff/14001/15003
> File base/file_util_unittest.cc (right):
> 
> http://codereview.chromium.org/1582022/diff/14001/15003#newcode1147
> base/file_util_unittest.cc:1147: test_dir_,
> On 2010/04/20 00:29:53, Erik Kay wrote:
> > nit: indent is off
> 
> Done.
> 
> http://codereview.chromium.org/1582022/diff/14001/15003#newcode1150
> base/file_util_unittest.cc:1150: EXPECT_TRUE(file_util::PathExists(new_dir));
> On 2010/04/20 00:29:53, Erik Kay wrote:
> > verify that test_dir_ is a parent of new_dir (FilePath::IsParent)
> 
> Done.
> 
> http://codereview.chromium.org/1582022/diff/14001/15004
> File base/file_util_win.cc (right):
> 
> http://codereview.chromium.org/1582022/diff/14001/15004#newcode517
> base/file_util_win.cc:517: //new_dir_name.append(IntToWString(rand() %
> kint16max));
> On 2010/04/20 00:29:53, Erik Kay wrote:
> > remove unused code
> 
> Done.
> 
> http://codereview.chromium.org/1582022/diff/14001/15004#newcode519
> base/file_util_win.cc:519: new_dir_name = StringPrintf(new_dir_name.c_str(),
> num.c_str());
> On 2010/04/20 00:29:53, Erik Kay wrote:
> > same comment here as the posix code.  use concatenation.
> 
> Done.
> 
> http://codereview.chromium.org/1582022/diff/14001/15005
> File base/scoped_temp_dir.cc (right):
> 
> http://codereview.chromium.org/1582022/diff/14001/15005#newcode34
> base/scoped_temp_dir.cc:34: // Create a new, uniquly named directory under .
> On 2010/04/20 00:29:53, Erik Kay wrote:
> > under |base_path|
> 
> Done.
> 
> http://codereview.chromium.org/1582022/diff/14001/15007
> File base/scoped_temp_dir_unittest.cc (right):
> 
> http://codereview.chromium.org/1582022/diff/14001/15007#newcode70
> base/scoped_temp_dir_unittest.cc:70:
> EXPECT_TRUE(file_util::DirectoryExists(test_path));
> On 2010/04/20 00:29:53, Erik Kay wrote:
> > verify IsParent
> 
> Done.
> 
> http://codereview.chromium.org/1582022/diff/14001/15008
> File chrome/browser/extensions/crx_installer.cc (right):
> 
> http://codereview.chromium.org/1582022/diff/14001/15008#newcode29
> chrome/browser/extensions/crx_installer.cc:29: 
> On 2010/04/20 00:29:53, Erik Kay wrote:
> > nit: remove extra newlines
> 
> Done.
> 
> http://codereview.chromium.org/1582022/diff/14001/15008#newcode78
> chrome/browser/extensions/crx_installer.cc:78:
> CHECK(PathService::Get(chrome::DIR_USER_DATA, &user_data_dir));
> On 2010/04/20 00:29:53, Erik Kay wrote:
> > Add a new directory to chrome_paths called something like DIR_USER_DATA_TEMP
> > rather than constructing this by hand at each callsite.
> 
> Done.
> 
> http://codereview.chromium.org/1582022/diff/14001/15009
> File chrome/browser/extensions/sandboxed_extension_unpacker.cc (right):
> 
> http://codereview.chromium.org/1582022/diff/14001/15009#newcode52
> chrome/browser/extensions/sandboxed_extension_unpacker.cc:52: extension_root_
=
> temp_dir_.path().AppendASCII("TEMP_INSTALL");
> On 2010/04/20 00:29:53, Erik Kay wrote:
> > maybe rename this to CRX_INSTALL since the user data temp dir may wind up
> being
> > shared.
> 
> Done.

[Erik does not do reviews, 2010-04-29 04:31:41]

Maybe, but we're pretty close to the wire.  Worst case, we can land it
as a patch after 5.0 goes to stable.  But first, let's land it on
trunk. ;-)

Erik


On Wed, Apr 28, 2010 at 9:15 PM,  <skerner@chromium.org> wrote:
> Erik,
>   I got the message below from a forum moderator regarding this issue.  I
> wonder if I should push to get this into version 5.0.
>
> Sam
>
>
> Comment #76 on issue 35198 by google-chrome-guide5@google.com: Extension
> Install
> Failure for all extensions: "could not create directory for unzipping"
>
> http://code.google.com/p/chromium/issues/detail?id=35198
>
> @skerner - Is there an estimate on the timeline? I don't mean to bug again,
> but
> this is
> one of our top issues in the forum right now. The growing traffic to these
> threads is
> concerning.
>
> Thanks!
>
> On 2010/04/20 20:46:37, Sam Kerner (Chrome) wrote:
>>
>> Ready for another look.
>
>> http://codereview.chromium.org/1582022/diff/14001/15001
>> File base/file_util.h (right):
>
>> http://codereview.chromium.org/1582022/diff/14001/15001#newcode251
>> base/file_util.h:251: bool CreateTempDirInDir(const FilePath& base_dir,
>> On 2010/04/20 00:29:53, Erik Kay wrote:
>> > we have a naming mismatch here.  The functions above use "Temporary" and
>> "Dir".
>> > The one below uses "Temp" and "Directory".  You're using one from each.
>> > :-)
>> > Seems like "Temporary" and "Dir" is the more common pattern.
>
>> Changed to CreateTemporaryDirInDir().
>
>> http://codereview.chromium.org/1582022/diff/14001/15002
>> File base/file_util_posix.cc (right):
>
>> http://codereview.chromium.org/1582022/diff/14001/15002#newcode406
>> base/file_util_posix.cc:406: // this should be OK since mkdtemp just
>> replaces
>> characters in place
>> On 2010/04/20 00:29:53, Erik Kay wrote:
>> > nit: newline above comment
>
>> Done.
>
>> http://codereview.chromium.org/1582022/diff/14001/15002#newcode418
>> base/file_util_posix.cc:418: CHECK(new_dir_tmpl.find("%ls") !=
>> FilePath::StringType::npos)
>> On 2010/04/20 00:29:53, Erik Kay wrote:
>> > This seems a bit fragile.  Why not just always append before the
>> > extension?
>> See
>> > FilePath::InsertBeforeExtension.
>
>> The item being created is a directory, so I am not sure an extension makes
>> sense.  I want to avoid changing the behavior of CreateNewTempDirectory().
>>  I
>> changed the code to append the characters that make the name unique.
>
>> http://codereview.chromium.org/1582022/diff/14001/15003
>> File base/file_util_unittest.cc (right):
>
>> http://codereview.chromium.org/1582022/diff/14001/15003#newcode1147
>> base/file_util_unittest.cc:1147: test_dir_,
>> On 2010/04/20 00:29:53, Erik Kay wrote:
>> > nit: indent is off
>
>> Done.
>
>> http://codereview.chromium.org/1582022/diff/14001/15003#newcode1150
>> base/file_util_unittest.cc:1150:
>> EXPECT_TRUE(file_util::PathExists(new_dir));
>> On 2010/04/20 00:29:53, Erik Kay wrote:
>> > verify that test_dir_ is a parent of new_dir (FilePath::IsParent)
>
>> Done.
>
>> http://codereview.chromium.org/1582022/diff/14001/15004
>> File base/file_util_win.cc (right):
>
>> http://codereview.chromium.org/1582022/diff/14001/15004#newcode517
>> base/file_util_win.cc:517: //new_dir_name.append(IntToWString(rand() %
>> kint16max));
>> On 2010/04/20 00:29:53, Erik Kay wrote:
>> > remove unused code
>
>> Done.
>
>> http://codereview.chromium.org/1582022/diff/14001/15004#newcode519
>> base/file_util_win.cc:519: new_dir_name =
>> StringPrintf(new_dir_name.c_str(),
>> num.c_str());
>> On 2010/04/20 00:29:53, Erik Kay wrote:
>> > same comment here as the posix code.  use concatenation.
>
>> Done.
>
>> http://codereview.chromium.org/1582022/diff/14001/15005
>> File base/scoped_temp_dir.cc (right):
>
>> http://codereview.chromium.org/1582022/diff/14001/15005#newcode34
>> base/scoped_temp_dir.cc:34: // Create a new, uniquly named directory under
>> .
>> On 2010/04/20 00:29:53, Erik Kay wrote:
>> > under |base_path|
>
>> Done.
>
>> http://codereview.chromium.org/1582022/diff/14001/15007
>> File base/scoped_temp_dir_unittest.cc (right):
>
>> http://codereview.chromium.org/1582022/diff/14001/15007#newcode70
>> base/scoped_temp_dir_unittest.cc:70:
>> EXPECT_TRUE(file_util::DirectoryExists(test_path));
>> On 2010/04/20 00:29:53, Erik Kay wrote:
>> > verify IsParent
>
>> Done.
>
>> http://codereview.chromium.org/1582022/diff/14001/15008
>> File chrome/browser/extensions/crx_installer.cc (right):
>
>> http://codereview.chromium.org/1582022/diff/14001/15008#newcode29
>> chrome/browser/extensions/crx_installer.cc:29:
>> On 2010/04/20 00:29:53, Erik Kay wrote:
>> > nit: remove extra newlines
>
>> Done.
>
>> http://codereview.chromium.org/1582022/diff/14001/15008#newcode78
>> chrome/browser/extensions/crx_installer.cc:78:
>> CHECK(PathService::Get(chrome::DIR_USER_DATA, &user_data_dir));
>> On 2010/04/20 00:29:53, Erik Kay wrote:
>> > Add a new directory to chrome_paths called something like
>> > DIR_USER_DATA_TEMP
>> > rather than constructing this by hand at each callsite.
>
>> Done.
>
>> http://codereview.chromium.org/1582022/diff/14001/15009
>> File chrome/browser/extensions/sandboxed_extension_unpacker.cc (right):
>
>> http://codereview.chromium.org/1582022/diff/14001/15009#newcode52
>> chrome/browser/extensions/sandboxed_extension_unpacker.cc:52:
>> extension_root_
>
> =
>>
>> temp_dir_.path().AppendASCII("TEMP_INSTALL");
>> On 2010/04/20 00:29:53, Erik Kay wrote:
>> > maybe rename this to CRX_INSTALL since the user data temp dir may wind
>> > up
>> being
>> > shared.
>
>> Done.
>
>
>
> http://codereview.chromium.org/1582022/show
>

[Erik does not do reviews, 2010-04-29 04:32:40]

Crap.  I just noticed - is this review waiting for me?  If so, sorry about that!

Erik


On Wed, Apr 28, 2010 at 9:31 PM, Erik Kay <erikkay@chromium.org> wrote:
> Maybe, but we're pretty close to the wire.  Worst case, we can land it
> as a patch after 5.0 goes to stable.  But first, let's land it on
> trunk. ;-)
>
> Erik
>
>
> On Wed, Apr 28, 2010 at 9:15 PM,  <skerner@chromium.org> wrote:
>> Erik,
>>   I got the message below from a forum moderator regarding this issue.  I
>> wonder if I should push to get this into version 5.0.
>>
>> Sam
>>
>>
>> Comment #76 on issue 35198 by google-chrome-guide5@google.com: Extension
>> Install
>> Failure for all extensions: "could not create directory for unzipping"
>>
>> http://code.google.com/p/chromium/issues/detail?id=35198
>>
>> @skerner - Is there an estimate on the timeline? I don't mean to bug again,
>> but
>> this is
>> one of our top issues in the forum right now. The growing traffic to these
>> threads is
>> concerning.
>>
>> Thanks!
>>
>> On 2010/04/20 20:46:37, Sam Kerner (Chrome) wrote:
>>>
>>> Ready for another look.
>>
>>> http://codereview.chromium.org/1582022/diff/14001/15001
>>> File base/file_util.h (right):
>>
>>> http://codereview.chromium.org/1582022/diff/14001/15001#newcode251
>>> base/file_util.h:251: bool CreateTempDirInDir(const FilePath& base_dir,
>>> On 2010/04/20 00:29:53, Erik Kay wrote:
>>> > we have a naming mismatch here.  The functions above use "Temporary" and
>>> "Dir".
>>> > The one below uses "Temp" and "Directory".  You're using one from each.
>>> > :-)
>>> > Seems like "Temporary" and "Dir" is the more common pattern.
>>
>>> Changed to CreateTemporaryDirInDir().
>>
>>> http://codereview.chromium.org/1582022/diff/14001/15002
>>> File base/file_util_posix.cc (right):
>>
>>> http://codereview.chromium.org/1582022/diff/14001/15002#newcode406
>>> base/file_util_posix.cc:406: // this should be OK since mkdtemp just
>>> replaces
>>> characters in place
>>> On 2010/04/20 00:29:53, Erik Kay wrote:
>>> > nit: newline above comment
>>
>>> Done.
>>
>>> http://codereview.chromium.org/1582022/diff/14001/15002#newcode418
>>> base/file_util_posix.cc:418: CHECK(new_dir_tmpl.find("%ls") !=
>>> FilePath::StringType::npos)
>>> On 2010/04/20 00:29:53, Erik Kay wrote:
>>> > This seems a bit fragile.  Why not just always append before the
>>> > extension?
>>> See
>>> > FilePath::InsertBeforeExtension.
>>
>>> The item being created is a directory, so I am not sure an extension makes
>>> sense.  I want to avoid changing the behavior of CreateNewTempDirectory().
>>>  I
>>> changed the code to append the characters that make the name unique.
>>
>>> http://codereview.chromium.org/1582022/diff/14001/15003
>>> File base/file_util_unittest.cc (right):
>>
>>> http://codereview.chromium.org/1582022/diff/14001/15003#newcode1147
>>> base/file_util_unittest.cc:1147: test_dir_,
>>> On 2010/04/20 00:29:53, Erik Kay wrote:
>>> > nit: indent is off
>>
>>> Done.
>>
>>> http://codereview.chromium.org/1582022/diff/14001/15003#newcode1150
>>> base/file_util_unittest.cc:1150:
>>> EXPECT_TRUE(file_util::PathExists(new_dir));
>>> On 2010/04/20 00:29:53, Erik Kay wrote:
>>> > verify that test_dir_ is a parent of new_dir (FilePath::IsParent)
>>
>>> Done.
>>
>>> http://codereview.chromium.org/1582022/diff/14001/15004
>>> File base/file_util_win.cc (right):
>>
>>> http://codereview.chromium.org/1582022/diff/14001/15004#newcode517
>>> base/file_util_win.cc:517: //new_dir_name.append(IntToWString(rand() %
>>> kint16max));
>>> On 2010/04/20 00:29:53, Erik Kay wrote:
>>> > remove unused code
>>
>>> Done.
>>
>>> http://codereview.chromium.org/1582022/diff/14001/15004#newcode519
>>> base/file_util_win.cc:519: new_dir_name =
>>> StringPrintf(new_dir_name.c_str(),
>>> num.c_str());
>>> On 2010/04/20 00:29:53, Erik Kay wrote:
>>> > same comment here as the posix code.  use concatenation.
>>
>>> Done.
>>
>>> http://codereview.chromium.org/1582022/diff/14001/15005
>>> File base/scoped_temp_dir.cc (right):
>>
>>> http://codereview.chromium.org/1582022/diff/14001/15005#newcode34
>>> base/scoped_temp_dir.cc:34: // Create a new, uniquly named directory under
>>> .
>>> On 2010/04/20 00:29:53, Erik Kay wrote:
>>> > under |base_path|
>>
>>> Done.
>>
>>> http://codereview.chromium.org/1582022/diff/14001/15007
>>> File base/scoped_temp_dir_unittest.cc (right):
>>
>>> http://codereview.chromium.org/1582022/diff/14001/15007#newcode70
>>> base/scoped_temp_dir_unittest.cc:70:
>>> EXPECT_TRUE(file_util::DirectoryExists(test_path));
>>> On 2010/04/20 00:29:53, Erik Kay wrote:
>>> > verify IsParent
>>
>>> Done.
>>
>>> http://codereview.chromium.org/1582022/diff/14001/15008
>>> File chrome/browser/extensions/crx_installer.cc (right):
>>
>>> http://codereview.chromium.org/1582022/diff/14001/15008#newcode29
>>> chrome/browser/extensions/crx_installer.cc:29:
>>> On 2010/04/20 00:29:53, Erik Kay wrote:
>>> > nit: remove extra newlines
>>
>>> Done.
>>
>>> http://codereview.chromium.org/1582022/diff/14001/15008#newcode78
>>> chrome/browser/extensions/crx_installer.cc:78:
>>> CHECK(PathService::Get(chrome::DIR_USER_DATA, &user_data_dir));
>>> On 2010/04/20 00:29:53, Erik Kay wrote:
>>> > Add a new directory to chrome_paths called something like
>>> > DIR_USER_DATA_TEMP
>>> > rather than constructing this by hand at each callsite.
>>
>>> Done.
>>
>>> http://codereview.chromium.org/1582022/diff/14001/15009
>>> File chrome/browser/extensions/sandboxed_extension_unpacker.cc (right):
>>
>>> http://codereview.chromium.org/1582022/diff/14001/15009#newcode52
>>> chrome/browser/extensions/sandboxed_extension_unpacker.cc:52:
>>> extension_root_
>>
>> =
>>>
>>> temp_dir_.path().AppendASCII("TEMP_INSTALL");
>>> On 2010/04/20 00:29:53, Erik Kay wrote:
>>> > maybe rename this to CRX_INSTALL since the user data temp dir may wind
>>> > up
>>> being
>>> > shared.
>>
>>> Done.
>>
>>
>>
>> http://codereview.chromium.org/1582022/show
>>
>

[Erik does not do reviews, 2010-04-29 04:45:08]

LGTM - sorry again about the delay!

http://codereview.chromium.org/1582022/diff/25001/26013
File chrome/common/chrome_paths.h (right):

http://codereview.chromium.org/1582022/diff/25001/26013#newcode34
chrome/common/chrome_paths.h:34: // this instead of Temp when a sandboxed
process
This really isn't specific to sandboxing.  The issue is when you need to create
a file in a temporary location and then move it into the profile.  When you do
that move, it needs to be on the same filesystem as the profile.

[Sam Kerner (Chrome), 2010-04-30 17:53:07]

http://codereview.chromium.org/1582022/diff/25001/26013
File chrome/common/chrome_paths.h (right):

http://codereview.chromium.org/1582022/diff/25001/26013#newcode34
chrome/common/chrome_paths.h:34: // this instead of Temp when a sandboxed
process
On 2010/04/29 04:45:09, Erik Kay wrote:
> This really isn't specific to sandboxing.  The issue is when you need to
create
> a file in a temporary location and then move it into the profile.  When you do
> that move, it needs to be on the same filesystem as the profile.

Fixed.