1115572 - Add newChannel2 (that takes loadinfo as an argument) to suite protocol handlers

Status: RESOLVED FIXED

(SeaMonkey :: General, defect)

Description

[Philip Chee]

Followup to Bug 1088748 - SeaMonkey part for Add newChannel2 to nsIProtocolHandler (Bug 1067471) and Extend NewChannel() with loadinfo argument in nsIAboutModule (Bug 1067468)

Referenced bug:
Bug 1067471 - Add newChannel2 to nsIProtocolHandler that takes a loadinfo as an argument
https://hg.mozilla.org/mozilla-central/rev/9b09703be36f

Comment 1

[Philip Chee]

Attachment: Patch v1.0 fix crashy protocol handlers

So now various parts of Gecko are now calling newChannel2() with a loadinfo argument. I think the callers will fallback to NewChannel() if newChannel2() is not available which is why certain things are still working. However I'm getting crashes with feeds (FeedConverter.js) and the stub nsGopherProtocolStubHandler.js. This patch fixes the newChannel2 implementations in our protocol handlers.

> +    var channel = !aLoadInfo ?
> +                  Services.io.newChannelFromURI(newURI) :
> +                  Services.io.newChannelFromURIWithLoadInfo(newURI, aLoadInfo);
If I do it the other way e.g.
> var channel = aLoadInfo ? Services.io.newChannelFromURIWithLoadInfo(newURI, aLoadInfo) ...
I get a "aLoadInfo is not defined" error. I don't know why

> -    var channel = Services.io.newChannelFromURI(uri);
> +    var channel = !aLoadinfo ? // aLoadinfo == null ?
> +                  Services.io.newChannelFromURI(uri) :
> +                  Services.io.newChannelFromURIWithLoadInfo(uri, aLoadinfo);
This shuffing along and changing the API a bit at a time is a real P.I.T.A.

Comment 2

[[e-mail]]

(In reply to Philip Chee from comment #1)
> If I do it the other way e.g.
> > var channel = aLoadInfo ? Services.io.newChannelFromURIWithLoadInfo(newURI, aLoadInfo) ...
> I get a "aLoadInfo is not defined" error. I don't know why

That makes no sense. Can you attach the appropriate patch and some STR?

Comment 3

[Philip Chee]

Attachment: Patch v1.1 use loadinfo if available.

> That makes no sense. Can you attach the appropriate patch and some STR?
Well now I can't reproduce it. This revised patch switches around the order of the ternary operator.

Comment 4

[Philip Chee]

Pushed to comm-central
http://hg.mozilla.org/comm-central/rev/2a69432c0202

Comment 5

[[e-mail]]

Comment on attachment 8541557 [details] [diff] [review]
Patch v1.1 use loadinfo if available.

>diff --git a/suite/common/src/nsAbout.js b/suite/common/src/nsAbout.js
>--- a/suite/common/src/nsAbout.js
>+++ b/suite/common/src/nsAbout.js
>@@ -39,19 +39,26 @@ About.prototype = {
>   getModule: function(aURI) {
>     return aURI.path.replace(/-|\W.*$/g, "").toLowerCase();
>   },
> 
>   getURIFlags: function(aURI) {
>     return this[this.getModule(aURI) + "Flags"];
>   },
> 
>-  newChannel: function(aURI, aLoadInfo) {
>+  newChannel: function(aURI) {
>+    return this.newChannel2(aURI, null);
>+  },
>+
>+  newChannel2: function(aURI, aLoadInfo) {
Whoops, nsAbout is an about module, not a protocol handler, so that this bit of the change was wrong. Please back it out on as many branches as you can. See http://hg.mozilla.org/mozilla-central/diff/7497797d45c4/netwerk/protocol/about/nsIAboutModule.idl

Comment 6

[[e-mail]]

(In reply to comment #5)
> See
See also bug 1088748!

Comment 7

[Philip Chee]

Attachment: Patch v2. Backout nsAbout change nsAbout is an about module, not a protocol handler.

(In reply to [e-mail] from comment #5)

>>-  newChannel: function(aURI, aLoadInfo) {
>>+  newChannel: function(aURI) {
>>+    return this.newChannel2(aURI, null);
>>+  },
>>+
>>+  newChannel2: function(aURI, aLoadInfo) {
> Whoops, nsAbout is an about module, not a protocol handler, so that this bit
> of the change was wrong. Please back it out on as many branches as you can.
> See
> http://hg.mozilla.org/mozilla-central/diff/7497797d45c4/netwerk/protocol/about/nsIAboutModule.idl

Comment 8

[Philip Chee]

Comment on attachment 8575391 [details] [diff] [review]
Patch v2. Backout nsAbout change nsAbout is an about module, not a protocol handler.

[Approval Request Comment]
Regression caused by (bug #):  Bug 1115572
User impact if declined: about: pages broken
Testing completed (on m-c, etc.): 
Risk to taking this patch (and alternatives if risky): bustage fix low risk.
String changes made by this patch: none.

Comment 9

[Philip Chee]

Backout pushed to comm-central
http://hg.mozilla.org/comm-central/rev/115057dbe195

Comment 10

[Philip Chee]

Pushed to branches.
http://hg.mozilla.org/releases/comm-aurora/rev/2670bbeba950
http://hg.mozilla.org/releases/comm-beta/rev/8c85927c474b