857884 - Use dom::EventTarget more instead of nsIDOMEventTarget

Status: RESOLVED FIXED

Description

[David Zbarsky (:dzbarsky)]

Created attachment 733154 [details] [diff] [review]
Patch

Comment 1

[Boris Zbarsky [:bz] (no decent commit message means r-)]

>+  nsDOMEvent* event = static_cast<nsDOMEvent*>(aDOMEvent);

You want aDOMEvent->InternalDOMEvent().

Comment 2

[Boris Zbarsky [:bz] (no decent commit message means r-)]

Also, is the point just deCOM kinda things, or something else?

Comment 3

[David Zbarsky (:dzbarsky)]

(In reply to Boris Zbarsky (:bz) from comment #2)
> Also, is the point just deCOM kinda things

Yup.

Comment 4

[David Zbarsky (:dzbarsky)]

Created attachment 733198 [details] [diff] [review]
Part 2

Comment 5

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

Comment on attachment 733154 [details] [diff] [review]
Patch

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

::: accessible/src/generic/RootAccessible.cpp
@@ +187,4 @@
>    // receive untrusted events (synthetic events generated by untrusted code).
>    // For example, XBL bindings implementations for elements that are hosted in
>    // non chrome document fire untrusted events.
> +  nsCOMPtr<EventTarget> nstarget(do_QueryInterface(mDocumentNode));

Don't need the QI.

@@ +204,5 @@
>  
>  nsresult
>  RootAccessible::RemoveEventListeners()
>  {
> +  nsCOMPtr<EventTarget> target(do_QueryInterface(mDocumentNode));

Same

@@ +235,5 @@
>  NS_IMETHODIMP
>  RootAccessible::HandleEvent(nsIDOMEvent* aDOMEvent)
>  {
>    MOZ_ASSERT(aDOMEvent);
> +  nsDOMEvent* event = static_cast<nsDOMEvent*>(aDOMEvent);

What bz said

@@ +267,5 @@
>  void
>  RootAccessible::ProcessDOMEvent(nsIDOMEvent* aDOMEvent)
>  {
>    MOZ_ASSERT(aDOMEvent);
> +  nsDOMEvent* event = static_cast<nsDOMEvent*>(aDOMEvent);

Same

::: content/base/src/nsCCUncollectableMarker.cpp
@@ +187,5 @@
>        nsEventListenerManager* elm = doc->GetListenerManager(false);
>        if (elm) {
>          elm->MarkForCC();
>        }
> +      nsCOMPtr<EventTarget> win = do_QueryInterface(doc->GetInnerWindow());

Might as well cast to nsGlobalWindow directly here.

::: content/events/public/nsEventDispatcher.h
@@ +11,5 @@
>  #include "nsEvent.h"
>  
>  class nsPresContext;
>  class nsIDOMEvent;
> +class nsIDOMEventTarget;

Either sort the list or drop this change, please

::: content/events/src/nsAsyncDOMEvent.cpp
@@ +42,5 @@
>        }
>        nsEventDispatcher::DispatchDOMEvent(target, nullptr, mEvent,
>                                            nullptr, nullptr);
>      } else {
> +      nsCOMPtr<EventTarget> target = do_QueryInterface(mEventNode);

No need for the QI

::: content/xbl/src/nsXBLService.cpp
@@ +245,1 @@
>    target->AddEventListener(NS_LITERAL_STRING("load"), this, false);

Call it on doc directly

::: content/xul/content/src/nsXULElement.cpp
@@ +1181,5 @@
>                  // sourceEvent will be the original command event that we're
>                  // handling.
>                  nsCOMPtr<nsIDOMEvent> domEvent = aVisitor.mDOMEvent;
>                  while (domEvent) {
> +                    nsDOMEvent* event = static_cast<nsDOMEvent*>(domEvent.get());

As before

::: dom/base/nsGlobalWindow.cpp
@@ +2571,5 @@
>      }
>      else {
> +      nsCOMPtr<nsIDOMEventTarget> handler;
> +      NS_NewWindowRoot(this, getter_AddRefs(handler));
> +      mChromeEventHandler = do_QueryInterface(handler);

Want to deCOM NS_NewWindowRoot too?

@@ +2706,5 @@
>      return NULL;
>    }
>  
> +  nsCOMPtr<EventTarget> eventTarget =
> +    do_QueryInterface(frameLoader->GetTabChildGlobalAsEventTarget());

Please remove this QI in your next patch.

Comment 6

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

Comment on attachment 733198 [details] [diff] [review]
Part 2

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

::: content/base/src/nsDocument.cpp
@@ +4452,4 @@
>  
>    if (mParentDocument) {
>      target_frame =
>        do_QueryInterface(mParentDocument->FindContentForSubDocument(this));

No QI

::: content/base/src/nsXMLHttpRequest.cpp
@@ +2138,5 @@
>    }
>    if (mIsHtml) {
>      NS_ASSERTION(!(mState & XML_HTTP_REQUEST_SYNCLOOPING),
>        "We weren't supposed to support HTML parsing with XHR!");
> +    nsCOMPtr<EventTarget> eventTarget = do_QueryInterface(mResponseXML);

Drop the QI

::: content/xbl/src/nsXBLEventHandler.cpp
@@ +44,5 @@
>    if (!EventMatched(aEvent))
>      return NS_OK;
>  
> +  mProtoHandler->ExecuteHandler(aEvent->InternalDOMEvent()->GetCurrentTarget(), 
> +                                aEvent);

Trailing whitespace

::: content/xbl/src/nsXBLWindowKeyHandler.cpp
@@ +545,5 @@
>      nsCOMPtr<nsIDOMElement> element = GetElement();
>      if (element) {
>        piTarget = do_QueryInterface(commandElt);
>      } else {
> +      piTarget = do_QueryInterface(mTarget);

You could pass the dom::EventTarget you have in nsXBLService::AttachGlobalKeyHandler through NS_NewXBLWindowKeyHandler so you have one here. Separate patch, though.

::: docshell/base/nsDocShell.h
@@ +749,5 @@
>      // Note these are intentionally not addrefd.  Doing so will create a cycle.
>      // For that reasons don't use nsCOMPtr.
>  
>      nsIDocShellTreeOwner *     mTreeOwner; // Weak Reference
> +    nsIDOMEventTarget *        mChromeEventHandler; //Weak Reference

Sure, I guess.

Comment 7

[David Zbarsky (:dzbarsky)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/183c3d7a2c18
https://hg.mozilla.org/integration/mozilla-inbound/rev/efd4cd61eb3d

Comment 8

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/183c3d7a2c18
https://hg.mozilla.org/mozilla-central/rev/efd4cd61eb3d

Comment 9

[David Zbarsky (:dzbarsky)]

Created attachment 734475 [details] [diff] [review]
Part 3

Comment 10

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

Comment on attachment 734475 [details] [diff] [review]
Part 3

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

::: content/xbl/src/nsXBLService.cpp
@@ +595,5 @@
>    manager->AddEventListenerByType(handler, NS_LITERAL_STRING("keypress"),
>                                    dom::TrustedEventsAtSystemGroupBubble());
>  
>    if (contentNode)
>      return contentNode->SetProperty(nsGkAtoms::listener, handler,

This should be handler.forget().get(), then, no?

Comment 11

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

Comment on attachment 734475 [details] [diff] [review]
Part 3

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

lgtm, with that comment fixed.

Comment 12

[David Zbarsky (:dzbarsky)]

Created attachment 736152 [details] [diff] [review]
Part 4

Comment 13

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

Comment on attachment 736152 [details] [diff] [review]
Part 4

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

::: dom/base/Navigator.cpp
@@ +692,2 @@
>  
>    if (hidden) {

Instead of the local variable, use if (!doc || doc->Hidden()) { here.

::: dom/base/nsDOMClassInfo.cpp
@@ +7790,5 @@
>  {
>    nsCOMPtr<nsIDOMEvent> event(do_QueryInterface(aInitialThis));
>    NS_ENSURE_TRUE(event, NS_ERROR_UNEXPECTED);
>  
> +  NS_ADDREF(*_retval = event->InternalDOMEvent()->GetTarget());

Does this need to be NS_IF_ADDREF?

::: dom/base/nsGlobalWindow.cpp
@@ +6181,5 @@
>                                    aPopupURI, aPopupWindowName,
>                                    aPopupWindowFeatures);
>        event->SetTrusted(true);
>  
> +      nsCOMPtr<EventTarget> targ(do_QueryInterface(aDoc));

Use =

::: dom/power/WakeLock.cpp
@@ +172,2 @@
>    if (window) {
> +    nsCOMPtr<nsIDocument> doc = do_QueryInterface(window->GetExtantDocument());

GetExtantDoc(), and drop the QI.

@@ +195,5 @@
>  {
>    nsCOMPtr<nsPIDOMWindow> window = do_QueryReferent(mWindow);
>  
>    if (window) {
> +    nsCOMPtr<nsIDocument> doc = do_QueryInterface(window->GetExtantDocument());

Same

Comment 14

[David Zbarsky (:dzbarsky)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/315f6b451d07
https://hg.mozilla.org/integration/mozilla-inbound/rev/a550dadafa30

Comment 15

[Ryan VanderMeulen [:RyanVM]]

We generally recommend that mochitests continue to pass after a patch lands. Backed out.
https://hg.mozilla.org/integration/mozilla-inbound/rev/eefef98b23fb

Logs:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=a550dadafa30

Comment 16

[David Zbarsky (:dzbarsky)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2b156ae54c3e

Comment 17

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/2b156ae54c3e

Comment 18

[David Zbarsky (:dzbarsky)]

Created attachment 738279 [details] [diff] [review]
Part 5

Comment 19

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

Comment on attachment 738279 [details] [diff] [review]
Part 5

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

::: docshell/base/nsDocShell.cpp
@@ +1801,5 @@
>  nsDocShell::GetChromeEventHandler(nsIDOMEventTarget** aChromeEventHandler)
>  {
>      NS_ENSURE_ARG_POINTER(aChromeEventHandler);
> +    nsCOMPtr<EventTarget> handler = mChromeEventHandler;
> +    *aChromeEventHandler = handler.forget().get();

handler.forget(aChromeEventHandler)

::: layout/generic/nsImageMap.cpp
@@ +971,5 @@
>  
>    //Set which one of our areas changed focus
> +  nsCOMPtr<nsIContent> targetContent = do_QueryInterface(
> +    aEvent->InternalDOMEvent()->GetTarget());
> +  if (targetContent) {

If you're reindenting anyway, early return for !targetContent

::: layout/printing/nsPrintEngine.cpp
@@ +342,5 @@
>    if (!mPrt->mPPEventListeners) {
>      nsCOMPtr<nsIDocShell> docShell = do_QueryReferent(mContainer);
>      nsCOMPtr<nsPIDOMWindow> win(do_GetInterface(docShell));
>      if (win) {
> +      nsCOMPtr<EventTarget> target = do_QueryInterface(win->GetFrameElementInternal());

Ugh, this still returns nsIDOMElement, I guess :/

::: layout/printing/nsPrintPreviewListener.cpp
@@ +27,5 @@
>  
>  //
>  // nsPrintPreviewListener ctor
>  //
> +nsPrintPreviewListener::nsPrintPreviewListener (EventTarget* aTarget)

Fix the space while you're here

Comment 20

[David Zbarsky (:dzbarsky)]

Created attachment 738865 [details] [diff] [review]
Part 6

Comment 21

[David Zbarsky (:dzbarsky)]

Created attachment 738866 [details] [diff] [review]
Part 7

This should be the last one :)

Comment 22

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

Comment on attachment 738865 [details] [diff] [review]
Part 6

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

::: accessible/src/base/DocManager.cpp
@@ +326,5 @@
>  DocManager::AddListeners(nsIDocument* aDocument,
>                           bool aAddDOMContentLoadedListener)
>  {
>    nsPIDOMWindow *window = aDocument->GetWindow();
> +  EventTarget *target = window->GetChromeEventHandler();

* to the left

::: content/base/src/WebSocket.cpp
@@ +1098,5 @@
>    }
>  
>    if (mKeepingAlive && !shouldKeepAlive) {
>      mKeepingAlive = false;
> +    static_cast<EventTarget*>(this)->Release();

... interesting.

::: dom/bluetooth/BluetoothAdapter.h
@@ +44,4 @@
>    nsISupports*
>    ToISupports() const
>    {
> +    return static_cast<EventTarget*>(const_cast<BluetoothAdapter*>(this));

See if you can just drop the constness... This is stupid.

::: dom/bluetooth/BluetoothDevice.h
@@ +45,4 @@
>    nsISupports*
>    ToISupports() const
>    {
> +    return static_cast<EventTarget*>(const_cast<BluetoothDevice*>(this));

Ditto

::: dom/system/nsDeviceSensors.cpp
@@ +341,5 @@
> +                                    EventTarget* target,
> +                                    uint32_t type,
> +                                    double x,
> +                                    double y,
> +                                    double z) {

Since you're touching this... { goes on the next line.

::: dom/telephony/Telephony.h
@@ +58,4 @@
>    nsISupports*
>    ToISupports() const
>    {
> +    return static_cast<EventTarget*>(const_cast<Telephony*>(this));

Who invents this stuff...

::: dom/telephony/TelephonyCall.h
@@ +43,4 @@
>    nsISupports*
>    ToISupports() const
>    {
> +    return static_cast<EventTarget*>(const_cast<TelephonyCall*>(this));

Ditto

::: extensions/widgetutils/src/nsWidgetUtils.cpp
@@ +362,5 @@
>    return rv;
>  }
>  
> +EventTarget*
> +nsWidgetUtils::GetChromeEventHandler(nsIDOMWindow *aDOMWin)

* to the left

@@ +367,2 @@
>  {
> +  nsCOMPtr<nsPIDOMWindow> privateDOMWindow(do_QueryInterface(aDOMWin));

= do_QI

Comment 23

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

Comment on attachment 738866 [details] [diff] [review]
Part 7

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

::: embedding/browser/webBrowser/nsDocShellTreeOwner.cpp
@@ +79,3 @@
>  //
>  static nsresult
> +GetDOMEventTarget( nsWebBrowser* inBrowser, EventTarget** aTarget)

Remove the space after the ( while you're here.

@@ +858,1 @@
>    GetDOMEventTarget(mWebBrowser, getter_AddRefs(target));

(Do all callers ignore the return value?)

@@ +1672,5 @@
>  
>    // XXX test for selected text
>  
>    uint16_t nodeType;
> +  nsresult res = node->GetNodeType(&nodeType);

:/

Comment 24

[David Zbarsky (:dzbarsky)]

(In reply to :Ms2ger from comment #23)
> Comment on attachment 738866 [details] [diff] [review]
> Part 7
> 

> @@ +858,1 @@
> >    GetDOMEventTarget(mWebBrowser, getter_AddRefs(target));
> 
> (Do all callers ignore the return value?)
> 

Yep.  I guess it never fails.

Comment 25

[David Zbarsky (:dzbarsky)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2eca17711eff
https://hg.mozilla.org/integration/mozilla-inbound/rev/ec91252c57d2
https://hg.mozilla.org/integration/mozilla-inbound/rev/f53ad2a10ff8
https://hg.mozilla.org/integration/mozilla-inbound/rev/e57ac5581703

Comment 26

[Matt Brubeck (:mbrubeck)]

Sorry, I backed this out on inbound because something in the push caused build errors in nsWindow on Windows and B2G (and possibly other non-Mac platforms):
https://hg.mozilla.org/integration/mozilla-inbound/rev/583bf36efce8

Comment 27

[David Zbarsky (:dzbarsky)]

Landed everything up to part 7
https://hg.mozilla.org/integration/mozilla-inbound/rev/16e42f1b8b58
https://hg.mozilla.org/integration/mozilla-inbound/rev/62ec34a45dfd
https://hg.mozilla.org/integration/mozilla-inbound/rev/dcee2cd3394a

Comment 28

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/16e42f1b8b58
https://hg.mozilla.org/mozilla-central/rev/62ec34a45dfd
https://hg.mozilla.org/mozilla-central/rev/dcee2cd3394a

Comment 29

[David Zbarsky (:dzbarsky)]

Part 7: https://hg.mozilla.org/integration/mozilla-inbound/rev/91cf58935be3

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/91cf58935be3