Closed
Bug 152663
Opened 22 years ago
Closed 14 years ago
Freeze nsIProxiedProtocolHandler and nsIProxyInfo
Categories
(Core :: Networking, defect, P5)
Tracking
()
RESOLVED
WONTFIX
People
(Reporter: dougt, Unassigned)
References
Details
Attachments
(2 files)
1.99 KB,
patch
|
Details | Diff | Splinter Review | |
1.99 KB,
patch
|
Details | Diff | Splinter Review |
The only way to write a protocol handler which can proxy is to implement these
two interfaces. Furthermore, the way nsIOService is written, you can not
override a nsIProtocolHandler without also implementing the above interfaces.
These interfaces should be frozen without modification (other than the ACString
change) so that we can have compatiblity with mozilla 1.0.
Comment 1•22 years ago
|
||
we probably want to complete the design for PAC failover before freezing these
interfaces.
Reporter | ||
Comment 2•22 years ago
|
||
marks interfaces UNDER_REVIEW
Reporter | ||
Comment 3•22 years ago
|
||
marks interfaces UNDER_REVIEW
changes nsIProxyInfo to use attributes and ACString
Not that this doesn't compile since we need to fix up netwerk/base/src, but I
wanted to get this out there for review.
Comment 4•22 years ago
|
||
yeah, looks good to me.
Comment 5•22 years ago
|
||
nsIProxyInfo is opaque. Only the proxiedprotocolhandler (and the ioserver -
darin, what happened to that patch of yours?) needs to deal with it.
The exception would be if you wanted to write a new protocol which also did
proxying (like http)
Reporter | ||
Comment 6•22 years ago
|
||
bradley, it cant be opaque, can it? How will a protocol handler be able to see
what port and host we are proxying over? Warren you had a concern here, right?
If it is opaque, lets make it a nsISupports on the nsIProxiedProtocolHandler and
be done with it.
Comment 7•22 years ago
|
||
nsIProxyInfo is opaque to anyone who doesn't need to check its contents. For
http proxying, the underlying protocol handler doesn't even see it. http does,
but thats because its doing the proxying. Similary, the socks code looks at it,
but other protocols (ftp, gopher, etc) just pass in the provided proxyinfo.
This was a design goal when I wrote this stuff last year. I cna't remmeber why
darin didn't want me to use an nsISupports, but we did discuss it.
If your goal is to enable people to write new protocol handlers which can be
used via socks proxies, you actually want to freeze nsIProxiedProtocolHandler.
There is hard coded per protocol logic in the protocolproxyservice though,
because the prefs are hard coded. protocols themselves don't need to talk to the
protocolproxyservice, and the (unfrozen) ioservice will do that for any
consumers. The only people needing to call that service directly (and then look
at the nsIProxyInfo result) are people who want to know where tehir request is
being proxied to. The code behind the C plugin API does this, but that API
requires the return of a PAC-style string, so this is requried. Since that code
is part of mozilla, it doesn't need it to be frozen.
This all assumes that the uriflags for the protocol handler are correct, of
course, but nsIProtocolHandler is frozen, so they should be.
Reporter | ||
Comment 8•22 years ago
|
||
> nsIProxyInfo is opaque to anyone who doesn't need to check its contents.
Lets define sets? Who shouldn't see this and who should? Would JS ever want to
see this?
Comment 9•22 years ago
|
||
JS needs to get a the actual thing so that js channels can be proxied (ie irc)
by passing it to he transport creation methods, which is why its scriptable.
Currently, it can't look at the result (all the [notxpcom] methods), but it
doesn't need to. This prohibits us from writing http in js, but ...
The only people who need to look at the result itsself are those who want to
know where we're proxying to.
Just pretend that nsIProxyInfo is only forward declared in those classes. We
can't do that because nsCOMPtr<forwardDeclClass> is deliberatly broken in
mozilla (some compilers don't support it), although we could work arround that
reasonably easily.
My point is that it should be treated as void*.
Comment 10•22 years ago
|
||
we decided to introduce nsIProxyInfo instead of nsISupports to avoid having our
implementation code QI all over the place. there was simply no need to hide the
fact that this object has to do with proxy info. there was also simply no need
to provide the nsIProxyInfo attributes via JS, so we went with notxpcom to avoid
getter_Copies. now that we have ACString, the situation changes a bit... we can
make nsIProxyInfo scriptable without paying any malloc penalty. so, why not
make it scriptable? in a nut shell, that's why i'm in favor of dougt's v.1 patch :)
Reporter | ||
Comment 11•22 years ago
|
||
so sr= it. :-)
Comment 12•22 years ago
|
||
doug: how about finishing the patch first? ;-)
Updated•22 years ago
|
Updated•22 years ago
|
Target Milestone: mozilla1.2alpha → mozilla1.2beta
Comment 14•22 years ago
|
||
i'd like to push back on this one and say WONTFIX for Mozilla 1.2... any
objections? (perhaps we'll discuss this more in tomorrows API review meeting.)
Updated•22 years ago
|
Severity: normal → minor
Priority: P3 → P5
Reporter | ||
Comment 15•22 years ago
|
||
Fine by me. -> 1.3
Comment 17•22 years ago
|
||
Should we remove the dependency on 157137 and mozilla1.2 keyword then?
Updated•18 years ago
|
Assignee: darin → nobody
Status: ASSIGNED → NEW
QA Contact: benc → networking
Target Milestone: Future → ---
We don't freeze interfaces anymore.
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → WONTFIX
You need to log in
before you can comment on or make changes to this bug.
Description
•