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

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:
1 year ago by vsm
Modified:
1 year 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) Lint Patch
M lib/dart_interop.js View 1 2 7 chunks +52 lines, -8 lines 0 comments ? errors Download
M lib/js.dart View 1 2 12 chunks +88 lines, -36 lines 4 comments ? errors Download
M test/js/browser_tests.dart View 1 2 3 chunks +45 lines, -0 lines 0 comments ? errors Download
M test/js/browser_tests_bootstrap.js View 1 2 1 chunk +14 lines, -0 lines 0 comments ? errors Download
Trybot results:
Commit:

Messages

Total messages: 10
vsm
1 year ago #1
kevmoo-old
On 2013/03/25 17:18:04, vsm wrote: 2 questions. 1) Do all tests pass w/ minify w/ ...
1 year ago #2
vsm
On 2013/03/25 17:21:16, kevmoo wrote: > On 2013/03/25 17:18:04, vsm wrote: > > 2 questions. ...
1 year ago #3
alexandre.ardhuin
LGTM for function binding. For modifications on FunctionProxy, I'm like you Vijay - not really ...
1 year ago #4
vsm
Please take another look. I had to redo the function binding. My initial solution was ...
1 year ago #5
kasperl
I actually find using proxy['foo'] fairly attractive compared to using proxy.foo. It's trivial to minify ...
1 year ago #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 ...
1 year ago #7
vsm
Committed patchset #3 manually as re561ab6 (presubmit successful).
1 year ago #8
alexandre.ardhuin
On 2013/04/07 05:15:34, kasperl wrote: > I actually find using proxy['foo'] fairly attractive compared to ...
1 year ago #9
vsm
1 year ago #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 1280:2d3e6564b7b6