[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 #5 from Eric U. <ericu at chromium.org>  2010-08-23 19:52:45 PST ---
(From update of attachment 65145)
WebCore/storage/DOMFilePath.cpp:43
 +  const char* DOMFilePath::root = "/";
const char DOMFilePath::root[] = "/"; ?

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?

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

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?

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.

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?

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

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

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.

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

WebCore/storage/DOMFilePath.cpp:74
 +  }
I think it might be better to return an empty string, or perhaps make sure not to be called in that case.  What's the directory of "foo"?  I suppose it could be ".".

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 "../"?

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

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.

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