Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9416119:
Add some magic to _getJSFunctionName so it will work in Internet Explorer. (Closed)
|
Created:
8 years, 10 months ago by eub Modified:
8 years, 10 months ago CC:
reviews_dartlang.org Visibility:
Public. |
DescriptionAdd some magic to _getJSFunctionName so it will work in Internet Explorer.
Committed: https://code.google.com/p/dart/source/detail?r=4650
Patch Set 1 #
Total comments: 8
Patch Set 2 : #Patch Set 3 : #Messages
Total messages: 10 (0 generated)
lgtm, with comments below https://chromiumcodereview.appspot.com/9416119/diff/1/frog/lib/isolate.dart File frog/lib/isolate.dart (right): https://chromiumcodereview.appspot.com/9416119/diff/1/frog/lib/isolate.dart#n... frog/lib/isolate.dart:760: /** Find a constructor given its name. */ thx :) https://chromiumcodereview.appspot.com/9416119/diff/1/frog/lib/isolate.dart#n... frog/lib/isolate.dart:769: /** Get a string name for the function, if possible. The result for (nit) style of the comment block: if it's multiline, we break the first and last line: /** * ... * ... */ https://chromiumcodereview.appspot.com/9416119/diff/1/frog/lib/isolate.dart#n... frog/lib/isolate.dart:774: // oh-so-convenient function .name property? some of these comments are useful, but note that comments within native blocks will be preserved in the generated output from frog until we have a minimizer in place. Maybe we can remove some of the self explanatory comments below?
Thanks, Siggi. https://chromiumcodereview.appspot.com/9416119/diff/1/frog/lib/isolate.dart File frog/lib/isolate.dart (right): https://chromiumcodereview.appspot.com/9416119/diff/1/frog/lib/isolate.dart#n... frog/lib/isolate.dart:769: /** Get a string name for the function, if possible. The result for On 2012/02/22 23:46:49, sigmund wrote: > (nit) style of the comment block: if it's multiline, we break the first and last > line: > /** > * ... > * ... > */ Done. https://chromiumcodereview.appspot.com/9416119/diff/1/frog/lib/isolate.dart#n... frog/lib/isolate.dart:774: // oh-so-convenient function .name property? On 2012/02/22 23:46:49, sigmund wrote: > some of these comments are useful, but note that comments within native blocks > will be preserved in the generated output from frog until we have a minimizer in > place. Maybe we can remove some of the self explanatory comments below? Removed a little chatter, and moved the implementation comment out of the native string (without letting it mash into the doc comment).
https://chromiumcodereview.appspot.com/9416119/diff/1/frog/lib/isolate.dart File frog/lib/isolate.dart (right): https://chromiumcodereview.appspot.com/9416119/diff/1/frog/lib/isolate.dart#n... frog/lib/isolate.dart:772: static String _getJSFunctionName(Function f) native """ Can't we simplify this whole thing by not using .name? We can always stick an extra field on the functions where we need to be able to look them up in the "global" namespace. That will also give us a trivial way to figure out if something is a static or a top-level function (not just some random function that happens to have a name that we can resolve in the global scope). I can think of three issues with my proposal: (1) Setting up the .name property is shorter. (2) We want to be able to spawn from functions that aren't written by us (pre-existing JavaScript functions?). (3) It's nice to have a good .name property anyway to make it easier to debug at the JavaScript level.
https://chromiumcodereview.appspot.com/9416119/diff/1/frog/lib/isolate.dart File frog/lib/isolate.dart (right): https://chromiumcodereview.appspot.com/9416119/diff/1/frog/lib/isolate.dart#n... frog/lib/isolate.dart:772: static String _getJSFunctionName(Function f) native """ On 2012/02/23 06:53:35, kasperl wrote: > Can't we simplify this whole thing by not using .name? We can always stick an > extra field on the functions where we need to be able to look them up in the > "global" namespace. That will also give us a trivial way to figure out if > something is a static or a top-level function (not just some random function > that happens to have a name that we can resolve in the global scope). > > I can think of three issues with my proposal: > > (1) Setting up the .name property is shorter. > (2) We want to be able to spawn from functions that aren't written by us > (pre-existing JavaScript functions?). > (3) It's nice to have a good .name property anyway to make it easier to debug at > the JavaScript level. Yes, we debated a bit about this, and were afraid of mainly #1. If we go down the route of setting a field on each static or top-level function, we could do this in 3 ways: a- setting it on every function b- asking the programmer to specify which functions to tag. c- setting it on functions that may be passed to spawn. (a) feels like too much, (b) feels like it is to awkward to work with, (c) would be nice, but we need a good static approach to approximate which functions may be passed to spawn. This morning I was just thinking of one possible approach for (c). We could say that we only tag functions that the program refers to as a symbol expression (not a call). For example, in this program: void a() {} void b() {} void c() {} void d() { return c; } main() { b(); final x = a; spawnFunction(x == null ? x : d()); } we would tag 'a' and 'c', but not 'b' or 'd'. We could also say that if 'dart:isolate' is never imported, then we don't tag any function. I believe this wouldn't be hard to implement in frog, but I'm not 100% sure. Thoughts?
My initial reaction to the toString option was "yuck", but I ended up tolerating this option best, because the use of _getJSFunctionName is so rare and specialized -- currently called only when spawning an isolate from a function. This convinced me that it was better to push all the yuck inside this function, rather than carry names on all functions (in *addition* to their browser-dependent .name properties) or add compiler logic to do even conservative tracking of where names will be needed. If we expect names or the machinery to add them to be used more broadly, that would tip me the other way.
https://chromiumcodereview.appspot.com/9416119/diff/1/frog/lib/isolate.dart File frog/lib/isolate.dart (right): https://chromiumcodereview.appspot.com/9416119/diff/1/frog/lib/isolate.dart#n... frog/lib/isolate.dart:772: static String _getJSFunctionName(Function f) native """ On 2012/02/23 17:13:23, sigmund wrote: > On 2012/02/23 06:53:35, kasperl wrote: > > Can't we simplify this whole thing by not using .name? We can always stick an > > extra field on the functions where we need to be able to look them up in the > > "global" namespace. That will also give us a trivial way to figure out if > > something is a static or a top-level function (not just some random function > > that happens to have a name that we can resolve in the global scope). > > > > I can think of three issues with my proposal: > > > > (1) Setting up the .name property is shorter. > > (2) We want to be able to spawn from functions that aren't written by us > > (pre-existing JavaScript functions?). > > (3) It's nice to have a good .name property anyway to make it easier to debug > at > > the JavaScript level. > > Yes, we debated a bit about this, and were afraid of mainly #1. If we go down > the route of setting a field on each static or top-level function, we could do > this in 3 ways: > a- setting it on every function > b- asking the programmer to specify which functions to tag. > c- setting it on functions that may be passed to spawn. > > (a) feels like too much, (b) feels like it is to awkward to work with, (c) would > be nice, but we need a good static approach to approximate which functions may > be passed to spawn. > > This morning I was just thinking of one possible approach for (c). We could say > that we only tag functions that the program refers to as a symbol expression > (not a call). For example, in this program: > > void a() {} > void b() {} > void c() {} > void d() { return c; } > main() { > b(); > final x = a; > spawnFunction(x == null ? x : d()); > } > we would tag 'a' and 'c', but not 'b' or 'd'. We could also say that if > 'dart:isolate' is never imported, then we don't tag any function. > > I believe this wouldn't be hard to implement in frog, but I'm not 100% sure. > Thoughts? Option (c) sounds good to me. That should be even better than always emitting named functions for static functions.
Oh, and by the way: LGTM.
Migrated this CL following Siggi's 9422019.
lgtm |
