Convert nsJSEventListener to EventHandlerNonNull and company

RESOLVED FIXED in mozilla19

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: bz, Assigned: bz)

Tracking

(Blocks: 1 bug)

unspecified
mozilla19
x86
Mac OS X
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(8 attachments)

3.54 KB, patch
smaug
: review+
Details | Diff | Splinter Review
32.31 KB, patch
smaug
: review+
Details | Diff | Splinter Review
8.26 KB, patch
smaug
: review+
Details | Diff | Splinter Review
18.69 KB, patch
smaug
: review+
Details | Diff | Splinter Review
7.79 KB, patch
smaug
: review+
Details | Diff | Splinter Review
8.85 KB, patch
smaug
: review+
Details | Diff | Splinter Review
25.33 KB, patch
smaug
: review+
Details | Diff | Splinter Review
5.37 KB, patch
smaug
: review+
Details | Diff | Splinter Review
We can do this once bug 779048 lands and we have native-to-JS conversions for unions done in bug 807224 (needed for onerror).
Blocks: 807371
No longer depends on: 807371
Created attachment 678048 [details] [diff] [review]
part 1.  Make it easier to do special things with beforeunload and error events as needed.
Attachment #678048 - Flags: review?(bugs)
Created attachment 678049 [details] [diff] [review]
part 2.  Change event handlers to store WebIDL callback functions.

Please pay particular attention to the changes in nsJSEventListener::IsBlackForCC and nsEventListenerManager::MarkForCC.
Attachment #678049 - Flags: review?(bugs)
Created attachment 678050 [details] [diff] [review]
part 3.  Change event handlers to call their WebIDL callbacks directly.
Attachment #678050 - Flags: review?(bugs)
Created 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.

Please pay special attention to nsJSEventListener::IsBlackForCC
Attachment #678051 - Flags: review?(bugs)
Created attachment 678053 [details] [diff] [review]
part 5.  Allow event handlers to have a null scope object if they don't have to compile from a string.
Attachment #678053 - Flags: review?(bugs)
Created attachment 678055 [details] [diff] [review]
part 6.  Centralize our IMPL_EVENT_HANDLER macro definitions in nsDOMEventTargetHelper.
Attachment #678055 - Flags: review?(bugs)
Created attachment 678056 [details] [diff] [review]
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.
Attachment #678056 - Flags: review?(bugs)
Created attachment 678057 [details] [diff] [review]
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
Attachment #678057 - Flags: review?(bugs)
Assignee: nobody → bzbarsky
Whiteboard: [need review]

Comment 9

5 years ago
Mn is not happy.
(I don't really know what Marionette tests do.)
Oh, it is just "Summary is empty"
Mn is not happy on vanillay pushes too, so I'm ignoring it.

Updated

5 years ago
Attachment #678048 - Flags: review?(bugs) → review+

Updated

5 years ago
Attachment #678055 - Flags: review?(bugs) → review+
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.
Attachment #678049 - Flags: review?(bugs) → review-
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.
Attachment #678050 - Flags: review?(bugs) → review+
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.
Attachment #678051 - Flags: review?(bugs) → review+

Updated

5 years ago
Attachment #678053 - Flags: review?(bugs) → review+
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.
Attachment #678049 - Flags: review- → review+

Updated

5 years ago
Attachment #678056 - Flags: review?(bugs) → review+
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.
Attachment #678057 - Flags: review?(bugs) → review+
> 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!
(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.

Updated

5 years ago
Depends on: 811226

Updated

5 years ago
Depends on: 811394

Updated

5 years ago
Duplicate of this bug: 714474

Updated

5 years ago
Duplicate of this bug: 419968

Updated

5 years ago
Duplicate of this bug: 404115
Depends on: 916685
You need to log in before you can comment on or make changes to this bug.