Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
60953003:
Don't use mirrors in pkg/path. (Closed)
|
Created:
7 years, 1 month ago by Anders Johnsen Modified:
7 years, 1 month ago CC:
reviews_dartlang.org Visibility:
Public. |
DescriptionDon't use mirrors in pkg/path.
BUG=
R=kasperl@google.com
Committed: https://code.google.com/p/dart/source/detail?r=30038
Patch Set 1 #
Total comments: 2
Patch Set 2 : Another attempth at detecting if in browser. #Patch Set 3 : Remove accidental file. #
Total comments: 14
Patch Set 4 : Addressed comments. #
Total comments: 2
Patch Set 5 : Clean up. #Messages
Total messages: 14 (0 generated)
This should help with both performance and Dart2JS code-size.
https://codereview.chromium.org/60953003/diff/1/pkg/path/lib/path.dart File pkg/path/lib/path.dart (right): https://codereview.chromium.org/60953003/diff/1/pkg/path/lib/path.dart#newcod... pkg/path/lib/path.dart:851: if (Uri.base.scheme != 'file') return Style.url; This does the wrong thing if you open a file in Dartium. You shouldn't get Windows or POSIX path semantics within a browser just because you happened to have opened a local file.
https://codereview.chromium.org/60953003/diff/1/pkg/path/lib/path.dart File pkg/path/lib/path.dart (right): https://codereview.chromium.org/60953003/diff/1/pkg/path/lib/path.dart#newcod... pkg/path/lib/path.dart:851: if (Uri.base.scheme != 'file') return Style.url; On 2013/11/06 18:00:36, Bob Nystrom wrote: > This does the wrong thing if you open a file in Dartium. You shouldn't get > Windows or POSIX path semantics within a browser just because you happened to > have opened a local file. Ahh, interesting point. I'll see if I can come up with a better way to detect it.
Thanks for looking into this! I'd definitely like to lose the mirrors dependency here, I just want to make sure we don't change any user-visible behavior to get there. - bob
Okay, trying another way. Let me know what you think.
This is great. I had tried to find some behavioral difference between the console and the browser that would allow us to tell whether a file: URI should be interpreted as URL style, but I hadn't found anything. https://codereview.chromium.org/60953003/diff/120001/pkg/path/lib/path.dart File pkg/path/lib/path.dart (right): https://codereview.chromium.org/60953003/diff/120001/pkg/path/lib/path.dart#n... pkg/path/lib/path.dart:76: return uri.toString(); We should be sure to return an actual directory here (this is also a problem with the old code). It should be the same directory that's used to resolve relative URLs. That means that if [uri] has a trailing slash, it should be [uri] without that slash; otherwise, it should be `dirname(uri.toString())`. https://codereview.chromium.org/60953003/diff/120001/pkg/path/lib/path.dart#n... pkg/path/lib/path.dart:80: return path.substring(0, path.length - 1); Add an assertion that the last character is '/' or '\'. https://codereview.chromium.org/60953003/diff/120001/pkg/path/lib/path.dart#n... pkg/path/lib/path.dart:852: // be running Dart scripts. I think this comment could be clearer. How about "If we're running a Dart file in the browser from a file: URI, [Uri.base] will point to a file. If we're running on the console, it will point to a directory. We can use that fact to determine which style to use." https://codereview.chromium.org/60953003/diff/120001/pkg/path/lib/path.dart#n... pkg/path/lib/path.dart:853: if (Uri.base.path.toString().endsWith('/') && Uri.base.scheme == 'file') { [Uri.base.path] is already a string; [toString] here is superfluous. https://codereview.chromium.org/60953003/diff/120001/pkg/path/lib/path.dart#n... pkg/path/lib/path.dart:854: if (new Uri.file('a/b').toFilePath() == 'a\\b') { Nit: the [new Uri.file] constructor here is unnecessary; just [new Uri] or [Uri.parse] would work. https://codereview.chromium.org/60953003/diff/120001/pkg/path/lib/path.dart#n... pkg/path/lib/path.dart:859: return Style.url; Style suggestion: reduce the nesting here by re-ordering the conditionals. For example: if (Uri.base.scheme != 'file') return Style.url; if (!Uri.base.path.endsWith('/')) return Style.url; if (new Uri(path: 'a/b').toFilePath() == 'a\\b') return Style.windows; return Style.posix;
Forgot one comment. https://codereview.chromium.org/60953003/diff/120001/pkg/path/lib/path.dart File pkg/path/lib/path.dart (right): https://codereview.chromium.org/60953003/diff/120001/pkg/path/lib/path.dart#n... pkg/path/lib/path.dart:72: /// return the current URL. The dart2js comment in this paragraph is no longer accurate. Also, "the current URL" should be changed to describe the directory I mentioned below.
https://codereview.chromium.org/60953003/diff/120001/pkg/path/lib/path.dart File pkg/path/lib/path.dart (right): https://codereview.chromium.org/60953003/diff/120001/pkg/path/lib/path.dart#n... pkg/path/lib/path.dart:72: /// return the current URL. On 2013/11/06 20:43:13, nweiz wrote: > The dart2js comment in this paragraph is no longer accurate. Also, "the current > URL" should be changed to describe the directory I mentioned below. Done. https://codereview.chromium.org/60953003/diff/120001/pkg/path/lib/path.dart#n... pkg/path/lib/path.dart:76: return uri.toString(); On 2013/11/06 19:37:09, nweiz wrote: > We should be sure to return an actual directory here (this is also a problem > with the old code). It should be the same directory that's used to resolve > relative URLs. That means that if [uri] has a trailing slash, it should be [uri] > without that slash; otherwise, it should be `dirname(uri.toString())`. Done. https://codereview.chromium.org/60953003/diff/120001/pkg/path/lib/path.dart#n... pkg/path/lib/path.dart:80: return path.substring(0, path.length - 1); On 2013/11/06 19:37:09, nweiz wrote: > Add an assertion that the last character is '/' or '\'. Done. https://codereview.chromium.org/60953003/diff/120001/pkg/path/lib/path.dart#n... pkg/path/lib/path.dart:852: // be running Dart scripts. On 2013/11/06 19:37:09, nweiz wrote: > I think this comment could be clearer. How about "If we're running a Dart file > in the browser from a file: URI, [Uri.base] will point to a file. If we're > running on the console, it will point to a directory. We can use that fact to > determine which style to use." Done. https://codereview.chromium.org/60953003/diff/120001/pkg/path/lib/path.dart#n... pkg/path/lib/path.dart:853: if (Uri.base.path.toString().endsWith('/') && Uri.base.scheme == 'file') { On 2013/11/06 19:37:09, nweiz wrote: > [Uri.base.path] is already a string; [toString] here is superfluous. Ah, yes, good catch! https://codereview.chromium.org/60953003/diff/120001/pkg/path/lib/path.dart#n... pkg/path/lib/path.dart:854: if (new Uri.file('a/b').toFilePath() == 'a\\b') { On 2013/11/06 19:37:09, nweiz wrote: > Nit: the [new Uri.file] constructor here is unnecessary; just [new Uri] or > [Uri.parse] would work. Done. https://codereview.chromium.org/60953003/diff/120001/pkg/path/lib/path.dart#n... pkg/path/lib/path.dart:859: return Style.url; On 2013/11/06 19:37:09, nweiz wrote: > Style suggestion: reduce the nesting here by re-ordering the conditionals. For > example: > > if (Uri.base.scheme != 'file') return Style.url; > if (!Uri.base.path.endsWith('/')) return Style.url; > if (new Uri(path: 'a/b').toFilePath() == 'a\\b') return Style.windows; > return Style.posix; Done.
+kasperl
LGTM. https://codereview.chromium.org/60953003/diff/170001/pkg/path/lib/path.dart File pkg/path/lib/path.dart (right): https://codereview.chromium.org/60953003/diff/170001/pkg/path/lib/path.dart#n... pkg/path/lib/path.dart:78: assert(path[path.length - 1] == '/' || path[path.length - 1] == '\\'); Maybe put 'path.length - 1' in a local variable (lastIndex).
https://codereview.chromium.org/60953003/diff/170001/pkg/path/lib/path.dart File pkg/path/lib/path.dart (right): https://codereview.chromium.org/60953003/diff/170001/pkg/path/lib/path.dart#n... pkg/path/lib/path.dart:78: assert(path[path.length - 1] == '/' || path[path.length - 1] == '\\'); On 2013/11/07 08:06:58, kasperl wrote: > Maybe put 'path.length - 1' in a local variable (lastIndex). Done.
Message was sent while issue was closed.
Committed patchset #5 manually as r30038 (presubmit successful).
Message was sent while issue was closed.
lgtm
Message was sent while issue was closed.
LGTM! |
