822399 - Make Event to use Paris bindings

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

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

Attachment: Patch v1

Since I still had this lying around

Comment 1

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

Comment on attachment 693041 [details] [diff] [review]
Patch v1

Let's do this all once we know the GetParentObject part.

Comment 2

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

Attachment: WIP

Tiny little WIP.
https://tbpl.mozilla.org/?tree=Try&rev=0db0b3fade94

Comment 3

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

Attachment: WIP 7

https://tbpl.mozilla.org/?tree=Try&rev=5ab7a6ef96d9

Comment 4

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

Attachment: WIP 8

https://tbpl.mozilla.org/?tree=Try&rev=104e2584fdfd

Might compile on b2g

Comment 5

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

Attachment: WIP 10

https://tbpl.mozilla.org/?tree=Try&rev=f6c998874861

Only Event QIs to WrapperCache. Seems to fix some test failures.

Comment 6

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

Attachment: WIP 11

Attr nodes weren't mozilla::dom::EventTargets

https://tbpl.mozilla.org/?tree=Try&rev=ea05834ec68e

Comment 7

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

Attachment: WIP 12

https://tbpl.mozilla.org/?tree=Try&rev=437366b141d3

Comment 8

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

Attachment: WIP 13

Some coding style nits.

The patch is huge, but by far just mechanical changes to add a new 
parameter to some methods.

Comment 9

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

Attachment: WIP 14

https://tbpl.mozilla.org/?tree=Try&rev=5f2c33520342

Sorry about the spam

Comment 10

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

Attachment: WIP 15

And still one is needed. Now all the failing tests pass locally.

Running more global try tests for this
https://tbpl.mozilla.org/?tree=Try&rev=049993ad7a53

Comment 11

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

Attachment: v19

Comment 12

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

Comment on attachment 719120 [details] [diff] [review]
v19

Time to start reviewing this.
Note, nsDOMEvent changes are kept minimum, so methods end up calling
xpidl versions.
There will be a followup to change that. And another followup to make mOwner handling simpler, but that requires all events to be converter to
use Paris bindings.

The patch is huge, but it is mostly just trivial changes to add
a new parameter.

b2g is still burning with the patch, but other platforms look ok.
I'm waiting for b2g try logs to become useful (bug 843303).
Just normal e10s testing wasn't enough to tell what is causing the crash.

Comment 13

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

Comment on attachment 719120 [details] [diff] [review]
v19

Hmm, I'll fix few nits still

Comment 14

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

Attachment: v20

Note, the patch doesn't actually remove test_Event-constructors.html.json,
but that will be done.

Comment 15

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

And note, this depends on Bug 845631

Comment 16

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

I did a desktop b2g build, and didn't see crashes there. I need help from b2g devs.

Comment 17

[Dave Hylands [:dhylands]]

Debug build of b2g emulator running xpcshell updater tests with this patch applied shows:

###!!! ASSERTION: Uh, fix QI!: 'target_qi == event', file /home/work/B2G-emulator-arm/mozilla-inbound/content/events/src/nsDOMEvent.h, line 63

Comment 18

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

Attachment: v21

just fixes broken nsISupports QI of DeviceMotionEvent

https://tbpl.mozilla.org/?tree=Try&rev=5fcf3759d914

Comment 19

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

https://tbpl.mozilla.org/?tree=Try&rev=94221383688e

Comment 20

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

Passes tests now.

Comment 21

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

Attachment: v 23 (same as v 21, but merged with up-to-date m-c)

Comment 22

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

Comment on attachment 721193 [details] [diff] [review]
v 23 (same as v 21, but merged with up-to-date m-c)

>@@ -28,16 +29,59 @@ interface Event {
>   readonly attribute boolean cancelable;
>   void preventDefault();
>   readonly attribute boolean defaultPrevented;
> 
>   readonly attribute boolean isTrusted;
>   readonly attribute DOMTimeStamp timeStamp;
> 
>   void initEvent(DOMString type, boolean bubbles, boolean cancelable);
This should actually throw.
The implementation of the method is trivial.
just forward to xpidl method and assign return value to aRv

Comment 23

[Peter Van der Beken [:peterv]]

Comment on attachment 719664 [details] [diff] [review]
v21

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

Are we planning to make the various things that create an event (NS_New*, nsEventDispatcher::CreateEvent, ...) return a nsDOMEvent? We might even be able to make the NS_New* ones return an |already_AddRefed<nsDOMEvent>|? Just asking because you're touching all the NS_New* functions here anyway. Up to you.

::: content/base/src/nsDOMAttribute.cpp
@@ +68,5 @@
>  // QueryInterface implementation for nsDOMAttribute
>  NS_INTERFACE_TABLE_HEAD(nsDOMAttribute)
>    NS_WRAPPERCACHE_INTERFACE_MAP_ENTRY
> +  NS_NODE_INTERFACE_TABLE5(nsDOMAttribute, nsIDOMAttr, nsIAttribute, nsIDOMNode,
> +                           nsIDOMEventTarget, mozilla::dom::EventTarget)

s/mozilla::dom//

::: content/base/src/nsINode.cpp
@@ +2421,5 @@
>    return CallQueryInterface(GetAttributes(), aAttributes);
>  }
> +
> +bool
> +mozilla::dom::EventTarget::DispatchEvent(mozilla::dom::NonNull<nsDOMEvent>& aEvent,

You should be able to use just |nsDOMEvent&|.

@@ +2425,5 @@
> +mozilla::dom::EventTarget::DispatchEvent(mozilla::dom::NonNull<nsDOMEvent>& aEvent,
> +                                         mozilla::ErrorResult& aRv)
> +{
> +  bool result = false;
> +  aRv = DispatchEvent(static_cast<nsIDOMEvent*>(aEvent.get()), &result);

I don't think you need that static_cast.

::: content/events/public/EventTarget.h
@@ +48,5 @@
>                             bool aCapture, mozilla::ErrorResult& aRv)
>    {
>      aRv = RemoveEventListener(aType, aCallback, aCapture);
>    }
> +  bool DispatchEvent(mozilla::dom::NonNull<nsDOMEvent>& aEvent,

You should be able to just use |nsDOMEvent&|.

@@ +49,5 @@
>    {
>      aRv = RemoveEventListener(aType, aCallback, aCapture);
>    }
> +  bool DispatchEvent(mozilla::dom::NonNull<nsDOMEvent>& aEvent,
> +                     mozilla::ErrorResult& aRv);

s/mozilla::dom:://

::: content/events/src/DOMWheelEvent.cpp
@@ +15,5 @@
>  
>  namespace mozilla {
>  namespace dom {
>  
> +DOMWheelEvent::DOMWheelEvent(mozilla::dom::EventTarget* aOwner,

s/mozilla::dom:://

::: content/events/src/DOMWheelEvent.h
@@ +16,5 @@
>  class DOMWheelEvent : public nsDOMMouseEvent,
>                        public nsIDOMWheelEvent
>  {
>  public:
> +  DOMWheelEvent(mozilla::dom::EventTarget* aOwner,

s/mozilla::dom:://

::: content/events/src/nsAsyncDOMEvent.cpp
@@ +12,5 @@
>  
>  nsAsyncDOMEvent::nsAsyncDOMEvent(nsINode *aEventNode, nsEvent &aEvent)
>    : mEventNode(aEventNode), mDispatchChromeOnly(false)
>  {
> +  if (!aEventNode) {

Does this actually happen? Seems like we should avoid it and assert :-(.

::: content/events/src/nsDOMEvent.cpp
@@ +338,5 @@
>  
> +//static
> +already_AddRefed<nsDOMEvent>
> +nsDOMEvent::Constructor(const mozilla::dom::GlobalObject& aGlobal,
> +                        const mozilla::dom::NonNull<nsAString>& aType,

You should be able to use |const nsAString&|.

@@ +358,5 @@
> +      }
> +    }
> +  }
> +  nsresult rv = e->InitEvent(aType, aParam.mBubbles, aParam.mCancelable);
> +  aRv = rv;

You can assign directly into aRv, no need for rv.

::: content/events/src/nsDOMEvent.h
@@ +27,5 @@
>  class JSObject;
> +
> +#define NS_DOMEVENT_IID \
> +{ 0x3610fb70, 0x718b, 0x41b7, \
> +  { 0x80, 0xad, 0x18, 0xce, 0xe0, 0xac, 0x7a, 0x7b } }

Why do we need this?

@@ +65,5 @@
> +#endif
> +    return static_cast<nsDOMEvent*>(event);
> +  }
> +
> +  static nsDOMEvent* CreateEvent(mozilla::dom::EventTarget* aOwner,

I'd be more comfortably if this returned |already_AddRefed<nsDOMEvent>|.

@@ +87,5 @@
> +
> +  virtual JSObject* WrapObject(JSContext* aCx, JSObject* aScope, bool* aTriedToWrap)
> +  {
> +    return mozilla::dom::EventBinding::Wrap(aCx, aScope, this, aTriedToWrap);
> +  }

I usually put this in the .cpp, to avoid including *Binding.h files in headers. It's virtual anyway, so shouldn't make a difference.

::: content/html/document/src/nsHTMLDocument.h
@@ +100,5 @@
>  
>    // nsIDOMHTMLDocument interface
>    NS_DECL_NSIDOMHTMLDOCUMENT
>  
> +  void RouteEvent(mozilla::dom::NonNull<nsDOMEvent>& aEvent)

You should be able to just use |nsDOMEvent&|.

@@ +102,5 @@
>    NS_DECL_NSIDOMHTMLDOCUMENT
>  
> +  void RouteEvent(mozilla::dom::NonNull<nsDOMEvent>& aEvent)
> +  {
> +    RouteEvent(static_cast<nsIDOMEvent*>(aEvent.get()));

I don't think you need that static_cast.

::: dom/bindings/BindingUtils.cpp
@@ +1621,5 @@
>  
> +bool
> +WrapObject(JSContext* cx, JSObject* scope, JSObject& p, JS::Value* vp)
> +{
> +  vp->setObjectOrNull(&p);

setObject(p)

@@ +1623,5 @@
> +WrapObject(JSContext* cx, JSObject* scope, JSObject& p, JS::Value* vp)
> +{
> +  vp->setObjectOrNull(&p);
> +  return true;
> +}

Inline in the header please.

::: dom/ipc/TabMessageUtils.cpp
@@ +24,5 @@
>    nsString type;
>    NS_ENSURE_TRUE(ReadParam(aMsg, aIter, &type), false);
>  
>    nsCOMPtr<nsIDOMEvent> event;
> +  nsEventDispatcher::CreateEvent(nullptr, nullptr, nullptr, type, getter_AddRefs(event));

Maybe add a comment that the owner will need to be set before dispatching this?

::: dom/webidl/Event.webidl
@@ +33,5 @@
>    readonly attribute boolean isTrusted;
>    readonly attribute DOMTimeStamp timeStamp;
>  
>    void initEvent(DOMString type, boolean bubbles, boolean cancelable);
> +  

Trailing whitespace.

@@ +34,5 @@
>    readonly attribute DOMTimeStamp timeStamp;
>  
>    void initEvent(DOMString type, boolean bubbles, boolean cancelable);
> +  
> +  const long MOUSEDOWN    = 0x00000001;

Can we put the mozilla-specific stuff in a partial interface so that that's real clear?

::: layout/base/nsPresContext.cpp
@@ +2061,5 @@
>    if (!ourWindow)
>      return;
>  
> +  nsCOMPtr<mozilla::dom::EventTarget> dispatchTarget = do_QueryInterface(ourWindow);
> +  nsCOMPtr<mozilla::dom::EventTarget> eventTarget = dispatchTarget;

s/mozilla::dom//

Comment 24

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

(In reply to Peter Van der Beken [:peterv] from comment #23)
> Comment on attachment 719664 [details] [diff] [review] 
> Are we planning to make the various things that create an event (NS_New*,
> nsEventDispatcher::CreateEvent, ...) return a nsDOMEvent? We might even be
> able to make the NS_New* ones return an |already_AddRefed<nsDOMEvent>|? Just
> asking because you're touching all the NS_New* functions here anyway.
At this point no. For C++ we need to have some Init* method anyway, and
returning nsDOMEvent doesn't really help.

Comment 25

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

Attachment: v25 (+review comments and merging)

https://tbpl.mozilla.org/?tree=Try&rev=c05d04d94255

Comment 26

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

Attachment: v26, more merging

https://tbpl.mozilla.org/?tree=Try&rev=59da29f9dcf4

Comment 27

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

Attachment: v27

About to land this once compiled locally.

Comment 28

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/eaff15332579

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/eaff15332579