347007 - Implement RepetitionEvent for Web Forms 2.0

Status: RESOLVED INVALID

(Core :: DOM: Events, defect)

Description

[Shawn Wilsher :sdwilsh]

I've already done most of this.  Once I get everything to compile I'll get a patch up for someone to review.

Comment 1

[Shawn Wilsher :sdwilsh]

Attachment: v0.1

So this is my initial work.  Right now I'm getting some compile error, but I'm not so sure it is related to this.

Some stuff isn't at all related to this, but is code from some of the other Web Forms 2.0 stuff from Alex.  I'll try to point it out here:

> +  // Web Forms 2.0
> +  eDOMClassInfo_DOMWF2ValidityState_id,
> +

> +// Web Forms 2.0
> +#include "nsIDOMWF2FormControl.h"
> +#include "nsIDOMWF2FormElement.h"
> +#include "nsIDOMWF2InputElement.h"
> +#include "nsIDOMWF2ValidityState.h"

> +  // Web Forms 2.0
> +  NS_DEFINE_CLASSINFO_DATA(DOMWF2ValidityState, nsDOMGenericSH,
> +                           DOM_DEFAULT_SCRIPTABLE_FLAGS)
> +

> +    DOM_CLASSINFO_MAP_ENTRY(nsIDOMWF2FormElement)

> +    DOM_CLASSINFO_MAP_ENTRY(nsIDOMWF2FormControl)
> +    DOM_CLASSINFO_MAP_ENTRY(nsIDOMWF2InputElement)

> +    DOM_CLASSINFO_MAP_ENTRY(nsIDOMWF2FormControl)
> +    DOM_CLASSINFO_MAP_ENTRY(nsIDOMWF2InputElement)

> +  // Web Forms 2.0
> +  DOM_CLASSINFO_MAP_BEGIN_NO_CLASS_IF(DOMWF2ValidityState, nsIDOMWF2ValidityState)
> +    DOM_CLASSINFO_MAP_ENTRY(nsIDOMWF2ValidityState)
> +  DOM_CLASSINFO_MAP_END
> +

----------
I would really like feedback from someone who knows more about events than me :)

Comment 2

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

Comment on attachment 231762 [details] [diff] [review]
v0.1

>Index: dom/public/idl/events/nsIDOMRepetitionEvent.idl

Should all WF2 interfaces have a prefix nsIDOMWF2 ? Or?


>+NS_IMETHODIMP
>+nsDOMRepetitionEvent::GetElement(nsIDOMWF2RepetitionElement* aElement)
>+{
>+  NS_ENSURE_ARG_POINTER(aElement);
>+
>+  *aElement = mElement;

Does this compile? Shouldn't this be
nsDOMRepetitionEvent::GetElement(nsIDOMWF2RepetitionElement** aElement)
and
NS_IF_ADDREF(*aElement = mElement);

>Index: content/events/src/nsEventDispatcher.cpp
...
>     return NS_NewDOMEvent(aDOMEvent, aPresContext, nsnull);
>+  if (aEventType.LowerCaseEqualsLiteral("repetitionevent"))  // WF2
>+    return NS_NewDOMRepetitionEvent(aDOMEvent, aPresContext, nsnull);

Is this according to WF2 draft? I couldn't find that this was
actually specified in any way.

> 
>+  // Web Forms 2.0
>+  eDOMClassInfo_DOMWF2ValidityState_id,
>+
...
>+// Web Forms 2.0
>+#include "nsIDOMWF2FormControl.h"
>+#include "nsIDOMWF2FormElement.h"
>+#include "nsIDOMWF2InputElement.h"
>+#include "nsIDOMWF2ValidityState.h"
...
>+  // Web Forms 2.0
>+  NS_DEFINE_CLASSINFO_DATA(DOMWF2ValidityState, nsDOMGenericSH,
>+                           DOM_DEFAULT_SCRIPTABLE_FLAGS)
>+
...
>   DOM_CLASSINFO_MAP_BEGIN(HTMLFormElement, nsIDOMHTMLFormElement)
>     DOM_CLASSINFO_MAP_ENTRY(nsIDOMHTMLFormElement)
>     DOM_CLASSINFO_MAP_ENTRY(nsIDOMNSHTMLFormElement)
>+    DOM_CLASSINFO_MAP_ENTRY(nsIDOMWF2FormElement)
>     DOM_CLASSINFO_GENERIC_HTML_MAP_ENTRIES
>   DOM_CLASSINFO_MAP_END
...
>   DOM_CLASSINFO_MAP_BEGIN(HTMLInputElement, nsIDOMHTMLInputElement)
>     DOM_CLASSINFO_MAP_ENTRY(nsIDOMHTMLInputElement)
>     DOM_CLASSINFO_MAP_ENTRY(nsIDOMNSHTMLInputElement)
>+    DOM_CLASSINFO_MAP_ENTRY(nsIDOMWF2FormControl)
>+    DOM_CLASSINFO_MAP_ENTRY(nsIDOMWF2InputElement)
>     DOM_CLASSINFO_GENERIC_HTML_MAP_ENTRIES
>   DOM_CLASSINFO_MAP_END
> 
...
>+    DOM_CLASSINFO_MAP_ENTRY(nsIDOMWF2FormControl)
>+    DOM_CLASSINFO_MAP_ENTRY(nsIDOMWF2InputElement)
>     DOM_CLASSINFO_GENERIC_HTML_MAP_ENTRIES
>   DOM_CLASSINFO_MAP_END
...
>+  // Web Forms 2.0
>+  DOM_CLASSINFO_MAP_BEGIN_NO_CLASS_IF(DOMWF2ValidityState, nsIDOMWF2ValidityState)
>+    DOM_CLASSINFO_MAP_ENTRY(nsIDOMWF2ValidityState)
>+  DOM_CLASSINFO_MAP_END
>+

I guess this aren't for this bug ;)

Comment 3

[Alex Vincent [:WeirdAl]]

(In reply to comment #2)
> Should all WF2 interfaces have a prefix nsIDOMWF2 ? Or?

That is a convention I've adopted for now, pending reviews of the basic patches in bug 345822.

> Does this compile? Shouldn't this be
> nsDOMRepetitionEvent::GetElement(nsIDOMWF2RepetitionElement** aElement)
> and
> NS_IF_ADDREF(*aElement = mElement);

smaug's right - out params usually are in this format, and NS_IF_ADDREF is normal if mElement can be null.  (If it can't, NS_ADDREF.)

> 
> >Index: content/events/src/nsEventDispatcher.cpp
> ...
> >     return NS_NewDOMEvent(aDOMEvent, aPresContext, nsnull);
> >+  if (aEventType.LowerCaseEqualsLiteral("repetitionevent"))  // WF2
> >+    return NS_NewDOMRepetitionEvent(aDOMEvent, aPresContext, nsnull);
> 
> Is this according to WF2 draft? I couldn't find that this was
> actually specified in any way.

Actually, WF2 hasn't yet specified how someone is to create a RepetitionEvent by script (and this is a bug in the spec, I think - cc'ing Hixie for clarification).

As for these lines here, it's probably slightly off - I think this is intended to be used in document.createEvent, and typically that would be a plural argument ("RepetitionEvents").  On the other hand, it could be the spec wants us to wrap this as part of HTMLEvents - but I doubt that would work well.

var evt = document.createEvent(???);  // This is what you're trying to do, I think.

> I guess this aren't for this bug ;)

Yeah, as I said before, much of this is pending reviews by sicking an an appropriate sr (I'm not going to ask sicking to r/sr on the same patch).

Comment 4

[Shawn Wilsher :sdwilsh]

Attachment: v0.2

This might be done, but I can't really test it yet.  This will compile with v0.1 on Bug 347070 assuming I got all the files for each patch.

Feedback is always appreciated.

Comment 5

[Shawn Wilsher :sdwilsh]

Comment on attachment 232474 [details] [diff] [review]
v0.2

This is going to have to land the same time as Bug 347070, and since I'm getting close to requesting review on that bug, I figure I should get review on this bug.

Smaug, I'm requesting review from you but I'm not sure who to get sr from, so if you have someone in mind, let me know.

Comment 6

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

Comment on attachment 232474 [details] [diff] [review]
v0.2


>@@ -64,11 +64,12 @@ XPIDLSRCS =					\
> 	nsIDOMKeyEvent.idl			\
> 	nsIDOMMutationEvent.idl			\
> 	nsIDOMNSUIEvent.idl			\
> 	nsIDOMPopupBlockedEvent.idl		\
> 	nsIDOMBeforeUnloadEvent.idl		\
> 	nsIDOMNSEventTarget.idl			\
> 	nsIDOMSmartCardEvent.idl                \
> 	nsIDOMPageTransitionEvent.idl		\
>+	nsIDOMRepetitionEvent.idl \



Nit, align '\' with others.

>+
>+#ifndef nsDOMRepetitionEvent_h__
>+#define nsDOMRepetitionEvent_h__
>+
>+#include "nsDOMEvent.h"
>+#include "nsIDOMWF2RepetitionElement.h"
>+#include "nsIDOMRepetitionEvent.h"
>+#include "nsGenericElement.h"

Do you need #include "nsGenericElement.h"?

looks ok.

But this can't go in anyway before repetitionelement.

Comment 7

[Lars Gunther]

shouldn't this be WONTFIX?

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

This was removed from the spec.