Closed
Bug 896807
Opened 11 years ago
Closed 2 years ago
Make runnables inherit from nsRunnable
Categories
(Core :: DOM: Core & HTML, defect, P5)
Tracking
()
RESOLVED
WONTFIX
People
(Reporter: jcranmer, Assigned: drexler)
Details
Attachments
(5 files)
27.03 KB,
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
13.87 KB,
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
14.99 KB,
patch
|
ehsan.akhgari
:
review+
benjamin
:
review-
|
Details | Diff | Splinter Review |
9.22 KB,
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
10.05 KB,
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
Bug 884061 comment 80: Hrm, could you file a followup to make ProgressRunnable, CloseRunnable, DestroyRunnable and FinishHelper to inherit nsRunnable (which takes care of refcounting) Also AsyncConnectionHelper, CheckPermissionsHelper, CommitHelper, AsyncDeleteFileRunnable, GetFileReferencesHelper, OpenDatabaseHelper, FinishTransactionRunnable, TransactionQueue, CheckQuotaHelper, AsyncUsageRunnable, WaitForTransactionsToFinishRunnable, WaitForLockedFilesToFinishRunnable, ScriptLoaderRunnable and WorkerRunnable should probably just extend nsRunnable.
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → andrew.quartey
Assignee | ||
Comment 1•11 years ago
|
||
Olli's review queue looks full...care to review these changes? :)
Attachment #814275 -
Flags: review?(ehsan)
Assignee | ||
Comment 2•11 years ago
|
||
Attachment #814276 -
Flags: review?(ehsan)
Assignee | ||
Comment 3•11 years ago
|
||
Attachment #814277 -
Flags: review?(ehsan)
Assignee | ||
Comment 4•11 years ago
|
||
Assignee | ||
Comment 5•11 years ago
|
||
Attachment #814279 -
Flags: review?(ehsan)
Assignee | ||
Updated•11 years ago
|
Attachment #814278 -
Flags: review?(ehsan)
Comment 6•11 years ago
|
||
Comment on attachment 814275 [details] [diff] [review] Part a: /dom changes. Review of attachment 814275 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/indexedDB/AsyncConnectionHelper.h @@ +85,1 @@ > class AsyncConnectionHelper : public HelperBase, Nit: mark this as final please if possible. ::: dom/indexedDB/IDBTransaction.cpp @@ +59,5 @@ > } > > // This runnable doesn't actually do anything beyond "prime the pump" and get > // transactions in the right order on the transaction thread pool. > +class StartTransactionRunnable : public nsRunnable This class is very weird. Ben, can you review this part of the patch, please? ::: dom/src/notification/Notification.cpp @@ +71,5 @@ > protected: > nsRefPtr<Notification> mNotification; > }; > > +class NotificationTask : public nsRunnable final
Attachment #814275 -
Flags: review?(ehsan)
Attachment #814275 -
Flags: review?(bent.mozilla)
Attachment #814275 -
Flags: review+
Comment 7•11 years ago
|
||
Comment on attachment 814276 [details] [diff] [review] Part b: /netwerk changes Review of attachment 814276 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/protocol/websocket/WebSocketChannel.cpp @@ +506,5 @@ > : mChannel(aChannel), > mData(aData), > mLen(aLen) {} > > NS_IMETHOD Run() Nit: MOZ_OVERRIDE. @@ +535,5 @@ > nsresult aReason) > : mChannel(aChannel), > mReason(aReason) {} > > NS_IMETHOD Run() Nit: MOZ_OVERRIDE. @@ +570,5 @@ > : mChannel(aChannel), > mCode(aCode), > mReason(aReason) {} > > NS_IMETHOD Run() Nit: MOZ_OVERRIDE. @@ +596,5 @@ > uint32_t aSize) > : mChannel(aChannel), > mSize(aSize) {} > > NS_IMETHOD Run() Nit: MOZ_OVERRIDE. @@ +624,5 @@ > nsIAsyncOutputStream *aSocketOut) > : mChannel(aChannel), > mTransport(aTransport), > mSocketIn(aSocketIn), > mSocketOut(aSocketOut) {} Nit: MOZ_OVERRIDE on Run() @@ +762,4 @@ > OutboundEnqueuer(WebSocketChannel *aChannel, OutboundMessage *aMsg) > : mChannel(aChannel), mMessage(aMsg) {} > > NS_IMETHOD Run() Nit: MOZ_OVERRIDE. ::: netwerk/test/TestCommon.h @@ +23,1 @@ > NS_IMETHOD Run() { Nit: MOZ_OVERRIDE. ::: netwerk/test/TestFileInput2.cpp @@ +127,5 @@ > } > > //////////////////////////////////////////////////////////////////////////////// > > +class FileSpecWorker : public nsRunnable { Nit: final. @@ +132,3 @@ > public: > > NS_IMETHOD Run() { Nit: MOZ_OVERRIDE. @@ +221,5 @@ > > #include "nsIIOService.h" > #include "nsIChannel.h" > > +class FileChannelWorker : public nsRunnable { Nit: MOZ_FINAL. @@ +226,3 @@ > public: > > NS_IMETHOD Run() { Nit: MOZ_OVERRIDE. ::: netwerk/test/TestIOThreads.cpp @@ +18,5 @@ > static PRLogModuleInfo *gTestLog = nullptr; > #endif > #define LOG(args) PR_LOG(gTestLog, PR_LOG_DEBUG, args) > > +class nsIOEvent : public nsRunnable { Nit: MOZ_FINAL. @@ +24,3 @@ > nsIOEvent(int i) : mIndex(i) {} > > NS_IMETHOD Run() { Nit: MOZ_OVERRIDE.
Attachment #814276 -
Flags: review?(ehsan) → review+
Comment 8•11 years ago
|
||
Comment on attachment 814277 [details] [diff] [review] Part c: /xpcom changes Review of attachment 814277 [details] [diff] [review]: ----------------------------------------------------------------- ::: xpcom/base/AvailableMemoryTracker.cpp @@ +408,5 @@ > + NS_IMETHOD Run() MOZ_OVERRIDE > + { > + MOZ_ASSERT(NS_IsMainThread()); > + > + #if defined(MOZ_MEMORY) Please do not indent preprocessor macros like this. ::: xpcom/base/nsMemoryImpl.cpp @@ -171,5 @@ > > -// XXX need NS_IMPL_STATIC_ADDREF/RELEASE > -NS_IMETHODIMP_(nsrefcnt) nsMemoryImpl::FlushEvent::AddRef() { return 2; } > -NS_IMETHODIMP_(nsrefcnt) nsMemoryImpl::FlushEvent::Release() { return 1; } > -NS_IMPL_QUERY_INTERFACE1(nsMemoryImpl::FlushEvent, nsIRunnable) I'm not sure if this is OK. Benjamin? ::: xpcom/tests/TestPipes.cpp @@ +102,5 @@ > } > return NS_OK; > } > > +class nsReceiver : public nsRunnable { Nit: MOZ_FINAL. @@ +108,1 @@ > NS_IMETHOD Run() { Nit: MOZ_OVERRIDE. @@ +189,5 @@ > } > > //////////////////////////////////////////////////////////////////////////////// > > +class nsShortReader : public nsRunnable { Nit: MOZ_FINAL. @@ +195,1 @@ > NS_IMETHOD Run() { Nit: MOZ_OVERRIDE. @@ +303,5 @@ > } > > //////////////////////////////////////////////////////////////////////////////// > > +class nsPump : public nsRunnable Nit: MOZ_FINAL. @@ +310,1 @@ > NS_IMETHOD Run() { Nit: MOZ_OVERRIDE. ::: xpcom/tests/TestThreadPool.cpp @@ +15,2 @@ > > +class Task : public nsRunnable Nit: MOZ_FINAL. @@ +19,3 @@ > Task(int i) : mIndex(i) {} > > NS_IMETHOD Run() Nit: MOZ_OVERRIDE. ::: xpcom/tests/TestThreads.cpp @@ +10,5 @@ > #include "nsCOMPtr.h" > #include "nsIServiceManager.h" > #include "nsXPCOM.h" > > +class nsRunner : public nsRunnable { Nit: MOZ_FINAL. @@ +16,1 @@ > NS_IMETHOD Run() { Nit: MOZ_OVERRIDE.
Attachment #814277 -
Flags: review?(ehsan)
Attachment #814277 -
Flags: review?(benjamin)
Attachment #814277 -
Flags: review+
Updated•11 years ago
|
Attachment #814278 -
Flags: review?(ehsan) → review+
Comment 9•11 years ago
|
||
Comment on attachment 814279 [details] [diff] [review] Part e: /hal /ipc /security changes. Review of attachment 814279 [details] [diff] [review]: ----------------------------------------------------------------- ::: security/manager/ssl/src/nsCrypto.cpp @@ +181,5 @@ > //This class is used to run the callback code > //passed to crypto.generateCRMFRequest > //We have to do that for backwards compatibility > //reasons w/ PSM 1.x and Communciator 4.x > +class nsCryptoRunnable : public nsRunnable { Nit: MOZ_FINAL. @@ +186,5 @@ > public: > nsCryptoRunnable(nsCryptoRunArgs *args); > virtual ~nsCryptoRunnable(); > > NS_IMETHOD Run (); Nit: MOZ_OVERRIDE. @@ +196,5 @@ > //We're going to inherit the memory passed > //into us. > //This class backs up an array of certificates > //as an event. > +class nsP12Runnable : public nsRunnable { Nit: MOZ_FINAL. @@ +201,5 @@ > public: > nsP12Runnable(nsIX509Cert **certArr, int32_t numCerts, nsIPK11Token *token); > virtual ~nsP12Runnable(); > > NS_IMETHOD Run(); Nit: MOZ_OVERRIDE. ::: security/manager/ssl/src/nsNSSComponent.cpp @@ +89,5 @@ > > #ifndef MOZ_DISABLE_CRYPTOLEGACY > //This class is used to run the callback code > //passed to the event handlers for smart card notification > +class nsTokenEventRunnable : public nsRunnable { Nit: MOZ_FINAL. @@ +94,5 @@ > public: > nsTokenEventRunnable(const nsAString &aType, const nsAString &aTokenName); > virtual ~nsTokenEventRunnable(); > > NS_IMETHOD Run (); Nit: MOZ_OVERRIDE.
Attachment #814279 -
Flags: review?(ehsan) → review+
Comment 10•11 years ago
|
||
Comment on attachment 814277 [details] [diff] [review] Part c: /xpcom changes Review of attachment 814277 [details] [diff] [review]: ----------------------------------------------------------------- ::: xpcom/base/nsMemoryImpl.cpp @@ -171,5 @@ > > -// XXX need NS_IMPL_STATIC_ADDREF/RELEASE > -NS_IMETHODIMP_(nsrefcnt) nsMemoryImpl::FlushEvent::AddRef() { return 2; } > -NS_IMETHODIMP_(nsrefcnt) nsMemoryImpl::FlushEvent::Release() { return 1; } > -NS_IMPL_QUERY_INTERFACE1(nsMemoryImpl::FlushEvent, nsIRunnable) This is definitely not ok, since FlushEvent is a static singleton. I don't think this should touch FlushEvent at all.
Attachment #814277 -
Flags: review?(benjamin) → review-
Comment on attachment 814275 [details] [diff] [review] Part a: /dom changes. I really don't support this patch (or the others). Favoring nsRunnable vs. nsIRunnable only makes the leak logs useless.
Attachment #814275 -
Flags: review?(bent.mozilla)
Comment 12•11 years ago
|
||
(In reply to ben turner [:bent] (needinfo? encouraged) from comment #11) > Comment on attachment 814275 [details] [diff] [review] > Part a: /dom changes. > > I really don't support this patch (or the others). Favoring nsRunnable vs. > nsIRunnable only makes the leak logs useless. Note that this is true in the cases where this patch does not add an inherited supports implementation. I will recede my r+es based on Ben's objection on those hunks.
Updated•6 years ago
|
Priority: -- → P5
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
Updated•2 years ago
|
Severity: normal → S3
Comment 13•2 years ago
|
||
There were some objections to some of these patches almost a decade ago, then no activity since, so I'm just going to go ahead and WONTFIX this. I haven't considered the merits of the patches myself.
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → WONTFIX
You need to log in
before you can comment on or make changes to this bug.
Description
•