[Webkit-unassigned] [Bug 44132] Implement virtual path utilities for FileSystem API

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=44132





--- Comment #7 from Kinuko Yasuda <kinuko at chromium.org>  2010-08-23 23:11:38 PST ---
Thanks for your careful review!

(In reply to comment #5)
> (From update of attachment 65145 [details])
> WebCore/storage/DOMFilePath.cpp:43
>  +  const char* DOMFilePath::root = "/";
> const char DOMFilePath::root[] = "/"; ?

Fixed.

> WebCore/storage/DOMFilePath.cpp:73
>  +      return path;
> This seems odd, when compared to getName.  Is returning path really the right thing to do in each case?

I wasn't sure what to return, but I changed this to return an empty string.  As you say probably "." or an empty string would be the expected value.

> WebCore/storage/DOMFilePath.cpp:78
>  +      return mayBeChild.startsWith(path);
> /foo/bar is not the parent of /foo/barrister.

Oh of course not... fixed.

> WebCore/storage/DOMFilePath.cpp:115
>  +      const char* p = utf8String.data();
> What happens here if the UTF-8 representation of a character in the path doesn't fit in a char?

Fixed.

> WebCore/storage/DOMFilePath.cpp:125
>  +      if (unallowedNamesRegExp1.match(path) >= 0)
> Is this match case-sensitive?  It needs to be case-insensitive on all of these.

All of RegExps are case-insensitive (CaseInsensitive).

> WebCore/storage/DOMFilePath.cpp:122
>  +      DEFINE_STATIC_LOCAL(RegularExpression, unallowedNamesRegExp1, ("/(CON|PRN|AUX|NUL)([\\./]|$)", TextCaseInsensitive));
> I've never used RegularExpressions from WebKit internally before.  Is that last '/' in the right place?

It's in the place I intended and my intension was: it should match if CON, PRN, AUX or NUL is followed by a period (\\.), slash (/) or the end of string ($).  Does that sound right?

> WebCore/storage/DOMFilePath.cpp:131
>  +      DEFINE_STATIC_LOCAL(RegularExpression, endingRegExp, ("[\\. ](/|$)", TextCaseInsensitive));
> What about other kinds of whitespace [tab, vtab, newline, cr, ...]

Replaced ' ' with '\s'.

> WebCore/storage/DOMFilePath.cpp:138
>  +      DEFINE_STATIC_LOCAL(RegularExpression, unallowedCharsRegExp, ("[\\\\<><>:\\?\\*\"|]", TextCaseInsensitive));
> You've got "<>" in there twice.

Fixed.

> WebCore/storage/DOMFilePath.cpp:108
>  +  bool DOMFilePath::isValid(const String& path)
> Perhaps "isValidPath" instead of just "isValid"?  One might call this by accident for a name and let something containing a directory separator slip through.

Fixed.

> WebCore/storage/DOMFilePath.h:44
>  +      static const char* root;
> How about static const char root[]?

Fixed.

> WebCore/storage/DOMFilePath.h:52
>  +      // Checks if a given path is a parent of mayBeChild. This method assumes given paths are absolute and do not have extra parent references.
> Explain what a parent reference is.  Do you mean "../"?

Yes I meant a reference to a parent, i.e. "..".  Changed the expression a bit.

> WebCore/storage/DOMFilePath.h:55
>  +      // Appends the separator at the end of the path.
> ...if it's not there already.

Fixed.

> WebCore/storage/DOMFilePath.h:71
>  +      // Removes extra parent ("..") references. This doesn't take all the references if it goes beyond the root directory.
> It does more than that.  How about something like this?
>
> // Evaluates all "../" and "./" segments.  Note that "/../" expands to "/", so you can't ever refer to anything above the root directory.

Changed the comment (and its behavior) as suggested.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.