767926 - Implement unions as member types of sequences or dictionaries for WebIDL

Status: RESOLVED FIXED

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

Comment 1

[Brendan Dahl [:bdahl]]

This is needed to implement an experimental mask API for canvas.  Example can be seen in http://lists.w3.org/Archives/Public/public-whatwg-archive/2013Jan/0325.html where  CanvasImageSource is (HTMLImageElement or HTMLVideoElement or HTMLCanvasElement or CanvasRenderingContext2D or ImageBitmap).

Work for this bug is going on in bug 829803.

Comment 3

[Boris Zbarsky [:bzbarsky]]

When this is fixed, we should grep our IDL for references to this bug and fix them.

Comment 4

[David Zbarsky (:dzbarsky)]

Attachment: Patch

Comment 5

[David Zbarsky (:dzbarsky)]

Attachment: Patch

This doesn't handle JSObject* in a union in a dictionary yet, and we're going to need to rename things at some point.  I'll do those as followups.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 793493 [details] [diff] [review]
Patch

>+                elif f.isDictionary():

I don't understand this part.  We already had an f.isDictionary() case...

>+               "%s.%s" % (value, CGDictionary.makeMemberName(str(member))))

Why does that make sense for a union?

In any case, this wrapping bit seems wrong: it'll try to wrap all the values one after another, when it should only wrap the one value that the union actually has...

>+                        methods.append(ClassMethod("SetStringData", "void",

The indent of the later arguments here is wrong, no?  I think you want a linebreak before ClassMethod here.

>+                            isInUnionReturnValue=True,

This could use some documentation.  Why is that a sensible thing to do for a dictionary?

Comment 7

[Boris Zbarsky [:bzbarsky]]

Oh, also, what about sequences?  The patch removes the isMember checks completely, but doesn't seem to do the sequence bits...

Comment 8

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

I know this is r- but, is a newer patch available? I tried to rebase it, but I couldn't get it working. Got this without any additions to the tree:

  WebIDL.WebIDLError: error: Cannot coerce type String to type FloatOrString.,
  TestCodeGen.webidl line 712:23
    (float or DOMString) floatOrString = "str";

fwiw

Comment 9

[Peter Van der Beken [:peterv]]

That looks like a separate bug, coerceToType needs to be fixed to deal with unions.

Comment 10

[Boris Zbarsky [:bzbarsky]]

This patch depends on the patch in bug 903277.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Though it's not like it applies on top of current tip with that bug fixed...

Also, I'm not happy with the header situation from this patch.

I think we should strongly consider making the union ctors/dtors non-inline if they'd otherwise force us to over-include or something.  We could do this on a struct-by-struct basis now that we're using CGClass for unions.

Comment 12

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Patch rebased (no other changes)

> This patch depends on the patch in bug 903277.

Thanks, with that I got it to work. Uploading same patch rebased to tip + 905392 + 903277

Comment 13

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Hmm, even with all patches applied (905392.patch, union-defaults, union-in-dict-rebased & constraints_webidl from bug 882145) I still can't un-comment this part:

dictionary MediaStreamConstraints {
    boolean fake = false;
//  (boolean or MediaTrackConstraints) video = false;
//  (boolean or MediaTrackConstraints) audio = false;
//  (boolean or MediaTrackConstraints) picture = false;
};

without having it fail:

 Traceback (most recent call last):
   File ".../config/pythonpath.py", line 56, in <module>
     main(sys.argv[1:])
   File ".../config/pythonpath.py", line 48, in main
     execfile(script, frozenglobals)
   File ".../dom/bindings/GlobalGen.py", line 81, in <module>
     main()
   File ".../dom/bindings/GlobalGen.py", line 76, in main
     generate_file(config, 'UnionTypes', 'declare')
   File ".../dom/bindings/GlobalGen.py", line 16, in generate_file
     root = getattr(GlobalGenRoots, name)(config)
   File ".../dom/bindings/Codegen.py", line 10536, in UnionTypes
     config)
   File ".../dom/bindings/Codegen.py", line 841, in UnionTypes
     map(addInfoForType, getAllTypes(descriptors, dictionaries, callbacks))
   File ".../dom/bindings/Codegen.py", line 808, in addInfoForType
     unionStructs[name] = CGUnionStruct(t, providers[0])
   File ".../dom/bindings/Codegen.py", line 6213, in __init__
     self.struct = self.getStruct()
   File ".../dom/bindings/Codegen.py", line 6248, in getStruct
     isReturnValue=self.isReturnValue)
   File ".../dom/bindings/Codegen.py", line 6132, in getUnionTypeTemplateVars
     raise TypeError("Can't handle dictionaries or sequences in unions")
 TypeError: Can't handle dictionaries or sequences in unions
 make[6]: *** [ParserResults.pkl] Error 1

Is something else required?

Comment 14

[Boris Zbarsky [:bzbarsky]]

Well, the patch doesn't remove that check-and-throw....

Comment 15

[David Zbarsky (:dzbarsky)]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #13)
> Hmm, even with all patches applied (905392.patch, union-defaults,
> union-in-dict-rebased & constraints_webidl from bug 882145) I still can't
> un-comment this part:
> 
> dictionary MediaStreamConstraints {
>     boolean fake = false;
> //  (boolean or MediaTrackConstraints) video = false;
> //  (boolean or MediaTrackConstraints) audio = false;
> //  (boolean or MediaTrackConstraints) picture = false;
> };
> 
> without having it fail:
> 
>  Traceback (most recent call last):
>    File ".../config/pythonpath.py", line 56, in <module>
>      main(sys.argv[1:])
>    File ".../config/pythonpath.py", line 48, in main
>      execfile(script, frozenglobals)
>    File ".../dom/bindings/GlobalGen.py", line 81, in <module>
>      main()
>    File ".../dom/bindings/GlobalGen.py", line 76, in main
>      generate_file(config, 'UnionTypes', 'declare')
>    File ".../dom/bindings/GlobalGen.py", line 16, in generate_file
>      root = getattr(GlobalGenRoots, name)(config)
>    File ".../dom/bindings/Codegen.py", line 10536, in UnionTypes
>      config)
>    File ".../dom/bindings/Codegen.py", line 841, in UnionTypes
>      map(addInfoForType, getAllTypes(descriptors, dictionaries, callbacks))
>    File ".../dom/bindings/Codegen.py", line 808, in addInfoForType
>      unionStructs[name] = CGUnionStruct(t, providers[0])
>    File ".../dom/bindings/Codegen.py", line 6213, in __init__
>      self.struct = self.getStruct()
>    File ".../dom/bindings/Codegen.py", line 6248, in getStruct
>      isReturnValue=self.isReturnValue)
>    File ".../dom/bindings/Codegen.py", line 6132, in getUnionTypeTemplateVars
>      raise TypeError("Can't handle dictionaries or sequences in unions")
>  TypeError: Can't handle dictionaries or sequences in unions
>  make[6]: *** [ParserResults.pkl] Error 1
> 
> Is something else required?

That's bug 767924.

Comment 16

[David Zbarsky (:dzbarsky)]

(In reply to Boris Zbarsky [:bz] from comment #6)
> Comment on attachment 793493 [details] [diff] [review]
> Patch


> >+               "%s.%s" % (value, CGDictionary.makeMemberName(str(member))))
> 
> Why does that make sense for a union?
> 
> In any case, this wrapping bit seems wrong: it'll try to wrap all the values
> one after another, when it should only wrap the one value that the union
> actually has...
> 

It's hard to tell what the right thing to do here is because we never generate this code.

(In reply to Boris Zbarsky [:bz] from comment #7)
> Oh, also, what about sequences?  The patch removes the isMember checks
> completely, but doesn't seem to do the sequence bits...

What do we need to do for sequences?  Here's the code we generate with this patch:

  if (!isNull && !temp.ref().isUndefined()) {
    mOurSequence10.Construct();
    if (temp.ref().isObject()) {
      JS::Rooted<JSObject*> seq(cx, &temp.ref().toObject());

      if (!IsArrayLike(cx, seq)) {
        ThrowErrorMessage(cx, MSG_NOT_SEQUENCE, "'ourSequence10' member of DictContainingSequence");
        return false;
      }
      uint32_t length;
      // JS_GetArrayLength actually works on all objects
      if (!JS_GetArrayLength(cx, seq, &length)) {
        return false;
      }
      Sequence<FloatOrStringReturnValue > &arr = (mOurSequence10.Value());
      if (!arr.SetCapacity(length)) {
        JS_ReportOutOfMemory(cx);
        return false;
      }
      for (uint32_t i = 0; i < length; ++i) {
        JS::Rooted<JS::Value> temp(cx);
        if (!JS_GetElement(cx, seq, i, &temp)) {
          return false;
        }
        FloatOrStringReturnValue& slot = *arr.AppendElement();
        {
          bool done = false, failed = false, tryNext;
          do {
            if (temp.isNumber()) {
              done = (failed = !slot.TrySetToFloat(cx, temp, &temp, tryNext)) || !tryNext;
              break;
            }
            done = (failed = !slot.TrySetToString(cx, temp, &temp, tryNext)) || !tryNext;
            break;
          } while (0);
          if (failed) {
            return false;
          }
          if (!done) {
            ThrowErrorMessage(cx, MSG_NOT_IN_UNION, "Element of 'ourSequence10' member of DictContainingSequence", "");
            return false;
          }
        }
      }
    } else {
      ThrowErrorMessage(cx, MSG_NOT_SEQUENCE, "'ourSequence10' member of DictContainingSequence");
      return false;
    }
  }

Comment 17

[David Zbarsky (:dzbarsky)]

Attachment: Updated patch

I took a guess about how the unwrapping code should look.  Maybe it should be accessing mUnion in there.  The reason that we don't need to worry about it for now is that we don't allow JSObject in strong unions.

Comment 18

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

(In reply to David Zbarsky (:dzbarsky) from comment #15)
> That's bug 767924.

I tried working around it with "(boolean or object)":

  dictionary MediaStreamConstraints {
    boolean fake = false;
    boolean audio = false;
->  (boolean or object) video;
  };

Gives me this with updated patch:

Traceback (most recent call last):
  File "/Users/Jan/moz/mozilla-central/config/pythonpath.py", line 56, in <module>
    main(sys.argv[1:])
  File "/Users/Jan/moz/mozilla-central/config/pythonpath.py", line 48, in main
    execfile(script, frozenglobals)
  File "/Users/Jan/moz/mozilla-central/dom/bindings/BindingGen.py", line 89, in <module>
    main()
  File "/Users/Jan/moz/mozilla-central/dom/bindings/BindingGen.py", line 86, in main
    generate_binding_files(config, outputPrefix, srcPrefix, webIDLFile);
  File "/Users/Jan/moz/mozilla-central/dom/bindings/BindingGen.py", line 21, in generate_binding_files
    root = CGBindingRoot(config, outputprefix, webidlfile)
  File "/Users/Jan/moz/mozilla-central/dom/bindings/Codegen.py", line 8834, in __init__
    lambda d: d.identifier.name)])
  File "/Users/Jan/moz/mozilla-central/dom/bindings/Codegen.py", line 8136, in __init__
    self.structs = self.getStructs()
  File "/Users/Jan/moz/mozilla-central/dom/bindings/Codegen.py", line 8313, in getStructs
    methods.append(self.traceDictionaryMethod())
  File "/Users/Jan/moz/mozilla-central/dom/bindings/Codegen.py", line 8288, in traceDictionaryMethod
    if typeNeedsRooting(m.type, self.descriptorProvider)]
  File "/Users/Jan/moz/mozilla-central/dom/bindings/Codegen.py", line 8485, in getMemberTrace
    assert 0 # unknown type
AssertionError

Comment 19

[David Zbarsky (:dzbarsky)]

Right.  That doesn't work because we currently don't support having object in an owning union.

Comment 20

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

(In reply to David Zbarsky (:dzbarsky) from comment #19)
> Right.  That doesn't work because we currently don't support having object
> in an owning union.

Is there a bug for that?

Comment 21

[Boris Zbarsky [:bzbarsky]]

In the special case of an owning union that is a member, we should add support for object.  It's clear how it should work, unlike the standalone union case: just need to have the thing the union is a member of trace the object.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 799124 [details] [diff] [review]
Updated patch

Can you wrap a scope around the "str" bit so that we know it won't collide with other stuff?  Or at least document why this won't be a problem (e.g. if it always ends up wrapped in other scopes)?

>+               "%s.%s" % (value, CGDictionary.makeMemberName(str(member))))

Again, why are we using CGDictionary.makeMemberName to determine the name of _union_ member?  Shouldn't we use the same algorithm as whatever declares the union member?

>+                memberWrap = CGWrapper(memberWrap,
>+                                       pre="if (mType == %s) {\n" % member,
>+                                       post="}")

CGIfWrapper, please?

>+                              inline=False, bodyInHeader=not isReturnValue,

Shouldn't those both be not isReturnValue?

>+                            # Set this to true so that we get an owning union.

Maybe a followup to rename the boolean?

The sequence codegen bits look good, thank you.

r=me with the above nits fixed.

Comment 23

[David Zbarsky (:dzbarsky)]

(In reply to Boris Zbarsky [:bz] from comment #22)
> Comment on attachment 799124 [details] [diff] [review]
> Updated patch
> 

> 
> Again, why are we using CGDictionary.makeMemberName to determine the name of
> _union_ member?  Shouldn't we use the same algorithm as whatever declares
> the union member?
> 

Aren't union member names determined the same way as dictionary members'?  Perhaps we should move/rename makeMemberName though.

> 
> >+                              inline=False, bodyInHeader=not isReturnValue,
> 
> Shouldn't those both be not isReturnValue?
> 

Yeah, until we get rid of inline or something.

> >+                            # Set this to true so that we get an owning union.
> 
> Maybe a followup to rename the boolean?
> 

Yep, I'll file.

Comment 24

[Boris Zbarsky [:bzbarsky]]

> Aren't union member names determined the same way as dictionary members'? 

Are they?  I just don't know.  I think union member names are gotten by prefixing "m" to something that depends on the type.  Dictionary member names are derived from the name of the dictionary member in the IDL, in a somewhat similar way... but I'd rather not assume identical way.  Better to just factor out the union-member naming code.  At least the code for computing the part after "m".

Comment 25

[David Zbarsky (:dzbarsky)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2fce3bcfb3c8

Comment 26

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/2fce3bcfb3c8

Comment 27

[Henri Sivonen (:hsivonen)]

This looks like a likely suspect for bug 915100.

Comment 28

[Henri Sivonen (:hsivonen)]

Never mind. This change set still builds.