Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1194643002:
Enhance dart:js interop in a backwards compatible manner. List objects can now be passed back and f… (Closed)
|
Created:
5 years, 6 months ago by Jacob Modified:
5 years, 6 months ago CC:
reviews_dartlang.org, ricow1 Base URL:
git@github.com:dart-lang/sdk.git@master Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionEnhance dart:js interop in a backwards compatible manner. List objects can now be passed back and forth to Dartium without requiring jsify
BUG=
R=terry@google.com, vsm@google.com
Committed: https://github.com/dart-lang/sdk/commit/3fdeb669ee40eead097009e59dfb5adcb414178e
Patch Set 1 #
Total comments: 1
Patch Set 2 : tweaks #Patch Set 3 : ran formatter added typed interop test #Patch Set 4 : ready for review #
Total comments: 36
Patch Set 5 : ptal #
Total comments: 26
Patch Set 6 : ptal #
Total comments: 3
Patch Set 7 : #
Messages
Total messages: 11 (1 generated)
jacobr@google.com changed reviewers: + terry@google.com, vsm@google.com
Ignore any formatting changes. I ran the latest dartfmt so if you have complaints bring them up with bob. Tests are skipped so that the build does not break while dartium changes are landing. The tests all pass locally in checked mode with my custom WebKit. When dartium changes land I will enable the two new test files. See https://codereview.chromium.org/1177953010 for the tightly coupled WebKit changes. https://codereview.chromium.org/1194643002/diff/1/sdk/lib/html/dartium/html_d... File sdk/lib/html/dartium/html_dartium.dart (right): https://codereview.chromium.org/1194643002/diff/1/sdk/lib/html/dartium/html_d... sdk/lib/html/dartium/html_dartium.dart:123: + 'JsObject': () => js.JsObjectImpl, These impl classes are now lazily created as required. We can't lazily create the JsObject, JsArray, and JsFunction classes as they could be referenced in user code.
I have a larger question when this is all enabled. When will jsify be eliminated? What happens in dart2js with jsify? Bunch of suggestions on tests. Otherwise LGTM. https://codereview.chromium.org/1194643002/diff/60001/sdk/lib/js/dartium/js_d... File sdk/lib/js/dartium/js_dartium.dart (right): https://codereview.chromium.org/1194643002/diff/60001/sdk/lib/js/dartium/js_d... sdk/lib/js/dartium/js_dartium.dart:545: object) native "JsObject_fromBrowserObject"; Not your problem but ugly pretty printing. Wonder if Bob is break on native keyword if => does he break on '=>' operator? https://codereview.chromium.org/1194643002/diff/60001/tests/html/js_array_tes... File tests/html/js_array_test.dart (right): https://codereview.chromium.org/1194643002/diff/60001/tests/html/js_array_tes... tests/html/js_array_test.dart:38: this.dartArray = obj; What does this do? https://codereview.chromium.org/1194643002/diff/60001/tests/html/js_array_tes... tests/html/js_array_test.dart:195: expect(list.length, equals(2)); I know these tests aren't enabled yet. What about testing that first is "a" and last is "b" may [3] is null after pruning the list to 2 entries. https://codereview.chromium.org/1194643002/diff/60001/tests/html/js_array_tes... tests/html/js_array_test.dart:206: // Make sure we are coercing to a JS number. After the setLength failures should list.length be checked that its still 4? https://codereview.chromium.org/1194643002/diff/60001/tests/html/js_array_tes... tests/html/js_array_test.dart:224: }); What about joining 2 real lists e.g., expect(callJsMethod(list, "join", listWithDartClasses), equals("3,42,foo,3,Instance of 'Foo',42,foo,Instance of 'Object'")); https://codereview.chromium.org/1194643002/diff/60001/tests/html/js_array_tes... tests/html/js_array_test.dart:255: expect(list.length, equals(0)); Pop once more when there's no items in the list? https://codereview.chromium.org/1194643002/diff/60001/tests/html/js_array_tes... tests/html/js_array_test.dart:263: expect(callPush(list, "bar"), equals(2)); May push a 'new Foo()' or 'new DivElement()' instead of all strings? https://codereview.chromium.org/1194643002/diff/60001/tests/html/js_array_tes... tests/html/js_array_test.dart:283: expect(list.length, equals(0)); One more shift with nothing in the array? https://codereview.chromium.org/1194643002/diff/60001/tests/html/js_array_tes... tests/html/js_array_test.dart:291: var list = [3, 42, "foo"]; Did you mean to have foo or div object in the list? https://codereview.chromium.org/1194643002/diff/60001/tests/html/js_array_tes... tests/html/js_array_test.dart:294: list = [3, 42]; is this test redundant? https://codereview.chromium.org/1194643002/diff/60001/tests/html/js_array_tes... tests/html/js_array_test.dart:309: expect(list[i], equals(copy[i])); Should test for list[3] === copy[3] and list[4] === copy[4] be done? To test that shallow copy happened? https://codereview.chromium.org/1194643002/diff/60001/tests/html/js_array_tes... tests/html/js_array_test.dart:324: }); What about testing slicing from end e.g., slice[-2]? https://codereview.chromium.org/1194643002/diff/60001/tests/html/js_array_tes... tests/html/js_array_test.dart:351: // context.callMethod('setValue', [list, -1, "foo"]); // Not a valid array index Would callJsMethod(list, 'splice', [-1, 0, "foo"]) work? Or is that a problem too? https://codereview.chromium.org/1194643002/diff/60001/tests/html/js_array_tes... tests/html/js_array_test.dart:387: var ret = context.callMethod("concatValues", [list]); Should comment like expected ret is ["1", "2", "a", "b", ["c", "d"], 42] and concat flattens the returned array be added? What about have an object in the list that's concatenated? https://codereview.chromium.org/1194643002/diff/60001/tests/html/js_array_tes... tests/html/js_array_test.dart:400: var ret = context.callMethod("concatOntoArray", [list]); Should comment like expected ret to be [1,2,3, "a", "b", "foo"] be added? https://codereview.chromium.org/1194643002/diff/60001/tests/html/js_array_tes... tests/html/js_array_test.dart:413: expect(context.callMethod("everyGreaterThanZero", [[1, 0]]), isFalse); What if empty list? https://codereview.chromium.org/1194643002/diff/60001/tests/html/js_test.html File tests/html/js_test.html (right): https://codereview.chromium.org/1194643002/diff/60001/tests/html/js_test.html... tests/html/js_test.html:4: <script src="js_test.dart" type="application/dart"></script> Is there a js_test.dart? https://codereview.chromium.org/1194643002/diff/60001/tests/html/js_typed_int... File tests/html/js_typed_interop_test.dart (right): https://codereview.chromium.org/1194643002/diff/60001/tests/html/js_typed_int... tests/html/js_typed_interop_test.dart:131: expect(() => registerJsInterfaces([Baz]), throws); Comment that registerJsInterfaces is internal check?
https://codereview.chromium.org/1194643002/diff/60001/sdk/lib/js/dartium/js_d... File sdk/lib/js/dartium/js_dartium.dart (right): https://codereview.chromium.org/1194643002/diff/60001/sdk/lib/js/dartium/js_d... sdk/lib/js/dartium/js_dartium.dart:545: object) native "JsObject_fromBrowserObject"; On 2015/06/19 14:57:04, terry wrote: > Not your problem but ugly pretty printing. Wonder if Bob could break on native > keyword if => does he break on '=>' operator? Talked to Bob added issue to track this see https://github.com/dart-lang/dart_style/issues/364
ptal https://codereview.chromium.org/1194643002/diff/60001/sdk/lib/js/dartium/js_d... File sdk/lib/js/dartium/js_dartium.dart (right): https://codereview.chromium.org/1194643002/diff/60001/sdk/lib/js/dartium/js_d... sdk/lib/js/dartium/js_dartium.dart:545: object) native "JsObject_fromBrowserObject"; On 2015/06/19 16:42:11, terry wrote: > On 2015/06/19 14:57:04, terry wrote: > > Not your problem but ugly pretty printing. Wonder if Bob could break on > native > > keyword if => does he break on '=>' operator? > > Talked to Bob added issue to track this see > https://github.com/dart-lang/dart_style/issues/364 Acknowledged. https://codereview.chromium.org/1194643002/diff/60001/sdk/lib/js/dartium/js_d... sdk/lib/js/dartium/js_dartium.dart:545: object) native "JsObject_fromBrowserObject"; On 2015/06/19 14:57:04, terry wrote: > Not your problem but ugly pretty printing. Wonder if Bob is break on native > keyword if => does he break on '=>' operator? Acknowledged. https://codereview.chromium.org/1194643002/diff/60001/tests/html/js_array_tes... File tests/html/js_array_test.dart (right): https://codereview.chromium.org/1194643002/diff/60001/tests/html/js_array_tes... tests/html/js_array_test.dart:38: this.dartArray = obj; On 2015/06/19 14:57:04, terry wrote: > What does this do? good catch... nothing! I was debugging and forgot to remove it. https://codereview.chromium.org/1194643002/diff/60001/tests/html/js_array_tes... tests/html/js_array_test.dart:195: expect(list.length, equals(2)); On 2015/06/19 14:57:04, terry wrote: > I know these tests aren't enabled yet. What about testing that first is "a" and > last is "b" may [3] is null after pruning the list to 2 entries. added thta test. btw these tests are actually running on my local machine so they are "real" https://codereview.chromium.org/1194643002/diff/60001/tests/html/js_array_tes... tests/html/js_array_test.dart:206: // Make sure we are coercing to a JS number. On 2015/06/19 14:57:04, terry wrote: > After the setLength failures should list.length be checked that its still 4? good idea. done. https://codereview.chromium.org/1194643002/diff/60001/tests/html/js_array_tes... tests/html/js_array_test.dart:224: }); On 2015/06/19 14:57:04, terry wrote: > What about joining 2 real lists e.g., > > expect(callJsMethod(list, "join", listWithDartClasses), > equals("3,42,foo,3,Instance of 'Foo',42,foo,Instance of 'Object'")); Are you thinking about the concat method? join on a JS array generates a string that is essentially the result of calling toString on each array element and adding the separator between each element. https://codereview.chromium.org/1194643002/diff/60001/tests/html/js_array_tes... tests/html/js_array_test.dart:255: expect(list.length, equals(0)); On 2015/06/19 14:57:04, terry wrote: > Pop once more when there's no items in the list? good idea. done. https://codereview.chromium.org/1194643002/diff/60001/tests/html/js_array_tes... tests/html/js_array_test.dart:255: expect(list.length, equals(0)); On 2015/06/19 14:57:04, terry wrote: > Pop once more when there's no items in the list? added and fixed behavior for that case to match JS. https://codereview.chromium.org/1194643002/diff/60001/tests/html/js_array_tes... tests/html/js_array_test.dart:263: expect(callPush(list, "bar"), equals(2)); On 2015/06/19 14:57:04, terry wrote: > May push a 'new Foo()' or 'new DivElement()' instead of all strings? Done. https://codereview.chromium.org/1194643002/diff/60001/tests/html/js_array_tes... tests/html/js_array_test.dart:283: expect(list.length, equals(0)); On 2015/06/19 14:57:04, terry wrote: > One more shift with nothing in the array? Done. https://codereview.chromium.org/1194643002/diff/60001/tests/html/js_array_tes... tests/html/js_array_test.dart:291: var list = [3, 42, "foo"]; On 2015/06/19 14:57:04, terry wrote: > Did you mean to have foo or div object in the list? Done. https://codereview.chromium.org/1194643002/diff/60001/tests/html/js_array_tes... tests/html/js_array_test.dart:294: list = [3, 42]; On 2015/06/19 14:57:04, terry wrote: > is this test redundant? testing with both an odd and even # of elements because a bad reverse algorithm might mess up one case. https://codereview.chromium.org/1194643002/diff/60001/tests/html/js_array_tes... tests/html/js_array_test.dart:309: expect(list[i], equals(copy[i])); On 2015/06/19 14:57:04, terry wrote: > Should test for list[3] === copy[3] and list[4] === copy[4] be done? To test > that shallow copy happened? Done. https://codereview.chromium.org/1194643002/diff/60001/tests/html/js_array_tes... tests/html/js_array_test.dart:324: }); On 2015/06/19 14:57:04, terry wrote: > What about testing slicing from end e.g., slice[-2]? > > > > Done. https://codereview.chromium.org/1194643002/diff/60001/tests/html/js_array_tes... tests/html/js_array_test.dart:351: // context.callMethod('setValue', [list, -1, "foo"]); // Not a valid array index On 2015/06/19 14:57:04, terry wrote: > Would callJsMethod(list, 'splice', [-1, 0, "foo"]) work? Or is that a problem > too? that would work fine. the issue is just that we are not reusing the JS wrappers each time you go back and forth between Dart and JS. https://codereview.chromium.org/1194643002/diff/60001/tests/html/js_array_tes... tests/html/js_array_test.dart:387: var ret = context.callMethod("concatValues", [list]); On 2015/06/19 14:57:04, terry wrote: > Should comment like > > expected ret is ["1", "2", "a", "b", ["c", "d"], 42] and concat flattens the > returned array > > be added? > > What about have an object in the list that's concatenated? Done. https://codereview.chromium.org/1194643002/diff/60001/tests/html/js_array_tes... tests/html/js_array_test.dart:400: var ret = context.callMethod("concatOntoArray", [list]); On 2015/06/19 14:57:04, terry wrote: > Should comment like > > expected ret to be [1,2,3, "a", "b", "foo"] > > be added? Done. https://codereview.chromium.org/1194643002/diff/60001/tests/html/js_array_tes... tests/html/js_array_test.dart:413: expect(context.callMethod("everyGreaterThanZero", [[1, 0]]), isFalse); On 2015/06/19 14:57:04, terry wrote: > What if empty list? Done. https://codereview.chromium.org/1194643002/diff/60001/tests/html/js_test.html File tests/html/js_test.html (right): https://codereview.chromium.org/1194643002/diff/60001/tests/html/js_test.html... tests/html/js_test.html:4: <script src="js_test.dart" type="application/dart"></script> ignore this file. Wasn't supposed to be committed. https://codereview.chromium.org/1194643002/diff/60001/tests/html/js_typed_int... File tests/html/js_typed_interop_test.dart (right): https://codereview.chromium.org/1194643002/diff/60001/tests/html/js_typed_int... tests/html/js_typed_interop_test.dart:131: expect(() => registerJsInterfaces([Baz]), throws); On 2015/06/19 14:57:04, terry wrote: > Comment that registerJsInterfaces is internal check? Done.
ptal https://codereview.chromium.org/1194643002/diff/60001/tests/html/js_array_tes... File tests/html/js_array_test.dart (right): https://codereview.chromium.org/1194643002/diff/60001/tests/html/js_array_tes... tests/html/js_array_test.dart:38: this.dartArray = obj; On 2015/06/19 14:57:04, terry wrote: > What does this do? good catch... nothing! I was debugging and forgot to remove it. https://codereview.chromium.org/1194643002/diff/60001/tests/html/js_array_tes... tests/html/js_array_test.dart:195: expect(list.length, equals(2)); On 2015/06/19 14:57:04, terry wrote: > I know these tests aren't enabled yet. What about testing that first is "a" and > last is "b" may [3] is null after pruning the list to 2 entries. added thta test. btw these tests are actually running on my local machine so they are "real" https://codereview.chromium.org/1194643002/diff/60001/tests/html/js_array_tes... tests/html/js_array_test.dart:206: // Make sure we are coercing to a JS number. On 2015/06/19 14:57:04, terry wrote: > After the setLength failures should list.length be checked that its still 4? good idea. done. https://codereview.chromium.org/1194643002/diff/60001/tests/html/js_array_tes... tests/html/js_array_test.dart:224: }); On 2015/06/19 14:57:04, terry wrote: > What about joining 2 real lists e.g., > > expect(callJsMethod(list, "join", listWithDartClasses), > equals("3,42,foo,3,Instance of 'Foo',42,foo,Instance of 'Object'")); Are you thinking about the concat method? join on a JS array generates a string that is essentially the result of calling toString on each array element and adding the separator between each element. https://codereview.chromium.org/1194643002/diff/60001/tests/html/js_array_tes... tests/html/js_array_test.dart:255: expect(list.length, equals(0)); On 2015/06/19 14:57:04, terry wrote: > Pop once more when there's no items in the list? good idea. done. https://codereview.chromium.org/1194643002/diff/60001/tests/html/js_array_tes... tests/html/js_array_test.dart:255: expect(list.length, equals(0)); On 2015/06/19 14:57:04, terry wrote: > Pop once more when there's no items in the list? added and fixed behavior for that case to match JS. https://codereview.chromium.org/1194643002/diff/60001/tests/html/js_array_tes... tests/html/js_array_test.dart:263: expect(callPush(list, "bar"), equals(2)); On 2015/06/19 14:57:04, terry wrote: > May push a 'new Foo()' or 'new DivElement()' instead of all strings? Done. https://codereview.chromium.org/1194643002/diff/60001/tests/html/js_array_tes... tests/html/js_array_test.dart:291: var list = [3, 42, "foo"]; On 2015/06/19 14:57:04, terry wrote: > Did you mean to have foo or div object in the list? Done. https://codereview.chromium.org/1194643002/diff/60001/tests/html/js_array_tes... tests/html/js_array_test.dart:294: list = [3, 42]; On 2015/06/19 14:57:04, terry wrote: > is this test redundant? testing with both an odd and even # of elements because a bad reverse algorithm might mess up one case. https://codereview.chromium.org/1194643002/diff/60001/tests/html/js_array_tes... tests/html/js_array_test.dart:309: expect(list[i], equals(copy[i])); On 2015/06/19 14:57:04, terry wrote: > Should test for list[3] === copy[3] and list[4] === copy[4] be done? To test > that shallow copy happened? Done. https://codereview.chromium.org/1194643002/diff/60001/tests/html/js_array_tes... tests/html/js_array_test.dart:324: }); On 2015/06/19 14:57:04, terry wrote: > What about testing slicing from end e.g., slice[-2]? > > > > Done. https://codereview.chromium.org/1194643002/diff/60001/tests/html/js_array_tes... tests/html/js_array_test.dart:387: var ret = context.callMethod("concatValues", [list]); On 2015/06/19 14:57:04, terry wrote: > Should comment like > > expected ret is ["1", "2", "a", "b", ["c", "d"], 42] and concat flattens the > returned array > > be added? > > What about have an object in the list that's concatenated? Done. https://codereview.chromium.org/1194643002/diff/60001/tests/html/js_array_tes... tests/html/js_array_test.dart:400: var ret = context.callMethod("concatOntoArray", [list]); On 2015/06/19 14:57:04, terry wrote: > Should comment like > > expected ret to be [1,2,3, "a", "b", "foo"] > > be added? Done. https://codereview.chromium.org/1194643002/diff/60001/tests/html/js_array_tes... tests/html/js_array_test.dart:413: expect(context.callMethod("everyGreaterThanZero", [[1, 0]]), isFalse); On 2015/06/19 14:57:04, terry wrote: > What if empty list? Done. https://codereview.chromium.org/1194643002/diff/60001/tests/html/js_test.html File tests/html/js_test.html (right): https://codereview.chromium.org/1194643002/diff/60001/tests/html/js_test.html... tests/html/js_test.html:4: <script src="js_test.dart" type="application/dart"></script> ignore this file. Wasn't supposed to be committed. https://codereview.chromium.org/1194643002/diff/60001/tests/html/js_typed_int... File tests/html/js_typed_interop_test.dart (right): https://codereview.chromium.org/1194643002/diff/60001/tests/html/js_typed_int... tests/html/js_typed_interop_test.dart:131: expect(() => registerJsInterfaces([Baz]), throws); On 2015/06/19 14:57:04, terry wrote: > Comment that registerJsInterfaces is internal check? Done.
https://codereview.chromium.org/1194643002/diff/80001/sdk/lib/js/dartium/js_d... File sdk/lib/js/dartium/js_dartium.dart (right): https://codereview.chromium.org/1194643002/diff/80001/sdk/lib/js/dartium/js_d... sdk/lib/js/dartium/js_dartium.dart:96: // to false in unchecked mode. That is still the default mode (unchecked). https://codereview.chromium.org/1194643002/diff/80001/sdk/lib/js/dartium/js_d... sdk/lib/js/dartium/js_dartium.dart:103: final Set<Type> _jsInterfaceTypes = new Set<Type>(); could drop the lhs type here. https://codereview.chromium.org/1194643002/diff/80001/sdk/lib/js/dartium/js_d... sdk/lib/js/dartium/js_dartium.dart:104: Iterable<Type> get jsInterfaceTypes => _jsInterfaceTypes.toList(); Do you need toList? I think a Set is an Iterable already. https://codereview.chromium.org/1194643002/diff/80001/sdk/lib/js/dartium/js_d... sdk/lib/js/dartium/js_dartium.dart:109: bool _checkType(obj, type) { This could be static. Is type a Type or a TypeMirror? Annotate? https://codereview.chromium.org/1194643002/diff/80001/sdk/lib/js/dartium/js_d... sdk/lib/js/dartium/js_dartium.dart:114: bool _checkReturnType(value) { Comment on what this function does? https://codereview.chromium.org/1194643002/diff/80001/sdk/lib/js/dartium/js_d... sdk/lib/js/dartium/js_dartium.dart:135: // TODO(jacobr): actually check method types against the function type returned line len https://codereview.chromium.org/1194643002/diff/80001/sdk/lib/js/dartium/js_d... sdk/lib/js/dartium/js_dartium.dart:146: var endPositional = math.min(parameters.length, positionalArguments.length); I don't think you need math.min - you already compared in line 142. https://codereview.chromium.org/1194643002/diff/80001/sdk/lib/js/dartium/js_d... sdk/lib/js/dartium/js_dartium.dart:156: var startPositional; startNamed? https://codereview.chromium.org/1194643002/diff/80001/sdk/lib/js/dartium/js_d... sdk/lib/js/dartium/js_dartium.dart:166: for (var j = startPositional; j < parameters.length; j++) { Perhaps worth making a HashSet to avoid the n^2? We have APIs with a bunch of named params. Or inverting this, walking over parameters, querying invocation, and making sure you match invocation.namedArguments.size keys? https://codereview.chromium.org/1194643002/diff/80001/sdk/lib/js/dartium/js_d... sdk/lib/js/dartium/js_dartium.dart:182: for (var member in _members) { Don't you need a name match? And, if so, _members could be a map keyed by name? https://codereview.chromium.org/1194643002/diff/80001/sdk/lib/js/dartium/js_d... sdk/lib/js/dartium/js_dartium.dart:203: Map<Symbol, List<mirrors.DeclarationMirror>> allMembers; Is this used? https://codereview.chromium.org/1194643002/diff/80001/sdk/lib/js/dartium/js_d... sdk/lib/js/dartium/js_dartium.dart:214: if (!declaration.isConst && !declaration.isFinal) treatAsSetter = braces for multiline if https://codereview.chromium.org/1194643002/diff/80001/sdk/lib/js/dartium/js_d... sdk/lib/js/dartium/js_dartium.dart:221: _allowedMethods This looks redundant with the code below also updating these sets.
ptal https://codereview.chromium.org/1194643002/diff/80001/sdk/lib/js/dartium/js_d... File sdk/lib/js/dartium/js_dartium.dart (right): https://codereview.chromium.org/1194643002/diff/80001/sdk/lib/js/dartium/js_d... sdk/lib/js/dartium/js_dartium.dart:96: // to false in unchecked mode. On 2015/06/22 22:27:48, vsm wrote: > That is still the default mode (unchecked). switched this to an actual ugly test for checked mode. https://codereview.chromium.org/1194643002/diff/80001/sdk/lib/js/dartium/js_d... sdk/lib/js/dartium/js_dartium.dart:103: final Set<Type> _jsInterfaceTypes = new Set<Type>(); On 2015/06/22 22:27:48, vsm wrote: > could drop the lhs type here. Done. https://codereview.chromium.org/1194643002/diff/80001/sdk/lib/js/dartium/js_d... sdk/lib/js/dartium/js_dartium.dart:104: Iterable<Type> get jsInterfaceTypes => _jsInterfaceTypes.toList(); On 2015/06/22 22:27:48, vsm wrote: > Do you need toList? I think a Set is an Iterable already. Done. https://codereview.chromium.org/1194643002/diff/80001/sdk/lib/js/dartium/js_d... sdk/lib/js/dartium/js_dartium.dart:109: bool _checkType(obj, type) { On 2015/06/22 22:27:48, vsm wrote: > This could be static. Is type a Type or a TypeMirror? Annotate? Done. https://codereview.chromium.org/1194643002/diff/80001/sdk/lib/js/dartium/js_d... sdk/lib/js/dartium/js_dartium.dart:114: bool _checkReturnType(value) { On 2015/06/22 22:27:48, vsm wrote: > Comment on what this function does? Done. https://codereview.chromium.org/1194643002/diff/80001/sdk/lib/js/dartium/js_d... sdk/lib/js/dartium/js_dartium.dart:135: // TODO(jacobr): actually check method types against the function type returned On 2015/06/22 22:27:48, vsm wrote: > line len Done. https://codereview.chromium.org/1194643002/diff/80001/sdk/lib/js/dartium/js_d... sdk/lib/js/dartium/js_dartium.dart:146: var endPositional = math.min(parameters.length, positionalArguments.length); On 2015/06/22 22:27:48, vsm wrote: > I don't think you need math.min - you already compared in line 142. Done. https://codereview.chromium.org/1194643002/diff/80001/sdk/lib/js/dartium/js_d... sdk/lib/js/dartium/js_dartium.dart:156: var startPositional; On 2015/06/22 22:27:48, vsm wrote: > startNamed? Done. https://codereview.chromium.org/1194643002/diff/80001/sdk/lib/js/dartium/js_d... sdk/lib/js/dartium/js_dartium.dart:166: for (var j = startPositional; j < parameters.length; j++) { On 2015/06/22 22:27:48, vsm wrote: > Perhaps worth making a HashSet to avoid the n^2? We have APIs with a bunch of > named params. Or inverting this, walking over parameters, querying invocation, > and making sure you match invocation.namedArguments.size keys? These are just named parameters in JS implemented API which I don't expect there will be that many. However, added A TODO with your suggestion. If many methods have >10 named arguments I expect there will be a significant performance difference. At one point micro benchmarks for C++ STL showed that for <20, vector was actually faster than set. No idea what it is for Dart but I still expect for typical <=3 named parameters methods it won't matter that much. https://codereview.chromium.org/1194643002/diff/80001/sdk/lib/js/dartium/js_d... sdk/lib/js/dartium/js_dartium.dart:182: for (var member in _members) { On 2015/06/22 22:27:48, vsm wrote: > Don't you need a name match? And, if so, _members could be a map keyed by name? This is within a DeclarationSet which is the set of all methods with the same name. Added a top level class comment. https://codereview.chromium.org/1194643002/diff/80001/sdk/lib/js/dartium/js_d... sdk/lib/js/dartium/js_dartium.dart:203: Map<Symbol, List<mirrors.DeclarationMirror>> allMembers; On 2015/06/22 22:27:48, vsm wrote: > Is this used? Done. https://codereview.chromium.org/1194643002/diff/80001/sdk/lib/js/dartium/js_d... sdk/lib/js/dartium/js_dartium.dart:214: if (!declaration.isConst && !declaration.isFinal) treatAsSetter = On 2015/06/22 22:27:48, vsm wrote: > braces for multiline if Done. https://codereview.chromium.org/1194643002/diff/80001/sdk/lib/js/dartium/js_d... sdk/lib/js/dartium/js_dartium.dart:221: _allowedMethods On 2015/06/22 22:27:48, vsm wrote: > This looks redundant with the code below also updating these sets. Done.
lgtm https://codereview.chromium.org/1194643002/diff/60001/tests/html/js_array_tes... File tests/html/js_array_test.dart (right): https://codereview.chromium.org/1194643002/diff/60001/tests/html/js_array_tes... tests/html/js_array_test.dart:224: }); On 2015/06/19 20:39:44, Jacob wrote: > On 2015/06/19 14:57:04, terry wrote: > > What about joining 2 real lists e.g., > > > > expect(callJsMethod(list, "join", listWithDartClasses), > > equals("3,42,foo,3,Instance of 'Foo',42,foo,Instance of 'Object'")); > > Are you thinking about the concat method? join on a JS array generates a string > that is essentially the result of calling toString on each array element and > adding the separator between each element. Your right join is not concat.
lgtm https://codereview.chromium.org/1194643002/diff/100001/sdk/lib/js/dartium/js_... File sdk/lib/js/dartium/js_dartium.dart (right): https://codereview.chromium.org/1194643002/diff/100001/sdk/lib/js/dartium/js_... sdk/lib/js/dartium/js_dartium.dart:96: final bool _checkJsInvocations = (() { I'd be tempted to just make this unconditionally true and just always do the checks below. Do we get any value out of skipping them in non-checked mode? https://codereview.chromium.org/1194643002/diff/100001/tests/html/js_array_te... File tests/html/js_array_test.dart (right): https://codereview.chromium.org/1194643002/diff/100001/tests/html/js_array_te... tests/html/js_array_test.dart:122: return array.reduce(function(previousValue, currentValue) { return previousValue + currentValue*2; }, 0); line len here and a few places below https://codereview.chromium.org/1194643002/diff/100001/tests/html/js_array_te... tests/html/js_array_test.dart:539: expect(context.callMethod("getOwnPropertyDescriptorJson", [listView, "length"]), equals("null")); line len
Message was sent while issue was closed.
Committed patchset #7 (id:120001) manually as 3fdeb669ee40eead097009e59dfb5adcb414178e (presubmit successful). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
