Make Event to use Paris bindings

RESOLVED FIXED in mozilla22

Status

()

Core
DOM: Events
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Ms2ger, Assigned: smaug)

Tracking

(Blocks: 1 bug)

Trunk
mozilla22
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 15 obsolete attachments)

v21
234.71 KB, patch
peterv
: review+
Details | Diff | Splinter Review
v27
234.86 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

5 years ago
Created attachment 693041 [details] [diff] [review]
Patch v1

Since I still had this lying around
Attachment #693041 - Flags: review?(bugs)
Comment on attachment 693041 [details] [diff] [review]
Patch v1

Let's do this all once we know the GetParentObject part.
Attachment #693041 - Flags: review?(bugs)
Assignee: Ms2ger → bugs
Summary: Add part of the WebIDL API for Event to nsDOMEvent → Make Event to use Paris bindings
Created attachment 716783 [details] [diff] [review]
WIP

Tiny little WIP.
https://tbpl.mozilla.org/?tree=Try&rev=0db0b3fade94
Attachment #693041 - Attachment is obsolete: true
Created attachment 717167 [details] [diff] [review]
WIP 7

https://tbpl.mozilla.org/?tree=Try&rev=5ab7a6ef96d9
Attachment #716783 - Attachment is obsolete: true
Created attachment 717184 [details] [diff] [review]
WIP 8

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

Might compile on b2g
Created attachment 717504 [details] [diff] [review]
WIP 10

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

Only Event QIs to WrapperCache. Seems to fix some test failures.
Attachment #717167 - Attachment is obsolete: true
Attachment #717184 - Attachment is obsolete: true
Created attachment 717506 [details] [diff] [review]
WIP 11

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

https://tbpl.mozilla.org/?tree=Try&rev=ea05834ec68e
Attachment #717504 - Attachment is obsolete: true
Created attachment 717654 [details] [diff] [review]
WIP 12

https://tbpl.mozilla.org/?tree=Try&rev=437366b141d3
Attachment #717506 - Attachment is obsolete: true
Created attachment 717657 [details] [diff] [review]
WIP 13

Some coding style nits.

The patch is huge, but by far just mechanical changes to add a new 
parameter to some methods.
Attachment #717654 - Attachment is obsolete: true
Created attachment 717668 [details] [diff] [review]
WIP 14

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

Sorry about the spam
Attachment #717657 - Attachment is obsolete: true
Created attachment 717672 [details] [diff] [review]
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
Attachment #717668 - Attachment is obsolete: true
Created attachment 719120 [details] [diff] [review]
v19
Attachment #717672 - Attachment is obsolete: true
Depends on: 845631
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.
Attachment #719120 - Flags: review?(peterv)
Comment on attachment 719120 [details] [diff] [review]
v19

Hmm, I'll fix few nits still
Attachment #719120 - Flags: review?(peterv)
Created attachment 719173 [details] [diff] [review]
v20

Note, the patch doesn't actually remove test_Event-constructors.html.json,
but that will be done.
Attachment #719120 - Attachment is obsolete: true
Attachment #719173 - Flags: review?(peterv)
And note, this depends on Bug 845631
I did a desktop b2g build, and didn't see crashes there. I need help from b2g devs.
Depends on: 843296
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
Created attachment 719664 [details] [diff] [review]
v21

just fixes broken nsISupports QI of DeviceMotionEvent

https://tbpl.mozilla.org/?tree=Try&rev=5fcf3759d914
Attachment #719173 - Attachment is obsolete: true
Attachment #719173 - Flags: review?(peterv)
Attachment #719664 - Flags: review?(peterv)
https://tbpl.mozilla.org/?tree=Try&rev=94221383688e
Passes tests now.
Blocks: 846316
Created attachment 721193 [details] [diff] [review]
v 23 (same as v 21, but merged with up-to-date m-c)
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
Blocks: 848827
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//
Attachment #719664 - Flags: review?(peterv) → review+
(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.
Created attachment 722810 [details] [diff] [review]
v25 (+review comments and merging)

https://tbpl.mozilla.org/?tree=Try&rev=c05d04d94255
Attachment #721193 - Attachment is obsolete: true
Created attachment 722838 [details] [diff] [review]
v26, more merging

https://tbpl.mozilla.org/?tree=Try&rev=59da29f9dcf4
Attachment #722810 - Attachment is obsolete: true
Created attachment 723058 [details] [diff] [review]
v27

About to land this once compiled locally.
Attachment #722838 - Attachment is obsolete: true
https://hg.mozilla.org/integration/mozilla-inbound/rev/eaff15332579
https://hg.mozilla.org/mozilla-central/rev/eaff15332579
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
Depends on: 851996
Duplicate of this bug: 846316
Depends on: 856167
Depends on: 860591
Depends on: 863492
You need to log in before you can comment on or make changes to this bug.