Closed Bug 152663 Opened 22 years ago Closed 14 years ago

Freeze nsIProxiedProtocolHandler and nsIProxyInfo

Categories

(Core :: Networking, defect, P5)

x86
Windows 2000
defect

Tracking

()

RESOLVED WONTFIX

People

(Reporter: dougt, Unassigned)

References

Details

Attachments

(2 files)

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.
we probably want to complete the design for PAC failover before freezing these
interfaces.
Depends on: 84798
Attached patch patch v.1Splinter Review
marks interfaces UNDER_REVIEW
Attached patch patch v.1Splinter Review
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.
yeah, looks good to me.
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)
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.
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.
> 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?
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*.
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 :)
Blocks: 153246
so sr= it. :-)
doug: how about finishing the patch first? ;-)
Blocks: 157137
to darin
Assignee: dougt → darin
Status: NEW → ASSIGNED
Keywords: mozilla1.2
Priority: -- → P3
Target Milestone: --- → mozilla1.2alpha
Target Milestone: mozilla1.2alpha → mozilla1.2beta
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.)
Severity: normal → minor
Priority: P3 → P5
Fine by me.  -> 1.3
that means future to me ;-)
Target Milestone: mozilla1.2beta → Future
Should we remove the dependency on 157137 and mozilla1.2 keyword then?
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.