Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
21039005:
Add support for file URIs and extracting the file path from file URIs (Closed)
|
Created:
7 years, 4 months ago by Søren Gjesse Modified:
7 years, 4 months ago CC:
reviews_dartlang.org Visibility:
Public. |
DescriptionAdd support for file URIs and extracting the file path from file URIs
A file URI can now be created directly using the constructor Uri.file.
The file path for a file URI can now be extracted using the toFilePath() method.
The path strings involved use either Windows or non-Windows
semantics. Both the Uri.file constructor and the toFilePath()
method have optional anamed arguments for specifying the
semantics. The default semantic is determined from the platform
Dart is running on.
R=ahe@google.com, whesse@google.com
BUG=
Committed: https://code.google.com/p/dart/source/detail?r=25872
Patch Set 1 #Patch Set 2 : Style fix #
Total comments: 58
Patch Set 3 : Addressed first round of review comments #
Total comments: 36
Patch Set 4 : Addressed second round of review comments #Patch Set 5 : Changed windowsPath to windows #Patch Set 6 : Rebased to r25869 #Patch Set 7 : Fix due to issue 12290 #
Messages
Total messages: 10 (0 generated)
dbc. https://codereview.chromium.org/21039005/diff/3001/sdk/lib/core/uri.dart File sdk/lib/core/uri.dart (right): https://codereview.chromium.org/21039005/diff/3001/sdk/lib/core/uri.dart#newc... sdk/lib/core/uri.dart:290: */ Give some examples. https://codereview.chromium.org/21039005/diff/3001/sdk/lib/core/uri.dart#newc... sdk/lib/core/uri.dart:291: factory Uri.file(String path, {bool windowsPath}) { doesn't sound like a boolean name, but so far haven't found a better one. https://codereview.chromium.org/21039005/diff/3001/sdk/lib/core/uri.dart#newc... sdk/lib/core/uri.dart:319: throw new ArgumentError("Illegal path character ${segment[j]}"); You could maybe use FormatException if you think that this is something that users could provide, and that is something that could happen without a programming error. https://codereview.chromium.org/21039005/diff/3001/sdk/lib/core/uri.dart#newc... sdk/lib/core/uri.dart:334: throw new ArgumentError("Illegal drive letter " + ditto. https://codereview.chromium.org/21039005/diff/3001/sdk/lib/core/uri.dart#newc... sdk/lib/core/uri.dart:809: */ add examples.
Generally, this is very nice. However, I'm concerned about not accepting forward-slashes on Windows. See: http://msdn.microsoft.com/en-us/library/windows/desktop/aa365247(v=vs.85).aspx, in particular: "Note File I/O functions in the Windows API convert "/" to "\" as part of converting the name to an NT-style name, except when using the "\\?\" prefix as detailed in the following sections." https://codereview.chromium.org/21039005/diff/3001/sdk/lib/core/uri.dart File sdk/lib/core/uri.dart (right): https://codereview.chromium.org/21039005/diff/3001/sdk/lib/core/uri.dart#newc... sdk/lib/core/uri.dart:271: * semantics. For Windows semantics the backslash separator is used For Windows semantics, one can use either backslash or slash. https://codereview.chromium.org/21039005/diff/3001/sdk/lib/core/uri.dart#newc... sdk/lib/core/uri.dart:321: throw new UnsupportedError("Illegal path character ${segment[j]}"); StateError? https://codereview.chromium.org/21039005/diff/3001/sdk/lib/core/uri.dart#newc... sdk/lib/core/uri.dart:337: throw new UnsupportedError("Illegal drive letter " + StateError? https://codereview.chromium.org/21039005/diff/3001/sdk/lib/core/uri.dart#newc... sdk/lib/core/uri.dart:349: return new Uri(pathSegments: path.split(sep)); Check that this works as you expect for the empty string. https://codereview.chromium.org/21039005/diff/3001/sdk/lib/core/uri.dart#newc... sdk/lib/core/uri.dart:353: static _makeWindowsFileUrl(String path) { I'm not sure this works if the path is using / instead of \. https://codereview.chromium.org/21039005/diff/3001/sdk/lib/core/uri.dart#newc... sdk/lib/core/uri.dart:788: * used to separate path segments. If the URI is absolute the path An absolute URI means that the URI has a scheme. Not that the path is absolute. What happens if I do: Uri.parse('file:fisk.txt').toFilePath() https://codereview.chromium.org/21039005/diff/3001/sdk/lib/core/uri.dart#newc... sdk/lib/core/uri.dart:789: * statrs with a path separator unless Windows semantics is used and statrs -> starts https://codereview.chromium.org/21039005/diff/3001/sdk/lib/core/uri.dart#newc... sdk/lib/core/uri.dart:805: * [UnsupportedError]. StateError? https://codereview.chromium.org/21039005/diff/3001/sdk/lib/core/uri.dart#newc... sdk/lib/core/uri.dart:808: * [UnsupportedError]. StateError? https://codereview.chromium.org/21039005/diff/3001/sdk/lib/core/uri.dart#newc... sdk/lib/core/uri.dart:821: throw new UnsupportedError( StateError? https://codereview.chromium.org/21039005/diff/3001/sdk/lib/core/uri.dart#newc... sdk/lib/core/uri.dart:826: if (isAbsolute) result.write("/"); isAbsolute doesn't mean what you think it means. https://codereview.chromium.org/21039005/diff/3001/sdk/lib/core/uri.dart#newc... sdk/lib/core/uri.dart:837: _checkWindowsDriveLetter(segments[0].codeUnitAt(0), false); Seems like you accept stuff like: c:b https://codereview.chromium.org/21039005/diff/3001/sdk/lib/core/uri.dart#newc... sdk/lib/core/uri.dart:838: if (segments[0].length > 2) { And then you just add another segment for 'b'? https://codereview.chromium.org/21039005/diff/3001/tests/corelib/uri_file_tes... File tests/corelib/uri_file_test.dart (right): https://codereview.chromium.org/21039005/diff/3001/tests/corelib/uri_file_tes... tests/corelib/uri_file_test.dart:64: //["file:///C:", "/C:", "C:\\"], What are these comments for? https://codereview.chromium.org/21039005/diff/3001/tests/corelib/uri_file_tes... tests/corelib/uri_file_test.dart:96: // Illegal characters in windows file names. Not the test of colon Not -> Note. https://codereview.chromium.org/21039005/diff/3001/tests/standalone/io/uri_pl... File tests/standalone/io/uri_platform_test.dart (right): https://codereview.chromium.org/21039005/diff/3001/tests/standalone/io/uri_pl... tests/standalone/io/uri_platform_test.dart:23: Expect.equals("a/b", Uri.parse("a/b").toFilePath()); Many of these should pass on Windows.
https://codereview.chromium.org/21039005/diff/3001/sdk/lib/core/uri.dart File sdk/lib/core/uri.dart (right): https://codereview.chromium.org/21039005/diff/3001/sdk/lib/core/uri.dart#newc... sdk/lib/core/uri.dart:271: * semantics. For Windows semantics the backslash separator is used On 2013/07/29 15:30:16, ahe wrote: > For Windows semantics, one can use either backslash or slash. Forward slash must be supported as a Windows path separator - plenty of code is ending up with a mixture of forward and backward slashes in Windows file paths, and we can't stop people from doing it, because it works in everything (including package path). https://codereview.chromium.org/21039005/diff/3001/sdk/lib/core/uri.dart#newc... sdk/lib/core/uri.dart:296: external static bool get _isWindows; Could you call this something else (like _isWindowsRuntime), and declare static bool _isWindows = _isWindowsRuntime; and let lazy static initialization take care of caching the value? https://codereview.chromium.org/21039005/diff/3001/sdk/lib/core/uri.dart#newc... sdk/lib/core/uri.dart:357: if (path.length == 2 || path.codeUnitAt(2) != _BACKSLASH) { Why not path.length >= 2? https://codereview.chromium.org/21039005/diff/3001/sdk/lib/core/uri.dart#newc... sdk/lib/core/uri.dart:360: } The windows paths "\foo\bar" and "c:" (and even "c:foo") are both weird, semi-absolute paths in their semantics, but you accept the first and not the second. I guess this is OK, but why accept the first? The semantics depend on the user's current drive. https://codereview.chromium.org/21039005/diff/3001/sdk/lib/core/uri.dart#newc... sdk/lib/core/uri.dart:837: _checkWindowsDriveLetter(segments[0].codeUnitAt(0), false); On 2013/07/29 15:30:16, ahe wrote: > Seems like you accept stuff like: > > c:b These cases have really weird semantics, and I think they should throw exceptions. At least they should have the same exceptions when encoding and when decoding. https://codereview.chromium.org/21039005/diff/3001/tests/corelib/uri_file_tes... File tests/corelib/uri_file_test.dart (right): https://codereview.chromium.org/21039005/diff/3001/tests/corelib/uri_file_tes... tests/corelib/uri_file_test.dart:153: // Windows file path. from the non-Windows URI, not the Windows URI.
PTAL Addressed all comments. Added windows semantics support for paths starting with \\?\. Please consider: 1. New name for windowsPath, how about windowsSemantics? 2. Should file://c:d throw when toFilePath(windowsPath: true) is called instead of returning C:\d? 3. Should we add \\?\ prefix for paths with reserved names on Windows, e.g turning C:\con into \\?\C:\con and turning \\server\share\con into \\?\UNC\server\share\con. The problem is that it does not work with relative paths. https://codereview.chromium.org/21039005/diff/3001/sdk/lib/core/uri.dart File sdk/lib/core/uri.dart (right): https://codereview.chromium.org/21039005/diff/3001/sdk/lib/core/uri.dart#newc... sdk/lib/core/uri.dart:271: * semantics. For Windows semantics the backslash separator is used On 2013/07/29 15:30:16, ahe wrote: > For Windows semantics, one can use either backslash or slash. Done. https://codereview.chromium.org/21039005/diff/3001/sdk/lib/core/uri.dart#newc... sdk/lib/core/uri.dart:271: * semantics. For Windows semantics the backslash separator is used On 2013/07/30 11:29:32, Bill Hesse wrote: > On 2013/07/29 15:30:16, ahe wrote: > > For Windows semantics, one can use either backslash or slash. > > Forward slash must be supported as a Windows path separator - plenty of code is > ending up with a mixture of forward and backward slashes in Windows file paths, > and we can't stop people from doing it, because it works in everything > (including package path). Done. https://codereview.chromium.org/21039005/diff/3001/sdk/lib/core/uri.dart#newc... sdk/lib/core/uri.dart:290: */ On 2013/07/29 15:16:54, floitsch wrote: > Give some examples. Done. https://codereview.chromium.org/21039005/diff/3001/sdk/lib/core/uri.dart#newc... sdk/lib/core/uri.dart:291: factory Uri.file(String path, {bool windowsPath}) { On 2013/07/29 15:16:54, floitsch wrote: > doesn't sound like a boolean name, but so far haven't found a better one. Agree that this is a pretty bad name - sounds like an alternative path value for use on Windows. How about windowsSemantics (my favorite) useWindowsSemantics Or add an enum class FilePathSemantics { static const DEFAULT = const FilePathSemantics._internal(0); static const NON_WINDOWS = const FilePathSemantics._internal(1); static const WINDOWS = const FilePathSemantics._internal(2); const FilePathSemantics._internal(int this._semantics); final int _semantics; } factory Uri.file(String path, {FilePathSemantics windowsPath: FilePathSemantics.DEFAULT}) https://codereview.chromium.org/21039005/diff/3001/sdk/lib/core/uri.dart#newc... sdk/lib/core/uri.dart:296: external static bool get _isWindows; On 2013/07/30 11:29:32, Bill Hesse wrote: > Could you call this something else (like _isWindowsRuntime), and > declare > > static bool _isWindows = _isWindowsRuntime; > > and let lazy static initialization take care of caching the value? Good point. Done. https://codereview.chromium.org/21039005/diff/3001/sdk/lib/core/uri.dart#newc... sdk/lib/core/uri.dart:319: throw new ArgumentError("Illegal path character ${segment[j]}"); On 2013/07/29 15:16:54, floitsch wrote: > You could maybe use FormatException if you think that this is something that > users could provide, and that is something that could happen without a > programming error. Shouldn't we stick to argument checking throwing ArgumentError? https://codereview.chromium.org/21039005/diff/3001/sdk/lib/core/uri.dart#newc... sdk/lib/core/uri.dart:321: throw new UnsupportedError("Illegal path character ${segment[j]}"); On 2013/07/29 15:30:16, ahe wrote: > StateError? I did also think about this. However as the Uri object is immutable this somehow sounds wrong, but I am fine with changing it, as UnsupportedError is also somewhat strange. https://codereview.chromium.org/21039005/diff/3001/sdk/lib/core/uri.dart#newc... sdk/lib/core/uri.dart:334: throw new ArgumentError("Illegal drive letter " + On 2013/07/29 15:16:54, floitsch wrote: > ditto. See above. https://codereview.chromium.org/21039005/diff/3001/sdk/lib/core/uri.dart#newc... sdk/lib/core/uri.dart:337: throw new UnsupportedError("Illegal drive letter " + On 2013/07/29 15:30:16, ahe wrote: > StateError? See above. https://codereview.chromium.org/21039005/diff/3001/sdk/lib/core/uri.dart#newc... sdk/lib/core/uri.dart:349: return new Uri(pathSegments: path.split(sep)); On 2013/07/29 15:30:16, ahe wrote: > Check that this works as you expect for the empty string. Added tests with empty path. https://codereview.chromium.org/21039005/diff/3001/sdk/lib/core/uri.dart#newc... sdk/lib/core/uri.dart:353: static _makeWindowsFileUrl(String path) { On 2013/07/29 15:30:16, ahe wrote: > I'm not sure this works if the path is using / instead of \. It did not, but changed it so it now does. Also updated tests. https://codereview.chromium.org/21039005/diff/3001/sdk/lib/core/uri.dart#newc... sdk/lib/core/uri.dart:357: if (path.length == 2 || path.codeUnitAt(2) != _BACKSLASH) { On 2013/07/30 11:29:32, Bill Hesse wrote: > Why not path.length >= 2? Not sure I understand. If path.length is == 2 I cannot look at path.codeUnitAt(2). https://codereview.chromium.org/21039005/diff/3001/sdk/lib/core/uri.dart#newc... sdk/lib/core/uri.dart:360: } On 2013/07/30 11:29:32, Bill Hesse wrote: > The windows paths "\foo\bar" and "c:" (and even "c:foo") are both weird, > semi-absolute paths in their semantics, but you accept the first and not the > second. > > I guess this is OK, but why accept the first? The semantics depend on the > user's current drive. I think we need to accept the first, as it is widely used. And if you run everything on the same drive it all works. The second is strange, and cannot be captured in the file: URI syntax. The code currently turns URIs like file://c:d into the Windows path of c:/d, but maybe we should just make that an error instead. https://codereview.chromium.org/21039005/diff/3001/sdk/lib/core/uri.dart#newc... sdk/lib/core/uri.dart:788: * used to separate path segments. If the URI is absolute the path On 2013/07/29 15:30:16, ahe wrote: > An absolute URI means that the URI has a scheme. Not that the path is absolute. > > What happens if I do: > > Uri.parse('file:fisk.txt').toFilePath() As print(Uri.parse('file:fisk.txt')) prints file:///fisk.txt This returns /test.txt or \test.txt. Which I think is the right thing. A URI with a scheme is absolute and then the path is absolute. On Windows I also made paths like C:a (relative to the current directory on a drive) absolute, that is Uri.parse('file:C:a').toFilePath() Returns either /C:a (non-Windows) or C:\a (Windows). Added tests for that. Maybe we should make this an error instead. See below. https://codereview.chromium.org/21039005/diff/3001/sdk/lib/core/uri.dart#newc... sdk/lib/core/uri.dart:789: * statrs with a path separator unless Windows semantics is used and On 2013/07/29 15:30:16, ahe wrote: > statrs -> starts Done. https://codereview.chromium.org/21039005/diff/3001/sdk/lib/core/uri.dart#newc... sdk/lib/core/uri.dart:805: * [UnsupportedError]. On 2013/07/29 15:30:16, ahe wrote: > StateError? See above. https://codereview.chromium.org/21039005/diff/3001/sdk/lib/core/uri.dart#newc... sdk/lib/core/uri.dart:808: * [UnsupportedError]. On 2013/07/29 15:30:16, ahe wrote: > StateError? See above. https://codereview.chromium.org/21039005/diff/3001/sdk/lib/core/uri.dart#newc... sdk/lib/core/uri.dart:809: */ On 2013/07/29 15:16:54, floitsch wrote: > add examples. Done. https://codereview.chromium.org/21039005/diff/3001/sdk/lib/core/uri.dart#newc... sdk/lib/core/uri.dart:821: throw new UnsupportedError( On 2013/07/29 15:30:16, ahe wrote: > StateError? See above. https://codereview.chromium.org/21039005/diff/3001/sdk/lib/core/uri.dart#newc... sdk/lib/core/uri.dart:826: if (isAbsolute) result.write("/"); On 2013/07/29 15:30:16, ahe wrote: > isAbsolute doesn't mean what you think it means. I think this is the right thing to do. Parsing file:xxx and file:///xxx results in exactly the same URI object which is absolute with one path component. https://codereview.chromium.org/21039005/diff/3001/sdk/lib/core/uri.dart#newc... sdk/lib/core/uri.dart:837: _checkWindowsDriveLetter(segments[0].codeUnitAt(0), false); On 2013/07/29 15:30:16, ahe wrote: > Seems like you accept stuff like: > > c:b Yes, file:///c:d is turned into path c:\d on Windows. My argument is that it is absolute and starts with a drive letter. There is now a test for it and it is in the examples. However we could also make it an error as the first path segment "c:d" contains an illegal character in a windows path. What I am currently doing is artificially splitting the first path segment into two segments - which is also unexpected. Btw. Chrome turns file:c:d into file:///c:/d when typed in the omnibox. https://codereview.chromium.org/21039005/diff/3001/sdk/lib/core/uri.dart#newc... sdk/lib/core/uri.dart:837: _checkWindowsDriveLetter(segments[0].codeUnitAt(0), false); On 2013/07/30 11:29:32, Bill Hesse wrote: > On 2013/07/29 15:30:16, ahe wrote: > > Seems like you accept stuff like: > > > > c:b > > These cases have really weird semantics, and I think they should throw > exceptions. At least they should have the same exceptions when encoding and > when decoding. I am OK with making it an error. https://codereview.chromium.org/21039005/diff/3001/sdk/lib/core/uri.dart#newc... sdk/lib/core/uri.dart:838: if (segments[0].length > 2) { On 2013/07/29 15:30:16, ahe wrote: > And then you just add another segment for 'b'? See above. https://codereview.chromium.org/21039005/diff/3001/tests/corelib/uri_file_tes... File tests/corelib/uri_file_test.dart (right): https://codereview.chromium.org/21039005/diff/3001/tests/corelib/uri_file_tes... tests/corelib/uri_file_test.dart:64: //["file:///C:", "/C:", "C:\\"], On 2013/07/29 15:30:16, ahe wrote: > What are these comments for? Scratch-pad. Removed. https://codereview.chromium.org/21039005/diff/3001/tests/corelib/uri_file_tes... tests/corelib/uri_file_test.dart:96: // Illegal characters in windows file names. Not the test of colon On 2013/07/29 15:30:16, ahe wrote: > Not -> Note. Done. https://codereview.chromium.org/21039005/diff/3001/tests/corelib/uri_file_tes... tests/corelib/uri_file_test.dart:153: // Windows file path. On 2013/07/30 11:29:32, Bill Hesse wrote: > from the non-Windows URI, not the Windows URI. Done. https://codereview.chromium.org/21039005/diff/3001/tests/standalone/io/uri_pl... File tests/standalone/io/uri_platform_test.dart (right): https://codereview.chromium.org/21039005/diff/3001/tests/standalone/io/uri_pl... tests/standalone/io/uri_platform_test.dart:23: Expect.equals("a/b", Uri.parse("a/b").toFilePath()); On 2013/07/29 15:30:16, ahe wrote: > Many of these should pass on Windows. Added tests to Windows case.
https://codereview.chromium.org/21039005/diff/3001/sdk/lib/core/uri.dart File sdk/lib/core/uri.dart (right): https://codereview.chromium.org/21039005/diff/3001/sdk/lib/core/uri.dart#newc... sdk/lib/core/uri.dart:291: factory Uri.file(String path, {bool windowsPath}) { On 2013/07/30 13:41:26, Søren Gjesse wrote: > On 2013/07/29 15:16:54, floitsch wrote: > > doesn't sound like a boolean name, but so far haven't found a better one. > > Agree that this is a pretty bad name - sounds like an alternative path value for > use on Windows. > > How about > > windowsSemantics (my favorite) > useWindowsSemantics > > Or add an enum > > class FilePathSemantics { > static const DEFAULT = const FilePathSemantics._internal(0); > static const NON_WINDOWS = const FilePathSemantics._internal(1); > static const WINDOWS = const FilePathSemantics._internal(2); > const FilePathSemantics._internal(int this._semantics); > final int _semantics; > } > > factory Uri.file(String path, {FilePathSemantics windowsPath: > FilePathSemantics.DEFAULT}) I think that just "windows:" would be a great name for this parameter. Uri.file("C:/My Docu~1/timeshee.xls", windows: true) for the win! https://codereview.chromium.org/21039005/diff/3001/sdk/lib/core/uri.dart#newc... sdk/lib/core/uri.dart:319: throw new ArgumentError("Illegal path character ${segment[j]}"); On 2013/07/30 13:41:26, Søren Gjesse wrote: > On 2013/07/29 15:16:54, floitsch wrote: > > You could maybe use FormatException if you think that this is something that > > users could provide, and that is something that could happen without a > > programming error. > > Shouldn't we stick to argument checking throwing ArgumentError? I think that a badly formatted file path should be an exception, rather than an error, so I like the idea of using format exception. It is not an input of the wrong type, for example. https://codereview.chromium.org/21039005/diff/3001/sdk/lib/core/uri.dart#newc... sdk/lib/core/uri.dart:357: if (path.length == 2 || path.codeUnitAt(2) != _BACKSLASH) { On 2013/07/30 13:41:26, Søren Gjesse wrote: > On 2013/07/30 11:29:32, Bill Hesse wrote: > > Why not path.length >= 2? > > Not sure I understand. If path.length is == 2 I cannot look at > path.codeUnitAt(2). Nevermind - I read || as && and 2 as 3. https://codereview.chromium.org/21039005/diff/3001/sdk/lib/core/uri.dart#newc... sdk/lib/core/uri.dart:837: _checkWindowsDriveLetter(segments[0].codeUnitAt(0), false); On 2013/07/30 13:41:26, Søren Gjesse wrote: > On 2013/07/30 11:29:32, Bill Hesse wrote: > > On 2013/07/29 15:30:16, ahe wrote: > > > Seems like you accept stuff like: > > > > > > c:b > > > > These cases have really weird semantics, and I think they should throw > > exceptions. At least they should have the same exceptions when encoding and > > when decoding. > > I am OK with making it an error. I am now more convinced by your argument that URI semantics imply that anything that specifies the drive or share (i.e., the "root") must be an absolute path, so that any conversions to or from URI to Windows path should convert weird c:relative_path to c:\relative_path. I think an error would be fine too.
lgtm https://codereview.chromium.org/21039005/diff/11001/runtime/lib/uri_patch.dart File runtime/lib/uri_patch.dart (right): https://codereview.chromium.org/21039005/diff/11001/runtime/lib/uri_patch.dar... runtime/lib/uri_patch.dart:4: // Dart core library. Remove this line? https://codereview.chromium.org/21039005/diff/11001/sdk/lib/core/uri.dart File sdk/lib/core/uri.dart (right): https://codereview.chromium.org/21039005/diff/11001/sdk/lib/core/uri.dart#new... sdk/lib/core/uri.dart:266: * Creates a new `file` URI from an absolute or relative file path. I think backquotes are for code snippets, I'm not sure it is the right thing to use here. https://codereview.chromium.org/21039005/diff/11001/sdk/lib/core/uri.dart#new... sdk/lib/core/uri.dart:272: * is used to separate path segments. Remove the rest of this paragraph from "For Windows semantics ...". It is redundant with the paragraph below. https://codereview.chromium.org/21039005/diff/11001/sdk/lib/core/uri.dart#new... sdk/lib/core/uri.dart:274: * With non-Windows semantics the slash `/` is used to separate path I'm not sure about this use of backquotes either. It would make more sense if it was: `"/"` https://codereview.chromium.org/21039005/diff/11001/sdk/lib/core/uri.dart#new... sdk/lib/core/uri.dart:279: * path is prefixed with `\\?\` in which case only the backslash `\` You could replace "is prefixed with" by "starts with". You could also rewrite it to something like: With Windows semantics, backslash ("\") and forward-slash ("/") are used to separate path segments, except if the path starts with "\\?\" in which case, only backslash ("\") separates path segments. https://codereview.chromium.org/21039005/diff/11001/sdk/lib/core/uri.dart#new... sdk/lib/core/uri.dart:280: * is treated as a path separator. Ditto for the backquotes in above paragraph. https://codereview.chromium.org/21039005/diff/11001/sdk/lib/core/uri.dart#new... sdk/lib/core/uri.dart:285: * with a drive letter then an absolute URI is created. This specification does not match the example below: new Uri.file(r"C:") throws an exception. https://codereview.chromium.org/21039005/diff/11001/sdk/lib/core/uri.dart#new... sdk/lib/core/uri.dart:290: * operating system. When running in the browser non-Windows "the browser" -> "a browser". https://codereview.chromium.org/21039005/diff/11001/sdk/lib/core/uri.dart#new... sdk/lib/core/uri.dart:314: * new Uri.file(r"C:"); // Throws as path with drive letter is not absolute. Long line. https://codereview.chromium.org/21039005/diff/11001/sdk/lib/core/uri.dart#new... sdk/lib/core/uri.dart:319: windowsPath = windowsPath == null ? Uri._isWindows : windowsPath; Remove Uri. prefix. https://codereview.chromium.org/21039005/diff/11001/sdk/lib/core/uri.dart#new... sdk/lib/core/uri.dart:325: static final bool _isWindows = _isWindowsPlatform; For dart2js, this code is suboptimal. I think it would be best if there was just one external getter for _isWindows (you can cache the result in the VM patch file). https://codereview.chromium.org/21039005/diff/11001/sdk/lib/core/uri.dart#new... sdk/lib/core/uri.dart:341: bool argumentError) { Indentation. https://codereview.chromium.org/21039005/diff/11001/sdk/lib/core/uri.dart#new... sdk/lib/core/uri.dart:359: if (_windowsPathReservedCharacters.contains(segment.codeUnitAt(j))) { I suspect that regular expressions would be faster for dart2js. This code feels simpler: segments.skip(firstSegment).forEach((segment) { if (segment.contains(new RegExp(r'["*/:<>?\|]'))) { throw ... } }); https://codereview.chromium.org/21039005/diff/11001/sdk/lib/core/uri.dart#new... sdk/lib/core/uri.dart:860: * operating system. When running in the browser non-Windows the browser -> a browser. https://codereview.chromium.org/21039005/diff/11001/sdk/lib/core/uri.dart#new... sdk/lib/core/uri.dart:873: * `c:\abc` and *not* `c:abc`. I thought this would throw. https://codereview.chromium.org/21039005/diff/11001/sdk/lib/core/uri.dart#new... sdk/lib/core/uri.dart:875: * Examples using non-Windows semantics (resulting of calling toFilePath in comment): Long line. https://codereview.chromium.org/21039005/diff/11001/sdk/lib/core/uri.dart#new... sdk/lib/core/uri.dart:906: return windowsPath ? _toWindowsFilePath() : _toFilePath(); You should probably check that all other fields but host and path are empty. https://codereview.chromium.org/21039005/diff/11001/tests/corelib/uri_file_te... File tests/corelib/uri_file_test.dart (right): https://codereview.chromium.org/21039005/diff/11001/tests/corelib/uri_file_te... tests/corelib/uri_file_test.dart:171: // Slash is an invalid character in file names on both non-Windows and Windows. Long line.
lgtm
Thanks for the review! Committing with one additional change the windowsPath optional boolean argument will be renamed to just windows. https://codereview.chromium.org/21039005/diff/11001/runtime/lib/uri_patch.dart File runtime/lib/uri_patch.dart (right): https://codereview.chromium.org/21039005/diff/11001/runtime/lib/uri_patch.dar... runtime/lib/uri_patch.dart:4: // Dart core library. On 2013/08/06 14:14:30, ahe wrote: > Remove this line? Done. https://codereview.chromium.org/21039005/diff/11001/sdk/lib/core/uri.dart File sdk/lib/core/uri.dart (right): https://codereview.chromium.org/21039005/diff/11001/sdk/lib/core/uri.dart#new... sdk/lib/core/uri.dart:266: * Creates a new `file` URI from an absolute or relative file path. On 2013/08/06 14:14:30, ahe wrote: > I think backquotes are for code snippets, I'm not sure it is the right thing to > use here. Done. https://codereview.chromium.org/21039005/diff/11001/sdk/lib/core/uri.dart#new... sdk/lib/core/uri.dart:272: * is used to separate path segments. On 2013/08/06 14:14:30, ahe wrote: > Remove the rest of this paragraph from "For Windows semantics ...". It is > redundant with the paragraph below. Done. https://codereview.chromium.org/21039005/diff/11001/sdk/lib/core/uri.dart#new... sdk/lib/core/uri.dart:274: * With non-Windows semantics the slash `/` is used to separate path On 2013/08/06 14:14:30, ahe wrote: > I'm not sure about this use of backquotes either. It would make more sense if it > was: `"/"` Ended up using ("/") and ("\"). https://codereview.chromium.org/21039005/diff/11001/sdk/lib/core/uri.dart#new... sdk/lib/core/uri.dart:279: * path is prefixed with `\\?\` in which case only the backslash `\` On 2013/08/06 14:14:30, ahe wrote: > You could replace "is prefixed with" by "starts with". > > You could also rewrite it to something like: > > With Windows semantics, backslash ("\") and forward-slash ("/") are used to > separate path segments, except if the path starts with "\\?\" in which case, > only backslash ("\") separates path segments. Thanks for rephrasing. https://codereview.chromium.org/21039005/diff/11001/sdk/lib/core/uri.dart#new... sdk/lib/core/uri.dart:280: * is treated as a path separator. On 2013/08/06 14:14:30, ahe wrote: > Ditto for the backquotes in above paragraph. Done. https://codereview.chromium.org/21039005/diff/11001/sdk/lib/core/uri.dart#new... sdk/lib/core/uri.dart:285: * with a drive letter then an absolute URI is created. On 2013/08/06 14:14:30, ahe wrote: > This specification does not match the example below: new Uri.file(r"C:") throws > an exception. Extended the comment to say that the drive letter must be followed by a colon and a path separator to become absolute with Windows semantics. https://codereview.chromium.org/21039005/diff/11001/sdk/lib/core/uri.dart#new... sdk/lib/core/uri.dart:290: * operating system. When running in the browser non-Windows On 2013/08/06 14:14:30, ahe wrote: > "the browser" -> "a browser". Done. https://codereview.chromium.org/21039005/diff/11001/sdk/lib/core/uri.dart#new... sdk/lib/core/uri.dart:314: * new Uri.file(r"C:"); // Throws as path with drive letter is not absolute. On 2013/08/06 14:14:30, ahe wrote: > Long line. Done. https://codereview.chromium.org/21039005/diff/11001/sdk/lib/core/uri.dart#new... sdk/lib/core/uri.dart:319: windowsPath = windowsPath == null ? Uri._isWindows : windowsPath; On 2013/08/06 14:14:30, ahe wrote: > Remove Uri. prefix. Done. https://codereview.chromium.org/21039005/diff/11001/sdk/lib/core/uri.dart#new... sdk/lib/core/uri.dart:325: static final bool _isWindows = _isWindowsPlatform; On 2013/08/06 14:14:30, ahe wrote: > For dart2js, this code is suboptimal. I think it would be best if there was just > one external getter for _isWindows (you can cache the result in the VM patch > file). Done. https://codereview.chromium.org/21039005/diff/11001/sdk/lib/core/uri.dart#new... sdk/lib/core/uri.dart:341: bool argumentError) { On 2013/08/06 14:14:30, ahe wrote: > Indentation. Done. https://codereview.chromium.org/21039005/diff/11001/sdk/lib/core/uri.dart#new... sdk/lib/core/uri.dart:359: if (_windowsPathReservedCharacters.contains(segment.codeUnitAt(j))) { On 2013/08/06 14:14:30, ahe wrote: > I suspect that regular expressions would be faster for dart2js. > > This code feels simpler: > > segments.skip(firstSegment).forEach((segment) { > if (segment.contains(new RegExp(r'["*/:<>?\|]'))) { > throw ... > } > }); Done. https://codereview.chromium.org/21039005/diff/11001/sdk/lib/core/uri.dart#new... sdk/lib/core/uri.dart:860: * operating system. When running in the browser non-Windows On 2013/08/06 14:14:30, ahe wrote: > the browser -> a browser. Done. https://codereview.chromium.org/21039005/diff/11001/sdk/lib/core/uri.dart#new... sdk/lib/core/uri.dart:873: * `c:\abc` and *not* `c:abc`. On 2013/08/06 14:14:30, ahe wrote: > I thought this would throw. It now throws. As discussed before I was not sure how to handle this. E.g. when entering file:///c:adb in Chrome it turned into file:///C:/abc. After thinking about it again it now throws, and I don't try to do artificial splitting of the first path segment if it contains a colon. https://codereview.chromium.org/21039005/diff/11001/sdk/lib/core/uri.dart#new... sdk/lib/core/uri.dart:875: * Examples using non-Windows semantics (resulting of calling toFilePath in comment): On 2013/08/06 14:14:30, ahe wrote: > Long line. Done. https://codereview.chromium.org/21039005/diff/11001/sdk/lib/core/uri.dart#new... sdk/lib/core/uri.dart:906: return windowsPath ? _toWindowsFilePath() : _toFilePath(); On 2013/08/06 14:14:30, ahe wrote: > You should probably check that all other fields but host and path are empty. Done and added tests. https://codereview.chromium.org/21039005/diff/11001/tests/corelib/uri_file_te... File tests/corelib/uri_file_test.dart (right): https://codereview.chromium.org/21039005/diff/11001/tests/corelib/uri_file_te... tests/corelib/uri_file_test.dart:171: // Slash is an invalid character in file names on both non-Windows and Windows. On 2013/08/06 14:14:30, ahe wrote: > Long line. Done.
Message was sent while issue was closed.
Committed patchset #7 manually as r25872 (presubmit successful). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
