Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dart2js dart:html constructors taking Dictionary are not handled correctly. #7030

Closed
DartBot opened this issue Nov 29, 2012 · 8 comments
Closed
Assignees
Labels
Milestone

Comments

@DartBot
Copy link

@DartBot DartBot commented Nov 29, 2012

This issue was originally filed by sami.ylonen...@gmail.com


What steps will reproduce the problem?

  1. Create app that uses WebRTC RTCPeerConnection.
  2. Compile to javascript
  3. See it fail with DOM exception

What is the expected output? What do you see instead?
Creating new RTCPeerConnection with literal map as parameter causes DOM exception when run as javascript.

dart code: new RTCPeerConnection({'iceServers': [ {'url':'stun:stun.l.google.com:19302'}]});

javascript code that fails: $.RTCPeerConnection_RTCPeerConnection($.makeLiteralMap(["iceServers", [$.makeLiteralMap(["url", "stun:stun.l.google.com:19302"])]]), $);

manually fixed javascript code that works: $.RTCPeerConnection_RTCPeerConnection({'iceServers': [{'url': 'stun:stun.l.google.com:19302'}]}, $);

What version of the product are you using? On what operating system?
Dart Editor version 0.2.6_r15355
Dart SDK version 0.2.6.0_r15355

Please provide any additional information below.
Same issue with other classes like RTCIceCandidate and RTCSessionDescription

@anders-sandholm
Copy link
Contributor

@anders-sandholm anders-sandholm commented Nov 30, 2012

Added Area-Dart2JS, Triaged labels.

@peter-ahe-google
Copy link
Contributor

@peter-ahe-google peter-ahe-google commented Nov 30, 2012

Stephen, please bump it to a later milestone if you like.


Set owner to @rakudrama.
Added this to the M2 milestone.

@kasperl
Copy link
Contributor

@kasperl kasperl commented Nov 30, 2012

Looks like a DOM issue to me. Shouldn't this be in Area-HTML?

@peter-ahe-google
Copy link
Contributor

@peter-ahe-google peter-ahe-google commented Nov 30, 2012

I figured Stephen would know :-)

@rakudrama
Copy link
Member

@rakudrama rakudrama commented Nov 30, 2012

I will leave the decision to blois@ whether the quick fix is M2 or M3.
The comprehensive fix is too complex for M2.

There are three problems here:

  1. Our automatic generation of conversions is only applied to methods, not constructors.
  2. Our automatic generation of constructors is not preserving the number of arguments.
  3. Even if (1) was implemented, the Dictionaries here are structured, containing lists and further dictionaries; our default handling of dictionaries is flat, assuming the first level values have primitive types.

The quickest way to fix this is to write a custom constructor, and use the serialized script value conversion - it will be close enough. Fix all the classes listed below.

+1 extra credit: fix the dictionary conversion to convert sublists and subdictionaries.

+10 extra credit: make the generator handle this correctly. It should dispatch on the number of types and do the conversions. This is pretty much like a static member function with funky syntax.

+100 extra credit: the compiler should recognize Dart literals that are being converted to JavaScript and convert them at compile time.

There are four classes with this specific problem, and one with a related problem (Intent's constructor takes a SerializedScriptValue).

Modules/intents/Intent.idl:30: Constructor(in DOMString action, in DOMString type, in [Optional=DefaultIsNullString, TransferList=transferList] SerializedScriptValue data, in [Optional=DefaultIsUndefined] Array transferList),
Modules/mediastream/RTCIceCandidate.idl:33: Constructor(in Dictionary dictionary),
Modules/mediastream/RTCPeerConnection.idl:34: Constructor(in Dictionary rtcIceServers, in [Optional] Dictionary mediaConstraints),
Modules/mediastream/RTCSessionDescription.idl:33: Constructor(in Dictionary dictionary),
Modules/notifications/Notification.idl:37: Constructor(in DOMString title, in [Optional=DefaultIsUndefined] Dictionary options),:


Set owner to @blois.
Removed Area-Dart2JS label.
Added Area-HTML label.
Changed the title to: "dart2js dart:html constructors taking Dictionary are not handled correctly.".

@blois
Copy link

@blois blois commented Dec 11, 2012

Removed this from the M2 milestone.
Added this to the M3 milestone.

@blois
Copy link

@blois blois commented Feb 6, 2013

cc @efortuna.
Set owner to @efortuna.

@efortuna
Copy link
Contributor

@efortuna efortuna commented Feb 11, 2013

The first version of the fix described was implemented and committed in r18320 https://codereview.chromium.org//12217089. Further improvements can be opened as separate bugs.


Added Fixed label.

@DartBot DartBot added this to the M3 milestone Feb 11, 2013
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
7 participants
You can’t perform that action at this time.