Closed
Bug 1027256
Opened 10 years ago
Closed 10 years ago
webidl union types with ByteString fail to compile
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla33
People
(Reporter: bkelly, Assigned: bkelly)
References
Details
Attachments
(2 files, 3 obsolete files)
(deleted),
patch
|
bkelly
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bkelly
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•10 years ago
|
||
Here's the test. Still need to figure out the correct fix for Part 1.
Assignee: nobody → bkelly
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•10 years ago
|
||
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()?
Flags: needinfo?(bzbarsky)
Comment 3•10 years ago
|
||
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.
Flags: needinfo?(bzbarsky)
Assignee | ||
Comment 4•10 years ago
|
||
Attachment #8442360 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 5•10 years ago
|
||
Attachment #8442316 -
Attachment is obsolete: true
Attachment #8442361 -
Flags: review?(bzbarsky)
Comment 6•10 years ago
|
||
Comment on attachment 8442360 [details] [diff] [review]
P1 Fix SetStringData() to exclude ByteString and otherwise use real type name.
r=me
Attachment #8442360 -
Flags: review?(bzbarsky) → review+
Comment 7•10 years ago
|
||
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!
Attachment #8442361 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 8•10 years ago
|
||
Attachment #8442360 -
Attachment is obsolete: true
Attachment #8442384 -
Flags: review+
Assignee | ||
Comment 9•10 years ago
|
||
Attachment #8442361 -
Attachment is obsolete: true
Attachment #8442386 -
Flags: review+
Assignee | ||
Comment 10•10 years ago
|
||
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
Keywords: checkin-needed
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 11•10 years ago
|
||
Comment 12•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/87e3c9420691
https://hg.mozilla.org/mozilla-central/rev/01fe163eca19
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•