Closed Bug 1075198 Opened 7 years ago Closed 7 years ago

Generated Constructor code for Events with Uint8Array not working

Categories

(Core :: DOM: Events, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: dgarnerlee, Assigned: dgarnerlee)

References

Details

Attachments

(3 files, 3 obsolete files)

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.
Blocks: 1071167
*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?
Though, I don't see any existing generated *EventInit using it, so could be just codegenerator
missing support for this.
Just checked: Adding "required" to the init dictionary field still results in the same error.
What about making the property nullable and defaulting to = null; ?
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?
Blocks: 991970
Hi, any updates?
Flags: needinfo?(bugs)
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)
Wrong patch directory...re-upload.
Attachment #8517894 - Attachment is obsolete: true
Attachment #8517894 - Flags: review?(bugs)
Attachment #8517896 - Flags: review?(bugs)
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-
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)
Ah, bindings layer ends up calling .setObjectOrNull(somejsobject); after calling the implementation method.
Attachment #8520252 - Flags: review?(bugs) → review+
Fixed indentation nit on else, and add r=.
Attachment #8520252 - Attachment is obsolete: true
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.
If you could still upload the generated code + some example .webidl which uses both required and '?' types.
Or pastebin at least.
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());
Attachment #8520941 - Attachment mime type: text/x-c++src → text/plain
Thanks. Try tests green on partial test set.
Keywords: checkin-needed
Keywords: checkin-needed
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/d24d15772d9a
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.