Add an AddExtension() method in FilePath

Add an AddExtension() method in FilePath

This provides an AddExtension() method in FilePath, which will append an
extension to the FilePath. This provides clearer, more deliberate functionality
than AppendASCII(), and allows us to add extensions without using
ReplaceExtension, which will not work on files which already have extension (or
appear to, e.g. temp files).

BUG=NONE
TEST=FilePathTest.AddExtension (added), previous FilePathTests.


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

[Devlin, 2012-04-12 03:36:30]

If you could look this over when you get a chance, it'd be much appreciated.

[darin (slow to review), 2012-04-16 15:52:08]

Mark should review this.

[Mark Mentovai, 2012-04-16 15:54:45]

https://chromiumcodereview.appspot.com/10067002/diff/1/base/file_path.h
File base/file_path.h (right):

https://chromiumcodereview.appspot.com/10067002/diff/1/base/file_path.h#newco...
base/file_path.h:247: FilePath AddExtension(
It’s hard to see why you need to use this without a use case, but why is this
better than ReplaceExtension? Are you planning on having
a.filename.with.multiple.extensions?

https://chromiumcodereview.appspot.com/10067002/diff/1/base/file_path.h#newco...
base/file_path.h:258: bool IsBaseEmptyOrSpecialCase() const;
Why do you need to expose this?

[Devlin, 2012-04-17 21:52:49]

https://chromiumcodereview.appspot.com/10067002/diff/1/base/file_path.h
File base/file_path.h (right):

https://chromiumcodereview.appspot.com/10067002/diff/1/base/file_path.h#newco...
base/file_path.h:247: FilePath AddExtension(
On 2012/04/16 15:54:45, Mark Mentovai wrote:
> It’s hard to see why you need to use this without a use case, but why is this
> better than ReplaceExtension? Are you planning on having
> a.filename.with.multiple.extensions?

The most common instance I've encountered is when using a tempfile generated by
Chrome.  These files can take the form of org.Chromium.abc.xyz (real example:
/tmp/.org.chromium.Chromium.hAlw5V); when we use ReplaceExtension it results in
a different path, and the behavior of using ReplaceExtension can be different
than when passing in a non-temp file.  AddExtension makes it so that there's no
ambiguity.

https://chromiumcodereview.appspot.com/10067002/diff/1/base/file_path.h#newco...
base/file_path.h:258: bool IsBaseEmptyOrSpecialCase() const;
On 2012/04/16 15:54:45, Mark Mentovai wrote:
> Why do you need to expose this?

My original thought was that these instances are fairly common occurrences of
paths people may wish to treat specially (or disallow).  However, I do not have
any concrete examples of when this would be the case; if you feel it better to
be moved to private scope (or within an anonymous namespace in the cc file
only), I'd be fine with that.

[Mark Mentovai, 2012-04-18 20:02:21]

https://chromiumcodereview.appspot.com/10067002/diff/1/base/file_path.h
File base/file_path.h (right):

https://chromiumcodereview.appspot.com/10067002/diff/1/base/file_path.h#newco...
base/file_path.h:258: bool IsBaseEmptyOrSpecialCase() const;
D Cronin wrote:
> On 2012/04/16 15:54:45, Mark Mentovai wrote:
> > Why do you need to expose this?
> 
> My original thought was that these instances are fairly common occurrences of
> paths people may wish to treat specially (or disallow).  However, I do not
have
> any concrete examples of when this would be the case; if you feel it better to
> be moved to private scope (or within an anonymous namespace in the cc file
> only), I'd be fine with that.

I’d just hide this in the cc file.

[Devlin, 2012-04-19 00:05:42]

https://chromiumcodereview.appspot.com/10067002/diff/1/base/file_path.h
File base/file_path.h (right):

https://chromiumcodereview.appspot.com/10067002/diff/1/base/file_path.h#newco...
base/file_path.h:258: bool IsBaseEmptyOrSpecialCase() const;
On 2012/04/18 20:02:21, Mark Mentovai wrote:
> D Cronin wrote:
> > On 2012/04/16 15:54:45, Mark Mentovai wrote:
> > > Why do you need to expose this?
> > 
> > My original thought was that these instances are fairly common occurrences
of
> > paths people may wish to treat specially (or disallow).  However, I do not
> have
> > any concrete examples of when this would be the case; if you feel it better
to
> > be moved to private scope (or within an anonymous namespace in the cc file
> > only), I'd be fine with that.
> 
> I’d just hide this in the cc file.

Done.

[Mark Mentovai, 2012-04-19 20:59:09]

https://chromiumcodereview.appspot.com/10067002/diff/7001/base/file_path.cc
File base/file_path.cc (right):

https://chromiumcodereview.appspot.com/10067002/diff/7001/base/file_path.cc#n...
base/file_path.cc:166: if (*(path.end() - 1) == FilePath::kExtensionSeparator) {
What’s this actually buy, other than making the code obtuse? Why don’t you just
get rid of this check and proceed directly to the
kCurrentDirectory/kParentDirectory check?

[Devlin, 2012-04-21 01:37:36]

https://chromiumcodereview.appspot.com/10067002/diff/7001/base/file_path.cc
File base/file_path.cc (right):

https://chromiumcodereview.appspot.com/10067002/diff/7001/base/file_path.cc#n...
base/file_path.cc:166: if (*(path.end() - 1) == FilePath::kExtensionSeparator) {
On 2012/04/19 20:59:09, Mark Mentovai wrote:
> What’s this actually buy, other than making the code obtuse? Why don’t you
just
> get rid of this check and proceed directly to the
> kCurrentDirectory/kParentDirectory check?

I didn't write this portion of the code (just refactored it), and noticed the
same thing.  My only guess was that it was to save that minor amount of work
since comparing the end character to see if a special case is even a concern is
faster than comparing the entirety of the path.  I'm not sure this will ever be
a concern (I'm unfamiliar with any instances of where a FilePath would be long
enough for this to matter), but I've realized a general pattern of people
preferring to not alter what is not broken.  If you like, I can simplify it.

[Mark Mentovai, 2012-04-21 15:46:48]

I would prefer it simplified, unless there’s a proven reason for the
optimization. I suspect that the code only wound up this way by accident.

[Devlin, 2012-04-21 16:58:52]

On 2012/04/21 15:46:48, Mark Mentovai wrote:
> I would prefer it simplified, unless there’s a proven reason for the
> optimization. I suspect that the code only wound up this way by accident.

Done.

[Mark Mentovai, 2012-04-21 17:02:00]

https://chromiumcodereview.appspot.com/10067002/diff/13001/base/file_path.cc
File base/file_path.cc (right):

https://chromiumcodereview.appspot.com/10067002/diff/13001/base/file_path.cc#...
base/file_path.cc:413: if (path_.empty())
This check is redundant because IsEmptyOrSpecialCase will be true if path_ is
empty, right?

https://chromiumcodereview.appspot.com/10067002/diff/13001/base/file_path.cc#...
base/file_path.cc:433: if (path_.empty())
Same question applies here.

[Devlin, 2012-04-21 17:26:23]

On 2012/04/21 17:02:00, Mark Mentovai wrote:
> https://chromiumcodereview.appspot.com/10067002/diff/13001/base/file_path.cc
> File base/file_path.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10067002/diff/13001/base/file_path.cc#...
> base/file_path.cc:413: if (path_.empty())
> This check is redundant because IsEmptyOrSpecialCase will be true if path_ is
> empty, right?
> 
>
https://chromiumcodereview.appspot.com/10067002/diff/13001/base/file_path.cc#...
> base/file_path.cc:433: if (path_.empty())
> Same question applies here.

Dang, shoulda caught those myself; sorry. Done.

[Mark Mentovai, 2012-04-21 17:38:30]

Great. And the test data already checks for the thing that the redundant code
you just removed addresses, so everything’s in tip-top shape from that
perspective too.

LGTM.

[commit-bot: I haz the power, 2012-04-21 18:14:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rdevlin.cronin@chromium.org/10067002/1...

[commit-bot: I haz the power, 2012-04-21 21:06:26]

Try job failure for 10067002-11002 (retry) on win_rel for step "browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-04-24 23:04:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rdevlin.cronin@chromium.org/10067002/2...

[commit-bot: I haz the power, 2012-04-25 01:53:46]

Change committed as 133839