1031051 - Remove the xpidl-based event generator

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Comment 1

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Part 1: Add a way to codegen InitFooEvent methods for legacy event types

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Part 2: Migrate HashChangeEvent

The empty type string when created from document.createEvent matches Chrome.

Comment 3

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Part 3: Migrate CloseEvent

Comment 4

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Part 4: Remove ElementReplaceEvent

William tells me this is dead.

Comment 5

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Part 5: Migrate PopupBlockedEvent

AFAICT this is Gecko only so we can remove the createEvent bit.

Comment 6

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Part 6: Migrate PopStateEvent

Comment 7

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Part 7: Migrate PageTransitionEvent

This also matches Chrome with the empty type string.

Comment 8

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Part 8: Migrate SmartCardEvent

Comment 9

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Part 9: Migrate RecordErrorEvent

Comment 10

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Part 10: Migrate mozSettings/apps events

Comment 11

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Part 11: Migrate DeviceOrientationEvent

Copying Chrome again (and the spec on the nullable bits).

Comment 12

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Part 12: Migrate DeviceStorageChangeEvent

Comment 13

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Part 14: Migrate DOMTransactionEvent

Comment 14

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Part 15: Migrate Speech events

Comment 15

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Part 15: Manually implement CustomEvent

We still need the XPIDL interface for this one unfortunately.

Comment 16

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Part 18: Remove the event generator

Comment 17

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

Comment on attachment 8446839 [details] [diff] [review]
Part 1: Add a way to codegen InitFooEvent methods for legacy event types

LegacyEventInit is a bit ugly, but ugly it can be since no one should
need to use it.

Comment 18

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

Comment on attachment 8446840 [details] [diff] [review]
Part 2: Migrate HashChangeEvent

I would leave web phasing API changes to another bug.
So, oldURL and newURL could stay nullable

Comment 19

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

Comment on attachment 8446841 [details] [diff] [review]
Part 3: Migrate CloseEvent

document.createEvent shouldn't set .type.
So, in EventDispatcher::CreateEvent s/NS_LITERAL_STRING("close")/EmptyString()/

+[Constructor(DOMString type, optional CloseEventInit eventInitDict),LegacyEventInit]
space after ,

Please don't remove ? from reason.

Comment 20

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

Comment on attachment 8446848 [details] [diff] [review]
Part 5: Migrate PopupBlockedEvent

You're firing wrong event.
No event should have type PopupBlockedEvents.

(I doubt you got green try results from this patch ;) )

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8446839 [details] [diff] [review]
Part 1: Add a way to codegen InitFooEvent methods for legacy event types

>+            self.wantsConstructorForNativeCaller = True

This member seems to be write-only.  Just nix it.

>+            if method.identifier.name == "init" + iface.identifier.name and \

I'd prefer parens around the condition to using backslash to continue line.

>+               self.originalArgs[0].argType == "const nsAString&" and \

Why not signature[1][0].type.isDOMString() ?

>+               self.originalArgs[1].argType == "bool" and \
>+               self.originalArgs[2].argType == "bool" and \

And similar here.

>+               len(self.originalArgs) - 3 == len(filter(lambda x: isinstance(x, IDLAttribute), iface.members)) - 1:

And len(signature[1])-3 on the LHS here.

Also, please document where the -3 and -1 come from.  And perhaps:

  len(x for x in iface.members if x.isAttr())

?  Or keep the filter() but isAttr() instead of using isinstance.

>+            i = 3 # Skip the boilerplate args

Maybe "Skip the boilerplate args: type, bubbles, cancelable."

>+            for m in self.descriptorProvider.getDescriptor(iface.identifier.name).interface.members:

This is moderately bogus.  We already assumed that self.descriptorProvider is a Descriptor when we got its .interface.  And in particular, iface == self.descriptorProvider.getDescriptor(iface.identifier.name), I would think.  So this can just be:

  for m in iface.members:

no?

>+                    # We initialize all the other member variables in the
>+                    # Constructor except those ones coming from the Event.

  We need to initialize all the member variables that do not come from Event.

perhaps?

>+                               cgClass.descriptor.interface).identifier.name == "Event":

Isn't cgClass.descriptor.interface == self.descriptorProvider.interface?  Might as well be consistent about which one we use, if so.  Pick one or the other.

However, in this case we should really use "iface" as the third arg, since "m" came from "iface".  The only reason this code works is because we explicitly avoid entering the loop at all when iface is "Event".

>+              aRv = rv;

  aRv.Throw(rv);

r=me with those fixed.

Comment 22

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 8446859 [details] [diff] [review]
Part 14: Migrate DOMTransactionEvent

Review of attachment 8446859 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/html/content/src/UndoManager.cpp
@@ +14,5 @@
>  #include "nsIVariant.h"
>  #include "nsVariant.h"
>  #include "nsINode.h"
> +#include "mozilla/dom/DOMTransactionEvent.h"
> +#include "mozilla/dom/BindingUtils.h"

What do you need BindingUtils.h for?

Comment 23

[William Chen [:wchen]]

Comment on attachment 8446842 [details] [diff] [review]
Part 4: Remove ElementReplaceEvent

Review of attachment 8446842 [details] [diff] [review]:
-----------------------------------------------------------------

This event was only ever used in a very old version of the custom elements spec.

Comment 24

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Part 5: Migrate PopupBlockedEvent

Comment 25

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Part 12.999: Support nsTArray<CallbackObject*> in ToJSValue

Comment 26

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8447569 [details] [diff] [review]
Part 12.999: Support nsTArray<CallbackObject*> in ToJSValue

>+ToJSValue(JSContext* aCx,
>+          T (&aArguments)[N],
>+          JS::MutableHandle<JS::Value> aValue)

Why was this needed for nsTArray?

In any case, please put this next to the existing overload that's just like that but taking const T[N]?

r=me

Comment 27

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I suspect it's not needed anymore.  It is an artifact of iterating.

Comment 28

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to :Ms2ger from comment #22)
> Comment on attachment 8446859 [details] [diff] [review]
> Part 13: Migrate DOMTransactionEvent
> 
> Review of attachment 8446859 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/html/content/src/UndoManager.cpp
> @@ +14,5 @@
> >  #include "nsIVariant.h"
> >  #include "nsVariant.h"
> >  #include "nsINode.h"
> > +#include "mozilla/dom/DOMTransactionEvent.h"
> > +#include "mozilla/dom/BindingUtils.h"
> 
> What do you need BindingUtils.h for?

I don't.  I actually need ToJSValue.h there.  Fixed.

Comment 29

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Part 13: Support nsTArray<CallbackObject*> in ToJSValue

comments addressed

Comment 30

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Part 16: Migrate Style*ChangeEvent

Comment 31

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Part 17: Manually implement CustomEvent

Comment 32

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

Comment on attachment 8446850 [details] [diff] [review]
Part 6: Migrate PopStateEvent

>+  bool result = true;
>+  nsPIDOMWindow* outerWindow = GetOuterWindow();
>+  nsCOMPtr<EventTarget> outerWindowET = do_QueryInterface(outerWindow);
>+  NS_ENSURE_TRUE(outerWindowET, NS_ERROR_FAILURE);
>+
>+  AutoJSAPI jsapi;
>+  result = jsapi.Init(outerWindow);
>+  NS_ENSURE_TRUE(result, NS_ERROR_FAILURE);
>+
>+  JSContext* cx = jsapi.cx();
>+  JS::Rooted<JS::Value> stateJSValue(cx, JSVAL_NULL);
JS::NullValue()

>+  if (aEventType.LowerCaseEqualsLiteral("popstateevent")) {
>+    AutoJSContext cx;
>+    RootedDictionary<PopStateEventInit> init(cx);
>+    nsRefPtr<PopStateEvent> event =
>+      PopStateEvent::Constructor(aOwner, NS_LITERAL_STRING("popstate"), init);
EmptyString(), not "popstate"


/me cries. Adding explicit JSAPI usage.

Comment 33

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

Comment on attachment 8446850 [details] [diff] [review]
Part 6: Migrate PopStateEvent

>-  if (aEventType.LowerCaseEqualsLiteral("popstateevent"))
>-    return NS_NewDOMPopStateEvent(aDOMEvent, aOwner, aPresContext, nullptr);
>   if (aEventType.LowerCaseEqualsLiteral("closeevent")) {
>     CloseEventInit init;
>     nsRefPtr<CloseEvent> event =
>       CloseEvent::Constructor(aOwner, NS_LITERAL_STRING("close"), init);
>     event.forget(aDOMEvent);
>     return NS_OK;
>   }
>+  if (aEventType.LowerCaseEqualsLiteral("popstateevent")) {
>+    AutoJSContext cx;
>+    RootedDictionary<PopStateEventInit> init(cx);
>+    nsRefPtr<PopStateEvent> event =
>+      PopStateEvent::Constructor(aOwner, NS_LITERAL_STRING("popstate"), init);
>+    event.forget(aDOMEvent);
>+    return NS_OK;
>+  }

Actually, since you remove init*Event for popstateevent, you should remove support for document.createEvent too.

Comment 34

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

Comment on attachment 8446851 [details] [diff] [review]
Part 7: Migrate PageTransitionEvent

>-  if (aEventType.LowerCaseEqualsLiteral("pagetransition"))
>-    return NS_NewDOMPageTransitionEvent(aDOMEvent, aOwner, aPresContext, nullptr);
>+  if (aEventType.LowerCaseEqualsLiteral("pagetransition")) {
>+    PageTransitionEventInit init;
>+    nsRefPtr<PageTransitionEvent> event =
>+      PageTransitionEvent::Constructor(aOwner, EmptyString(), init);
>+    event.forget(aDOMEvent);
>+    return NS_OK;
>+  }
Since you remove init*Event, you should remove support for document.createEvent("pagetransition") too

Comment 35

[Boris Zbarsky [:bzbarsky]]

> /me cries. Adding explicit JSAPI usage.

We could do a followup to store the state as a JS::Value, not a variant, right?

Comment 36

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

Comment on attachment 8446857 [details] [diff] [review]
Part 11: Migrate DeviceOrientationEvent

? changes are unrelated to this bug, but fine.

Comment 37

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

Comment on attachment 8446859 [details] [diff] [review]
Part 14: Migrate DOMTransactionEvent

Could you file a followup to sort out this transaction event / undomanager stuff.
AFAIK we don't expose Undomanager by default, so why do we have
transaction event?

CC ehsan to that bug.

Comment 38

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

Comment on attachment 8447701 [details] [diff] [review]
Part 17: Manually implement CustomEvent

>+CustomEvent::Constructor(const GlobalObject& aGlobal,
>+                         const nsAString& aType,
>+                         const CustomEventInit& aParam,
>+                         ErrorResult& aRv)
>+{
>+  nsCOMPtr<mozilla::dom::EventTarget> t = do_QueryInterface(aGlobal.GetAsSupports());
>+  nsRefPtr<CustomEvent> e = new CustomEvent(t, nullptr, nullptr);
>+  bool trusted = e->Init(t);
>+  JS::Rooted<JS::Value> detail(aGlobal.Context(), aParam.mDetail);
Why do you need rooted detail? Just pass aParam.mDetail.

>+CustomEvent::InitCustomEvent(JSContext* aCx,
>+                             const nsAString& aType,
>+                             bool aCanBubble,
>+                             bool aCancelable,
>+                             JS::Handle<JS::Value> aDetail,
>+                             ErrorResult& aRv)
>+{
>+  nsCOMPtr<nsIVariant> detail;
>+  nsContentUtils::XPConnect()->JSToVariant(aCx, aDetail, getter_AddRefs(detail));
Hmm, I guess we don't execute this when xpconnect is down.
I wouldn't mind a null check, though.

Comment 39

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/498be46fad67
https://hg.mozilla.org/integration/mozilla-inbound/rev/b38a925d0e20
https://hg.mozilla.org/integration/mozilla-inbound/rev/48e18a5e85af
https://hg.mozilla.org/integration/mozilla-inbound/rev/74e65e94d581
https://hg.mozilla.org/integration/mozilla-inbound/rev/481efa441243
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f60138bce8d
https://hg.mozilla.org/integration/mozilla-inbound/rev/e6377ca32f3d
https://hg.mozilla.org/integration/mozilla-inbound/rev/528eecdaf3a1
https://hg.mozilla.org/integration/mozilla-inbound/rev/12a583e2ac71
https://hg.mozilla.org/integration/mozilla-inbound/rev/026d050cf2c0
https://hg.mozilla.org/integration/mozilla-inbound/rev/b58f41a7759f
https://hg.mozilla.org/integration/mozilla-inbound/rev/f9b3e45fa2bf
https://hg.mozilla.org/integration/mozilla-inbound/rev/3bc87b9ddd5f
https://hg.mozilla.org/integration/mozilla-inbound/rev/bfc886fd4f83
https://hg.mozilla.org/integration/mozilla-inbound/rev/09613e3e814a
https://hg.mozilla.org/integration/mozilla-inbound/rev/64e6f04bb410
https://hg.mozilla.org/integration/mozilla-inbound/rev/73a841e35df9
https://hg.mozilla.org/integration/mozilla-inbound/rev/be19bc1d493e

Comment 40

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/498be46fad67
https://hg.mozilla.org/mozilla-central/rev/b38a925d0e20
https://hg.mozilla.org/mozilla-central/rev/48e18a5e85af
https://hg.mozilla.org/mozilla-central/rev/74e65e94d581
https://hg.mozilla.org/mozilla-central/rev/481efa441243
https://hg.mozilla.org/mozilla-central/rev/1f60138bce8d
https://hg.mozilla.org/mozilla-central/rev/e6377ca32f3d
https://hg.mozilla.org/mozilla-central/rev/528eecdaf3a1
https://hg.mozilla.org/mozilla-central/rev/12a583e2ac71
https://hg.mozilla.org/mozilla-central/rev/026d050cf2c0
https://hg.mozilla.org/mozilla-central/rev/b58f41a7759f
https://hg.mozilla.org/mozilla-central/rev/f9b3e45fa2bf
https://hg.mozilla.org/mozilla-central/rev/3bc87b9ddd5f
https://hg.mozilla.org/mozilla-central/rev/bfc886fd4f83
https://hg.mozilla.org/mozilla-central/rev/09613e3e814a
https://hg.mozilla.org/mozilla-central/rev/64e6f04bb410
https://hg.mozilla.org/mozilla-central/rev/73a841e35df9
https://hg.mozilla.org/mozilla-central/rev/be19bc1d493e