Closed
Bug 857884
Opened 8 years ago
Closed 8 years ago
Use dom::EventTarget more instead of nsIDOMEventTarget
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla23
People
(Reporter: dzbarsky, Assigned: dzbarsky)
Details
Attachments
(7 files)
65.79 KB,
patch
|
Ms2ger
:
review+
|
Details | Diff | Splinter Review |
46.52 KB,
patch
|
Ms2ger
:
review+
|
Details | Diff | Splinter Review |
8.59 KB,
patch
|
Ms2ger
:
review+
|
Details | Diff | Splinter Review |
25.24 KB,
patch
|
Ms2ger
:
review+
|
Details | Diff | Splinter Review |
27.27 KB,
patch
|
Ms2ger
:
review+
|
Details | Diff | Splinter Review |
62.88 KB,
patch
|
Ms2ger
:
review+
|
Details | Diff | Splinter Review |
58.93 KB,
patch
|
Ms2ger
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Attachment #733154 -
Flags: review?(Ms2ger)
![]() |
||
Comment 1•8 years ago
|
||
>+ nsDOMEvent* event = static_cast<nsDOMEvent*>(aDOMEvent);
You want aDOMEvent->InternalDOMEvent().
![]() |
||
Comment 2•8 years ago
|
||
Also, is the point just deCOM kinda things, or something else?
Assignee | ||
Comment 3•8 years ago
|
||
(In reply to Boris Zbarsky (:bz) from comment #2) > Also, is the point just deCOM kinda things Yup.
Assignee | ||
Comment 4•8 years ago
|
||
Comment 5•8 years ago
|
||
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.
Attachment #733154 -
Flags: review?(Ms2ger) → review+
Comment 6•8 years ago
|
||
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.
Attachment #733198 -
Flags: review?(Ms2ger) → review+
Assignee | ||
Comment 7•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/183c3d7a2c18 https://hg.mozilla.org/integration/mozilla-inbound/rev/efd4cd61eb3d
Whiteboard: [leave open]
Comment 8•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/183c3d7a2c18 https://hg.mozilla.org/mozilla-central/rev/efd4cd61eb3d
Assignee | ||
Comment 9•8 years ago
|
||
Attachment #734475 -
Flags: review?(Ms2ger)
Comment 10•8 years ago
|
||
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•8 years ago
|
||
Comment on attachment 734475 [details] [diff] [review] Part 3 Review of attachment 734475 [details] [diff] [review]: ----------------------------------------------------------------- lgtm, with that comment fixed.
Attachment #734475 -
Flags: review?(Ms2ger) → review+
Assignee | ||
Comment 12•8 years ago
|
||
Attachment #736152 -
Flags: review?(Ms2ger)
Comment 13•8 years ago
|
||
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
Attachment #736152 -
Flags: review?(Ms2ger) → review+
Assignee | ||
Comment 14•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/315f6b451d07 https://hg.mozilla.org/integration/mozilla-inbound/rev/a550dadafa30
Comment 15•8 years ago
|
||
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
Assignee | ||
Comment 16•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/2b156ae54c3e
Assignee | ||
Updated•8 years ago
|
Summary: Use dom::EventTarget more in content/ and dom/ → Use dom::EventTarget more instead of nsIDOMEventTarget
Assignee | ||
Comment 18•8 years ago
|
||
Attachment #738279 -
Flags: review?(Ms2ger)
Comment 19•8 years ago
|
||
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
Attachment #738279 -
Flags: review?(Ms2ger) → review+
Assignee | ||
Comment 20•8 years ago
|
||
Attachment #738865 -
Flags: review?(Ms2ger)
Assignee | ||
Comment 21•8 years ago
|
||
This should be the last one :)
Attachment #738866 -
Flags: review?(Ms2ger)
Comment 22•8 years ago
|
||
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
Attachment #738865 -
Flags: review?(Ms2ger) → review+
Comment 23•8 years ago
|
||
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); :/
Attachment #738866 -
Flags: review?(Ms2ger) → review+
Updated•8 years ago
|
OS: Mac OS X → All
Hardware: x86 → All
Whiteboard: [leave open]
Version: unspecified → Trunk
Assignee | ||
Comment 24•8 years ago
|
||
(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.
Assignee | ||
Comment 25•8 years ago
|
||
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•8 years ago
|
||
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
Assignee | ||
Comment 27•8 years ago
|
||
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
Whiteboard: [leave open]
Comment 28•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/16e42f1b8b58 https://hg.mozilla.org/mozilla-central/rev/62ec34a45dfd https://hg.mozilla.org/mozilla-central/rev/dcee2cd3394a
Assignee | ||
Comment 29•8 years ago
|
||
Part 7: https://hg.mozilla.org/integration/mozilla-inbound/rev/91cf58935be3
Whiteboard: [leave open]
Comment 30•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/91cf58935be3
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
Updated•2 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•