Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9750003:
Write our JS blobs for handling native classes in Dart. (Closed)
|
Created:
8 years, 9 months ago by ngeoffray Modified:
8 years, 9 months ago CC:
reviews_dartlang.org, Lasse Reichstein Nielsen, karlklose Visibility:
Public. |
DescriptionWrite our JS blobs for handling native classes in Dart.
Committed: https://code.google.com/p/dart/source/detail?r=5867
Patch Set 1 : #
Total comments: 4
Patch Set 2 : #
Total comments: 2
Patch Set 3 : #Patch Set 4 : #Patch Set 5 : #
Total comments: 19
Patch Set 6 : #
Total comments: 34
Patch Set 7 : #Patch Set 8 : #Patch Set 9 : #
Total comments: 6
Patch Set 10 : #
Total comments: 29
Messages
Total messages: 22 (0 generated)
Initial comments. https://chromiumcodereview.appspot.com/9750003/diff/3009/frog/leg/lib/js_help... File frog/leg/lib/js_helper.dart (right): https://chromiumcodereview.appspot.com/9750003/diff/3009/frog/leg/lib/js_help... frog/leg/lib/js_helper.dart:1183: JS('void', @'''#.toString = #''', jsError, toStringWrapper); Use JS_TO_CLOSURE here as well?
dbc. https://chromiumcodereview.appspot.com/9750003/diff/3009/frog/leg/ssa/builder... File frog/leg/ssa/builder.dart (right): https://chromiumcodereview.appspot.com/9750003/diff/3009/frog/leg/ssa/builder... frog/leg/ssa/builder.dart:1793: if (type is !LiteralString) { No need for this test. if it's a StringNode and it's not an interpolation, then it's safe to get .dartString from it.
https://chromiumcodereview.appspot.com/9750003/diff/3009/frog/leg/lib/js_help... File frog/leg/lib/js_helper.dart (right): https://chromiumcodereview.appspot.com/9750003/diff/3009/frog/leg/lib/js_help... frog/leg/lib/js_helper.dart:1183: JS('void', @'''#.toString = #''', jsError, toStringWrapper); On 2012/03/20 11:13:43, ahe wrote: > Use JS_TO_CLOSURE here as well? Good catch. Done. https://chromiumcodereview.appspot.com/9750003/diff/3009/frog/leg/ssa/builder... File frog/leg/ssa/builder.dart (right): https://chromiumcodereview.appspot.com/9750003/diff/3009/frog/leg/ssa/builder... frog/leg/ssa/builder.dart:1793: if (type is !LiteralString) { On 2012/03/20 11:17:22, Lasse Reichstein Nielsen wrote: > No need for this test. if it's a StringNode and it's not an interpolation, then > it's safe to get .dartString from it. As discussed, this is for the type of the expression. Restricting to one string literal seems fair :)
https://chromiumcodereview.appspot.com/9750003/diff/6001/frog/leg/ssa/builder... File frog/leg/ssa/builder.dart (right): https://chromiumcodereview.appspot.com/9750003/diff/6001/frog/leg/ssa/builder... frog/leg/ssa/builder.dart:1790: if (literal is !StringNode || literal.dynamic.isInterpolation) { this .dynamic should be unnecessary. "isInterpolation" is defined on the StringNode class.
https://chromiumcodereview.appspot.com/9750003/diff/6001/frog/leg/ssa/builder... File frog/leg/ssa/builder.dart (right): https://chromiumcodereview.appspot.com/9750003/diff/6001/frog/leg/ssa/builder... frog/leg/ssa/builder.dart:1790: if (literal is !StringNode || literal.dynamic.isInterpolation) { On 2012/03/20 12:01:23, Lasse Reichstein Nielsen wrote: > this .dynamic should be unnecessary. "isInterpolation" is defined on the > StringNode class. Good point. But I should actually not type the nodes with the types I expect. Changed both variables to be of type Node.
ping
Comments so far. Sorry for forgetting about this. https://chromiumcodereview.appspot.com/9750003/diff/10001/frog/leg/lib/js_hel... File frog/leg/lib/js_helper.dart (right): https://chromiumcodereview.appspot.com/9750003/diff/10001/frog/leg/lib/js_hel... frog/leg/lib/js_helper.dart:1183: JS('void', @'''#.toString = #''', jsError, JS_TO_CLOSURE(toStringWrapper)); This method is doing "Dart closure to JS", right? Perhaps it should be called: DART_CLOSURE_TO_JS https://chromiumcodereview.appspot.com/9750003/diff/10001/frog/leg/lib/native... File frog/leg/lib/native_helper.dart (right): https://chromiumcodereview.appspot.com/9750003/diff/10001/frog/leg/lib/native... frog/leg/lib/native_helper.dart:29: String constructorNameWithFallback(obj) { What is the fallback here? When I see a method like this, I expect there to be a fallback argument. As far as I can tell, this is the fallback. The word that is throwing me off is "with". Would it make sense to rename the method to: constructorNameFallback or safeButSlowConstructorName https://chromiumcodereview.appspot.com/9750003/diff/10001/frog/leg/lib/native... frog/leg/lib/native_helper.dart:61: } else if (userAgent.contains(const RegExp('Firefox'))) { Why a regexp here? https://chromiumcodereview.appspot.com/9750003/diff/10001/frog/leg/lib/native... frog/leg/lib/native_helper.dart:63: } else if (userAgent.contains('MSIE')) { and not here?
https://chromiumcodereview.appspot.com/9750003/diff/10001/frog/leg/lib/js_hel... File frog/leg/lib/js_helper.dart (right): https://chromiumcodereview.appspot.com/9750003/diff/10001/frog/leg/lib/js_hel... frog/leg/lib/js_helper.dart:1183: JS('void', @'''#.toString = #''', jsError, JS_TO_CLOSURE(toStringWrapper)); On 2012/03/26 13:01:55, ahe wrote: > This method is doing "Dart closure to JS", right? Perhaps it should be called: > DART_CLOSURE_TO_JS Done. https://chromiumcodereview.appspot.com/9750003/diff/10001/frog/leg/lib/native... File frog/leg/lib/native_helper.dart (right): https://chromiumcodereview.appspot.com/9750003/diff/10001/frog/leg/lib/native... frog/leg/lib/native_helper.dart:29: String constructorNameWithFallback(obj) { On 2012/03/26 13:01:55, ahe wrote: > What is the fallback here? When I see a method like this, I expect there to be a > fallback argument. As far as I can tell, this is the fallback. The word that is > throwing me off is "with". Would it make sense to rename the method to: > > constructorNameFallback or safeButSlowConstructorName Done. https://chromiumcodereview.appspot.com/9750003/diff/10001/frog/leg/lib/native... frog/leg/lib/native_helper.dart:61: } else if (userAgent.contains(const RegExp('Firefox'))) { On 2012/03/26 13:01:55, ahe wrote: > Why a regexp here? Yes, removed.
More comments. Still working my way through the "native" functions. https://chromiumcodereview.appspot.com/9750003/diff/10001/frog/leg/ssa/builde... File frog/leg/ssa/builder.dart (right): https://chromiumcodereview.appspot.com/9750003/diff/10001/frog/leg/ssa/builde... frog/leg/ssa/builder.dart:1856: addGenericSendArgumentsToList(link, inputs); if (node.arguments.isEmpty() || node.arguments.tail.isEmpty()) { compiler.cancel('At least two arguments expected', node: node.arguments); } https://chromiumcodereview.appspot.com/9750003/diff/10001/frog/leg/ssa/builde... frog/leg/ssa/builder.dart:1952: if (!node.arguments.tail.isEmpty()) { See above. You should probably test for node.arguments.isEmpty() in the begging of this method: if (node.arguments.isEmpty() || !node.arguments.tail.isEmpty()) { cancel https://chromiumcodereview.appspot.com/9750003/diff/10001/frog/leg/ssa/builde... frog/leg/ssa/builder.dart:1954: 'Invalid number of arguments in JS_TO_CLOSURE', 'exactly one argument required' https://chromiumcodereview.appspot.com/9750003/diff/10001/frog/leg/ssa/builde... frog/leg/ssa/builder.dart:1955: node: node); node: node.arguments https://chromiumcodereview.appspot.com/9750003/diff/10001/frog/leg/ssa/builde... frog/leg/ssa/builder.dart:1982: if (element === compiler.findHelper(const SourceString('JS'))) { This is weird, I think. Wouldn't this be simpler: if (element.name == const SourceString('JS')) { ...
https://chromiumcodereview.appspot.com/9750003/diff/10001/frog/leg/ssa/builde... File frog/leg/ssa/builder.dart (right): https://chromiumcodereview.appspot.com/9750003/diff/10001/frog/leg/ssa/builde... frog/leg/ssa/builder.dart:1856: addGenericSendArgumentsToList(link, inputs); On 2012/03/26 13:43:45, ahe wrote: > if (node.arguments.isEmpty() || node.arguments.tail.isEmpty()) { > compiler.cancel('At least two arguments expected', node: node.arguments); > } Done. https://chromiumcodereview.appspot.com/9750003/diff/10001/frog/leg/ssa/builde... frog/leg/ssa/builder.dart:1952: if (!node.arguments.tail.isEmpty()) { On 2012/03/26 13:43:45, ahe wrote: > See above. You should probably test for node.arguments.isEmpty() in the begging > of this method: > > if (node.arguments.isEmpty() || !node.arguments.tail.isEmpty()) { > cancel Done. https://chromiumcodereview.appspot.com/9750003/diff/10001/frog/leg/ssa/builde... frog/leg/ssa/builder.dart:1954: 'Invalid number of arguments in JS_TO_CLOSURE', On 2012/03/26 13:43:45, ahe wrote: > 'exactly one argument required' Done. https://chromiumcodereview.appspot.com/9750003/diff/10001/frog/leg/ssa/builde... frog/leg/ssa/builder.dart:1955: node: node); On 2012/03/26 13:43:45, ahe wrote: > node: node.arguments Done. https://chromiumcodereview.appspot.com/9750003/diff/10001/frog/leg/ssa/builde... frog/leg/ssa/builder.dart:1982: if (element === compiler.findHelper(const SourceString('JS'))) { On 2012/03/26 13:43:45, ahe wrote: > This is weird, I think. Wouldn't this be simpler: > > if (element.name == const SourceString('JS')) { > ... Yes, but I thought we agreed on comparing elements and not names.
More preliminary comments. Still working my way through the blobs. https://chromiumcodereview.appspot.com/9750003/diff/10001/frog/leg/ssa/builde... File frog/leg/ssa/builder.dart (right): https://chromiumcodereview.appspot.com/9750003/diff/10001/frog/leg/ssa/builde... frog/leg/ssa/builder.dart:1982: if (element === compiler.findHelper(const SourceString('JS'))) { On 2012/03/26 13:55:32, ngeoffray wrote: > On 2012/03/26 13:43:45, ahe wrote: > > This is weird, I think. Wouldn't this be simpler: > > > > if (element.name == const SourceString('JS')) { > > ... > > Yes, but I thought we agreed on comparing elements and not names. In general yes, however, this is going too far in the other direction :-) You could add this assertion if that would make you feel better: assert(element.getLibrary() === compiler.jsHelperLibrary); https://chromiumcodereview.appspot.com/9750003/diff/12003/frog/leg/lib/native... File frog/leg/lib/native_helper.dart (right): https://chromiumcodereview.appspot.com/9750003/diff/12003/frog/leg/lib/native... frog/leg/lib/native_helper.dart:5: String chromeTypeNameOf(obj) { Generally, I would prefer if these methods followed this naming convention: descriptiveNameBrowserName For example, typeNameOfChrome https://chromiumcodereview.appspot.com/9750003/diff/12003/frog/leg/lib/native... frog/leg/lib/native_helper.dart:24: if (name == 'Document' && JS('bool', '#.xmlVersion', obj)) return 'Document'; I doubt that this is really a bool: JS('bool', '#.xmlVersion', obj) https://chromiumcodereview.appspot.com/9750003/diff/12003/frog/leg/lib/native... frog/leg/lib/native_helper.dart:31: if (JS('bool', "typeof(#) == 'function'", constructor)) { I think we have generally used this pattern, but isn't this better: if ('function' === JS('String', 'typeof(#)', constructor)) { We could even have: String typeof(obj) => JS('String', 'typeof(#)', obj);
https://chromiumcodereview.appspot.com/9750003/diff/10001/frog/leg/ssa/builde... File frog/leg/ssa/builder.dart (right): https://chromiumcodereview.appspot.com/9750003/diff/10001/frog/leg/ssa/builde... frog/leg/ssa/builder.dart:1982: if (element === compiler.findHelper(const SourceString('JS'))) { On 2012/03/26 14:01:23, ahe wrote: > On 2012/03/26 13:55:32, ngeoffray wrote: > > On 2012/03/26 13:43:45, ahe wrote: > > > This is weird, I think. Wouldn't this be simpler: > > > > > > if (element.name == const SourceString('JS')) { > > > ... > > > > Yes, but I thought we agreed on comparing elements and not names. > > In general yes, however, this is going too far in the other direction :-) > > You could add this assertion if that would make you feel better: > > assert(element.getLibrary() === compiler.jsHelperLibrary); No that's ok (a foreign element can only be created by us), I'll just check against the name. https://chromiumcodereview.appspot.com/9750003/diff/12003/frog/leg/lib/native... File frog/leg/lib/native_helper.dart (right): https://chromiumcodereview.appspot.com/9750003/diff/12003/frog/leg/lib/native... frog/leg/lib/native_helper.dart:5: String chromeTypeNameOf(obj) { On 2012/03/26 14:01:23, ahe wrote: > Generally, I would prefer if these methods followed this naming convention: > > descriptiveNameBrowserName > > For example, > > typeNameOfChrome Done. https://chromiumcodereview.appspot.com/9750003/diff/12003/frog/leg/lib/native... frog/leg/lib/native_helper.dart:24: if (name == 'Document' && JS('bool', '#.xmlVersion', obj)) return 'Document'; On 2012/03/26 14:01:23, ahe wrote: > I doubt that this is really a bool: JS('bool', '#.xmlVersion', obj) Done. https://chromiumcodereview.appspot.com/9750003/diff/12003/frog/leg/lib/native... frog/leg/lib/native_helper.dart:31: if (JS('bool', "typeof(#) == 'function'", constructor)) { On 2012/03/26 14:01:23, ahe wrote: > I think we have generally used this pattern, but isn't this better: > > if ('function' === JS('String', 'typeof(#)', constructor)) { Yeah, that could be better. > > We could even have: > > String typeof(obj) => JS('String', 'typeof(#)', obj); True, but code that does typeof in our core library is so performance critical, that I'd rather wait that we can inline before using this helper.
LGTM, but I still don't feel I fully understand this. https://chromiumcodereview.appspot.com/9750003/diff/12003/frog/leg/lib/native... File frog/leg/lib/native_helper.dart (right): https://chromiumcodereview.appspot.com/9750003/diff/12003/frog/leg/lib/native... frog/leg/lib/native_helper.dart:41: && JS('bool', '#', name) This is not a boolean and I think it is unnecessary. https://chromiumcodereview.appspot.com/9750003/diff/12003/frog/leg/lib/native... frog/leg/lib/native_helper.dart:47: return string.substring(8, string.length - 1); I'm concerned about this. We should share this with Primitives.objectToString. I think the regular expression there is more correct. https://chromiumcodereview.appspot.com/9750003/diff/12003/frog/leg/lib/native... frog/leg/lib/native_helper.dart:81: if (_getTypeNameOf == null) _getTypeNameOf = getTypeNameOfFunction(); === https://chromiumcodereview.appspot.com/9750003/diff/12003/frog/leg/lib/native... frog/leg/lib/native_helper.dart:107: * and invokes the JS code. This comment is convoluted and it doesn't match what the method is doing. The method *looks* *up* the type name of [obj] in [methods]. What is methods (I assume a JS object)? https://chromiumcodereview.appspot.com/9750003/diff/12003/frog/leg/lib/native... frog/leg/lib/native_helper.dart:110: String name, What is the name for? https://chromiumcodereview.appspot.com/9750003/diff/12003/frog/leg/lib/native... frog/leg/lib/native_helper.dart:116: if (method === null && _dynamicMetadata !== null) { What is _dynamicMetadata and how does it relate to the documentation comment of this method? https://chromiumcodereview.appspot.com/9750003/diff/12003/frog/leg/lib/native... frog/leg/lib/native_helper.dart:133: '#(#, #, Array.prototype.slice.call(arguments));' Would this work: new List.from(arguments) We could change List.from to call slice. https://chromiumcodereview.appspot.com/9750003/diff/12003/frog/leg/lib/native... frog/leg/lib/native_helper.dart:171: if (f !== null && JS('bool', '#.methods', f)) { I doubt that #.methods is a boolean. https://chromiumcodereview.appspot.com/9750003/diff/12003/frog/leg/lib/native... frog/leg/lib/native_helper.dart:195: class MetaInfo { Exactly what information is this information about? https://chromiumcodereview.appspot.com/9750003/diff/12003/frog/leg/lib/native... frog/leg/lib/native_helper.dart:207: return JS('void', '\$dynamicMetadata = #', table); Do you really need to return from here? https://chromiumcodereview.appspot.com/9750003/diff/12003/frog/leg/lib/native... frog/leg/lib/native_helper.dart:212: * classes. Add example? https://chromiumcodereview.appspot.com/9750003/diff/12003/frog/leg/lib/native... frog/leg/lib/native_helper.dart:228: var isChecksHelper; What is this for and who initializes it?
Thanks very much Peter for all the comments. Note that I'm also trying to find my way through this code, especially the encoding if dynamicMetadata, that was written by Stephen, and I haven't investigated thoroughly. https://chromiumcodereview.appspot.com/9750003/diff/12003/frog/leg/lib/native... File frog/leg/lib/native_helper.dart (right): https://chromiumcodereview.appspot.com/9750003/diff/12003/frog/leg/lib/native... frog/leg/lib/native_helper.dart:41: && JS('bool', '#', name) On 2012/03/26 14:57:28, ahe wrote: > This is not a boolean and I think it is unnecessary. It's necessary because it's checking if the string is empty. I have changed the code to 'name.isEmpty()'. https://chromiumcodereview.appspot.com/9750003/diff/12003/frog/leg/lib/native... frog/leg/lib/native_helper.dart:47: return string.substring(8, string.length - 1); On 2012/03/26 14:57:28, ahe wrote: > I'm concerned about this. We should share this with Primitives.objectToString. I > think the regular expression there is more correct. Probably, but I'd wait another CL to make sure it's correct on all browsers to use Primitives.objectToString. Note that this only concerns DOM types. https://chromiumcodereview.appspot.com/9750003/diff/12003/frog/leg/lib/native... frog/leg/lib/native_helper.dart:81: if (_getTypeNameOf == null) _getTypeNameOf = getTypeNameOfFunction(); On 2012/03/26 14:57:28, ahe wrote: > === Done. https://chromiumcodereview.appspot.com/9750003/diff/12003/frog/leg/lib/native... frog/leg/lib/native_helper.dart:107: * and invokes the JS code. On 2012/03/26 14:57:28, ahe wrote: > This comment is convoluted and it doesn't match what the method is doing. > > The method *looks* *up* the type name of [obj] in [methods]. > > What is methods (I assume a JS object)? Comment updated. Yes [methods] is a JS object, see TODO in dynamicFunction, where I say that we could try to make it a map. https://chromiumcodereview.appspot.com/9750003/diff/12003/frog/leg/lib/native... frog/leg/lib/native_helper.dart:110: String name, On 2012/03/26 14:57:28, ahe wrote: > What is the name for? That's the name of the method. Used to patch the prototype of obj. https://chromiumcodereview.appspot.com/9750003/diff/12003/frog/leg/lib/native... frog/leg/lib/native_helper.dart:116: if (method === null && _dynamicMetadata !== null) { On 2012/03/26 14:57:28, ahe wrote: > What is _dynamicMetadata and how does it relate to the documentation comment of > this method? I updated the comment. https://chromiumcodereview.appspot.com/9750003/diff/12003/frog/leg/lib/native... frog/leg/lib/native_helper.dart:133: '#(#, #, Array.prototype.slice.call(arguments));' On 2012/03/26 14:57:28, ahe wrote: > Would this work: new List.from(arguments) > We could change List.from to call slice. I'm not sure I want to mix regular code that calls new List, with code that deals with the arguments array, which is very special, and may be treated differently by the JS engines. https://chromiumcodereview.appspot.com/9750003/diff/12003/frog/leg/lib/native... frog/leg/lib/native_helper.dart:171: if (f !== null && JS('bool', '#.methods', f)) { On 2012/03/26 14:57:28, ahe wrote: > I doubt that #.methods is a boolean. Done. https://chromiumcodereview.appspot.com/9750003/diff/12003/frog/leg/lib/native... frog/leg/lib/native_helper.dart:195: class MetaInfo { On 2012/03/26 14:57:28, ahe wrote: > Exactly what information is this information about? Added a comment. https://chromiumcodereview.appspot.com/9750003/diff/12003/frog/leg/lib/native... frog/leg/lib/native_helper.dart:207: return JS('void', '\$dynamicMetadata = #', table); On 2012/03/26 14:57:28, ahe wrote: > Do you really need to return from here? No, return removed. https://chromiumcodereview.appspot.com/9750003/diff/12003/frog/leg/lib/native... frog/leg/lib/native_helper.dart:212: * classes. On 2012/03/26 14:57:28, ahe wrote: > Add example? Done. https://chromiumcodereview.appspot.com/9750003/diff/12003/frog/leg/lib/native... frog/leg/lib/native_helper.dart:228: var isChecksHelper; On 2012/03/26 14:57:28, ahe wrote: > What is this for and who initializes it? It's a Js object initialized by the compiler. It will be removed soon: http://codereview.chromium.org/9773026/
SLGTM https://chromiumcodereview.appspot.com/9750003/diff/12003/frog/leg/lib/native... File frog/leg/lib/native_helper.dart (right): https://chromiumcodereview.appspot.com/9750003/diff/12003/frog/leg/lib/native... frog/leg/lib/native_helper.dart:31: if (JS('bool', "typeof(#) == 'function'", constructor)) { On 2012/03/26 14:13:44, ngeoffray wrote: > On 2012/03/26 14:01:23, ahe wrote: > > I think we have generally used this pattern, but isn't this better: > > > > if ('function' === JS('String', 'typeof(#)', constructor)) { > > Yeah, that could be better. > > > > > We could even have: > > > > String typeof(obj) => JS('String', 'typeof(#)', obj); > > True, but code that does typeof in our core library is so performance critical, > that I'd rather wait that we can inline before using this helper. I don't know if it is a problem to use === on strings. Florian has complained about it before, and it is definitely very implementation dependent. https://chromiumcodereview.appspot.com/9750003/diff/12003/frog/leg/lib/native... frog/leg/lib/native_helper.dart:133: '#(#, #, Array.prototype.slice.call(arguments));' On 2012/03/27 09:17:35, ngeoffray wrote: > On 2012/03/26 14:57:28, ahe wrote: > > Would this work: new List.from(arguments) > > We could change List.from to call slice. > > I'm not sure I want to mix regular code that calls new List, with code that > deals with the arguments array, which is very special, and may be treated > differently by the JS engines. I would find this code easier to understand if it was something like: copyArgumentsArray(arguments) { return JS('List', 'Array.prototype.slice.call(#)', arguments); } But since you need to generate a JS function, I'm not sure my suggestion is helpful. https://chromiumcodereview.appspot.com/9750003/diff/12003/frog/leg/lib/native... frog/leg/lib/native_helper.dart:228: var isChecksHelper; On 2012/03/27 09:17:35, ngeoffray wrote: > On 2012/03/26 14:57:28, ahe wrote: > > What is this for and who initializes it? > > It's a Js object initialized by the compiler. It will be removed soon: > http://codereview.chromium.org/9773026/ Add TODO? https://chromiumcodereview.appspot.com/9750003/diff/18001/frog/leg/lib/native... File frog/leg/lib/native_helper.dart (right): https://chromiumcodereview.appspot.com/9750003/diff/18001/frog/leg/lib/native... frog/leg/lib/native_helper.dart:42: && name !== 'Object') { != https://chromiumcodereview.appspot.com/9750003/diff/18001/frog/leg/lib/native... frog/leg/lib/native_helper.dart:224: List<MetaInfo> get _dynamicMetadata() { Is there a particular reason for these methods being private? https://chromiumcodereview.appspot.com/9750003/diff/18001/frog/leg/lib/native... frog/leg/lib/native_helper.dart:244: void dynamicSetMetadata(List<List<String>> inputTable) { This method would be clearer if it was: _dynamicMetadata = computeDynamicMetadata(inputTable);
https://chromiumcodereview.appspot.com/9750003/diff/12003/frog/leg/lib/native... File frog/leg/lib/native_helper.dart (right): https://chromiumcodereview.appspot.com/9750003/diff/12003/frog/leg/lib/native... frog/leg/lib/native_helper.dart:133: '#(#, #, Array.prototype.slice.call(arguments));' On 2012/03/27 09:42:08, ahe wrote: > On 2012/03/27 09:17:35, ngeoffray wrote: > > On 2012/03/26 14:57:28, ahe wrote: > > > Would this work: new List.from(arguments) > > > We could change List.from to call slice. > > > > I'm not sure I want to mix regular code that calls new List, with code that > > deals with the arguments array, which is very special, and may be treated > > differently by the JS engines. > > I would find this code easier to understand if it was something like: > > copyArgumentsArray(arguments) { > return JS('List', 'Array.prototype.slice.call(#)', arguments); > } > > But since you need to generate a JS function, I'm not sure my suggestion is > helpful. I could change it to: '#(#, #, #(arguments));', DART_CLOSURE_TO_JS(throwNoSuchMethod), obj, name, DART_CLOSURE_TO_JS(copyArgumentsArray) Let me know what you prefer. Having too much # may hurt readability. https://chromiumcodereview.appspot.com/9750003/diff/18001/frog/leg/lib/native... File frog/leg/lib/native_helper.dart (right): https://chromiumcodereview.appspot.com/9750003/diff/18001/frog/leg/lib/native... frog/leg/lib/native_helper.dart:42: && name !== 'Object') { On 2012/03/27 09:42:08, ahe wrote: > != Why? https://chromiumcodereview.appspot.com/9750003/diff/18001/frog/leg/lib/native... frog/leg/lib/native_helper.dart:224: List<MetaInfo> get _dynamicMetadata() { On 2012/03/27 09:42:08, ahe wrote: > Is there a particular reason for these methods being private? The reason was that dynamicMetadata is the name of the global. So I wanted another name. There is no true need of making them private. https://chromiumcodereview.appspot.com/9750003/diff/18001/frog/leg/lib/native... frog/leg/lib/native_helper.dart:244: void dynamicSetMetadata(List<List<String>> inputTable) { On 2012/03/27 09:42:08, ahe wrote: > This method would be clearer if it was: > > _dynamicMetadata = computeDynamicMetadata(inputTable); OK, will do in another CL.
So how did the change affect code size? e.g. swarm (which already uses Set) and something simple like client/tests/client/dom/CanvasTest.dart https://chromiumcodereview.appspot.com/9750003/diff/20004/frog/leg/lib/native... File frog/leg/lib/native_helper.dart (right): https://chromiumcodereview.appspot.com/9750003/diff/20004/frog/leg/lib/native... frog/leg/lib/native_helper.dart:75: Function _getTypeNameOf; Could you initialize this with the following function so that the calls avoid the 'if'
LGTM. sorry for the late review. https://chromiumcodereview.appspot.com/9750003/diff/20004/frog/leg/lib/js_hel... File frog/leg/lib/js_helper.dart (right): https://chromiumcodereview.appspot.com/9750003/diff/20004/frog/leg/lib/js_hel... frog/leg/lib/js_helper.dart:1191: JS('void', @'''#.toString = #''', jsError, DART_CLOSURE_TO_JS(toStringWrapper)); 80 chars. https://chromiumcodereview.appspot.com/9750003/diff/20004/frog/leg/lib/native... File frog/leg/lib/native_helper.dart (right): https://chromiumcodereview.appspot.com/9750003/diff/20004/frog/leg/lib/native... frog/leg/lib/native_helper.dart:24: if (name == 'Document' && JS('bool', '!!#.xmlVersion', obj)) return 'Document'; 80 chars. https://chromiumcodereview.appspot.com/9750003/diff/20004/frog/leg/lib/native... frog/leg/lib/native_helper.dart:54: Function getTypeNameOfFunction() { maybe getFunctionForTypeNameOf ? Otherwise it reads to much as if "of function" was together. https://chromiumcodereview.appspot.com/9750003/diff/20004/frog/leg/lib/native... frog/leg/lib/native_helper.dart:117: var methods, Add comment what the type of methods is (a native Hashtable like {}). https://chromiumcodereview.appspot.com/9750003/diff/20004/frog/leg/lib/native... frog/leg/lib/native_helper.dart:122: if (method === null && _dynamicMetadata !== null) { how can _dynamicMetadata be null? https://chromiumcodereview.appspot.com/9750003/diff/20004/frog/leg/lib/native... frog/leg/lib/native_helper.dart:152: if (JS('bool', '!#.hasOwnProperty(#)', proto, name)) { how can this happen? https://chromiumcodereview.appspot.com/9750003/diff/20004/frog/leg/lib/native... frog/leg/lib/native_helper.dart:186: var dartMethod = JS('var', 'Object.getPrototypeOf(#)[#]', new Object(), name); const Object() would work too. No preference. https://chromiumcodereview.appspot.com/9750003/diff/20004/frog/leg/lib/native... frog/leg/lib/native_helper.dart:225: if (JS('var', 'typeof(\$dynamicMetadata)') === 'undefined') { why is \$dynamicMetadata not a normal Dart global field? https://chromiumcodereview.appspot.com/9750003/diff/20004/frog/leg/lib/native... frog/leg/lib/native_helper.dart:240: * class B native "*B" {} extends A https://chromiumcodereview.appspot.com/9750003/diff/20004/frog/leg/lib/native... frog/leg/lib/native_helper.dart:242: * Will generate: Will generate a list of: (the inputTable) https://chromiumcodereview.appspot.com/9750003/diff/20004/frog/leg/lib/native... frog/leg/lib/native_helper.dart:243: * ['A', 'A|B'] explain. ex: where 'A' is the class the information is for, and 'A|B' is the list of subclasses. Also add ['B', 'B'] ? https://chromiumcodereview.appspot.com/9750003/diff/20004/frog/leg/lib/native... frog/leg/lib/native_helper.dart:261: // Initialized by the compiler. Describe type. (if I see correctly a native JS hashtable of native JS hashtables). https://chromiumcodereview.appspot.com/9750003/diff/20004/frog/leg/native_emi... File frog/leg/native_emitter.dart (right): https://chromiumcodereview.appspot.com/9750003/diff/20004/frog/leg/native_emi... frog/leg/native_emitter.dart:318: void assembleCode(StringBuffer other) { s/other/targetBuffer ?
https://chromiumcodereview.appspot.com/9750003/diff/20004/frog/leg/lib/native... File frog/leg/lib/native_helper.dart (right): https://chromiumcodereview.appspot.com/9750003/diff/20004/frog/leg/lib/native... frog/leg/lib/native_helper.dart:122: if (method === null && _dynamicMetadata !== null) { On 2012/03/28 23:36:33, floitsch wrote: > how can _dynamicMetadata be null? It is not generated for programs that call only methods on leaf classes. https://chromiumcodereview.appspot.com/9750003/diff/20004/frog/leg/lib/native... frog/leg/lib/native_helper.dart:139: '#(#, #, Array.prototype.slice.call(arguments));' There is a missing exact type check here. class A native ... {} class B extends A native ... { foo() {...}; } (1) call get_instance_of_A().dynamic.foo() in a try-catch. The above function will be placed on A.prototype.foo. (2) call get_instance_of_B().foo() The exception is thrown again instead of calling the correct method. I'm fixing it in Frog, I'll ask you to review that fix, and then you can fix it here. https://chromiumcodereview.appspot.com/9750003/diff/20004/frog/leg/lib/native... frog/leg/lib/native_helper.dart:152: if (JS('bool', '!#.hasOwnProperty(#)', proto, name)) { On 2012/03/28 23:36:33, floitsch wrote: > how can this happen? This prevents (new Object()).dynamic.foo() from overwriting the hook for foo with the thunk above that throws the NoSuchMethodException.
For Swarm: from 1398619 to 1401737 (+ 0.22%) with bailout methods. from 941374 to 941052 (-0.03%) without bailout methods. For CanvasTest: from 207920 to 247119 (+18.8%) with bailout methods. from 165397 to 180676 (+9.2%) without bailout methods. https://chromiumcodereview.appspot.com/9750003/diff/20004/frog/leg/lib/native... File frog/leg/lib/native_helper.dart (right): https://chromiumcodereview.appspot.com/9750003/diff/20004/frog/leg/lib/native... frog/leg/lib/native_helper.dart:75: Function _getTypeNameOf; On 2012/03/27 18:05:27, sra1 wrote: > Could you initialize this with the following function so that the calls avoid > the 'if' I cannot right now, as it's not valid Dart code to initialize with a non-constant value.
Thanks all. Follow-up CL: https://chromiumcodereview.appspot.com/9921010 https://chromiumcodereview.appspot.com/9750003/diff/20004/frog/leg/lib/js_hel... File frog/leg/lib/js_helper.dart (right): https://chromiumcodereview.appspot.com/9750003/diff/20004/frog/leg/lib/js_hel... frog/leg/lib/js_helper.dart:1191: JS('void', @'''#.toString = #''', jsError, DART_CLOSURE_TO_JS(toStringWrapper)); On 2012/03/28 23:36:33, floitsch wrote: > 80 chars. Done. https://chromiumcodereview.appspot.com/9750003/diff/20004/frog/leg/lib/native... File frog/leg/lib/native_helper.dart (right): https://chromiumcodereview.appspot.com/9750003/diff/20004/frog/leg/lib/native... frog/leg/lib/native_helper.dart:24: if (name == 'Document' && JS('bool', '!!#.xmlVersion', obj)) return 'Document'; On 2012/03/28 23:36:33, floitsch wrote: > 80 chars. Done. https://chromiumcodereview.appspot.com/9750003/diff/20004/frog/leg/lib/native... frog/leg/lib/native_helper.dart:54: Function getTypeNameOfFunction() { On 2012/03/28 23:36:33, floitsch wrote: > maybe getFunctionForTypeNameOf ? > Otherwise it reads to much as if "of function" was together. Done. https://chromiumcodereview.appspot.com/9750003/diff/20004/frog/leg/lib/native... frog/leg/lib/native_helper.dart:117: var methods, On 2012/03/28 23:36:33, floitsch wrote: > Add comment what the type of methods is (a native Hashtable like {}). Done. https://chromiumcodereview.appspot.com/9750003/diff/20004/frog/leg/lib/native... frog/leg/lib/native_helper.dart:139: '#(#, #, Array.prototype.slice.call(arguments));' On 2012/03/29 01:14:23, sra1 wrote: > There is a missing exact type check here. > > class A native ... {} > class B extends A native ... { > foo() {...}; > } > > (1) call get_instance_of_A().dynamic.foo() in a try-catch. > > The above function will be placed on A.prototype.foo. > > (2) call get_instance_of_B().foo() > The exception is thrown again instead of calling the correct method. > > I'm fixing it in Frog, I'll ask you to review that fix, and then you can fix it > here. Done. https://chromiumcodereview.appspot.com/9750003/diff/20004/frog/leg/lib/native... frog/leg/lib/native_helper.dart:186: var dartMethod = JS('var', 'Object.getPrototypeOf(#)[#]', new Object(), name); On 2012/03/28 23:36:33, floitsch wrote: > const Object() would work too. No preference. Good point. Done. https://chromiumcodereview.appspot.com/9750003/diff/20004/frog/leg/lib/native... frog/leg/lib/native_helper.dart:225: if (JS('var', 'typeof(\$dynamicMetadata)') === 'undefined') { On 2012/03/28 23:36:33, floitsch wrote: > why is \$dynamicMetadata not a normal Dart global field? Because it is shared with isolates. Making it global is not ideal, and I'll try to see if we can get something better. I added a comment. https://chromiumcodereview.appspot.com/9750003/diff/20004/frog/leg/lib/native... frog/leg/lib/native_helper.dart:240: * class B native "*B" {} On 2012/03/28 23:36:33, floitsch wrote: > extends A Done. https://chromiumcodereview.appspot.com/9750003/diff/20004/frog/leg/lib/native... frog/leg/lib/native_helper.dart:243: * ['A', 'A|B'] On 2012/03/28 23:36:33, floitsch wrote: > explain. ex: where 'A' is the class the information is for, and 'A|B' is the > list of subclasses. > Also add ['B', 'B'] ? I'm reluctant on adding comments as this is copied-code that I haven't tried to change, optimize, or understand yet. AFAIK, ['B', 'B'] is not generated because it is not needed. https://chromiumcodereview.appspot.com/9750003/diff/20004/frog/leg/lib/native... frog/leg/lib/native_helper.dart:261: // Initialized by the compiler. On 2012/03/28 23:36:33, floitsch wrote: > Describe type. (if I see correctly a native JS hashtable of native JS > hashtables). This code was removed. https://chromiumcodereview.appspot.com/9750003/diff/20004/frog/leg/native_emi... File frog/leg/native_emitter.dart (right): https://chromiumcodereview.appspot.com/9750003/diff/20004/frog/leg/native_emi... frog/leg/native_emitter.dart:318: void assembleCode(StringBuffer other) { On 2012/03/28 23:36:33, floitsch wrote: > s/other/targetBuffer ? Done. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
