Some callers of NewRunnableMethod have overridden their RunnableMethodTraits to do nothing. These are easy to convert. NB: A bunch of the RefPtr<T> runnable stuff will go away again later.

Like part 2 but for cancelable things. After this there are no uses of RunnableMethodTraits left.

This textually undoes a lot of the previous patches, but it was useful to verify I was doing things correctly along the way.

Comment on attachment 8746327 [details] [diff] [review] Part 1: Be explicit about which NewRunnableMethod callers want CancelableRunnables Review of attachment 8746327 [details] [diff] [review]: ----------------------------------------------------------------- Explicit is better than implicit.

Comment on attachment 8746328 [details] [diff] [review] Part 2: Replace some NewRunnableMethods with NS_NewNonOwningRunnableMethod Review of attachment 8746328 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/ipc/ProcessHangMonitor.cpp @@ +380,5 @@ > + unsigned>(this, > + &HangMonitorChild::NotifySlowScriptAsync, > + id, filename, aLineNo); > + MonitorLoop()->PostTask(runnable.forget()); > + Nit: whitespace.

Comment on attachment 8746329 [details] [diff] [review] Part 3: Replace some NewRunnableMethods with NS_NewNonOwningCancelableRunnableMethod Review of attachment 8746329 [details] [diff] [review]: ----------------------------------------------------------------- The patch summary should really read "replace some NewCancelableRunnableMethods with NS_NewNonOwningCancelableRunnableMethod", right? ::: gfx/layers/AtomicRefCountedWithFinalize.h @@ -68,5 @@ > template<class U> > friend struct mozilla::RefPtrTraits; > > - template<class U> > - friend struct ::RunnableMethodTraits; \o/ ::: gfx/layers/ipc/ImageBridgeParent.cpp @@ +100,5 @@ > { > // Can't alloc/dealloc shmems from now on. > mClosed = true; > > + if (mSubprocess) { Nit: indentation went wonky here. @@ +106,5 @@ > mSubprocess = nullptr; > } > > + RefPtr<Runnable> runnable = NS_NewRunnableMethod(this, &ImageBridgeParent::DeferredDestroy); > + MessageLoop::current()->PostTask(runnable.forget()); Does this change go someplace else? ::: ipc/glue/MessageChannel.cpp @@ +497,5 @@ > > + RefPtr<CancelableRunnable> runnable = > + NS_NewNonOwningCancelableRunnableMethod(this, &MessageChannel::OnMaybeDequeueOne); > + mDequeueOneTask = new RefCountedTask(runnable.forget()); > + Nit: whitespace. ::: xpcom/glue/nsThreadUtils.h @@ +286,5 @@ > + bool Owning = true, > + bool Cancelable = false> > +class nsRunnableMethod : public mozilla::Conditional<!Cancelable, > + mozilla::Runnable, > + mozilla::CancelableRunnable>::Type Nice. @@ +716,5 @@ > } > return NS_OK; > } > + nsresult Cancel() { > + static_assert(Cancelable, "Don't use me!"); This works even when Cancelable is false? Someday I will understand all the template instantiation rules...

Comment on attachment 8746331 [details] [diff] [review] Part 5: Make NS_NewRunnableMethod able to call const functions Review of attachment 8746331 [details] [diff] [review]: ----------------------------------------------------------------- ::: xpcom/glue/nsThreadUtils.h @@ +342,5 @@ > static const bool can_cancel = Cancelable; > }; > > +template<class C, typename R, bool Owning, bool Cancelable, typename... As> > +struct nsRunnableMethodTraits<R(C::*)(As...) const, Owning, Cancelable> Probably worth duplicating this for the NS_HAVE_STDCALL cases below just so somebody doesn't wind up pounding their head against template-fu later on.

Comment on attachment 8746332 [details] [diff] [review] Part 6: Replace NewRunnableMethod Review of attachment 8746332 [details] [diff] [review]: ----------------------------------------------------------------- r=me with changes below and pending adequate explanations to the questions. ::: dom/ipc/ContentBridgeChild.cpp @@ +34,5 @@ > void > ContentBridgeChild::ActorDestroy(ActorDestroyReason aWhy) > { > + RefPtr<Runnable> runnable = NS_NewRunnableMethod(this, &ContentBridgeChild::DeferredDestroy); > + MessageLoop::current()->PostTask(runnable.forget()); I'm starting to think NS_New*RunnableMethod should just return an already_AddRefed, so we can make this sort of stuff shorter. Dollars to donuts somebody is going to complain about this when you announce the changes, if they haven't already. This can be a followup. ::: dom/ipc/ContentBridgeParent.h @@ +81,5 @@ > > + void Close() > + { > + // Trick NS_NewRunnableMethod > + PContentBridgeParent::Close(); Uh. This is weird. Can you elaborate? Why not cast to PContentBridgeParent when calling NS_NewRunnableMethod? ::: dom/ipc/TabChild.cpp @@ +604,5 @@ > mAsyncPanZoomEnabled = gfxPlatform::AsyncPanZoomEnabled(); > > nsWeakPtr weakPtrThis(do_GetWeakReference(static_cast<nsITabChild*>(this))); // for capture by the lambda > mSetAllowedTouchBehaviorCallback = [weakPtrThis](uint64_t aInputBlockId, > + nsTArray<TouchBehaviorFlags>&& aFlags) So we have a couple of these sorts of changes...can you explain why they're necessary and safe? Some of them look pretty dodgy. ::: dom/media/gmp/GMPDecryptorChild.cpp @@ +60,5 @@ > } else { > // Use const reference when we have to. > auto m = &GMPDecryptorChild::CallMethod< > decltype(aMethod), typename AddConstReference<ParamType>::Type...>; > + RefPtr<mozilla::Runnable> t = shitty_code::NewRunnableMethod(this, m, aMethod, Forward<ParamType>(aParams)...); Why is this? Is NS_NewRunnableMethodWithArgs's template-fu not strong enough here? ::: ipc/chromium/src/base/task.h @@ +304,5 @@ > }; > > +namespace shitty_code { > + > +// Don't add new uses of this!!!! I appreciate the tactic here; the name is a tad unprofessional. How about |namespace dont_use_this_or_khuey_will_hunt_you_down|? Is that any less unprofessional? Maybe |namespace dont_use_this_without_khuey_reviewing|? ::: widget/nsBaseWidget.cpp @@ +981,5 @@ > + StoreCopyPassByRRef<nsTArray<TouchBehaviorFlags>>>(treeManager, > + &APZCTreeManager::SetAllowedTouchBehavior, > + aInputBlockId, mozilla::Move(aFlags)); > + APZThreadUtils::RunOnControllerThread(runnable.forget()); > + Nit: whitespace.

Comment on attachment 8746333 [details] [diff] [review] Part 7: Move NS_NewRunnableMethod* to mozilla::NewRunnableMethod Review of attachment 8746333 [details] [diff] [review]: ----------------------------------------------------------------- I appreciate this change being broken out into a separate patch. ::: xpcom/glue/nsThreadUtils.h @@ +749,2 @@ > template<typename PtrType, typename Method> > +already_AddRefed<typename nsRunnableMethodTraits<Method, true, false>::base_type> Bonus points for making this change. @@ +883,5 @@ > } > return *this; > } > > + const nsRevocableEventPtr& operator=(already_AddRefed<T> aEvent) Why is this necessary amidst the sea of renaming?

(In reply to Nathan Froyd [:froydnj] from comment #13) > @@ +883,5 @@ > > } > > return *this; > > } > > > > + const nsRevocableEventPtr& operator=(already_AddRefed<T> aEvent) > > Why is this necessary amidst the sea of renaming? Actually, never mind, I think I understand why this is, given the changes to NS_New*Runnable*.

(In reply to Nathan Froyd [:froydnj] from comment #10) > The patch summary should really read "replace some > NewCancelableRunnableMethods with NS_NewNonOwningCancelableRunnableMethod", > right? Yes. > @@ +106,5 @@ > > mSubprocess = nullptr; > > } > > > > + RefPtr<Runnable> runnable = NS_NewRunnableMethod(this, &ImageBridgeParent::DeferredDestroy); > > + MessageLoop::current()->PostTask(runnable.forget()); > > Does this change go someplace else? No ... > @@ +716,5 @@ > > } > > return NS_OK; > > } > > + nsresult Cancel() { > > + static_assert(Cancelable, "Don't use me!"); > > This works even when Cancelable is false? Someday I will understand all the > template instantiation rules... Yes. I believe it's because if Cancelable is false this doesn't override anything which means it's a regular member function on a template class, and only gets instantiated if it is used by something. Whereas if it were overriding a virtual function on the base class then it would have to be instantiated.

(In reply to Nathan Froyd [:froydnj] from comment #12) > Comment on attachment 8746332 [details] [diff] [review] > Part 6: Replace NewRunnableMethod > > Review of attachment 8746332 [details] [diff] [review]: > ----------------------------------------------------------------- > > r=me with changes below and pending adequate explanations to the questions. > > ::: dom/ipc/ContentBridgeChild.cpp > @@ +34,5 @@ > > void > > ContentBridgeChild::ActorDestroy(ActorDestroyReason aWhy) > > { > > + RefPtr<Runnable> runnable = NS_NewRunnableMethod(this, &ContentBridgeChild::DeferredDestroy); > > + MessageLoop::current()->PostTask(runnable.forget()); > > I'm starting to think NS_New*RunnableMethod should just return an > already_AddRefed, so we can make this sort of stuff shorter. Dollars to > donuts somebody is going to complain about this when you announce the > changes, if they haven't already. This can be a followup. :) > ::: dom/ipc/ContentBridgeParent.h > @@ +81,5 @@ > > > > + void Close() > > + { > > + // Trick NS_NewRunnableMethod > > + PContentBridgeParent::Close(); > > Uh. This is weird. Can you elaborate? Why not cast to > PContentBridgeParent when calling NS_NewRunnableMethod? NS_NewRunnableMethod(instance, &class::function); binds to the type of class, not the type of instance. So here we do NS_NewRunnableMethod(contentBridgeParent, &PContentBridgeParent::Close), and it fails to compile because PContentBridgeParent is not a refcounted type. > ::: dom/ipc/TabChild.cpp > @@ +604,5 @@ > > mAsyncPanZoomEnabled = gfxPlatform::AsyncPanZoomEnabled(); > > > > nsWeakPtr weakPtrThis(do_GetWeakReference(static_cast<nsITabChild*>(this))); // for capture by the lambda > > mSetAllowedTouchBehaviorCallback = [weakPtrThis](uint64_t aInputBlockId, > > + nsTArray<TouchBehaviorFlags>&& aFlags) > > So we have a couple of these sorts of changes...can you explain why they're > necessary and safe? Some of them look pretty dodgy. Basically this array was being copied at every point and the chromium templates were hiding that. I probably should have broken this out separately ... > ::: dom/media/gmp/GMPDecryptorChild.cpp > @@ +60,5 @@ > > } else { > > // Use const reference when we have to. > > auto m = &GMPDecryptorChild::CallMethod< > > decltype(aMethod), typename AddConstReference<ParamType>::Type...>; > > + RefPtr<mozilla::Runnable> t = shitty_code::NewRunnableMethod(this, m, aMethod, Forward<ParamType>(aParams)...); > > Why is this? Is NS_NewRunnableMethodWithArgs's template-fu not strong > enough here? Right. NS_NewRunnableMethodWithArg requires you to specify the storage types, whereas the chromium template guesses. The macro will have to be unwound manually to get rid of this. > ::: ipc/chromium/src/base/task.h > @@ +304,5 @@ > > }; > > > > +namespace shitty_code { > > + > > +// Don't add new uses of this!!!! > > I appreciate the tactic here; the name is a tad unprofessional. > > How about |namespace dont_use_this_or_khuey_will_hunt_you_down|? Is that > any less unprofessional? Maybe |namespace > dont_use_this_without_khuey_reviewing|? Fixed.

(In reply to Nathan Froyd [:froydnj] from comment #13) > @@ +883,5 @@ > > } > > return *this; > > } > > > > + const nsRevocableEventPtr& operator=(already_AddRefed<T> aEvent) > > Why is this necessary amidst the sea of renaming? Because I changed the method-previously-known-as-NS_NewRunnableMethod to return already_AddRefed, and nsRevocableEventPtr doesn't accept already_AddRefed yet. This has nothing to do with the renaming.

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #16) > (In reply to Nathan Froyd [:froydnj] from comment #12) > > ::: dom/ipc/ContentBridgeChild.cpp > > @@ +34,5 @@ > > > void > > > ContentBridgeChild::ActorDestroy(ActorDestroyReason aWhy) > > > { > > > + RefPtr<Runnable> runnable = NS_NewRunnableMethod(this, &ContentBridgeChild::DeferredDestroy); > > > + MessageLoop::current()->PostTask(runnable.forget()); > > > > I'm starting to think NS_New*RunnableMethod should just return an > > already_AddRefed, so we can make this sort of stuff shorter. Dollars to > > donuts somebody is going to complain about this when you announce the > > changes, if they haven't already. This can be a followup. > > :) khuey, responding to review comments before they're written. > > ::: dom/ipc/ContentBridgeParent.h > > @@ +81,5 @@ > > > > > > + void Close() > > > + { > > > + // Trick NS_NewRunnableMethod > > > + PContentBridgeParent::Close(); > > > > Uh. This is weird. Can you elaborate? Why not cast to > > PContentBridgeParent when calling NS_NewRunnableMethod? > > NS_NewRunnableMethod(instance, &class::function); binds to the type of > class, not the type of instance. So here we do > NS_NewRunnableMethod(contentBridgeParent, &PContentBridgeParent::Close), and > it fails to compile because PContentBridgeParent is not a refcounted type. Ah, fair enough. > > ::: dom/ipc/TabChild.cpp > > @@ +604,5 @@ > > > mAsyncPanZoomEnabled = gfxPlatform::AsyncPanZoomEnabled(); > > > > > > nsWeakPtr weakPtrThis(do_GetWeakReference(static_cast<nsITabChild*>(this))); // for capture by the lambda > > > mSetAllowedTouchBehaviorCallback = [weakPtrThis](uint64_t aInputBlockId, > > > + nsTArray<TouchBehaviorFlags>&& aFlags) > > > > So we have a couple of these sorts of changes...can you explain why they're > > necessary and safe? Some of them look pretty dodgy. > > Basically this array was being copied at every point and the chromium > templates were hiding that. I probably should have broken this out > separately ... OK, I don't think my r+ is enough to cover whether they can be Move'd safely. Can you either make the storage type StoreCopyPassByConstLRef (which I think doesn't require any changes), or get somebody to review the Move()s into the NewRunnableMethod invocations and determine whether they're safe? I think they're all in APZ code, so perhaps Botond is a good choice.

https://hg.mozilla.org/integration/mozilla-inbound/rev/af51821b2fc5 https://hg.mozilla.org/integration/mozilla-inbound/rev/fd833da413ad https://hg.mozilla.org/integration/mozilla-inbound/rev/3f2f3fcac66b https://hg.mozilla.org/integration/mozilla-inbound/rev/6a8d57e8fa8e https://hg.mozilla.org/integration/mozilla-inbound/rev/726f7361e0b8 https://hg.mozilla.org/integration/mozilla-inbound/rev/4bcb784492bb https://hg.mozilla.org/integration/mozilla-inbound/rev/85ce8cb0639a

sorry had to back this out for frequent memory leaks like https://treeherder.mozilla.org/logviewer.html#?job_id=26873803&repo=mozilla-inbound

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/9979c96310a0 https://hg.mozilla.org/integration/mozilla-inbound/rev/c6884584fa07 https://hg.mozilla.org/integration/mozilla-inbound/rev/4bd91d8dd4f5 https://hg.mozilla.org/integration/mozilla-inbound/rev/c70220f1b234 https://hg.mozilla.org/integration/mozilla-inbound/rev/8e6e8bbf54ea https://hg.mozilla.org/integration/mozilla-inbound/rev/7755d12b8f1c https://hg.mozilla.org/integration/mozilla-inbound/rev/4026758d1d02

https://hg.mozilla.org/integration/mozilla-inbound/rev/bf5001ed992f https://hg.mozilla.org/integration/mozilla-inbound/rev/c8b8d67ee833 https://hg.mozilla.org/integration/mozilla-inbound/rev/9f23832812fd https://hg.mozilla.org/integration/mozilla-inbound/rev/25c97bee4358 https://hg.mozilla.org/integration/mozilla-inbound/rev/4ae434c0a2c8 https://hg.mozilla.org/integration/mozilla-inbound/rev/67b25748b63a https://hg.mozilla.org/integration/mozilla-inbound/rev/114ca1fc9c51

https://hg.mozilla.org/mozilla-central/rev/bf5001ed992f https://hg.mozilla.org/mozilla-central/rev/c8b8d67ee833 https://hg.mozilla.org/mozilla-central/rev/9f23832812fd https://hg.mozilla.org/mozilla-central/rev/25c97bee4358 https://hg.mozilla.org/mozilla-central/rev/4ae434c0a2c8 https://hg.mozilla.org/mozilla-central/rev/67b25748b63a https://hg.mozilla.org/mozilla-central/rev/114ca1fc9c51