807226 - Convert nsJSEventListener to EventHandlerNonNull and company

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

We can do this once bug 779048 lands and we have native-to-JS conversions for unions done in bug 807224 (needed for onerror).

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Make it easier to do special things with beforeunload and error events as needed.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Change event handlers to store WebIDL callback functions.

Please pay particular attention to the changes in nsJSEventListener::IsBlackForCC and nsEventListenerManager::MarkForCC.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Change event handlers to call their WebIDL callbacks directly.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: part 4. Allow event handlers to have a null nsIScriptContext if they won't have to compile from a string.

Please pay special attention to nsJSEventListener::IsBlackForCC

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: part 5. Allow event handlers to have a null scope object if they don't have to compile from a string.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: part 6. Centralize our IMPL_EVENT_HANDLER macro definitions in nsDOMEventTargetHelper.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: part 7. Move creation of our event handlers out to the relevant API methods. that once we switch all these guys to WebIDL bindings they'll

automatically get the callback objects passed in from binding code.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: part 8. Remove the exceptions for EventHandler in WebIDL codegen.

Note: a sort of try run for this is at https://tbpl.mozilla.org/?tree=Try&rev=0b68d33a6dea

Comment 9

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

Mn is not happy.
(I don't really know what Marionette tests do.)

Comment 10

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

Oh, it is just "Summary is empty"

Comment 11

[Boris Zbarsky [:bzbarsky]]

Mn is not happy on vanillay pushes too, so I'm ignoring it.

Comment 12

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

Comment on attachment 678049 [details] [diff] [review]
part 2.  Change event handlers to store WebIDL callback functions.


>@@ -830,17 +835,53 @@ nsEventListenerManager::CompileEventHand
>     NS_ENSURE_SUCCESS(result, result);
>   }
> 
>   if (handler) {
>     // Bind it
>     nsScriptObjectHolder<JSObject> boundHandler(context);
>     context->BindCompiledEventHandler(mTarget, listener->GetEventScope(),
>                                       handler.get(), boundHandler);
>-    listener->SetHandler(boundHandler.get());
>+    if (listener->EventName() == nsGkAtoms::onerror && win) {
>+      bool ok;
>+      JSAutoRequest ar(context->GetNativeContext());
>+      nsRefPtr<OnErrorEventHandlerNonNull> handlerCallback =
>+        new OnErrorEventHandlerNonNull(context->GetNativeContext(),
>+                                       listener->GetEventScope(),
>+                                       boundHandler.get(), &ok);
>+      if (!ok) {
>+        return NS_ERROR_OUT_OF_MEMORY;
>+      }
This looks odd. Could you add some comment about possible causes of OOM


>+    } else if (listener->EventName() == nsGkAtoms::onbeforeunload) {
>+      // XXXbz Should we really do the special beforeunload handler on
>+      // non-Window objects?  Per spec, we shouldn't even be compiling
>+      // the beforeunload content attribute on random elements!
Could you file a followup and add the bug number here.


>+  if (aEventName == nsGkAtoms::onerror && win) {
>+    bool ok;
>+    nsRefPtr<OnErrorEventHandlerNonNull> handlerCallback =
>+      new OnErrorEventHandlerNonNull(cx, aScope, callable, &ok);
>+    if (!ok) {
>+      return NS_ERROR_OUT_OF_MEMORY;
>+    }
>+    handler.SetHandler(handlerCallback);
>+  } else if (aEventName == nsGkAtoms::onbeforeunload) {
>+    MOZ_ASSERT(win,
>+               "Should not have onbeforeunload handlers on non-Window objects");
>+    bool ok;
>+    nsRefPtr<BeforeUnloadEventHandlerNonNull> handlerCallback =
>+      new BeforeUnloadEventHandlerNonNull(cx, aScope, callable, &ok);
>+    if (!ok) {
>+      return NS_ERROR_OUT_OF_MEMORY;
>+    }
>+    handler.SetHandler(handlerCallback);
>+  } else {
>+    bool ok;
>+    nsRefPtr<EventHandlerNonNull> handlerCallback =
>+      new EventHandlerNonNull(cx, aScope, callable, &ok);
>+    if (!ok) {
>+      return NS_ERROR_OUT_OF_MEMORY;
>+    }
>+    handler.SetHandler(handlerCallback);

Hmm, code duplication. Could you have a helper method which is called in this method
and in nsEventListenerManager::CompileEventHandler

>-  // Set a handler for this event listener.  Must not be called if
>-  // there is already a handler!  The handler must already be bound to
>-  // the right target.
>-  virtual void SetHandler(JSObject *aHandler) = 0;
>+  nsIAtom *EventName() const
nsIAtom*

>+  void SetHandler(const nsEventHandler& aHandler) {
>+    mHandler.SetHandler(aHandler);
>+  }
>+  void SetHandler(mozilla::dom::EventHandlerNonNull* aHandler) {
>+    mHandler.SetHandler(aHandler);
>+  }
>+  void SetHandler(mozilla::dom::BeforeUnloadEventHandlerNonNull* aHandler) {
>+    mHandler.SetHandler(aHandler);
>+  }
>+  void SetHandler(mozilla::dom::OnErrorEventHandlerNonNull* aHandler) {
>+    mHandler.SetHandler(aHandler);
>+  }
{ goes to next line

>+  bool HasGrayCallable() const
>+  {
>+    return xpc_IsGrayGCThing(mCallable);
>+  }
return mCallable && xpc_IsGrayGCThing(mCallable);
I think

I could take another look after the code duplication is fixed.

Comment 13

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

Comment on attachment 678050 [details] [diff] [review]
part 3.  Change event handlers to call their WebIDL callbacks directly.


>+      errorMsg = scriptEvent->errorMsg;
>+      msgOrEvent.SetAsString() = static_cast<nsAString*>(&errorMsg);
This is somewhat odd looking.
Would it be possible to change SetAsString to take a parameter?
Though, I guess SetAsString() is consistent with Optional<...>.Value()
So, no need to change.

Comment 14

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

Comment on attachment 678051 [details] [diff] [review]
part 4.  Allow event handlers to have a null nsIScriptContext if they won't have to compile from a string.

Looking good.

Comment 15

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

Comment on attachment 678049 [details] [diff] [review]
part 2.  Change event handlers to store WebIDL callback functions.

Ah, the code duplication is fixed in another patch.

Comment 16

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

Comment on attachment 678057 [details] [diff] [review]
part 8.  Remove the exceptions for EventHandler in WebIDL codegen.

This is all great :)

Hopefully I haven't missed anything important.

Comment 17

[Boris Zbarsky [:bzbarsky]]

> return mCallable && xpc_IsGrayGCThing(mCallable);

mCallable is only null if we've been unlinked (well, of if the constructor returns a false "ok", but people better not be doing anything with the object then)...  Will we end up having HasGrayCallable() called after we have been unlinked?

> This is somewhat odd looking.

Yeah, I'll file a followup on giving unions better ergonomics.

Thanks for the reviews!

Comment 18

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

(In reply to Boris Zbarsky (:bz) from comment #17)
> > return mCallable && xpc_IsGrayGCThing(mCallable);
> 
> mCallable is only null if we've been unlinked (well, of if the constructor
> returns a false "ok", but people better not be doing anything with the
> object then)...  Will we end up having HasGrayCallable() called after we
> have been unlinked?

As of now no, but if we add more skippability, yet have some missing unlinking elsewhere, we might.
So I think adding null check would be good there.

Comment 19

[Boris Zbarsky [:bzbarsky]]

   https://hg.mozilla.org/integration/mozilla-inbound/rev/a1566a0fa1c8
   https://hg.mozilla.org/integration/mozilla-inbound/rev/e2727ec34a16
   https://hg.mozilla.org/integration/mozilla-inbound/rev/0ee3b6d2415a
   https://hg.mozilla.org/integration/mozilla-inbound/rev/f64f70e0649c
   https://hg.mozilla.org/integration/mozilla-inbound/rev/f244d412e485
   https://hg.mozilla.org/integration/mozilla-inbound/rev/741e28cf55ee
   https://hg.mozilla.org/integration/mozilla-inbound/rev/f9e7d57d435a
   https://hg.mozilla.org/integration/mozilla-inbound/rev/69eccdae84d7
   https://hg.mozilla.org/integration/mozilla-inbound/rev/bd9729ca4de2

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a1566a0fa1c8
https://hg.mozilla.org/mozilla-central/rev/e2727ec34a16
https://hg.mozilla.org/mozilla-central/rev/0ee3b6d2415a
https://hg.mozilla.org/mozilla-central/rev/f64f70e0649c
https://hg.mozilla.org/mozilla-central/rev/f244d412e485
https://hg.mozilla.org/mozilla-central/rev/741e28cf55ee
https://hg.mozilla.org/mozilla-central/rev/f9e7d57d435a
https://hg.mozilla.org/mozilla-central/rev/69eccdae84d7
https://hg.mozilla.org/mozilla-central/rev/bd9729ca4de2