Adds a command line switch to allow the user to specify the location...

Adds a command line switch to allow the user to specify the location
of the disk cache, rather than in the profile directory. The command
line switch overrides the cache location regardless the chosen
profile.
The changed code ignores the value of the command line switch if it
is not usable for this purpose.
In this implementation, the supplied directory name must should not
contain spaces.

BUG=6688

[Brian Duff, 2009-01-22 20:56:11]

http://codereview.chromium.org/18630/diff/1/2
File chrome/browser/profile.cc (right):

http://codereview.chromium.org/18630/diff/1/2#newcode502
Line 502: // necessary. This should be done recursively, but since this is not
file_util::CreateDirectory "Creates a directory, as well as creating any parent
directories", so this comment seems unnecessary.

http://codereview.chromium.org/18630/diff/1/2#newcode505
Line 505: !!file_util::CreateDirectory(user_cache_dir)) {
why the double negation (!!)?

[bekkra, 2009-01-22 21:04:10]

On 2009/01/22 20:56:11, Brian Duff wrote:
> http://codereview.chromium.org/18630/diff/1/2
> File chrome/browser/profile.cc (right):
> 
> http://codereview.chromium.org/18630/diff/1/2#newcode502
> Line 502: // necessary. This should be done recursively, but since this is not
> file_util::CreateDirectory "Creates a directory, as well as creating any
parent
> directories", so this comment seems unnecessary.
> 
> http://codereview.chromium.org/18630/diff/1/2#newcode505
> Line 505: !!file_util::CreateDirectory(user_cache_dir)) {
> why the double negation (!!)?

Hm ... the construct is used elsewhere in the source to give proper (?) BOOL to
bool conversion. Not entirely necessary, since the "zero is false, anything else
is true" rule applies to BOOL values.
Right. I guess I used it to "fit in" the code more.

[Brian Duff, 2009-01-22 21:08:44]

On 2009/01/22 21:04:10, bekkra wrote:
> On 2009/01/22 20:56:11, Brian Duff wrote:
> > http://codereview.chromium.org/18630/diff/1/2
> > File chrome/browser/profile.cc (right):
> > 
> > http://codereview.chromium.org/18630/diff/1/2#newcode502
> > Line 502: // necessary. This should be done recursively, but since this is
not
> > file_util::CreateDirectory "Creates a directory, as well as creating any
> parent
> > directories", so this comment seems unnecessary.
> > 
> > http://codereview.chromium.org/18630/diff/1/2#newcode505
> > Line 505: !!file_util::CreateDirectory(user_cache_dir)) {
> > why the double negation (!!)?
> 
> Hm ... the construct is used elsewhere in the source to give proper (?) BOOL
to
> bool conversion. Not entirely necessary, since the "zero is false, anything
else
> is true" rule applies to BOOL values.
> Right. I guess I used it to "fit in" the code more.

file_util::CreateDirectory() returns a bool anyway, so no conversion is needed.
It's probably better to just use spaces to line up the code rather than the
double negation (IMHO).

[bekkra, 2009-01-22 21:18:01]

On 2009/01/22 21:08:44, Brian Duff wrote:
> On 2009/01/22 21:04:10, bekkra wrote:
> > On 2009/01/22 20:56:11, Brian Duff wrote:
> > > http://codereview.chromium.org/18630/diff/1/2
> > > File chrome/browser/profile.cc (right):
> > > 
> > > http://codereview.chromium.org/18630/diff/1/2#newcode502
> > > Line 502: // necessary. This should be done recursively, but since this is
> not
> > > file_util::CreateDirectory "Creates a directory, as well as creating any
> > parent
> > > directories", so this comment seems unnecessary.
> > > 
> > > http://codereview.chromium.org/18630/diff/1/2#newcode505
> > > Line 505: !!file_util::CreateDirectory(user_cache_dir)) {
> > > why the double negation (!!)?
> > 
> > Hm ... the construct is used elsewhere in the source to give proper (?) BOOL
> to
> > bool conversion. Not entirely necessary, since the "zero is false, anything
> else
> > is true" rule applies to BOOL values.
> > Right. I guess I used it to "fit in" the code more.
> 
> file_util::CreateDirectory() returns a bool anyway, so no conversion is
needed.
> It's probably better to just use spaces to line up the code rather than the
> double negation (IMHO).

I agree about the negations.
The CreateDirectory thing disturbs me, here's why: when I first tested the
change, Chromium did not create the "Cache" subdirectory, unless I created the
parent first :o
At that time, I browsed the code to find the definition of the CreateDeirectory
call, and I found no more information than what I believed was the Win32 call. I
never found the SHCreate... function that is documented as you say.
So I will have to test drive a little more to find why the directory was not
created.
I'll post a fix later :)

[Brian Duff, 2009-01-22 21:50:27]

On 2009/01/22 21:18:01, bekkra wrote:
> On 2009/01/22 21:08:44, Brian Duff wrote:
> > On 2009/01/22 21:04:10, bekkra wrote:
> > > On 2009/01/22 20:56:11, Brian Duff wrote:
> > > > http://codereview.chromium.org/18630/diff/1/2
> > > > File chrome/browser/profile.cc (right):
> > > > 
> > > > http://codereview.chromium.org/18630/diff/1/2#newcode502
> > > > Line 502: // necessary. This should be done recursively, but since this
is
> > not
> > > > file_util::CreateDirectory "Creates a directory, as well as creating any
> > > parent
> > > > directories", so this comment seems unnecessary.
> > > > 
> > > > http://codereview.chromium.org/18630/diff/1/2#newcode505
> > > > Line 505: !!file_util::CreateDirectory(user_cache_dir)) {
> > > > why the double negation (!!)?
> > > 
> > > Hm ... the construct is used elsewhere in the source to give proper (?)
BOOL
> > to
> > > bool conversion. Not entirely necessary, since the "zero is false,
anything
> > else
> > > is true" rule applies to BOOL values.
> > > Right. I guess I used it to "fit in" the code more.
> > 
> > file_util::CreateDirectory() returns a bool anyway, so no conversion is
> needed.
> > It's probably better to just use spaces to line up the code rather than the
> > double negation (IMHO).
> 
> I agree about the negations.
> The CreateDirectory thing disturbs me, here's why: when I first tested the
> change, Chromium did not create the "Cache" subdirectory, unless I created the
> parent first :o
> At that time, I browsed the code to find the definition of the
CreateDeirectory
> call, and I found no more information than what I believed was the Win32 call.
I
> never found the SHCreate... function that is documented as you say.
> So I will have to test drive a little more to find why the directory was not
> created.
> I'll post a fix later :)

The documentation I quoted for file_util::CreateDirectory is in
src/base/file_util.h, see
http://www.google.com/codesearch/p?hl=en#YwaaF8DorRY/src/base/file_util.h&q=C...

The windows implementation (see src/base/file_util_win.cc) calls
SHCreateDirectoryEx in Win32
(http://msdn.microsoft.com/en-us/library/bb762131(VS.85).aspx), which is
documented as, "This function creates a file system folder whose fully qualified
path is given by pszPath. If one or more of the intermediate folders do not
exist, they are created as well."

It's odd that it's not doing this in your case, let me know what you find (I'll
give it a try later when I have access to a Windows box too) :)

[bekkra, 2009-01-22 21:55:40]

On 2009/01/22 21:50:27, Brian Duff wrote:
> On 2009/01/22 21:18:01, bekkra wrote:
> > On 2009/01/22 21:08:44, Brian Duff wrote:
> > > On 2009/01/22 21:04:10, bekkra wrote:
> > > > On 2009/01/22 20:56:11, Brian Duff wrote:
> > > > > http://codereview.chromium.org/18630/diff/1/2
> > > > > File chrome/browser/profile.cc (right):
> > > > > 
> > > > > http://codereview.chromium.org/18630/diff/1/2#newcode502
> > > > > Line 502: // necessary. This should be done recursively, but since
this
> is
> > > not
> > > > > file_util::CreateDirectory "Creates a directory, as well as creating
any
> > > > parent
> > > > > directories", so this comment seems unnecessary.
> > > > > 
> > > > > http://codereview.chromium.org/18630/diff/1/2#newcode505
> > > > > Line 505: !!file_util::CreateDirectory(user_cache_dir)) {
> > > > > why the double negation (!!)?
> > > > 
> > > > Hm ... the construct is used elsewhere in the source to give proper (?)
> BOOL
> > > to
> > > > bool conversion. Not entirely necessary, since the "zero is false,
> anything
> > > else
> > > > is true" rule applies to BOOL values.
> > > > Right. I guess I used it to "fit in" the code more.
> > > 
> > > file_util::CreateDirectory() returns a bool anyway, so no conversion is
> > needed.
> > > It's probably better to just use spaces to line up the code rather than
the
> > > double negation (IMHO).
> > 
> > I agree about the negations.
> > The CreateDirectory thing disturbs me, here's why: when I first tested the
> > change, Chromium did not create the "Cache" subdirectory, unless I created
the
> > parent first :o
> > At that time, I browsed the code to find the definition of the
> CreateDeirectory
> > call, and I found no more information than what I believed was the Win32
call.
> I
> > never found the SHCreate... function that is documented as you say.
> > So I will have to test drive a little more to find why the directory was not
> > created.
> > I'll post a fix later :)
> 
> The documentation I quoted for file_util::CreateDirectory is in
> src/base/file_util.h, see
>
http://www.google.com/codesearch/p?hl=en#YwaaF8DorRY/src/base/file_util.h&q=C...
> 
> The windows implementation (see src/base/file_util_win.cc) calls
> SHCreateDirectoryEx in Win32
> (http://msdn.microsoft.com/en-us/library/bb762131(VS.85).aspx), which is
> documented as, "This function creates a file system folder whose fully
qualified
> path is given by pszPath. If one or more of the intermediate folders do not
> exist, they are created as well."
> 
> It's odd that it's not doing this in your case, let me know what you find
(I'll
> give it a try later when I have access to a Windows box too) :)

Heh thanks for the references. I just don't know what to say, because the
file_util::CreateDirectory function does behave as it should ( like you say ),
on all previous builds that I had patched. I can't for the life of me understand
what went wrong when it did.
Nevertheless, an update is even technically necessary, since the CommandLine
constructor is now not usable as I tried, so a new construct will have to be
tested. Building now ...
//

[bekkra, 2009-01-23 23:26:00]

... build finished [OK]
... tests finished [OK]
... manual tests finised XP i386 [OK]
... manual tests finised Vista x64 [OK]

[bekkra, 2009-01-26 19:14:01]

All tests are OK, I'm ready for more comments :)

//

[rvargas (doing something else), 2009-01-27 03:55:20]

http://codereview.chromium.org/18630/diff/203/15
File chrome/browser/profile.cc (right):

http://codereview.chromium.org/18630/diff/203/15#newcode497
Line 497: // Override the cache location if specified so
nit: Each comment has to be a complete sentence (and end with a period). Maybe
".. if specified by the user."

http://codereview.chromium.org/18630/diff/203/15#newcode499
Line 499: CommandLine::ForCurrentProcess()->GetSwitchValue(
nit: wrong indentation. See http://dev.chromium.org/developers/coding-style

http://codereview.chromium.org/18630/diff/203/15#newcode504
Line 504: if (file_util::DirectoryExists(user_cache_dir) ||
Remove all the code that creates the folder. That should be performed by the
lower layers.

http://codereview.chromium.org/18630/diff/203/15#newcode540
Line 540: 
nit: remove

http://codereview.chromium.org/18630/diff/203/16
File chrome/common/chrome_switches.h (right):

http://codereview.chromium.org/18630/diff/203/16#newcode143
Line 143: extern const wchar_t kDiskCacheLocation[];
nit: maybe put this one next to user-data-dir. And what about kDiskCacheDir?

[bekkra, 2009-01-27 20:41:40]

Now we're talking !
Nice work, both of you so far. 

On 2009/01/27 03:55:20, rvargas wrote:
> http://codereview.chromium.org/18630/diff/203/15
> File chrome/browser/profile.cc (right):
> 
> http://codereview.chromium.org/18630/diff/203/15#newcode497
> Line 497: // Override the cache location if specified so
> nit: Each comment has to be a complete sentence (and end with a period). Maybe
> ".. if specified by the user."

I should not comment on this, but is it possible to misunderstand the comment ?
Heh, still, your correction makes the comment friendlier. I see.

> 
> http://codereview.chromium.org/18630/diff/203/15#newcode499
> Line 499: CommandLine::ForCurrentProcess()->GetSwitchValue(
> nit: wrong indentation. See http://dev.chromium.org/developers/coding-style
>

Damn. I have really tried to write and invisible change... I can't even spot
this one, so I will have to look at some clear examples. :|
 
> http://codereview.chromium.org/18630/diff/203/15#newcode504
> Line 504: if (file_util::DirectoryExists(user_cache_dir) ||
> Remove all the code that creates the folder. That should be performed by the
> lower layers.
> 

Nice catch. As reported above, I had some mysterious problem with the cache
directory not being created, which made me add that call too. Nice to remove
more code - the simpler, the better :)

> http://codereview.chromium.org/18630/diff/203/15#newcode540
> Line 540: 
> nit: remove

Wow... Mr Hawkeye ? ;)

> 
> http://codereview.chromium.org/18630/diff/203/16
> File chrome/common/chrome_switches.h (right):
> 
> http://codereview.chromium.org/18630/diff/203/16#newcode143
> Line 143: extern const wchar_t kDiskCacheLocation[];
> nit: maybe put this one next to user-data-dir. And what about kDiskCacheDir?

OK. I failed to find the description of how we are supposed to add new variables
and the like. Yes, it is useful to group command like switches by functional
area, but that changes the order of them. Is this the expected way to, say, add
new member variables to a class as well ?

//

[rvargas (doing something else), 2009-01-28 23:49:22]

On 2009/01/27 20:41:40, bekkra wrote:
> Now we're talking !
> Nice work, both of you so far. 
> 
> On 2009/01/27 03:55:20, rvargas wrote:
> > http://codereview.chromium.org/18630/diff/203/15
> > File chrome/browser/profile.cc (right):
> > 
> > http://codereview.chromium.org/18630/diff/203/15#newcode497
> > Line 497: // Override the cache location if specified so
> > nit: Each comment has to be a complete sentence (and end with a period).
Maybe
> > ".. if specified by the user."
> 
> I should not comment on this, but is it possible to misunderstand the comment
?
> Heh, still, your correction makes the comment friendlier. I see.
> 
> > 
> > http://codereview.chromium.org/18630/diff/203/15#newcode499
> > Line 499: CommandLine::ForCurrentProcess()->GetSwitchValue(
> > nit: wrong indentation. See http://dev.chromium.org/developers/coding-style
> >
> 
> Damn. I have really tried to write and invisible change... I can't even spot
> this one, so I will have to look at some clear examples. :|
>  
> > http://codereview.chromium.org/18630/diff/203/15#newcode504
> > Line 504: if (file_util::DirectoryExists(user_cache_dir) ||
> > Remove all the code that creates the folder. That should be performed by the
> > lower layers.
> > 
> 
> Nice catch. As reported above, I had some mysterious problem with the cache
> directory not being created, which made me add that call too. Nice to remove
> more code - the simpler, the better :)
> 
> > http://codereview.chromium.org/18630/diff/203/15#newcode540
> > Line 540: 
> > nit: remove
> 
> Wow... Mr Hawkeye ? ;)
> 
> > 
> > http://codereview.chromium.org/18630/diff/203/16
> > File chrome/common/chrome_switches.h (right):
> > 
> > http://codereview.chromium.org/18630/diff/203/16#newcode143
> > Line 143: extern const wchar_t kDiskCacheLocation[];
> > nit: maybe put this one next to user-data-dir. And what about kDiskCacheDir?
> 
> OK. I failed to find the description of how we are supposed to add new
variables
> and the like. Yes, it is useful to group command like switches by functional
> area, but that changes the order of them. Is this the expected way to, say,
add
> new member variables to a class as well ?

That's kind of a case by case scenario. In general it is better to have them
together, and add them at the end if there is no clear connection with something
else (for members of a class). The command line switches are not really
organized by anything, and that makes them harder to grep, but the few that deal
with folders are together.

[bekkra, 2009-02-01 21:30:37]

Now.
I have a new patch set, but I have lost the .svn directory that contained the
update ID.

How can I upload a new version of this change ?

[rvargas (doing something else), 2009-02-02 19:04:43]

On 2009/02/01 21:30:37, bekkra wrote:
> Now.
> I have a new patch set, but I have lost the .svn directory that contained the
> update ID.
> 
> How can I upload a new version of this change ?

Just create a new change.