611888 - XForms broke due to changes in nsEventState

Status: RESOLVED FIXED

(Core Graveyard :: XForms, defect)

Description

[Philipp Wagner [:imphil]]

Attachment: patch v1

The changes made in bug 595036 caused XForms to break. A patch is attached.

It's not really nice, as we cannot transfer |nsEventStates| across XPCOM interfaces, only it's internal type, PRUint64, but there seems to be no other way. I used |nsEventStates::InternalType| in all interface implementations to make sure that we get compiler errors if |InternalType| is changed and we need to change the interfaces accordingly.

Comment 1

[alexander :surkov (:asurkov)]

How are these XPCOM state methods supposed to be used? Are they indented to be used from script?

Comment 2

[Philipp Wagner [:imphil]]

I don't know about the intention, but currently no script uses these methods. XForms seems to have the general idea of making all interfaces available over XPCOM, even though they're never used (e.g. nsIModelElementPrivate). Can we remove the methods using nsEventStates from the IDL and define it only in the class? Or what's the right way to proceed here?

Comment 3

[alexander :surkov (:asurkov)]

(In reply to comment #2)

> XForms seems to have the general idea of making all interfaces available over
> XPCOM, even though they're never used (e.g. nsIModelElementPrivate). 

that were in times when we were XPCOM oriented, we don't need this anymore.

> but currently no script uses these methods.
> Can we
> remove the methods using nsEventStates from the IDL and define it only in the
> class?

yes, that's how we should deal with.

Comment 4

[Philipp Wagner [:imphil]]

hm, looks not as straightforward as I initially thought. When I move the affected methods (nsIModelElementPrivate::GetStates, nsIXFormsControl::GetDefaultIntrinsicState and nsIXFormsControl::GetDisabledIntrinsicState) from the IDL to the implementation (e.g. from nsIXFormsModelElementPrivate.idl to nsXFormsModelElement.h) I obviously also need to change the code calling these three methods. Now the question is: is there a sane method to cast from e.g. nsCOMPtr<nsIModelElementPrivate> to nsXFormsModelElement* to call the method GetStates from there? 

Probably the final solution would be to get rid of nsIXFormsModelElement as XPCOM interface altogether, but that's unfortunately nothing I have time to do right now. 

If there is no sane way to do the cast, could we take this patch as preliminary fix to get XForms working again and I open a follow-up to get this fixed in a better way?

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

If we have non-scriptable interface which nsXFormsModelElement implements, 
QI first to that, and the static_cast to nsXFormsModelElement*.
(if you have nsCOMPtr<someinterface> foo, static_cast<nsXFormsModelElement*>(foo.get()))

Comment 6

[Philipp Wagner [:imphil]]

Attachment: patch v2

Since the interfaces are non-scriptable, we can simply convert them to use nsEventStates directly. The only places where conversion is still needed if we hand nsEventStates to XTFWrapper. All in all it's much nicer now. Thanks Olli for the hints!

Comment 7

[alexander :surkov (:asurkov)]

Comment on attachment 491653 [details] [diff] [review]
patch v2

looks correct, r=me

Comment 8

[Philipp Wagner [:imphil]]

http://hg.mozilla.org/xforms/rev/106651d63221