1027256 - webidl union types with ByteString fail to compile

Status: RESOLVED FIXED

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

Description

[Ben Kelly [:bkelly, not reviewing]]

In bug 1025183 I am basing ScalarValueString off of the current implementation of ByteString as an example.

Unfortunately, there appears to be a bug in the ByteString implementation.  It currently cannot be used in union types.

Adding:

  void passUnionWithByteString((ByteString or long) arg);

To TestCodeGen.webidl results in errors like:

 0:10.32 ../../dist/include/mozilla/dom/UnionTypes.h: In member function ‘void mozilla::dom::OwningByteStringOrLong::SetStringData(const char_type*, nsAString_internal::size_type)’:
 0:10.33 ../../dist/include/mozilla/dom/UnionTypes.h:6536:20: error: ‘RawSetAsString’ was not declared in this scope

The problem appears to be that the union type gets a RawSetAsByteString(), yet because its parser type returns true from isString(), it tries to add the SetStringData() method to the union.  The SetStringData() method always tries to use RawSetAsString() which is not defined in this case.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Add test case for ByteString union type

Here's the test.  Still need to figure out the correct fix for Part 1.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Hmm, I can change Codegen.py to use the write method name:

-                                        body="RawSetAsString().Assign(aData, aLength);\n"))
+                                        body="RawSetAs%s().Assign(aData, aLength);\n" % t.name))

But the argument types are wrong.  A ByteString wants a narrow nsCString, but SetStringData() only has a wide character const nsString::char_type*.

Should I simply avoid generating SetStringData() if isByteString()?

Comment 3

[Boris Zbarsky [:bzbarsky]]

We shouldn't be generating SetStringData for ByteString.  It's only used for unions containing a DOMString (or once it's added a ScalarValueString) with a string-type default value.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Fix SetStringData() to exclude ByteString and otherwise use real type name.

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Add test case for ByteString union type (v1)

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8442360 [details] [diff] [review]
P1 Fix SetStringData() to exclude ByteString and otherwise use real type name.

r=me

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8442361 [details] [diff] [review]
P2 Add test case for ByteString union type (v1)

Please put this inside the ifdef DEBUG block to avoid generating the new union type in release builds, and please add this to TestExampleGen.webidl and TestJSImplGen.webidl as well.  r=me with that, and thank you for adding tests!

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Fix SetStringData() to exclude ByteString and otherwise use real type name. r=bz

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Add test case for ByteString union type. r=bz

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

Since mozilla-inbound is closed and I have to log off shortly, checkin-needed.  Requisite t-style try:

  https://tbpl.mozilla.org/?tree=Try&rev=cb550f1ba270

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/87e3c9420691
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/01fe163eca19

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/87e3c9420691
https://hg.mozilla.org/mozilla-central/rev/01fe163eca19