Chromium Code Reviews
Help | Chromium Project | Sign in
(333)

Issue 12457030: [js-interop] Fix function binding and avoid noSuchMethod when using map notation (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 years, 4 months ago by vsm
Modified:
2 years, 3 months ago
CC:
reviews_dartlang.org, erikcorry
Base URL:
https://github.com/dart-lang/js-interop.git@master
Visibility:
Public.

Description

[js-interop] Fix function binding and avoid noSuchMethod when using map notation This change does two things: (1) It avoids noSuchMethod and dartbug.com/9283 when map notation is used. I don't like losing the varargs-like behavior of NSM, so I'm not entirely sure we should land this part. (2) This also binds receivers on returned functions to make a['foo']() and a.foo() equivalent. JS has odd semantics around this. In JS: a['foo']() a is the receiver (i.e., the "this") in the function invocation. But, in the following: var tmp = a['foo']; tmp(); it is not. We don't distinguish between the two in js-interop. This change essentially forces the former semantics over the latter. Note: kevmoo's pop-pop-win app in dartbug.com/9283 works with in dart2js minified with these changes along with the source change in comment #15. Committed: https://github.com/dart-lang/js-interop/commit/e561ab6

Patch Set 1 #

Patch Set 2 : Added comment on NSM removal #

Patch Set 3 : Fix for constructors plus cleanup #

Total comments: 4
Unified diffs Side-by-side diffs Delta from patch set Stats (+199 lines, -44 lines) Patch
M lib/dart_interop.js View 1 2 7 chunks +52 lines, -8 lines 0 comments Download
M lib/js.dart View 1 2 12 chunks +88 lines, -36 lines 4 comments Download
M test/js/browser_tests.dart View 1 2 3 chunks +45 lines, -0 lines 0 comments Download
M test/js/browser_tests_bootstrap.js View 1 2 1 chunk +14 lines, -0 lines 0 comments Download
Trybot results:
Commit: (user is not authenticated) CQ not working?

Messages

Total messages: 10 (0 generated)
vsm
2 years, 4 months ago (2013-03-25 17:18:04 UTC) #1
kevmoo-old
On 2013/03/25 17:18:04, vsm wrote: 2 questions. 1) Do all tests pass w/ minify w/ ...
2 years, 4 months ago (2013-03-25 17:21:16 UTC) #2
vsm
On 2013/03/25 17:21:16, kevmoo wrote: > On 2013/03/25 17:18:04, vsm wrote: > > 2 questions. ...
2 years, 4 months ago (2013-03-25 17:32:00 UTC) #3
alexandre.ardhuin
LGTM for function binding. For modifications on FunctionProxy, I'm like you Vijay - not really ...
2 years, 4 months ago (2013-03-25 22:39:21 UTC) #4
vsm
Please take another look. I had to redo the function binding. My initial solution was ...
2 years, 3 months ago (2013-04-07 03:53:46 UTC) #5
kasperl
I actually find using proxy['foo'] fairly attractive compared to using proxy.foo. It's trivial to minify ...
2 years, 3 months ago (2013-04-07 05:15:34 UTC) #6
alexandre.ardhuin
lgtm https://chromiumcodereview.appspot.com/12457030/diff/8001/lib/js.dart File lib/js.dart (right): https://chromiumcodereview.appspot.com/12457030/diff/8001/lib/js.dart#newcode391 lib/js.dart:391: typeof(message.object) == 'function') { Could typeof(message.object) be something ...
2 years, 3 months ago (2013-04-07 21:20:20 UTC) #7
vsm
Committed patchset #3 manually as re561ab6 (presubmit successful).
2 years, 3 months ago (2013-04-07 21:31:50 UTC) #8
alexandre.ardhuin
On 2013/04/07 05:15:34, kasperl wrote: > I actually find using proxy['foo'] fairly attractive compared to ...
2 years, 3 months ago (2013-04-07 21:32:18 UTC) #9
vsm
2 years, 3 months ago (2013-04-07 21:37:36 UTC) #10
Message was sent while issue was closed.
Thanks for the review!

https://chromiumcodereview.appspot.com/12457030/diff/8001/lib/js.dart
File lib/js.dart (right):

https://chromiumcodereview.appspot.com/12457030/diff/8001/lib/js.dart#newcode391
lib/js.dart:391: typeof(message.object) == 'function') {
It shouldn't.  I'll change this to assert that.

On 2013/04/07 21:20:20, alexandre.ardhuin wrote:
> Could typeof(message.object) be something else that function ?

https://chromiumcodereview.appspot.com/12457030/diff/8001/lib/js.dart#newcode...
lib/js.dart:1001: } else if (member == 'call') {
I'm uncomfortable making an assumptions on minified names based on current
behavior.  I would expect it to change.  Let's see where bug 9283 ends up. 

On 2013/04/07 21:20:20, alexandre.ardhuin wrote:
> If what I have observed on minification of call is something that will not
> change (ie. member looks like "call$1" "call$2"...), you can add the following
> to the if.
> 
> member == 'call' || new RegExp(r"^call\$\d+$").hasMatch(member)
> 
> Thus even with minification of dart2js, the test "call JS function with
> varargs." is passing.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 3ea459f