File system path resolving

File system path resolving

[Wladimir Palant, 2013-04-24 06:13:51]

I already noted the relative paths when Felix implemented FileSystem. His
thought was that the clients would simply set current work directory to make
sure resolving file paths becomes unnecessary. So in your case you could set the
current work directory to "%APPDATA%/LocalLow/Adblock Plus" and everything
should work without further changes.

[Oleksandr, 2013-04-24 08:27:28]

On 2013/04/24 06:13:51, Wladimir Palant wrote:
> I already noted the relative paths when Felix implemented FileSystem. His
> thought was that the clients would simply set current work directory to make
> sure resolving file paths becomes unnecessary. So in your case you could set
the
> current work directory to "%APPDATA%/LocalLow/Adblock Plus" and everything
> should work without further changes.

That doesn't seem like a good idea, IMHO. Changing a working directory of IE
process can lead to unexpected results. I am sure one can come up with some sort
of security breach this will open. Not to mention that changing a working
directory on multithreaded/multimodule  project is generally considered an ill
practice, afaik.

[Felix Dahlke, 2013-04-24 08:35:53]

On 2013/04/24 08:27:28, Oleksandr wrote:
> On 2013/04/24 06:13:51, Wladimir Palant wrote:
> > I already noted the relative paths when Felix implemented FileSystem. His
> > thought was that the clients would simply set current work directory to make
> > sure resolving file paths becomes unnecessary. So in your case you could set
> the
> > current work directory to "%APPDATA%/LocalLow/Adblock Plus" and everything
> > should work without further changes.
> 
> That doesn't seem like a good idea, IMHO. Changing a working directory of IE
> process can lead to unexpected results. I am sure one can come up with some
sort
> of security breach this will open. Not to mention that changing a working
> directory on multithreaded/multimodule  project is generally considered an ill
> practice, afaik.

Hm, didn't think it'd be a problem, but you have a point, IE is more than just
our plugin.

I still don't like hard coding LocalLow in libadblockplus, the path is
application specific.

The file system can work with absolute paths just fine, so how about always
supplying absolute paths? Alternatively, we could have a setter for a working
directory that will be used to resolve relative paths if it has been set.

[Wladimir Palant, 2013-04-24 09:17:34]

Altogether I'm fine with this change - this is mostly along the lines of what I
suggested from the start. However, the base path to resolve against needs to be
supplied by the client application, e.g. via DefaultFileSystem::SetBasePath()
(default would still be the current directory then).

http://codereview.adblockplus.org/10369004/diff/1/src/DefaultFileSystem.cpp
File src/DefaultFileSystem.cpp (right):

http://codereview.adblockplus.org/10369004/diff/1/src/DefaultFileSystem.cpp#n...
src/DefaultFileSystem.cpp:93: return std::string(path);
Why not just return path; here?

http://codereview.adblockplus.org/10369004/diff/1/src/DefaultFileSystem.cpp#n...
src/DefaultFileSystem.cpp:98: std::string resolved(resolvedW.begin(),
resolvedW.end());
Please use Utils::ToUTF8String() here (and we are currently going to be in
trouble either way if we have Unicode characters in the path).

http://codereview.adblockplus.org/10369004/diff/1/src/DefaultFileSystem.cpp#n...
src/DefaultFileSystem.cpp:103: return std::string(path);
Why not just return path; here?

http://codereview.adblockplus.org/10369004/diff/1/src/FileSystemJsObject.cpp
File src/FileSystemJsObject.cpp (right):

http://codereview.adblockplus.org/10369004/diff/1/src/FileSystemJsObject.cpp#...
src/FileSystemJsObject.cpp:328: return v8::String::New(resolved.c_str(),
resolved.length());
Please use Utils::ToV8String() here.

[Wladimir Palant, 2013-04-24 09:18:58]

On 2013/04/24 08:35:53, Felix H. Dahlke wrote:
> The file system can work with absolute paths just fine, so how about always
> supplying absolute paths?

The JS code needs to know the absolute paths for that - that's exactly what the
Resolve callback does.

[Oleksandr, 2013-04-30 08:13:07]

http://codereview.adblockplus.org/10369004/diff/8001/src/DefaultWebRequestWin...
File src/DefaultWebRequestWinInet.cpp (right):

http://codereview.adblockplus.org/10369004/diff/8001/src/DefaultWebRequestWin...
src/DefaultWebRequestWinInet.cpp:184: std::string headersString = "";
This fixes an issue on XP. std::string's length is undefined otherwise.

[Felix Dahlke, 2013-04-30 09:14:52]

http://codereview.adblockplus.org/10369004/diff/8001/include/AdblockPlus/File...
File include/AdblockPlus/FileSystem.h (right):

http://codereview.adblockplus.org/10369004/diff/8001/include/AdblockPlus/File...
include/AdblockPlus/FileSystem.h:35: std::string basePath;
I think we should either not declare basePath here or provide an implementation
of SetBasePath that sets it. I'm voting for the former.

http://codereview.adblockplus.org/10369004/diff/8001/src/DefaultFileSystem.cpp
File src/DefaultFileSystem.cpp (right):

http://codereview.adblockplus.org/10369004/diff/8001/src/DefaultFileSystem.cp...
src/DefaultFileSystem.cpp:88: return basePath + "\\" + path;
\\ won't work on UNIX, but I think / should work everywhere.

http://codereview.adblockplus.org/10369004/diff/8001/src/DefaultFileSystem.cp...
src/DefaultFileSystem.cpp:94: if (path.rfind('\\') == (path.length() - 1))
How about if (*path.rbegin() == '\\')?

This also needs to check for / on Linux, maybe both on Windows since / is AFAIK
generally a legal path separator in Windows.

http://codereview.adblockplus.org/10369004/diff/8001/src/DefaultFileSystem.cp...
src/DefaultFileSystem.cpp:96: basePath = path.substr(0, path.length() - 1);
Never used this, but basePath.pop_back(); seems to be possible in C++0x.

http://codereview.adblockplus.org/10369004/diff/8001/src/DefaultWebRequestWin...
File src/DefaultWebRequestWinInet.cpp (right):

http://codereview.adblockplus.org/10369004/diff/8001/src/DefaultWebRequestWin...
src/DefaultWebRequestWinInet.cpp:184: std::string headersString = "";
On 2013/04/30 08:13:07, Oleksandr wrote:
> This fixes an issue on XP. std::string's length is undefined otherwise.

Worth a comment I think. Or is this not the only place where we have that fix?

http://codereview.adblockplus.org/10369004/diff/8001/test/FileSystemJsObject.cpp
File test/FileSystemJsObject.cpp (right):

http://codereview.adblockplus.org/10369004/diff/8001/test/FileSystemJsObject....
test/FileSystemJsObject.cpp:83: return;
I don't think this return is necessary.

[Wladimir Palant, 2013-04-30 09:29:50]

http://codereview.adblockplus.org/10369004/diff/8001/include/AdblockPlus/File...
File include/AdblockPlus/FileSystem.h (right):

http://codereview.adblockplus.org/10369004/diff/8001/include/AdblockPlus/File...
include/AdblockPlus/FileSystem.h:33: virtual void SetBasePath(const std::string&
path) = 0;
This shouldn't be a method of the FileSystem interface, rather one of the
DefaultFileSystem class. Other FileSystem implementation can work differently,
e.g. with an implicit base path or no base path at all (like the
MockFileSystem).

http://codereview.adblockplus.org/10369004/diff/8001/src/DefaultFileSystem.cpp
File src/DefaultFileSystem.cpp (right):

http://codereview.adblockplus.org/10369004/diff/8001/src/DefaultFileSystem.cp...
src/DefaultFileSystem.cpp:88: return basePath + "\\" + path;
1) This path separator is Windows-specific. Please define it in a cross-platform
way, e.g.:

#ifdef _WIN32
#define PATH_SEPARATOR '\\'
#else
#define PATH_SEPARATOR '/'
#endif

And use PATH_SEPARATOR here.

2) What if path is already an absolute path? You need to call PathIsRelative()
on Windows and check first whether the path starts with / on all other platforms
- absolute paths should be returned unchanged.

3) What if SetBasePath() was never called (basePath is empty)? We should return
the path unchanged then, the caller shouldn't be required to call SetBasePath().

4) While we are at it, maybe we should already prefix Windows paths with \\?\
unless PathIsUNC() returns true. This makes sure we don't hit the MAX_PATH
limit.

http://codereview.adblockplus.org/10369004/diff/8001/src/DefaultFileSystem.cp...
src/DefaultFileSystem.cpp:94: if (path.rfind('\\') == (path.length() - 1))
Please search for PATH_SEPARATOR here as well, not for Windows-specific path
separators. Btw, I would rather write that check as:

  if (path.length() && path[path.length() - 1] == PATH_SEPARATOR)

But that's pretty much a matter of taste.

http://codereview.adblockplus.org/10369004/diff/8001/test/BaseJsTest.h
File test/BaseJsTest.h (right):

http://codereview.adblockplus.org/10369004/diff/8001/test/BaseJsTest.h#newcod...
test/BaseJsTest.h:110: AdblockPlus::Sleep(100000);
SetBasePath() isn't a function called from JS, no callbacks here - so this
doesn't need to hang up, doing nothing is fine. Not that this method should be
necessary in the first place...

[Wladimir Palant, 2013-05-06 06:18:30]

http://codereview.adblockplus.org/10369004/diff/19001/src/DefaultFileSystem.cpp
File src/DefaultFileSystem.cpp (right):

http://codereview.adblockplus.org/10369004/diff/19001/src/DefaultFileSystem.c...
src/DefaultFileSystem.cpp:101: if (PathIsRelative(Utils::ToUTF16String(path,
path.length()).c_str()))
This is Windows-specific and won't compile on other systems, should be:

#ifdef _WIN32
  if (PathIsRelative(Utils::ToUTF16String(path, path.length()).c_str()))
#else
  if (path.length() && *path.begin() == PATH_SEPARATOR)
#endif

http://codereview.adblockplus.org/10369004/diff/19001/src/DefaultFileSystem.c...
src/DefaultFileSystem.cpp:116: if ((*basePath.rbegin() == PATH_SEPARATOR))
Nit: You seem to have an extra pair parentheses here.

[Wladimir Palant, 2013-05-06 06:19:51]

http://codereview.adblockplus.org/10369004/diff/19001/src/DefaultFileSystem.cpp
File src/DefaultFileSystem.cpp (right):

http://codereview.adblockplus.org/10369004/diff/19001/src/DefaultFileSystem.c...
src/DefaultFileSystem.cpp:101: if (PathIsRelative(Utils::ToUTF16String(path,
path.length()).c_str()))
On 2013/05/06 06:18:30, Wladimir Palant wrote:
>   if (path.length() && *path.begin() == PATH_SEPARATOR)

Oops, exactly the other way round of course:

   if (!path.length() || *path.begin() != PATH_SEPARATOR)

[Oleksandr, 2013-05-17 08:32:16]

Looks like Wladimir has already addressed his comments himself. Mind if I close
this?

[Wladimir Palant, 2013-05-17 09:35:37]

On 2013/05/17 08:32:16, Oleksandr wrote:
> Looks like Wladimir has already addressed his comments himself.

No, it was actually you (revision 83968838a345). Looks like you fixed the
relative path check being Windows-specific before pushing. The extra parentheses
are still there but I can fix that myself indeed, LGTM.