Closed Bug 896807 Opened 11 years ago Closed 2 years ago

Make runnables inherit from nsRunnable

Categories

(Core :: DOM: Core & HTML, defect, P5)

x86_64
Windows 7
defect

Tracking

()

RESOLVED WONTFIX

People

(Reporter: jcranmer, Assigned: drexler)

Details

Attachments

(5 files)

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: nobody → andrew.quartey
Olli's review queue looks full...care to review these changes? :)
Attachment #814275 - Flags: review?(ehsan)
Attachment #814276 - Flags: review?(ehsan)
Attachment #814277 - Flags: review?(ehsan)
Attachment #814279 - Flags: review?(ehsan)
Attachment #814278 - Flags: review?(ehsan)
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 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 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+
Attachment #814278 - Flags: review?(ehsan) → review+
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 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)
(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.
Priority: -- → P5
Component: DOM → DOM: Core & HTML
Severity: normal → S3

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.

Attachment

General

Created:
Updated:
Size: