Closed Bug 1029812 Opened 10 years ago Closed 10 years ago

webidl Sequence and MozMap (OpenEndedDictionary) should be distinguishable

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: bkelly, Assigned: bkelly)

References

Details

Attachments

(1 file, 4 obsolete files)

Currently our implementation of OpenEndedDictionary, MozMap, is not distinguishable from Sequence.  We need this to work, however, for fetch in order to use the two types in a union.
This is what I have so far.  It compiles and passes tests, so not quite sure what else needs to be done.  Boris, on IRC you implied I needed to tweak some other parts, but I didn't quite understand what you wanted.  Figured I'd flag for review and you could clarify in comments if appropriate.  Thanks!
Attachment #8445546 - Flags: review?(bzbarsky)
Hmm, but now I get this error:

  TypeError: Unknown type; we don't know how to wrap it in constructor arguments: ByteStringMozMap

If I add a constructor like:

 NamedConstructor=Test3((OnlyForUseInConstructor or MozMap<ByteString>) arg1)
This seems to fix the issue mentioned in the last comment.  Maybe it should be split into a separate bug, but including it here for now as I need it for the same fetch constructor.
Attachment #8445546 - Attachment is obsolete: true
Attachment #8445546 - Flags: review?(bzbarsky)
Attachment #8445602 - Flags: review?(bzbarsky)
Comment on attachment 8445602 [details] [diff] [review]
Make webidl Sequence and MozMap distinguishable. (v1)

The wrapping changes look good.  I'm a bit worried about exposing the non-const Get(), but ideally it'll only be usable from bindings code since callees get a const ref.

>+++ b/dom/bindings/parser/WebIDL.py

It might be worth making sequence and dictionary distinguishable too, but that can be a followup.  That said, the changes are pretty darned similar...

>+++ b/dom/bindings/parser/tests/test_distinguishability.py
>     setDistinguishable("sequence<long>", nonUserObjects)
>+    setDistinguishable("sequence<long>", mozMaps)

I think I'd prefer:

     setDistinguishable("sequence<long>", nonUserObjects + mozMaps)

and similar for the other bits being added here.

> so not quite sure what else needs to be done.

Two things:

1)  Add tests that test overloading of a sequence and a MozMap.
2)  Examine the generated code for the overload and union cases.

#2 is needed because we don't actually have functional tests for the codegen; just because the generated code compiles doesn't mean it's right.  ;)

I just looked, and it looks like the union case is ok, because it outputs the attempt to set as mozmap after the one for sequence.

But overloads are not OK.  Something like this:

  void overload17(sequence<DOMString> arg);
  void overload17(MozMap<DOMString> arg);

doesn't output an attempt to use the MozMap at all.  And same for something like this:

  void overload18(MozMap<DOMString> arg);
  void overload18(sequence<DOMString> arg);

This is at least partly because I actually didn't correctly fix overload resolution to deal with MozMap; a case for it should have been added to objectSigs in CGMethodCall.__init__.  That's a problem without your patch too.

I _think_ that just adding such a case after the call that adds sequences to objectSigs will do the right thing.

r=me with that fixed.
Attachment #8445602 - Flags: review?(bzbarsky) → review+
Address review feedback.

Based on IRC conversation there is still a problem with ordering of the generated code for the overload18() case.
Attachment #8445602 - Attachment is obsolete: true
Attachment #8445893 - Flags: review+
Attachment #8445893 - Attachment is obsolete: true
Attachment #8445906 - Flags: review?(bzbarsky)
Attachment #8445906 - Attachment is obsolete: true
Attachment #8445906 - Flags: review?(bzbarsky)
Attachment #8445909 - Flags: review?(bzbarsky)
Comment on attachment 8445909 [details] [diff] [review]
Make webidl Sequence and MozMap distinguishable. (v4)

r=me. Thanks!
Attachment #8445909 - Flags: review?(bzbarsky) → review+
Follow-on for dictionaries in bug 1030192.
https://hg.mozilla.org/mozilla-central/rev/5d7d9d0b94a3
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: