Let addon-sdk/source/lib/sdk/io/stream.js use STREAM transport service instead

RESOLVED FIXED

Status

Add-on SDK
General
P2
major
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mayhemer, Assigned: mayhemer)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

http://hg.mozilla.org/mozilla-central/annotate/37d3dcbf23a9/addon-sdk/source/lib/sdk/io/stream.js#l296

Where the eventTarget is socket transport service.  It should use "@mozilla.org/network/stream-transport-service;1".
Assignee: nobody → honzab.moz
Status: NEW → ASSIGNED
Created attachment 8583998 [details] [diff] [review]
v1
Attachment #8583998 - Flags: review?(wbamberg)
thanks honza!
(In reply to Honza Bambas (:mayhemer) from comment #0)
> http://hg.mozilla.org/mozilla-central/annotate/37d3dcbf23a9/addon-sdk/source/
> lib/sdk/io/stream.js#l296
> 
> Where the eventTarget is socket transport service.  It should use
> "@mozilla.org/network/stream-transport-service;1".

This there a bug describing this change which this bug should be dependent on?
Flags: needinfo?(honzab.moz)
Priority: -- → P2
Comment on attachment 8583998 [details] [diff] [review]
v1

I'm not qualified to review this. Erik, would you mind taking it?
Flags: needinfo?(evold)
Attachment #8583998 - Flags: review?(wbamberg)
(In reply to Erik Vold [:erikvold] (please needinfo? me) from comment #3)
> (In reply to Honza Bambas (:mayhemer) from comment #0)
> > http://hg.mozilla.org/mozilla-central/annotate/37d3dcbf23a9/addon-sdk/source/
> > lib/sdk/io/stream.js#l296
> > 
> > Where the eventTarget is socket transport service.  It should use
> > "@mozilla.org/network/stream-transport-service;1".
> 
> This there a bug describing this change which this bug should be dependent
> on?

I don't understand the question.
Flags: needinfo?(honzab.moz)
(In reply to Honza Bambas (:mayhemer) from comment #5)
> (In reply to Erik Vold [:erikvold] (please needinfo? me) from comment #3)
> > (In reply to Honza Bambas (:mayhemer) from comment #0)
> > > http://hg.mozilla.org/mozilla-central/annotate/37d3dcbf23a9/addon-sdk/source/
> > > lib/sdk/io/stream.js#l296
> > > 
> > > Where the eventTarget is socket transport service.  It should use
> > > "@mozilla.org/network/stream-transport-service;1".
> > 
> > This there a bug describing this change which this bug should be dependent
> > on?
> 
> I don't understand the question.

I'm not sure why `stream-transport-service` should be instead of `socket-transport-service` here.

Though the purpose of the module is streaming, it appears we can use either method to get the same nsIEventTarget service.
Flags: needinfo?(honzab.moz)
Flags: needinfo?(evold)
Flags: needinfo?(dtownsend)
(In reply to Erik Vold [:erikvold] (please needinfo? me) from comment #6)
> (In reply to Honza Bambas (:mayhemer) from comment #5)
> > (In reply to Erik Vold [:erikvold] (please needinfo? me) from comment #3)
> > > (In reply to Honza Bambas (:mayhemer) from comment #0)
> > > > http://hg.mozilla.org/mozilla-central/annotate/37d3dcbf23a9/addon-sdk/source/
> > > > lib/sdk/io/stream.js#l296
> > > > 
> > > > Where the eventTarget is socket transport service.  It should use
> > > > "@mozilla.org/network/stream-transport-service;1".
> > > 
> > > This there a bug describing this change which this bug should be dependent
> > > on?
> > 
> > I don't understand the question.
> 
> I'm not sure why `stream-transport-service` should be instead of
> `socket-transport-service` here.
> 
> Though the purpose of the module is streaming, it appears we can use either
> method to get the same nsIEventTarget service.

The two implementations of nsIEventTarget differ though. In the socket service there is just a single thread that any events are dispatched to, the stream transport on the other hand manages a pool of 25 threads. I'd guess that using the socket service might mean we block actual network traffic but either way seems sensible to use the stream service given that we're using the stream APIs.
Flags: needinfo?(dtownsend)
Comment on attachment 8583998 [details] [diff] [review]
v1

Given Mossop's feedback
Attachment #8583998 - Flags: review+

Comment 9

3 years ago
Commit pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/e3caddf2da1b3441706abd530064e26bbc0e1b67
Bug 1147921 - Let addon-sdk/source/lib/sdk/io/stream.js use STREAM transport service instead r=erikvold
Flags: needinfo?(honzab.moz)
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.