Eliminate nsPIProtocolProxyService

RESOLVED FIXED in mozilla1.9alpha1

Status

()

Core
Networking
P3
minor
RESOLVED FIXED
13 years ago
12 years ago

People

(Reporter: Darin Fisher, Assigned: Darin Fisher)

Tracking

Trunk
mozilla1.9alpha1
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

8.76 KB, patch
Ben Goodger (use ben at mozilla dot org for email)
: review+
neil@parkwaycc.co.uk
: review+
Details | Diff | Splinter Review
33.51 KB, patch
Details | Diff | Splinter Review
(Assignee)

Description

13 years ago
Eliminate nsPIProtocolProxyService

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

Updated

13 years ago
Status: NEW → ASSIGNED
Depends on: 282442
Priority: -- → P3
Target Milestone: --- → mozilla1.8beta2
(Assignee)

Updated

13 years ago
Target Milestone: mozilla1.8beta2 → mozilla1.9alpha
(Assignee)

Comment 1

12 years ago
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()"
(Assignee)

Comment 2

12 years ago
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.
(Assignee)

Comment 3

12 years ago
Created attachment 215836 [details] [diff] [review]
v1 patch
Attachment #215836 - Flags: superreview?(bzbarsky)
Attachment #215836 - Flags: review?(cbiesinger)
(Assignee)

Comment 4

12 years ago
Created attachment 215837 [details] [diff] [review]
UI patch for pref panels
Attachment #215837 - Flags: review?(bugs)
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)
Attachment #215836 - Flags: review?(cbiesinger) → review+
(Assignee)

Comment 6

12 years ago
> 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.
(Assignee)

Comment 7

12 years ago
Created attachment 215916 [details] [diff] [review]
v1.1 patch

revised per comments from biesi
Attachment #215836 - Attachment is obsolete: true
Attachment #215916 - Flags: superreview?(bzbarsky)
Attachment #215836 - Flags: superreview?(bzbarsky)
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.
Attachment #215837 - Flags: review?(bugs) → review+

Comment 9

12 years ago
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 on attachment 215837 [details] [diff] [review]
UI patch for pref panels

Neil, could you check out the xpfe changes?
Attachment #215837 - Flags: review?(neil)
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

12 years ago
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?
(Assignee)

Comment 13

12 years ago
(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.
(Assignee)

Comment 14

12 years ago
Boris: Would it help if I split the interface freezing off into a separate bug?
With speed of review for the other changes, yes.
(Assignee)

Comment 16

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

Comment 17

12 years ago
(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.
(Assignee)

Comment 18

12 years ago
> 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 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.
Attachment #215916 - Flags: superreview?(bzbarsky) → superreview+
(Assignee)

Comment 20

12 years ago
I think I meant "If canceled..." :-)

Comment 21

12 years ago
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.
Attachment #215837 - Flags: review?(neil) → review+
(Assignee)

Comment 22

12 years ago
> 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.
(Assignee)

Comment 23

12 years ago
fixed-on-trunk w/o interface freezing, which i will pull off into a second bug.
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
> 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.
You need to log in before you can comment on or make changes to this bug.