Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9392005:
initial frogpad (Closed)
|
Created:
8 years, 10 months ago by mattsh Modified:
8 years, 10 months ago CC:
reviews_dartlang.org Visibility:
Public. |
Descriptioninitial frogpad
BUG=
TEST=
Committed: https://code.google.com/p/dart/source/detail?r=4275
Patch Set 1 #Patch Set 2 : updated comment #
Total comments: 58
Patch Set 3 : code review fixes #Patch Set 4 : added timing #
Total comments: 4
Patch Set 5 : moved to tools/testing #Patch Set 6 : restored comment #Messages
Total messages: 13 (0 generated)
Initial version of frogpad scripts. (Kasper i'm including you just fyi, because this is going into a subdirectory of the frog directory for now. I may move it later if there's a better location for it.)
Also, adding Bob as reviewer just to look at the htmlEscape TODO - did we ever find a location for this?
Lots of style comments. :) https://chromiumcodereview.appspot.com/9392005/diff/2001/frog/pad/frogpad.dart File frog/pad/frogpad.dart (right): https://chromiumcodereview.appspot.com/9392005/diff/2001/frog/pad/frogpad.dar... frog/pad/frogpad.dart:22: String toString() { return message; } Why not just: String toString() => message; https://chromiumcodereview.appspot.com/9392005/diff/2001/frog/pad/frogpad.dar... frog/pad/frogpad.dart:25: // id of script element containing name of the main dart file Should be a complete sentence and capitalized. https://chromiumcodereview.appspot.com/9392005/diff/2001/frog/pad/frogpad.dar... frog/pad/frogpad.dart:27: final String MAIN_ID = "main_id"; I think our (undocumented at this point) convention is to prefer single-quoted strings when possible. https://chromiumcodereview.appspot.com/9392005/diff/2001/frog/pad/frogpad.dar... frog/pad/frogpad.dart:31: HtmlFileSystem fs = new HtmlFileSystem(); These type annotations seem pretty redundant... https://chromiumcodereview.appspot.com/9392005/diff/2001/frog/pad/frogpad.dar... frog/pad/frogpad.dart:35: // TODO - figure out why parseOptions ignores its first two arguments TODO(mattsh)... https://chromiumcodereview.appspot.com/9392005/diff/2001/frog/pad/frogpad.dar... frog/pad/frogpad.dart:44: world.messageHandler = void _( Why not just: world.messageHandler = (prefix, message, span) { https://chromiumcodereview.appspot.com/9392005/diff/2001/frog/pad/frogpad.dar... frog/pad/frogpad.dart:47: if (span !== null) { You can make this a one-liner if you like. https://chromiumcodereview.appspot.com/9392005/diff/2001/frog/pad/frogpad.dar... frog/pad/frogpad.dart:60: html.Element element = html.document.query("#" + id); Interpolate. https://chromiumcodereview.appspot.com/9392005/diff/2001/frog/pad/frogpad.dar... frog/pad/frogpad.dart:62: throw new Exception_("Can't find element " + id); Here too. https://chromiumcodereview.appspot.com/9392005/diff/2001/frog/pad/frogpad.dar... frog/pad/frogpad.dart:75: // TODO(rnystrom): should exist in standard lib somewhere No, this still doesn't have a home. Maybe file a bug for it? https://chromiumcodereview.appspot.com/9392005/diff/2001/frog/pad/frogpad.dar... frog/pad/frogpad.dart:87: print("read $filename (${text.length} bytes)"); Is this for debugging? If so, I'd remove it. https://chromiumcodereview.appspot.com/9392005/diff/2001/frog/pad/frogpad.dar... frog/pad/frogpad.dart:92: * Returns the id of the <script> element that contains The wrapping is strange here. Should be at 80 chars. https://chromiumcodereview.appspot.com/9392005/diff/2001/frog/pad/frogpad.dar... frog/pad/frogpad.dart:116: // grab the last two components (the directory name and file name): "Grab" https://chromiumcodereview.appspot.com/9392005/diff/2001/frog/pad/frogpad.dar... frog/pad/frogpad.dart:121: // join components with underscore, and also replace dots with underscore Should be a sentence. https://chromiumcodereview.appspot.com/9392005/diff/2001/frog/pad/frogpad.dar... frog/pad/frogpad.dart:130: return true; Shouldn't you check for the <div> here? https://chromiumcodereview.appspot.com/9392005/diff/2001/frog/pad/frogpad.dar... frog/pad/frogpad.dart:136: void removeDirectory(String path, [bool recursive]) { Add a blank line above this. https://chromiumcodereview.appspot.com/9392005/diff/2001/frog/pad/frogpad.py File frog/pad/frogpad.py (right): https://chromiumcodereview.appspot.com/9392005/diff/2001/frog/pad/frogpad.py#... frog/pad/frogpad.py:1: #!/usr/bin/python What made you decide to write this in Python instead of Dart?
Bob pretty well covered everything. My comments are pretty much additional places where Bob's comments apply. It will be exciting to have this running! :-D https://chromiumcodereview.appspot.com/9392005/diff/2001/frog/pad/frogpad.dart File frog/pad/frogpad.dart (right): https://chromiumcodereview.appspot.com/9392005/diff/2001/frog/pad/frogpad.dar... frog/pad/frogpad.dart:38: // TODO - figure out whether this home directory argument is needed TODO(mattsh) (again) https://chromiumcodereview.appspot.com/9392005/diff/2001/frog/pad/frogpad.dar... frog/pad/frogpad.dart:70: throw new Exception_("Can't find element " + id); Interpolate. https://chromiumcodereview.appspot.com/9392005/diff/2001/frog/pad/frogpad.py File frog/pad/frogpad.py (right): https://chromiumcodereview.appspot.com/9392005/diff/2001/frog/pad/frogpad.py#... frog/pad/frogpad.py:115: def GenerateHtml(self): Google Python style says to have lower case methods, with underscores instead of CamelCase. I have seen both in our codebase, so I suppose you could go either way. I'd recommend the official Google style, personally.
What about putting this in dart/tools/testing? On 2012/02/13 18:27:58, Emily Fortuna wrote: > Bob pretty well covered everything. My comments are pretty much additional > places where Bob's comments apply. It will be exciting to have this running! :-D > > https://chromiumcodereview.appspot.com/9392005/diff/2001/frog/pad/frogpad.dart > File frog/pad/frogpad.dart (right): > > https://chromiumcodereview.appspot.com/9392005/diff/2001/frog/pad/frogpad.dar... > frog/pad/frogpad.dart:38: // TODO - figure out whether this home directory > argument is needed > TODO(mattsh) (again) > > https://chromiumcodereview.appspot.com/9392005/diff/2001/frog/pad/frogpad.dar... > frog/pad/frogpad.dart:70: throw new Exception_("Can't find element " + id); > Interpolate. > > https://chromiumcodereview.appspot.com/9392005/diff/2001/frog/pad/frogpad.py > File frog/pad/frogpad.py (right): > > https://chromiumcodereview.appspot.com/9392005/diff/2001/frog/pad/frogpad.py#... > frog/pad/frogpad.py:115: def GenerateHtml(self): > Google Python style says to have lower case methods, with underscores instead of > CamelCase. I have seen both in our codebase, so I suppose you could go either > way. I'd recommend the official Google style, personally.
https://chromiumcodereview.appspot.com/9392005/diff/2001/frog/pad/frogpad.dart File frog/pad/frogpad.dart (right): https://chromiumcodereview.appspot.com/9392005/diff/2001/frog/pad/frogpad.dar... frog/pad/frogpad.dart:18: // TODO - remove once corelib has an Exception base class I don't think you need this becase you don't extend from [Exception_], [Exception] has a default implementation, so below you can simply do: new Exception(s) instead of new Exception_(s) https://chromiumcodereview.appspot.com/9392005/diff/2001/frog/pad/frogpad.dar... frog/pad/frogpad.dart:35: // TODO - figure out why parseOptions ignores its first two arguments it currently does because in all shell programms arg[0] is the binary, in script programs, arg[0] is the script runtime, while arg[1] is the script itself (frog in this particular example). The actual command line arguments start in arg[2] https://chromiumcodereview.appspot.com/9392005/diff/2001/frog/pad/frogpad.dar... frog/pad/frogpad.dart:38: // TODO - figure out whether this home directory argument is needed IIRC, this is only needed to determine where to find the libraries in the file system.
https://chromiumcodereview.appspot.com/9392005/diff/2001/frog/pad/frogpad.dart File frog/pad/frogpad.dart (right): https://chromiumcodereview.appspot.com/9392005/diff/2001/frog/pad/frogpad.dar... frog/pad/frogpad.dart:18: // TODO - remove once corelib has an Exception base class On 2012/02/13 18:43:49, sigmund wrote: > I don't think you need this becase you don't extend from [Exception_], > [Exception] has a default implementation, so below you can simply do: > new Exception(s) > instead of > new Exception_(s) Agree https://chromiumcodereview.appspot.com/9392005/diff/2001/frog/pad/frogpad.py File frog/pad/frogpad.py (right): https://chromiumcodereview.appspot.com/9392005/diff/2001/frog/pad/frogpad.py#... frog/pad/frogpad.py:83: DART_LIBRARIES = { This worries me, since it will need to interact with our import hooks and be kept in sync with the VM/DartC/Frog library list. How will we make that work? If this file was a Dart script, you could just use Frog to resolve imports like Bob did in his import_map script. FWIW I think the dart:io is enough to support all the needs of this file. And it'd be great to get more feedback to them on it. https://chromiumcodereview.appspot.com/9392005/diff/2001/frog/pad/frogpad.py#... frog/pad/frogpad.py:115: def GenerateHtml(self): On 2012/02/13 18:27:58, Emily Fortuna wrote: > Google Python style says to have lower case methods, with underscores instead of > CamelCase. I have seen both in our codebase, so I suppose you could go either > way. I'd recommend the official Google style, personally. +1. Here is citation: http://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=Naming... https://chromiumcodereview.appspot.com/9392005/diff/2001/frog/pad/frogpad.py#... frog/pad/frogpad.py:136: def LoadBuiltins(self): These could be resolved on demand when dart: is seen? https://chromiumcodereview.appspot.com/9392005/diff/2001/frog/pad/frogpad.py#... frog/pad/frogpad.py:208: print "generated '%s' (%d bytes)" % (filename, len(html)) I'd remove prints, unless we think they'll be useful in the buildbot lots.
Thanks for reviews. Small fixes below. I'm going to doing a bit more testing and will move to the dart/tools/testing dir before submitting. https://chromiumcodereview.appspot.com/9392005/diff/2001/frog/pad/frogpad.dart File frog/pad/frogpad.dart (right): https://chromiumcodereview.appspot.com/9392005/diff/2001/frog/pad/frogpad.dar... frog/pad/frogpad.dart:18: // TODO - remove once corelib has an Exception base class On 2012/02/13 18:43:49, sigmund wrote: > I don't think you need this becase you don't extend from [Exception_], > [Exception] has a default implementation, so below you can simply do: > new Exception(s) > instead of > new Exception_(s) Done. https://chromiumcodereview.appspot.com/9392005/diff/2001/frog/pad/frogpad.dar... frog/pad/frogpad.dart:18: // TODO - remove once corelib has an Exception base class On 2012/02/13 19:12:12, John Messerly wrote: > On 2012/02/13 18:43:49, sigmund wrote: > > I don't think you need this becase you don't extend from [Exception_], > > [Exception] has a default implementation, so below you can simply do: > > new Exception(s) > > instead of > > new Exception_(s) > > Agree Done. https://chromiumcodereview.appspot.com/9392005/diff/2001/frog/pad/frogpad.dar... frog/pad/frogpad.dart:22: String toString() { return message; } On 2012/02/13 18:10:18, Bob Nystrom wrote: > Why not just: > > String toString() => message; Done. https://chromiumcodereview.appspot.com/9392005/diff/2001/frog/pad/frogpad.dar... frog/pad/frogpad.dart:25: // id of script element containing name of the main dart file On 2012/02/13 18:10:18, Bob Nystrom wrote: > Should be a complete sentence and capitalized. Prefer to keep it lowercase since then it matches how id looks in html attributes. https://chromiumcodereview.appspot.com/9392005/diff/2001/frog/pad/frogpad.dar... frog/pad/frogpad.dart:27: final String MAIN_ID = "main_id"; On 2012/02/13 18:10:18, Bob Nystrom wrote: > I think our (undocumented at this point) convention is to prefer single-quoted > strings when possible. Prefer to keep double quotes here. https://chromiumcodereview.appspot.com/9392005/diff/2001/frog/pad/frogpad.dar... frog/pad/frogpad.dart:31: HtmlFileSystem fs = new HtmlFileSystem(); On 2012/02/13 18:10:18, Bob Nystrom wrote: > These type annotations seem pretty redundant... Prefer to keep type annotations. https://chromiumcodereview.appspot.com/9392005/diff/2001/frog/pad/frogpad.dar... frog/pad/frogpad.dart:35: // TODO - figure out why parseOptions ignores its first two arguments On 2012/02/13 18:43:49, sigmund wrote: > it currently does because in all shell programms arg[0] is the binary, in script > programs, arg[0] is the script runtime, while arg[1] is the script itself (frog > in this particular example). The actual command line arguments start in arg[2] Done. https://chromiumcodereview.appspot.com/9392005/diff/2001/frog/pad/frogpad.dar... frog/pad/frogpad.dart:35: // TODO - figure out why parseOptions ignores its first two arguments On 2012/02/13 18:10:18, Bob Nystrom wrote: > TODO(mattsh)... Done. https://chromiumcodereview.appspot.com/9392005/diff/2001/frog/pad/frogpad.dar... frog/pad/frogpad.dart:38: // TODO - figure out whether this home directory argument is needed On 2012/02/13 18:43:49, sigmund wrote: > IIRC, this is only needed to determine where to find the libraries in the file > system. Done. https://chromiumcodereview.appspot.com/9392005/diff/2001/frog/pad/frogpad.dar... frog/pad/frogpad.dart:38: // TODO - figure out whether this home directory argument is needed On 2012/02/13 18:27:58, Emily Fortuna wrote: > TODO(mattsh) (again) Done. https://chromiumcodereview.appspot.com/9392005/diff/2001/frog/pad/frogpad.dar... frog/pad/frogpad.dart:44: world.messageHandler = void _( On 2012/02/13 18:10:18, Bob Nystrom wrote: > Why not just: > > world.messageHandler = (prefix, message, span) { We want static type checking of the return type, so we need to specify the return type. https://chromiumcodereview.appspot.com/9392005/diff/2001/frog/pad/frogpad.dar... frog/pad/frogpad.dart:47: if (span !== null) { On 2012/02/13 18:10:18, Bob Nystrom wrote: > You can make this a one-liner if you like. Prefer to keep the curlies. https://chromiumcodereview.appspot.com/9392005/diff/2001/frog/pad/frogpad.dar... frog/pad/frogpad.dart:60: html.Element element = html.document.query("#" + id); On 2012/02/13 18:10:18, Bob Nystrom wrote: > Interpolate. Done. https://chromiumcodereview.appspot.com/9392005/diff/2001/frog/pad/frogpad.dar... frog/pad/frogpad.dart:62: throw new Exception_("Can't find element " + id); On 2012/02/13 18:10:18, Bob Nystrom wrote: > Here too. Done. https://chromiumcodereview.appspot.com/9392005/diff/2001/frog/pad/frogpad.dar... frog/pad/frogpad.dart:70: throw new Exception_("Can't find element " + id); On 2012/02/13 18:27:58, Emily Fortuna wrote: > Interpolate. Done. https://chromiumcodereview.appspot.com/9392005/diff/2001/frog/pad/frogpad.dar... frog/pad/frogpad.dart:75: // TODO(rnystrom): should exist in standard lib somewhere On 2012/02/13 18:10:18, Bob Nystrom wrote: > No, this still doesn't have a home. Maybe file a bug for it? http://code.google.com/p/dart/issues/detail?id=1657 Done. https://chromiumcodereview.appspot.com/9392005/diff/2001/frog/pad/frogpad.dar... frog/pad/frogpad.dart:87: print("read $filename (${text.length} bytes)"); On 2012/02/13 18:10:18, Bob Nystrom wrote: > Is this for debugging? If so, I'd remove it. Done. https://chromiumcodereview.appspot.com/9392005/diff/2001/frog/pad/frogpad.dar... frog/pad/frogpad.dart:92: * Returns the id of the <script> element that contains On 2012/02/13 18:10:18, Bob Nystrom wrote: > The wrapping is strange here. Should be at 80 chars. Keep as is. https://chromiumcodereview.appspot.com/9392005/diff/2001/frog/pad/frogpad.dar... frog/pad/frogpad.dart:116: // grab the last two components (the directory name and file name): On 2012/02/13 18:10:18, Bob Nystrom wrote: > "Grab" Done. https://chromiumcodereview.appspot.com/9392005/diff/2001/frog/pad/frogpad.dar... frog/pad/frogpad.dart:121: // join components with underscore, and also replace dots with underscore On 2012/02/13 18:10:18, Bob Nystrom wrote: > Should be a sentence. Done. https://chromiumcodereview.appspot.com/9392005/diff/2001/frog/pad/frogpad.dar... frog/pad/frogpad.dart:130: return true; On 2012/02/13 18:10:18, Bob Nystrom wrote: > Shouldn't you check for the <div> here? Added a comment. https://chromiumcodereview.appspot.com/9392005/diff/2001/frog/pad/frogpad.dar... frog/pad/frogpad.dart:136: void removeDirectory(String path, [bool recursive]) { On 2012/02/13 18:10:18, Bob Nystrom wrote: > Add a blank line above this. Done. https://chromiumcodereview.appspot.com/9392005/diff/2001/frog/pad/frogpad.py File frog/pad/frogpad.py (right): https://chromiumcodereview.appspot.com/9392005/diff/2001/frog/pad/frogpad.py#... frog/pad/frogpad.py:1: #!/usr/bin/python On 2012/02/13 18:10:18, Bob Nystrom wrote: > What made you decide to write this in Python instead of Dart? See comment below. https://chromiumcodereview.appspot.com/9392005/diff/2001/frog/pad/frogpad.py#... frog/pad/frogpad.py:83: DART_LIBRARIES = { On 2012/02/13 19:12:12, John Messerly wrote: > This worries me, since it will need to interact with our import hooks and be > kept in sync with the VM/DartC/Frog library list. How will we make that work? > > If this file was a Dart script, you could just use Frog to resolve imports like > Bob did in his import_map script. > > FWIW I think the dart:io is enough to support all the needs of this file. And > it'd be great to get more feedback to them on it. I want to keep dependencies to a minimum, so I'd prefer to keep this in python for now. https://chromiumcodereview.appspot.com/9392005/diff/2001/frog/pad/frogpad.py#... frog/pad/frogpad.py:115: def GenerateHtml(self): On 2012/02/13 19:12:12, John Messerly wrote: > On 2012/02/13 18:27:58, Emily Fortuna wrote: > > Google Python style says to have lower case methods, with underscores instead > of > > CamelCase. I have seen both in our codebase, so I suppose you could go either > > way. I'd recommend the official Google style, personally. > > +1. > > Here is citation: > http://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=Naming... Done. https://chromiumcodereview.appspot.com/9392005/diff/2001/frog/pad/frogpad.py#... frog/pad/frogpad.py:136: def LoadBuiltins(self): On 2012/02/13 19:12:12, John Messerly wrote: > These could be resolved on demand when dart: is seen? Yes, good suggestion. Done. https://chromiumcodereview.appspot.com/9392005/diff/2001/frog/pad/frogpad.py#... frog/pad/frogpad.py:208: print "generated '%s' (%d bytes)" % (filename, len(html)) On 2012/02/13 19:12:12, John Messerly wrote: > I'd remove prints, unless we think they'll be useful in the buildbot lots. Yes, will do after a bit more testing is completed.
https://chromiumcodereview.appspot.com/9392005/diff/2001/frog/pad/frogpad.dart File frog/pad/frogpad.dart (right): https://chromiumcodereview.appspot.com/9392005/diff/2001/frog/pad/frogpad.dar... frog/pad/frogpad.dart:25: // id of script element containing name of the main dart file On 2012/02/13 20:57:36, mattsh wrote: > On 2012/02/13 18:10:18, Bob Nystrom wrote: > > Should be a complete sentence and capitalized. > Prefer to keep it lowercase since then it matches how id looks in html > attributes. Then do "The id"? https://chromiumcodereview.appspot.com/9392005/diff/2001/frog/pad/frogpad.dar... frog/pad/frogpad.dart:27: final String MAIN_ID = "main_id"; On 2012/02/13 20:57:36, mattsh wrote: > On 2012/02/13 18:10:18, Bob Nystrom wrote: > > I think our (undocumented at this point) convention is to prefer single-quoted > > strings when possible. > Prefer to keep double quotes here. Why? https://chromiumcodereview.appspot.com/9392005/diff/2001/frog/pad/frogpad.dar... frog/pad/frogpad.dart:92: * Returns the id of the <script> element that contains On 2012/02/13 20:57:36, mattsh wrote: > On 2012/02/13 18:10:18, Bob Nystrom wrote: > > The wrapping is strange here. Should be at 80 chars. > > Keep as is. Why?
OK, I have now moved to the tools/testing/frogpad directory, and done style tweaks. I'd like to submit because as of the latest sync, this reveals a bug in frog about how it does name mangling when the same identifier is used in dart:html and another library.
lgtm, with comments. https://chromiumcodereview.appspot.com/9392005/diff/8001/frog/pad/frogpad.dart File frog/pad/frogpad.dart (right): https://chromiumcodereview.appspot.com/9392005/diff/8001/frog/pad/frogpad.dar... frog/pad/frogpad.dart:89: String idOfFilename(String filename) { Why'd the comments for this function go away? I thought they were helpful.. https://chromiumcodereview.appspot.com/9392005/diff/8001/frog/pad/frogpad.py File frog/pad/frogpad.py (right): https://chromiumcodereview.appspot.com/9392005/diff/8001/frog/pad/frogpad.py#... frog/pad/frogpad.py:129: def _create_tag(id, contents): Out of curiosity, are you using the starting _'s like in dart to denote methods that in theory are private like in Dart? If so, might make some comment to that effect, so we understand the convention.
https://chromiumcodereview.appspot.com/9392005/diff/8001/frog/pad/frogpad.dart File frog/pad/frogpad.dart (right): https://chromiumcodereview.appspot.com/9392005/diff/8001/frog/pad/frogpad.dar... frog/pad/frogpad.dart:89: String idOfFilename(String filename) { On 2012/02/14 19:56:53, Emily Fortuna wrote: > Why'd the comments for this function go away? I thought they were helpful.. Good catch. I'm not sure how it got deleted, but restored the comment now. https://chromiumcodereview.appspot.com/9392005/diff/8001/frog/pad/frogpad.py File frog/pad/frogpad.py (right): https://chromiumcodereview.appspot.com/9392005/diff/8001/frog/pad/frogpad.py#... frog/pad/frogpad.py:129: def _create_tag(id, contents): On 2012/02/14 19:56:53, Emily Fortuna wrote: > Out of curiosity, are you using the starting _'s like in dart to denote methods > that in theory are private like in Dart? If so, might make some comment to that > effect, so we understand the convention. I thought that's the convention - search for _lower_with_under() in the Naming section. http://google-styleguide.googlecode.com/svn/trunk/pyguide.html#Naming
> On 2012/02/14 19:56:53, Emily Fortuna wrote: > > Out of curiosity, are you using the starting _'s like in dart to denote > methods > > that in theory are private like in Dart? If so, might make some comment to > that > > effect, so we understand the convention. > > I thought that's the convention - search for _lower_with_under() in the Naming > section. > http://google-styleguide.googlecode.com/svn/trunk/pyguide.html#Naming You're right. Siggi pointed that out to me after I wrote this. Looks good! |
