Label runnables in the following classes AsyncEventDispatcher IMEContentObserver EventListenerService

DataTransferItem

(In reply to Ming-Chou Shih [:stone] from comment #0) > Label runnables in the following classes > AsyncEventDispatcher I think AsyncEventDispatcher has been labeled by :billm in bug 1318506 if you're talking about AsyncEventDispatcher::PostDOMEvent(). > IMEContentObserver > EventListenerService

Comment on attachment 8849879 [details] [diff] [review] Label runnables in dom/events Review of attachment 8849879 [details] [diff] [review]: ----------------------------------------------------------------- Some use cases could be simplified. Please see my comments below. Thanks! ::: dom/events/EventListenerService.cpp @@ +365,5 @@ > } > > if (!mPendingListenerChanges) { > mPendingListenerChanges = nsArrayBase::Create(); > + nsCOMPtr<nsIRunnable> runnable = The name can be given by the other overrided NewRunnableMethod(char * aName, ...). So we can modify these as followed: nsCOMPtr<nsIRunnable> runnable = NewRunnableMethod("EventListenerService::NotifyAboutMainThreadListenerChangeInternal", this, &EventListenerService::NotifyPendingChanges); if (nsCOMPtr<nsIGlobalObject> global = aTarget->GetOwnerGlobal()) { global->Dispatch(nullptr, TaskCategory::Other, runnable.forget()); } else if (nsCOMPtr<nsINode> node = do_QueryInterface(aTarget)) { node->OwnerDoc()->Dispatch(nullptr, TaskCategory::Other, runnable.forget()); } else { NS_DispatchToMainThread(runnable); } ::: dom/events/IMEContentObserver.cpp @@ +1415,5 @@ > // it may kick runnable event immediately after DOM tree is changed but > // the selection range isn't modified yet. > mQueuedSender = new IMENotificationSender(this); > + MOZ_ASSERT(mDocShell); > + if (nsCOMPtr<nsPIDOMWindowOuter> win = mDocShell->GetWindow()) { we can simplify these as followed: if (nsCOMPtr<nsIScriptGlobalObject> globalObject = mDocShell->GetGetScriptGlobalObject()) { RefPtr<IMENotificationSender> queuedSender = mQueuedSender; globalObject->Dispatch("IMEContentObserver::FlushMergeableNotifications", TaskCategory::Other, queuedSender.forget()); } else { NS_DispatchToCurrentThread(mQueuedSender); } @@ +1548,5 @@ > mIMEContentObserver->mQueuedSender = > new IMENotificationSender(mIMEContentObserver); > + MOZ_ASSERT(mIMEContentObserver->mDocShell); > + if (nsCOMPtr<nsPIDOMWindowOuter> win = > + mIMEContentObserver->mDocShell->GetWindow()) { ditto @@ +1620,5 @@ > "posting IMENotificationSender to current thread", this)); > mIMEContentObserver->mQueuedSender = > new IMENotificationSender(mIMEContentObserver); > + MOZ_ASSERT(mIMEContentObserver->mDocShell); > + if (nsCOMPtr<nsPIDOMWindowOuter> win = ditto

Comment on attachment 8850259 [details] [diff] [review] Label runnables in dom/events (v3) Review of attachment 8850259 [details] [diff] [review]: ----------------------------------------------------------------- f=me, after the following comment are addressed. ::: dom/events/EventListenerService.cpp @@ +366,5 @@ > > if (!mPendingListenerChanges) { > mPendingListenerChanges = nsArrayBase::Create(); > + nsCOMPtr<nsIRunnable> runnable = > + NewRunnableMethod("EventListenerService::NotifyAboutMainThreadListenerChangeInternal", Please follow the naming convention in https://wiki.mozilla.org/Quantum/DOM#Runnable_naming_convention NewRunnableMethod("EventListenerService::NotifyPendingChanges", ...); ::: dom/events/IMEContentObserver.cpp @@ +1417,5 @@ > + MOZ_ASSERT(mDocShell); > + if (nsCOMPtr<nsIScriptGlobalObject> globalObject = > + mDocShell->GetScriptGlobalObject()) { > + RefPtr<IMENotificationSender> queuedSender = mQueuedSender; > + globalObject->Dispatch("IMEContentObserver::FlushMergeableNotifications", Please following the convention of runnable name in https://wiki.mozilla.org/Quantum/DOM#Runnable_naming_convention The runnable's name in the case is "IMENotificationSender". @@ +1551,5 @@ > + if (nsCOMPtr<nsIScriptGlobalObject> globalObject = > + mIMEContentObserver->mDocShell->GetScriptGlobalObject()) { > + RefPtr<IMENotificationSender> queuedSender = > + mIMEContentObserver->mQueuedSender; > + globalObject->Dispatch("IMENotificationSender::Run", ditto @@ +1624,5 @@ > + if (nsCOMPtr<nsIScriptGlobalObject> globalObject = > + mIMEContentObserver->mDocShell->GetScriptGlobalObject()) { > + RefPtr<IMENotificationSender> queuedSender = > + mIMEContentObserver->mQueuedSender; > + globalObject->Dispatch("IMENotificationSender::Run", ditto

Comment on attachment 8850436 [details] [diff] [review] Label runnables in dom/events (v4) >+ nsCOMPtr<nsINode> parentNode = >+ do_QueryInterface(mDataTransfer->GetParentObject()); >+ MOZ_ASSERT(parentNode); > RefPtr<GASRunnable> runnable = new GASRunnable(aCallback, stringData); >- rv = NS_DispatchToMainThread(runnable); >+ if (nsIDocument* doc = parentNode->OwnerDoc()) { >+ rv = doc->Dispatch("GASRunnable", TaskCategory::Other, >+ runnable.forget()); >+ } else { >+ rv = NS_DispatchToMainThread(runnable); >+ } Huh, the GASRunnable code is really weirdly commented. Comments hint that there is code running on non-main-thread, but that isn't the case, as far as I see So, looks ok to me > // If contents in selection range is modified, the selection range still > // has removed node from the tree. In such case, nsContentIterator won't > // work well. Therefore, we shouldn't use AddScriptRunnder() here since > // it may kick runnable event immediately after DOM tree is changed but > // the selection range isn't modified yet. > mQueuedSender = new IMENotificationSender(this); >- NS_DispatchToCurrentThread(mQueuedSender); >- >+ MOZ_ASSERT(mDocShell); >+ if (nsCOMPtr<nsIScriptGlobalObject> globalObject = >+ mDocShell->GetScriptGlobalObject()) { >+ RefPtr<IMENotificationSender> queuedSender = mQueuedSender; >+ globalObject->Dispatch("IMENotificationSender", >+ TaskCategory::Other, queuedSender.forget()); Could you use nsIScriptGlobalObject* here, not nsCOMPtr. nsCOMPtr does "slow" AddRef/Release and I don't see reason to explicitly have strong variable here. >@@ -1535,17 +1542,26 @@ IMEContentObserver::IMENotificationSende > // destroyed, mNeedsToNotifyIMEOfFocusSet is never set true again. > if (mIMEContentObserver->mNeedsToNotifyIMEOfFocusSet) { > MOZ_ASSERT(!mIMEContentObserver->mIMEHasFocus); > MOZ_LOG(sIMECOLog, LogLevel::Debug, > ("0x%p IMEContentObserver::IMENotificationSender::Run(), " > "posting IMENotificationSender to current thread", this)); > mIMEContentObserver->mQueuedSender = > new IMENotificationSender(mIMEContentObserver); >- NS_DispatchToCurrentThread(mIMEContentObserver->mQueuedSender); >+ MOZ_ASSERT(mIMEContentObserver->mDocShell); Useless assertion. We crash anyhow if mDocShell is null. Just add a null check inside the 'if' >+ if (nsCOMPtr<nsIScriptGlobalObject> globalObject = >+ mIMEContentObserver->mDocShell->GetScriptGlobalObject()) { >+ RefPtr<IMENotificationSender> queuedSender = >+ mIMEContentObserver->mQueuedSender; >+ globalObject->Dispatch("IMENotificationSender", >+ TaskCategory::Other, queuedSender.forget()); >+ } else { >+ NS_DispatchToCurrentThread(mIMEContentObserver->mQueuedSender); >+ } Same here, no need for nsCOMPtr for the global object > MOZ_LOG(sIMECOLog, LogLevel::Debug, > ("0x%p IMEContentObserver::IMENotificationSender::Run(), " > "posting IMENotificationSender to current thread", this)); > mIMEContentObserver->mQueuedSender = > new IMENotificationSender(mIMEContentObserver); >- NS_DispatchToCurrentThread(mIMEContentObserver->mQueuedSender); >+ MOZ_ASSERT(mIMEContentObserver->mDocShell); again, useless assertion. Just add check inside 'if' (or prove that mDocShell isn't ever null). >+ if (nsCOMPtr<nsIScriptGlobalObject> globalObject = >+ mIMEContentObserver->mDocShell->GetScriptGlobalObject()) { No need for nsCOMPtr Those fixed, r+

Pushed by cbook@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/ce25d866bd65 Label runnables in dom/events. f=bevistseng. r=smaug.

Backed out at smaug's request for causing bug 1351328. https://hg.mozilla.org/mozilla-central/rev/0e0eb96528a1

Comment on attachment 8854654 [details] [diff] [review] Label runnables in dom/events (v6) Review of attachment 8854654 [details] [diff] [review]: ----------------------------------------------------------------- f=me after the comments below are addressed, thanks! ::: dom/events/EventListenerService.cpp @@ +361,2 @@ > { > MOZ_ASSERT(NS_IsMainThread()); The aTarget will never be nullptr according to its call sites. Please add assertion to aTarget here if nullptr check on aTarget is not necessary: MOZ_ASSERT(aTarget); ::: dom/events/IMEContentObserver.cpp @@ +1418,5 @@ > + mDocShell->GetScriptGlobalObject() : > + nullptr; > + if (globalObject) { > + RefPtr<IMENotificationSender> queuedSender = mQueuedSender; > + globalObject->Dispatch("IMENotificationSender", The IMENotificationSender was named in its constructor. We can just pass nullptr here without naming it again in Dispatcher: http://searchfox.org/mozilla-central/source/xpcom/threads/Dispatcher.cpp#124-128 @@ +1553,5 @@ > + mIMEContentObserver->mDocShell->GetScriptGlobalObject() : nullptr; > + if (globalObject) { > + RefPtr<IMENotificationSender> queuedSender = > + mIMEContentObserver->mQueuedSender; > + globalObject->Dispatch("IMENotificationSender", ditto @@ +1627,5 @@ > + mIMEContentObserver->mDocShell->GetScriptGlobalObject() : nullptr; > + if (globalObject) { > + RefPtr<IMENotificationSender> queuedSender = > + mIMEContentObserver->mQueuedSender; > + globalObject->Dispatch("IMENotificationSender", ditto

Comment on attachment 8856436 [details] [diff] [review] Label runnables in dom/events (v7) yeah, DataTransfer is rather annoying.

Updated the patch summary. The tried results are in https://treeherder.mozilla.org/#/jobs?repo=try&revision=9fbfc7aab13b4f4ac6e78657f55b776fc030165b Looks like there are some infra problems on Windows 10 x64 asan

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/f52e34fa7a0b Label runnables in dom/events. f=bevistseng, r=smaug