287646 - Eliminate nsPIProtocolProxyService

Status: RESOLVED FIXED

(Core :: Networking, defect, P3)

Description

[Darin Fisher]

Eliminate nsPIProtocolProxyService

The method configureFromPAC needs to be eliminated in favor of having consumers
toggle the value of the network.proxy.autoconfig_url preference.

Comment 1

[Darin Fisher]

My solution to this bug will do the following:

 1) Introduce nsIProtocolProxyService2, which defines a "reloadPAC()" method.
 2) Eliminate nsPIProtocolProxyService
 3) Make nsProtocolProxyService implement nsIClassInfo
 4) Change the pref UI to use "reloadPAC()" instead of "configureFromPAC()"

Comment 2

[Darin Fisher]

I'm also going to mark the following interfaces frozen:

  nsIProtocolProxyService
  nsIProtocolProxyFilter
  nsIProtocolProxyCallback
  nsIProxyInfo
  nsICancelable

I'll post to dev.platform about this change.  I'm open to suggestions about changes to these interfaces, but I'd like to avoid any changes that would be
incompatible with the version of these interfaces that shipped with Gecko 1.8.

Comment 3

[Darin Fisher]

Attachment: v1 patch

Comment 4

[Darin Fisher]

Attachment: UI patch for pref panels

Comment 5

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 215836 [details] [diff] [review]
v1 patch

I don't see the removal of nsPIProtocolProxyService.idl in this patch, shouldn't it be there?

Now, for the frozen interfaces:
nsIProtocolProxyCallback:
this should probably document what a failure status means for aProxyInfo (probably that it's null?)

nsIProtocolProxyService

this should probably document that new flags may be added even though the interface is frozen

also, asyncResolve should probably mention that calling cancel() on the result will notify the callback with the given result (it does, right? :) )

I'm a bit concerned about freezing the list of supported proxy types... should the interface docs say that new ones may be supported in the future?
(same in nsIProxyInfo)

Comment 6

[Darin Fisher]

> I don't see the removal of nsPIProtocolProxyService.idl in this patch,
> shouldn't it be there?

Yeah, I didn't include a "cvs remove" before cutting the patch.


> Now, for the frozen interfaces:
> nsIProtocolProxyCallback:
> this should probably document what a failure status means for aProxyInfo
> (probably that it's null?)

Good suggestion.


> this should probably document that new flags may be added even though the
> interface is frozen

Yes, I don't intend to limit the possibility of defining new flags in the future.


> also, asyncResolve should probably mention that calling cancel() on the result
> will notify the callback with the given result (it does, right? :) )

Yes, it does, and I'll add that to the comments.


> I'm a bit concerned about freezing the list of supported proxy types... should
> the interface docs say that new ones may be supported in the future?
> (same in nsIProxyInfo)

Yes, I think we definitely want to be able to add new proxy types in the future.  I'll add appropriate comments for that.

Comment 7

[Darin Fisher]

Attachment: v1.1 patch

revised per comments from biesi

Comment 8

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 215837 [details] [diff] [review]
UI patch for pref panels

the browser parts look ok to me. scott was going to take a quick peek at the mail.

Comment 9

[Scott MacGregor]

Comment on attachment 215837 [details] [diff] [review]
UI patch for pref panels

the mail changes for the pref UI look fine to me. Thanks Darin.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 215837 [details] [diff] [review]
UI patch for pref panels

Neil, could you check out the xpfe changes?

Comment 11

[Boris Zbarsky [:bzbarsky]]

Darin, this looks ok at first glance, but I want to read through the interfaces carefully to see what we're freezing.  I'll try to do that first week of April.

Comment 12

[[e-mail]]

Comment on attachment 215837 [details] [diff] [review]
UI patch for pref panels

>+    var prefs =
>+        Components.classes["@mozilla.org/preferences-service;1"].
>+        getService(Components.interfaces.nsIPrefBranch);
>+    var pacURL = prefs.getCharPref("network.proxy.autoconfig_url");
Nit: Suite style is to use
parent.hPrefWindow.getPref("string", "network.proxy.autoconfig_url");
or
parent.hPrefWindow.pref.getCharPref("network.proxy.autoconfig_url");

>+
>+    var disableReloadPref =
>+        document.getElementById("pref.advanced.proxies.disable_button.reload");
>+    disableReloadPref.disabled = (proxyType != 2 || typedURL != pacURL);
What if the user has changed the proxy type to PAC but not saved it yet?

>+    Components.classes["@mozilla.org/network/protocol-proxy-service;1"].
>+        getService().reloadPAC();
Apart from a shorter line of code, what benefit does class info give us here?

Comment 13

[Darin Fisher]

(In reply to comment #12)
> (From update of attachment 215837 [details] [diff] [review] [edit])
> >+    var prefs =
> >+        Components.classes["@mozilla.org/preferences-service;1"].
> >+        getService(Components.interfaces.nsIPrefBranch);
> >+    var pacURL = prefs.getCharPref("network.proxy.autoconfig_url");
> Nit: Suite style is to use
> parent.hPrefWindow.getPref("string", "network.proxy.autoconfig_url");
> or
> parent.hPrefWindow.pref.getCharPref("network.proxy.autoconfig_url");

Thanks, I'll make that change.


> >+
> >+    var disableReloadPref =
> >+        document.getElementById("pref.advanced.proxies.disable_button.reload");
> >+    disableReloadPref.disabled = (proxyType != 2 || typedURL != pacURL);
> What if the user has changed the proxy type to PAC but not saved it yet?

That's fine.  The XUL preference object is updated in real time as if the user had changed the pref.  That's why I had to go to the pref system for the other pref.


> >+    Components.classes["@mozilla.org/network/protocol-proxy-service;1"].
> >+        getService().reloadPAC();
> Apart from a shorter line of code, what benefit does class info give us here?

As a consumer of the PPS, you don't need to know which interface defines reloadPAC.  But, mostly it just helps to minimize typing.  I plan on extending most of the xpcom and necko components to support classinfo in the future.

Comment 14

[Darin Fisher]

Boris: Would it help if I split the interface freezing off into a separate bug?

Comment 15

[Boris Zbarsky [:bzbarsky]]

With speed of review for the other changes, yes.

Comment 16

[Darin Fisher]

Boris: Ok, feel free to only SR code changes for now if you like.  There's no rush to freeze these interfaces.

Comment 17

[[e-mail]]

(In reply to comment #13)
>(In reply to comment #12)
>>(From update of attachment 215837 [details] [diff] [review] [edit] [edit])
>>>+
>>>+    var disableReloadPref =
>>>+        document.getElementById("pref.advanced.proxies.disable_button.reload");
>>>+    disableReloadPref.disabled = (proxyType != 2 || typedURL != pacURL);
>>What if the user has changed the proxy type to PAC but not saved it yet?
>The XUL preference object is updated in real time as if the user had changed
>the pref. That's why I had to go to the pref system for the other pref.
I don't think this answers what my question was supposed to be. You are checking that the proxy type UI is PAC and that the URL matches the pref, but shouldn't you also checking that the current proxy type pref is in fact PAC too?

It occurs to me that suite's online indicator context menu might benefit from a reload PAC menuitem.

Comment 18

[Darin Fisher]

> but shouldn't you also checking that the current proxy type pref is in fact PAC
> too?

Yes, I think I should.  Good catch.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 215916 [details] [diff] [review]
v1.1 patch

>Index: base/public/nsIProtocolProxyService.idl
>+     *         The canceled, the cancelation status (aReason) will be forwarded

s/he canceled, // I would guess?

sr=bzbarsky on the rest, minus the freezing (comment and makefile) changes.

Comment 20

[Darin Fisher]

I think I meant "If canceled..." :-)

Comment 21

[[e-mail]]

Comment on attachment 215837 [details] [diff] [review]
UI patch for pref panels

OK, r=me with the pref getter change and the extra pref check.

Comment 22

[Darin Fisher]

> Nit: Suite style is to use
> parent.hPrefWindow.getPref("string", "network.proxy.autoconfig_url");
> or
> parent.hPrefWindow.pref.getCharPref("network.proxy.autoconfig_url");

Actually, this doesn't seem to work in FF or TB.

Comment 23

[Darin Fisher]

fixed-on-trunk w/o interface freezing, which i will pull off into a second bug.

Comment 24

[Boris Zbarsky [:bzbarsky]]

> Actually, this doesn't seem to work in FF or TB.

Right.  Neil was talking about the suite style; prefwindow is completely forked, so I'd doubt if anything that touches the prefwindow itself that works in suite works in FF/TB or vice versa.