Closed Bug 1115572 Opened 5 years ago Closed 5 years ago

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

Categories

(SeaMonkey :: General, defect)

defect
Not set

Tracking

(seamonkey2.34 fixed)

RESOLVED FIXED
seamonkey2.34
Tracking Status
seamonkey2.34 --- fixed

People

(Reporter: philip.chee, Assigned: philip.chee)

References

Details

Attachments

(2 files, 1 obsolete file)

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
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.
Assignee: nobody → philip.chee
Status: NEW → ASSIGNED
Attachment #8541516 - Flags: review?(neil)
(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?
> 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.
Attachment #8541516 - Attachment is obsolete: true
Attachment #8541516 - Flags: review?(neil)
Attachment #8541557 - Flags: review?(neil)
Attachment #8541557 - Flags: review?(neil) → review+
Pushed to comm-central
http://hg.mozilla.org/comm-central/rev/2a69432c0202
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.34
Blocks: 1099585
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
(In reply to comment #5)
> See
See also bug 1088748!
(In reply to neil@parkwaycc.co.uk 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
Attachment #8575391 - Flags: review?(neil)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Attachment #8575391 - Flags: approval-comm-beta?
Attachment #8575391 - Flags: approval-comm-aurora?
Attachment #8575391 - Flags: review?(neil) → review+
Backout pushed to comm-central
http://hg.mozilla.org/comm-central/rev/115057dbe195
Attachment #8575391 - Flags: approval-comm-beta?
Attachment #8575391 - Flags: approval-comm-beta+
Attachment #8575391 - Flags: approval-comm-aurora?
Attachment #8575391 - Flags: approval-comm-aurora+
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.