Closed
Bug 1075198
Opened 9 years ago
Closed 8 years ago
Generated Constructor code for Events with Uint8Array not working
Categories
(Core :: DOM: Events, defect)
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: dgarnerlee, Assigned: dgarnerlee)
References
Details
Attachments
(3 files, 3 obsolete files)
2.29 KB,
patch
|
Details | Diff | Splinter Review | |
720 bytes,
text/x-csrc
|
Details | |
3.53 KB,
text/plain
|
Details |
Code generation for event WebIDLs with a Uint8Array typed array field will result in a type mismatch compile error: Ex: [Constructor(DOMString type, optional HCIEventTransactionEventInit eventInitDict)] interface HCIEventTransactionEvent: Event { readonly attribute Uint8Array aid; readonly attribute DOMString origin; }; dictionary HCIEventTransactionEventInit: EventInit { Uint8Array aid; DOMString origin = ""; }; This outputs: already_AddRefed<HCIEventTransactionEvent> HCIEventTransactionEvent::Constructor(mozilla::dom::EventTarget* aOwner, const nsAString& aType, const HCIEventTransactionEventInit& aEventInitDict) { ... e->mAid = aEventInitDict.mAid; e->mOrigin = aEventInitDict.mOrigin; ... } class HCIEventTransactionEvent : public Event { ... JS::Heap<JSObject*> mAid; nsString mOrigin; ... } That causes this operator= error: In file included from UnifiedBindings5.cpp:66:0: HCIEventTransactionEvent.cpp: In static member function 'static already_AddRefed<mozilla::dom::HCIEventTransactionEvent> mozilla::dom::HCIEventTransactionEvent::Constructor(mozilla::dom::EventTarget*, const nsAString_internal&, const mozilla::dom::HCIEventTransactionEventInit&)': HCIEventTransactionEvent.cpp:72:28: error: no match for 'operator=' in 'e.nsRefPtr<T>::operator-><mozilla::dom::HCIEventTransactionEvent>()->mozilla::dom::HCIEventTransactionEvent::mAid = aEventInitDict.mozilla::dom::HCIEventTransactionEventInit::mAid' HCIEventTransactionEvent.cpp:72:28: note: candidates are: In file included from ../../dist/include/mozilla/dom/AudioNodeBinding.h:6:0, from GainNodeBinding.cpp:3, from UnifiedBindings5.cpp:2: ../../dist/include/js/RootingAPI.h:243:14: note: JS::Heap<T>& JS::Heap<T>::operator=(T) [with T = JSObject*] ../../dist/include/js/RootingAPI.h:243:14: note: no known conversion for argument 1 from 'const mozilla::dom::Optional<mozilla::dom::TypedArray<unsigned char, js::UnwrapUint8Array, JS_GetUint8ArrayData, js::GetUint8ArrayLengthAndData, JS_NewUint8Array> >' to 'JSObject*' ../../dist/include/js/RootingAPI.h:248:14: note: JS::Heap<T>& JS::Heap<T>::operator=(const JS::Heap<T>&) [with T = JSObject*] ../../dist/include/js/RootingAPI.h:248:14: note: no known conversion for argument 1 from 'const mozilla::dom::Optional<mozilla::dom::TypedArray<unsigned char, js::UnwrapUint8Array, JS_GetUint8ArrayData, js::GetUint8ArrayLengthAndData, JS_NewUint8Array> >' to 'const JS::Heap<JSObject*>&' Expected: Event WebIDL code gen should output vaild a constructor if passed an dictionary initializer.
Comment 1•9 years ago
|
||
*EventInit needs to either define default value for the properties, or mark properties required. Is the missing default value or missing 'required' causing this issue?
Comment 2•9 years ago
|
||
Though, I don't see any existing generated *EventInit using it, so could be just codegenerator missing support for this.
Assignee | ||
Comment 3•9 years ago
|
||
Just checked: Adding "required" to the init dictionary field still results in the same error.
Comment 4•9 years ago
|
||
What about making the property nullable and defaulting to = null; ?
Assignee | ||
Comment 5•9 years ago
|
||
Uint8Array? aid = null; Similar error: HCIEventTransactionEvent.cpp:72:28: note: candidates are: In file included from ../../dist/include/mozilla/dom/FunctionBinding.h:6:0, from FunctionBinding.cpp:3, from UnifiedBindings5.cpp:2: ../../dist/include/js/RootingAPI.h:243:14: note: JS::Heap<T>& JS::Heap<T>::operator=(T) [with T = JSObject*] ../../dist/include/js/RootingAPI.h:243:14: note: no known conversion for argument 1 from 'const mozilla::dom::Nullable<mozilla::dom::TypedArray<unsigned char, js::UnwrapUint8Array, JS_GetUint8ArrayData, js::GetUint8ArrayLengthAndData, JS_NewUint8Array> >' to 'JSObject*' ../../dist/include/js/RootingAPI.h:248:14: note: JS::Heap<T>& JS::Heap<T>::operator=(const JS::Heap<T>&) [with T = JSObject*] ../../dist/include/js/RootingAPI.h:248:14: note: no known conversion for argument 1 from 'const mozilla::dom::Nullable<mozilla::dom::TypedArray<unsigned char, js::UnwrapUint8Array, JS_GetUint8ArrayData, js::GetUint8ArrayLengthAndData, JS_NewUint8Array> >' to 'const JS::Heap<JSObject*>&' There's a case statement for sequence, and TypedArrays appear to fall to "else" in CodeGen.py (if that's the right place). It also seems like TypedArrays are a special case elsewhere?
No longer blocks: 991970
Assignee | ||
Comment 7•8 years ago
|
||
This patch should add support for TypedArrays for Codegen'ed based on IRC chats. Please take a look. Thanks!
Assignee: nobody → dgarnerlee
Status: NEW → ASSIGNED
Flags: needinfo?(bugs)
Attachment #8517894 -
Flags: review?(bugs)
Assignee | ||
Comment 8•8 years ago
|
||
Wrong patch directory...re-upload.
Attachment #8517894 -
Attachment is obsolete: true
Attachment #8517894 -
Flags: review?(bugs)
Attachment #8517896 -
Flags: review?(bugs)
Comment 9•8 years ago
|
||
Comment on attachment 8517896 [details] [diff] [review] 0003-Bug-1075198-Support-code-generation-for-TypedArrays-.patch >+ elif m.type.isSpiderMonkeyInterface(): >+ if cxOfMembers == "": >+ cxOfMembers = dedent( >+ """ >+ AutoJSAPI jsapi; >+ jsapi.Init(); Actually, you don't need this anymore, assuming you limit types to required and nullable types. (And this should have inited jsapi with the right global) >+ JSContext *cx = jsapi.cx(); >+ """) >+ srcname = "%s.%s" % (self.args[1].name, name) >+ members += fill("if (${varname}.WasPassed()) {\n", varname=srcname) >+ declType = "" >+ if m.type.nullable(): >+ declType = m.type.inner.name >+ else: >+ declType = m.type.name So we don't deal with the nullable part at all. Based on my testing nullable typed array (the one with '?' ) and non-nullable will get exactly the same code. But since the dictionary has different values for them, the nullable one doesn't actually compile. >+ members += fill( >+ """ >+ const ${declType}& ${varname} = ${srcname}.Value(); >+ ${varname}.ComputeLengthAndData(); >+ e->${varname} = ${varname}.Obj(); >+ } else { >+ e->${varname} = ${declType}::Create(cx, aOwner, 0, nullptr); >+ } This else part is unusual, I think we should not support Uint8Array propertyName; in the dictionary, but only required Uint8Array propertyName; and Uint8Array? propertyName = null; That would be consistent with other object-like types.
Attachment #8517896 -
Flags: review?(bugs) → review-
Assignee | ||
Comment 10•8 years ago
|
||
I did some updates per review comments/irc. Typed arrays ( isSpiderMonkeyInterface()) is just stored as JS::Heap<JSObject*>, and now handles nulls. Dictionary inits will only allow only required or optional with a default (the two cases above). Compile after Codegen fails otherwise as a side effect of types not matching. The event init arg type doesn't seem to be available to ask at generation time to throw an error earlier.
Attachment #8517896 -
Attachment is obsolete: true
Attachment #8520252 -
Flags: review?(bugs)
Comment 11•8 years ago
|
||
Ah, bindings layer ends up calling .setObjectOrNull(somejsobject); after calling the implementation method.
Updated•8 years ago
|
Attachment #8520252 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 12•8 years ago
|
||
Fixed indentation nit on else, and add r=.
Attachment #8520252 -
Attachment is obsolete: true
Assignee | ||
Comment 13•8 years ago
|
||
Trying: https://tbpl.mozilla.org/?tree=Try&rev=fc330698aae4 I'll probably create some test cases in the dependent bug or separate bug, unless desired in this bug.
Comment 14•8 years ago
|
||
If you could still upload the generated code + some example .webidl which uses both required and '?' types. Or pastebin at least.
Assignee | ||
Comment 15•8 years ago
|
||
Assignee | ||
Comment 16•8 years ago
|
||
Relevant portion: // Dict: Uint8Array? aid = null; if (aEventInitDict.mAid.IsNull()) { e->mAid = nullptr; } else { e->mAid = aEventInitDict.mAid.Value().Obj(); } // Dict: required Uint8Array payload; e->mPayload.set(aEventInitDict.mPayload.Obj());
Updated•8 years ago
|
Attachment #8520941 -
Attachment mime type: text/x-c++src → text/plain
Assignee | ||
Comment 17•8 years ago
|
||
Thanks. Try tests green on partial test set.
Keywords: checkin-needed
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 18•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/d24d15772d9a
Keywords: checkin-needed
Comment 19•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/d24d15772d9a
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in
before you can comment on or make changes to this bug.
Description
•