Closed Bug 665708 Opened 9 years ago Closed 6 years ago

Gopher links show "Launch Application" in SeaMonkey 2.1

Categories

(SeaMonkey :: Download & File Handling, defect)

SeaMonkey 2.1 Branch
x86
Windows XP
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey2.28

People

(Reporter: dsmutil, Assigned: ewong)

Details

Attachments

(2 files, 5 obsolete files)

Attached image Gopher error
When I follow one of my gopher links, I now get an error from SeaMonkey:

Launch Application
Send to:
  SeaMonkey
Choose an Application

I select SeaMonkey but it just keeps coming up again. I found some bugs that mention that Gopher may be removed in a future version of SeaMonkey but the error page is unclear if it has been removed. A group of us just started creating gopher pages about a year ago and now it's broken.
Gopher support has been removed from SM and FF (Bug 388195).

You might want to try Overbite (https://addons.mozilla.org/en-US/seamonkey/addon/overbiteff/).
I agree this could be clearer. We didn't take Bug 572000 in the end (see Bug 572000 comment 52) but if someone were to adapt the patch in that bug to SeaMonkey and submit it here, we would certainly consider it.
Thanks. I put a note over on bug 572000 asking for help. Should something be put in the release notes? Or, could Overbite be bundled with the installation, such as ChatZilla, DOM Inspector, and the JavaScript Debugger, so it could be installed with a custom install?
No plans to bundle OverbiteFF with SeaMonkey. Bundling is already too complicated as it is.
Philip, I'd be willing to port if you're willing to review/accept.
Hi Cameron..  just wondering if you'll be doing the patch.  It wouldn't be right if I took that patch from that other bug and adapt it to suite/.
Just to clarify, we are thinking of taking the Gopher stub protocol handler patch in Bug 572000 and adapting it for SeaMonkey. Since this will be in comm-central/suite/ we won't have to get any of the Core/networking people to sign off on it.
Flags: needinfo?(spectre)
I'm fine with whatever. I don't have a build of SeaMonkey up right now, so I'm not in a position to test this easily. If you can do so, Edmund, go for it.
(curse you Bugzilla)
Flags: needinfo?(spectre)
Assignee: nobody → ewong
Status: NEW → ASSIGNED
Version: SeaMonkey 2.1 Branch → Trunk
Attached patch WIP patch. (obsolete) — Splinter Review
Attached patch Updated WIP. (obsolete) — Splinter Review
Attachment #8350864 - Attachment is obsolete: true
Attached patch proposed patch (v1) (obsolete) — Splinter Review
Attachment #8350865 - Attachment is obsolete: true
Version: Trunk → SeaMonkey 2.1 Branch
Attachment #8350866 - Flags: review?(neil)
Comment on attachment 8350866 [details] [diff] [review]
proposed patch (v1)

>diff --git a/suite/common/gopher/Makefile.in b/suite/common/gopher/Makefile.in
This file is unnecessary.

>diff --git a/suite/common/gopher/gopherAddon.xhtml b/suite/common/gopher/gopherAddon.xhtml
This doesn't deserve its own directory, just put it in suite/common. (Move the .dtd too and update jar.mn and newChannel etc.)

>+    <link rel="stylesheet" href="chrome://global/skin/netError.css" type="text/css" media="all" />
Nit: don't need the media attribute, or the space before />

>+         document.location = "http://addon.mozilla.org/addon/7685";
Nit: addons.mozilla.org
Hmm, I wonder whether we're safe to hard-code the url here.

>diff --git a/suite/common/gopher/moz.build b/suite/common/gopher/moz.build
This file is unnecessary.

>+
>+
Nit: doubled blank line

>+  classDescription : "Gopher protocol handler stub",
Nit: no space before : please

>+  contractID : "@mozilla.org/network/protocol;1?name=gopher",
Nit: contactID no longer necessary

>+  // nsISupports
>+  QueryInterface : XPCOMUtils.generateQI([Components.interfaces.nsISupports,
>+                                          Components.interfaces.nsIProtocolHandler]),
XPCOMUtils.generateQI already, um, supports, nsISupports.

>+  protocolFlags: Components.interfaces.nsIProtocolHandler.URI_NORELATIVE
>+    | Components.interfaces.nsIProtocolHandler.URI_NOAUTH
>+    | Components.interfaces.nsIProtocolHandler.URI_LOADABLE_BY_ANYONE,
Nit: | belongs at the end of the previous line.

>+  allowPort : function GP_allowPort(port, scheme) {
>+    return true; // meaningless.
It might be meaningless, but it should still return false.

>+  newURI : function GP_newURI(spec, charset, baseURI) {
>+    var uri = Components.classes["@mozilla.org/network/standard-url;1"].
>+              createInstance(Components.interfaces.nsIURI);
>+    uri.spec = spec;
Ideally this would use nsIStandardURL and call init(URLTYPE_STANDARD, this.defaultPort, spec, charset, baseURI)
(URL_TYPE_STANDARD is a constant on nsIStandardURL)

>+    var chan = Components.classes["@mozilla.org/network/io-service;1"].
>+      getService(Components.interfaces.nsIIOService).
>+      newChannel("chrome://communicator/content/gopher/gopherAddon.xhtml", null, null);
Nit: . at the start of the line, lined up with the . in the previous line. (Same below.)

>diff --git a/suite/installer/removed-files.in b/suite/installer/removed-files.in
I don't know how this file works, check with someone else please?

>+<!ENTITY loadError.label        "Gopher servers require an add-on">
"Page Load Error"

>+<!ENTITY goToAddOn.label        "Go To Add-on Page">
Nit: This goes last in the file.

>+<!ENTITY gopherAddon.title      "Add-on Required">
"Gopher Protocol"

>+<!ENTITY gopherAddon.shortDesc  "Gopher URLs require an add-on to access them.">
"gopher is not a registered protocol."

>+<!ENTITY gopherAddon.longDesc   "You can get a compatible add-on to access this server from Mozilla Add-ons.">
Prefix this sentence:
"The address specifies the gopher protocol which is no longer supported, so the browser cannot connect to the site."
Attachment #8350866 - Flags: review?(neil) → review-
Comment on attachment 8350866 [details] [diff] [review]
proposed patch (v1)

requesting review on specifically the removed-files.in part.
Attachment #8350866 - Flags: review?(bugspam.Callek)
Attached patch proposed patch (v2) (obsolete) — Splinter Review
Attachment #8350866 - Attachment is obsolete: true
Attachment #8350866 - Flags: review?(bugspam.Callek)
Attachment #8351087 - Flags: review?(neil)
Comment on attachment 8351087 [details] [diff] [review]
proposed patch (v2)

Specific review for the changes in removed-files.in.
Attachment #8351087 - Flags: review?(bugspam.Callek)
Comment on attachment 8351087 [details] [diff] [review]
proposed patch (v2)

>+    <link rel="stylesheet" href="chrome://global/skin/netError.css" type="text/css" />
Nit: still no space before /> please.

>+         document.location = "http://addons.mozilla.org/addon/7685";
Still need to decide whether we're allowed to hard-code URLs like this or whether they should be properties. (So don't bother uploading a new patch until then.) Oh, and this needs to be https: of course.

>+    return false // meaningless.
Nit: missing semicolon

>+<!ENTITY goToAddOn.label        "Go To Add-on Page">
Nit: Go to
Comment on attachment 8351087 [details] [diff] [review]
proposed patch (v2)

OK, looks like we're good to go with the hard-coded URL, in which case r=me with those nits fixed and the packaging fixes independently checked.
Attachment #8351087 - Flags: review?(neil) → review+
Attached patch proposed patch(v3) (obsolete) — Splinter Review
Attachment #8351087 - Attachment is obsolete: true
Attachment #8351087 - Flags: review?(bugspam.Callek)
Attachment #8354987 - Flags: review+
Comment on attachment 8354987 [details] [diff] [review]
proposed patch(v3)

Asking review for the packaging part of this patch.
Attachment #8354987 - Flags: review?(bugspam.Callek)
Comment on attachment 8354987 [details] [diff] [review]
proposed patch(v3)

Review request for packaging part
Attachment #8354987 - Flags: review?(bugzilla)
Comment on attachment 8354987 [details] [diff] [review]
proposed patch(v3)

Edmund: In general the packaging changes are ok, but you don't need the change to suite/installer/removed-files.in. That section in removed-files.in is for files that were shipped before we enabled omnijar by default, so that they get removed on update. I know some people added more and more files to that section after we enabled omnijar, but it's not really necessary (and slows down the installer a tiny bit). We only ship official releases with omnijar enabled.
Attachment #8354987 - Flags: review?(bugzilla) → review+
Attachment #8354987 - Flags: review?(bugspam.Callek)
This patch is without the removed-files.in changes.
Attachment #8354987 - Attachment is obsolete: true
Attachment #8362042 - Flags: review+
Pushed to comm-central:
https://hg.mozilla.org/comm-central/rev/8e381db999ec
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.28
You need to log in before you can comment on or make changes to this bug.