Freeze nsIProxiedProtocolHandler and nsIProxyInfo

RESOLVED WONTFIX

Status

()

P5
minor
RESOLVED WONTFIX
17 years ago
8 years ago

People

(Reporter: dougt, Unassigned)

Tracking

Trunk
x86
Windows 2000
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Reporter)

Description

17 years ago
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

17 years ago
we probably want to complete the design for PAC failover before freezing these
interfaces.

Updated

17 years ago
Depends on: 84798
(Reporter)

Comment 2

17 years ago
Created attachment 88187 [details] [diff] [review]
patch v.1

marks interfaces UNDER_REVIEW
(Reporter)

Comment 3

17 years ago
Created attachment 88194 [details] [diff] [review]
patch v.1

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

17 years ago
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)
(Reporter)

Comment 6

17 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.
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

17 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?
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

17 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 :)

Updated

17 years ago
Blocks: 153246
(Reporter)

Comment 11

17 years ago
so sr= it. :-)

Comment 12

17 years ago
doug: how about finishing the patch first? ;-)
(Reporter)

Updated

17 years ago
Blocks: 157137
(Reporter)

Comment 13

17 years ago
to darin
Assignee: dougt → darin

Updated

17 years ago
Status: NEW → ASSIGNED
Keywords: mozilla1.2
Priority: -- → P3
Target Milestone: --- → mozilla1.2alpha

Updated

16 years ago
Target Milestone: mozilla1.2alpha → mozilla1.2beta

Comment 14

16 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

16 years ago
Severity: normal → minor
Priority: P3 → P5
(Reporter)

Comment 15

16 years ago
Fine by me.  -> 1.3

Comment 16

16 years ago
that means future to me ;-)
Target Milestone: mozilla1.2beta → Future

Comment 17

16 years ago
Should we remove the dependency on 157137 and mozilla1.2 keyword then?

Updated

13 years ago
Assignee: darin → nobody
Status: ASSIGNED → NEW
QA Contact: benc → networking
Target Milestone: Future → ---
We don't freeze interfaces anymore.
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.