[webkit-reviews] review granted: [Bug 194344] [WTF] Add getting/changing current working directory to FileSystem : [Attachment 361404] Add get/change current working directory functions, fix a few issues

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Stephan Szabo
<stephan.szabo at sony.com>'s request for review:
Bug 194344: [WTF] Add getting/changing current working directory to FileSystem
https://bugs.webkit.org/show_bug.cgi?id=194344

Attachment 361404: Add get/change current working directory functions, fix a
few issues

https://bugs.webkit.org/attachment.cgi?id=361404&action=review




--- Comment #8 from Darin Adler <darin at apple.com> ---
Comment on attachment 361404
  --> https://bugs.webkit.org/attachment.cgi?id=361404
Add get/change current working directory functions, fix a few issues

View in context: https://bugs.webkit.org/attachment.cgi?id=361404&action=review

I’d like to see the one first use of this in JSC in the same patch as
introducing it. Not a huge fan of adding something unused to WTF.

> Source/WTF/wtf/glib/FileSystemGlib.cpp:470
> +    GUniquePtr<char> currentDir(g_get_current_dir());

When does this give a different value than getcwd?

> Source/WTF/wtf/glib/FileSystemGlib.cpp:476
> +    CString fsRep = fileSystemRepresentation(filePath);

Seems wasteful to have a local variable here.

> Source/WTF/wtf/glib/FileSystemGlib.cpp:477
> +    return !chdir(fsRep.data());

This is the same as the POSIX one. Maybe we can rearrange things so we don’t
have two copies. Does this even work when glib is running on Windows?

> Source/WTF/wtf/posix/FileSystemPOSIX.cpp:506
> +    CString fsRep = fileSystemRepresentation(filePath);

Seems wasteful to have a local variable here. Just do it all on one line.