Fixed bug: we can now handle "a:b" as a file name.

Fixed bug: we can now handle "a:b" as a file name.

BUG=113569
TEST=LayoutTests/fast/filesystem/op-restricted-chars.html


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

[ericu, 2012-02-23 00:13:47]

Kinuko, can you take a look at this when you get the chance?  It's not urgent,
but I think it's ready to go.

I haven't made any changes in the isolated filesystem code; I think those paths
can stay as-is.

[kinuko, 2012-02-23 06:32:30]

On 2012/02/23 00:13:47, ericu wrote:
> Kinuko, can you take a look at this when you get the chance?  It's not urgent,
> but I think it's ready to go.
> 
> I haven't made any changes in the isolated filesystem code; I think those
paths
> can stay as-is.

I see the intention here but the fact that we cannot call a method defined on
the class of itself worries me a bit.  Defining a new class sounds too much
though... could there be a way we could add some support in base/file_path.h to
deceive/replace FindDriveLetter?

[ericu, 2012-02-27 19:44:00]

On Wed, Feb 22, 2012 at 10:32 PM,  <kinuko@chromium.org> wrote:
> On 2012/02/23 00:13:47, ericu wrote:
>>
>> Kinuko, can you take a look at this when you get the chance?  It's not
>> urgent,
>> but I think it's ready to go.
>
>
>> I haven't made any changes in the isolated filesystem code; I think those
>
> paths
>>
>> can stay as-is.
>
>
> I see the intention here but the fact that we cannot call a method defined
> on
> the class of itself worries me a bit.  Defining a new class sounds too much
> though... could there be a way we could add some support in base/file_path.h
> to
> deceive/replace FindDriveLetter?

I was trying to avoid messing with FilePath, since it's supposed to be
a platform path, and we're abusing it by using it for a different path
specification entirely.  The smallest way to mess with FilePath would
probably be to add something like BaseNameIgnoringDriveLetter.  Of
course, it would need a modified StripTraiilngSeparatorsInternal, too.
 Do you think that would be better?

> https://chromiumcodereview.appspot.com/9370045/

[ericu, 2012-02-28 19:21:07]

On Tue, Feb 28, 2012 at 10:53 AM,  <kinuko@chromium.org> wrote:
> On 2012/02/27 19:44:00, ericu wrote:
>>
>> On Wed, Feb 22, 2012 at 10:32 PM,  <mailto:kinuko@chromium.org> wrote:
>> > On 2012/02/23 00:13:47, ericu wrote:
>> >>
>> >> Kinuko, can you take a look at this when you get the chance? &nbsp;It's
>> >> not
>>
>> >> urgent,
>> >> but I think it's ready to go.
>> >
>> >
>> >> I haven't made any changes in the isolated filesystem code; I think
>> >> those
>> >
>> > paths
>> >>
>> >> can stay as-is.
>> >
>> >
>> > I see the intention here but the fact that we cannot call a method
>> > defined
>> > on
>> > the class of itself worries me a bit. &nbsp;Defining a new class sounds
>> > too
>
> much
>>
>> > though... could there be a way we could add some support in
>> > base/file_path.h
>> > to
>> > deceive/replace FindDriveLetter?
>
>
>> I was trying to avoid messing with FilePath, since it's supposed to be
>> a platform path, and we're abusing it by using it for a different path
>> specification entirely.  The smallest way to mess with FilePath would
>> probably be to add something like BaseNameIgnoringDriveLetter.  Of
>> course, it would need a modified StripTraiilngSeparatorsInternal, too.
>>  Do you think that would be better?
>
>
> Your point makes sense too.  Another thought... if FilePath is supposed to
> be a
> platform path could we / should we just use UTF8 string for virtual paths?
>  The
> FileUtil's that know their paths are platform paths can easily create a
> FilePath
> from a given string.

That's not a bad idea.  We'd have to implement DirName as well, and
there would be a bit of churn, but it would make the code safer from
bugs like this, because you'd always call the right BaseName function.
 It would also let us support backslash in filenames, which was a
small annoyance in the spec.  We only didn't support it due to
implementation difficulty == FilePath got confused.

I'd suggest doing that in a different CL, though, as it'll be a big
churn with hopefully no visible effects, and this is a small change
with some visible effect, and it's simpler to test/review if we don't
stack them.

>> > https://chromiumcodereview.appspot.com/9370045/
>
>
>
>
> https://chromiumcodereview.appspot.com/9370045/

[kinuko, 2012-02-28 19:28:02]

https://chromiumcodereview.appspot.com/9370045/diff/9002/webkit/fileapi/file_...
File webkit/fileapi/file_system_util.h (right):

https://chromiumcodereview.appspot.com/9370045/diff/9002/webkit/fileapi/file_...
webkit/fileapi/file_system_util.h:28: // what platform you're on.
Let's add TODO here to explicitly note that file_path should be a mere UTF8
string, and also also note that the returned file_path may not be a
platform_path and the caller may need to use VirtualPath methods.

https://chromiumcodereview.appspot.com/9370045/diff/9002/webkit/fileapi/file_...
webkit/fileapi/file_system_util.h:37: FilePath GetVirtualBaseName(const
FilePath& virtual_path);
Could we group these methods into a static-methods-only class or something? 
Like:

class VirtualPath {
  static FilePath BaseName(...);
};

[ericu, 2012-02-29 21:31:34]

https://chromiumcodereview.appspot.com/9370045/diff/9002/webkit/fileapi/file_...
File webkit/fileapi/file_system_util.h (right):

https://chromiumcodereview.appspot.com/9370045/diff/9002/webkit/fileapi/file_...
webkit/fileapi/file_system_util.h:28: // what platform you're on.
On 2012/02/28 19:28:02, kinuko wrote:
> Let's add TODO here to explicitly note that file_path should be a mere UTF8
> string, and also also note that the returned file_path may not be a
> platform_path and the caller may need to use VirtualPath methods.

Done.

https://chromiumcodereview.appspot.com/9370045/diff/9002/webkit/fileapi/file_...
webkit/fileapi/file_system_util.h:37: FilePath GetVirtualBaseName(const
FilePath& virtual_path);
On 2012/02/28 19:28:02, kinuko wrote:
> Could we group these methods into a static-methods-only class or something? 
> Like:
> 
> class VirtualPath {
>   static FilePath BaseName(...);
> };

OK.  We might even expand on this if+when we switch over to std::string for
paths.  We could derive VirtualPath from std::string and make the methods
non-static, so they'd look just like those on FilePath.  Or perhaps we could
move them to FileSystemPath instead.  But for now, I'll just make the class you
recommend.

[kinuko, 2012-03-01 00:56:04]

LGTM

On 2012/02/29 21:31:34, ericu wrote:
>
https://chromiumcodereview.appspot.com/9370045/diff/9002/webkit/fileapi/file_...
> File webkit/fileapi/file_system_util.h (right):
> 
>
https://chromiumcodereview.appspot.com/9370045/diff/9002/webkit/fileapi/file_...
> webkit/fileapi/file_system_util.h:28: // what platform you're on.
> On 2012/02/28 19:28:02, kinuko wrote:
> > Let's add TODO here to explicitly note that file_path should be a mere UTF8
> > string, and also also note that the returned file_path may not be a
> > platform_path and the caller may need to use VirtualPath methods.
> 
> Done.
> 
>
https://chromiumcodereview.appspot.com/9370045/diff/9002/webkit/fileapi/file_...
> webkit/fileapi/file_system_util.h:37: FilePath GetVirtualBaseName(const
> FilePath& virtual_path);
> On 2012/02/28 19:28:02, kinuko wrote:
> > Could we group these methods into a static-methods-only class or something? 
> > Like:
> > 
> > class VirtualPath {
> >   static FilePath BaseName(...);
> > };
> 
> OK.  We might even expand on this if+when we switch over to std::string for
> paths.  We could derive VirtualPath from std::string and make the methods
> non-static, so they'd look just like those on FilePath.  Or perhaps we could
> move them to FileSystemPath instead.

Yes, we may want to do that... but doing that in this patch will be just
overkilling.

>  But for now, I'll just make the class you
> recommend.

[commit-bot: I haz the power, 2012-03-05 21:03:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ericu@chromium.org/9370045/29002

[commit-bot: I haz the power, 2012-03-05 23:45:45]

Change committed as 125022