Stub for Gopher redirecting to OverbiteFF add-on

RESOLVED WONTFIX

Status

()

Core
Networking
RESOLVED WONTFIX
7 years ago
5 years ago

People

(Reporter: spectre, Assigned: spectre)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(blocking2.0 -)

Details

(Whiteboard: [strings])

Attachments

(1 attachment, 12 obsolete attachments)

13.93 KB, patch
Gavin
: review-
Pike
: feedback+
Details | Diff | Splinter Review
(Assignee)

Description

7 years ago
User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X 10.4; en; rv:1.9.0.19) Gecko/2010051911 Camino/2.0.3 (like Firefox/3.0.19)
Build Identifier: 

Based on bug 388195, this is to implement a stub gopher handler that directs gopher:// access to OverbiteFF's AMO page.

I can implement this in C++, or this is not preferred (it's not my first choice), I can do it in JavaScript assuming that it is not blocked on reverse CPOWs as well.

Reproducible: Always

Comment 1

7 years ago
Implementing in JS sounds better to be, all else being equal.
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Comment 2

7 years ago
Created attachment 451217 [details] [diff] [review]
Gopher stub handler in JavaScript

Working patch. However, I'm open to suggestions to implement it better. I was also considering adding a quick alert to tell people what is happening, unless this is not desirable.
Attachment #451217 - Flags: review?

Comment 3

7 years ago
You should point to a URL without the locale and app so AMO can autodetect:
https://addons.mozilla.org/addon/7685/
(Assignee)

Comment 4

7 years ago
Created attachment 451233 [details] [diff] [review]
Gopher stub handler in JavaScript (v2)

URL corrected as advised.
Attachment #451217 - Attachment is obsolete: true
Attachment #451233 - Flags: review?
Attachment #451217 - Flags: review?
(Assignee)

Comment 5

7 years ago
Also, who would be the best one to ask to review this?
(Assignee)

Comment 6

7 years ago
Created attachment 461813 [details] [diff] [review]
Gopher stub handler in JavaScript (v3)

Unbitrotted for Fx 4.0b3pre. Also, I talked to Justin Hart and Robert Kaiser, and they suggested a friendlier interface, so this one pops a localizable prompter.
Attachment #451233 - Attachment is obsolete: true
Attachment #461813 - Flags: review?
Attachment #451233 - Flags: review?
(Assignee)

Comment 7

7 years ago
Created attachment 466210 [details] [diff] [review]
Gopher stub handler in JavaScript (v4)

Unbitrotted. Gavin, would you be the right person to ask for a review (see bug 388195 for the justification)? If not, who would be appropriate?
Attachment #461813 - Attachment is obsolete: true
Attachment #466210 - Flags: review?
Attachment #461813 - Flags: review?
(Assignee)

Comment 8

7 years ago
Created attachment 466212 [details] [diff] [review]
Gopher stub handler in JavaScript (v4)

Sorry, forgot the commit header.
Attachment #466210 - Attachment is obsolete: true
Attachment #466212 - Flags: review?
Attachment #466210 - Flags: review?
(Assignee)

Comment 9

7 years ago
Created attachment 468910 [details] [diff] [review]
Gopher protocol stub handler in JavaScript (v5)

Updated to current.
Attachment #466212 - Attachment is obsolete: true
Attachment #468910 - Flags: ui-review?
Attachment #468910 - Flags: review?
Attachment #466212 - Flags: review?

Comment 10

7 years ago
I'm temporarily moving this bug to Firefox as all the files the latest patch touches are in /browser/. But if this is put in /toolkit/ then SeaMonkey won't have to fork these files hopefully.
Status: NEW → ASSIGNED
Component: Networking → General
Product: Core → Firefox
QA Contact: networking → general

Comment 11

7 years ago
Comment on attachment 468910 [details] [diff] [review]
Gopher protocol stub handler in JavaScript (v5)

># HG changeset patch
># User Cameron Kaiser <spectre@armory.com>
># Date 1282704746 25200
># Node ID 601a446df49637c3f1f95e2f7aea9bfae3d0dcc6
># Parent  99bebf5ca9b4ecff4ff308b1205ff1d12fde1de8
>Gopher protocol stub handler in JavaScript
>
>diff --git a/browser/components/BrowserComponents.manifest b/browser/components/BrowserComponents.manifest
>--- a/browser/components/BrowserComponents.manifest
>+++ b/browser/components/BrowserComponents.manifest
>@@ -26,8 +26,13 @@ category command-line-handler x-default 
> category command-line-validator b-browser @mozilla.org/browser/clh;1 application={ec8030f7-c20a-464f-9b0e-13a3a9e97384}
> 
> # nsBrowserGlue.js
> component {eab9012e-5f74-4cbc-b2b5-a590235513cc} nsBrowserGlue.js
> contract @mozilla.org/browser/browserglue;1 {eab9012e-5f74-4cbc-b2b5-a590235513cc}
> category app-startup nsBrowserGlue service,@mozilla.org/browser/browserglue;1
> component {C6E8C44D-9F39-4AF7-BCC0-76E38A8310F5} nsBrowserGlue.js
> contract @mozilla.org/geolocation/prompt;1 {C6E8C44D-9F39-4AF7-BCC0-76E38A8310F5}
>+
>+# nsGopherProtocolStubHandler.js
>+component {a892e4e0-4dc8-11dd-ae16-0800200c9a66} nsGopherProtocolStubHandler.js
>+contract @mozilla.org/network/protocol;1?name=gopher {a892e4e0-4dc8-11dd-ae16-0800200c9a66}
>+
>diff --git a/browser/components/Makefile.in b/browser/components/Makefile.in
>--- a/browser/components/Makefile.in
>+++ b/browser/components/Makefile.in
>@@ -49,16 +49,17 @@ XPIDLSRCS = \
>   nsIBrowserGlue.idl \
>   nsIBrowserHandler.idl \
>   $(NULL)
> 
> EXTRA_PP_COMPONENTS = \
>   BrowserComponents.manifest \
>   nsBrowserContentHandler.js \
>   nsBrowserGlue.js \
>+  nsGopherProtocolStubHandler.js \
>   $(NULL)
> 
> EXTRA_JS_MODULES = distribution.js
> 
> PARALLEL_DIRS = \
>   about \
>   certerror \
>   dirprovider \
>diff --git a/browser/components/nsGopherProtocolStubHandler.js b/browser/components/nsGopherProtocolStubHandler.js
>new file mode 100644
>--- /dev/null
>+++ b/browser/components/nsGopherProtocolStubHandler.js
>@@ -0,0 +1,130 @@
>+/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
>+/* ***** BEGIN LICENSE BLOCK *****
>+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1
>+ *
>+ * The contents of this file are subject to the Mozilla Public License Version
>+ * 1.1 (the "License"); you may not use this file except in compliance with
>+ * the License. You may obtain a copy of the License at
>+ * http://www.mozilla.org/MPL/
>+ *
>+ * Software distributed under the License is distributed on an "AS IS" basis,
>+ * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
>+ * for the specific language governing rights and limitations under the
>+ * License.
>+ *
>+ * The Original Code is the Gopher Stub Protocol Handler code.
>+ * The Initial Developer of the Original Code is Cameron Kaiser.
>+ * Portions created by the Initial Developer are Copyright (C) 2010
>+ * the Initial Developer. All Rights Reserved.
>+ *
>+ * Contributors:
>+ *   Cameron Kaiser <ckaiser@floodgap.com>
>+ *
>+ * Alternatively, the contents of this file may be used under the terms of
>+ * either the GNU General Public License Version 2 or later (the "GPL"), or
>+ * the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
>+ * in which case the provisions of the GPL or the LGPL are applicable instead
>+ * of those above. If you wish to allow use of your version of this file only
>+ * under the terms of either the GPL or the LGPL, and not to allow others to
>+ * use your version of this file under the terms of the MPL, indicate your
>+ * decision by deleting the provisions above and replace them with the notice
>+ * and other provisions required by the GPL or the LGPL. If you do not delete
>+ * the provisions above, a recipient may use your version of this file under
>+ * the terms of any one of the MPL, the GPL or the LGPL.
>+ *
>+ * ***** END LICENSE BLOCK ***** */
>+
>+const Ci = Components.interfaces;
>+const Cc = Components.classes;
>+const Cr = Components.results;
>+const Cu = Components.utils;
>+
>+Cu.import("resource://gre/modules/XPCOMUtils.jsm");
>+Cu.import("resource://gre/modules/Services.jsm");
>+
>+/* This is a simple module which can be used as a template for any newly
>+   unsupported protocol. In this case, it redirects gopher:// protocol
>+   requests to the Mozilla Add-Ons page for OverbiteFF, which is a
>+   cross-platform extension for Gopherspace. See bug 572000. */
>+
>+const UNSUPP_PROTOCOL = "gopher";
>+const UNSUPP_PROTOCOL_FRIENDLY_NAME = "Gopher";
>+const UNSUPP_PROTOCOL_DEFAULT_PORT = 70; // not much meaning here.
>+const UNSUPP_HANDLER_CONTRACTID = "@mozilla.org/network/protocol;1?name="+
>+                                   UNSUPP_PROTOCOL;
>+const UNSUPP_HANDLER_CID = Components.ID(
>+                           "{a892e4e0-4dc8-11dd-ae16-0800200c9a66}");
>+const UNSUPP_REDIRECT_URL =
>+  "https://addons.mozilla.org/addon/7685/";
>+
>+function GopherProtocol()
>+{
>+}
>+
>+GopherProtocol.prototype = {
>+  classDescription : UNSUPP_PROTOCOL_FRIENDLY_NAME+" protocol handler stub",
>+  contractID : UNSUPP_HANDLER_CONTRACTID,
>+  classID : UNSUPP_HANDLER_CID,
>+
>+  // nsISupports
>+  QueryInterface : XPCOMUtils.generateQI([Ci.nsISupports,
>+                                          Ci.nsIProtocolHandler,
>+                                          Ci.nsIProxiedProtocolHandler]),
>+
>+  // nsIProtocolHandler
>+  scheme: UNSUPP_PROTOCOL,
>+  defaultPort: UNSUPP_PROTOCOL_DEFAULT_PORT,
>+  protocolFlags: 0
>+      | Ci.nsIProtocolHandler.ALLOWS_PROXY
>+      | Ci.nsIProtocolHandler.ALLOWS_HTTP_PROXY
>+      | Ci.nsIProtocolHandler.URI_LOADABLE_BY_ANYONE
>+    ,
>+  
>+  allowPort : function GP_allowPort(port, scheme) {
>+    return true; // meaningless also.
>+  },
>+
>+  newURI : function GP_newURI(spec, charset, baseURI) {
>+    var uri = Cc["@mozilla.org/network/standard-url;1"].
>+              createInstance(Ci.nsIURI);
>+    uri.spec = spec;
>+    return uri;
>+  },
>+
>+  newChannel : function GP_newChannel(input_uri) {
>+    // Don't actually give them a channel. This isn't supposed to be one.
>+    // Just pop up the requester giving them a chance to get the extension
>+    // (using newProxiedChannel).
>+    return this.newProxiedChannel(input_uri, null); // which returns null
>+  },
>+
>+  // nsIProxiedProtocolHandler 
>+  newProxiedChannel : function GP_newProxiedChannel(input_uri, proxyinfo) {
>+    // Don't actually give them a channel. This isn't supposed to be one.
>+    // Just pop up the requester giving them a chance to get the extension.
>+    var prompter = Cc["@mozilla.org/embedcomp/prompt-service;1"].
>+                   getService(Ci.nsIPromptService);
>+    var bundle = Services.strings.
>+                 createBundle("chrome://browser/locale/browser.properties");
>+    var rv = prompter.confirm(null,
>+             bundle.formatStringFromName("unsupportedProtocol.promptTitle",
>+                    [ UNSUPP_PROTOCOL_FRIENDLY_NAME ], 1),
>+             bundle.formatStringFromName("unsupportedProtocol.promptMessage",
>+                    [ UNSUPP_PROTOCOL_FRIENDLY_NAME ], 1));
>+    if (rv == true) {
>+        var wm = Cc["@mozilla.org/appshell/window-mediator;1"].
>+                 getService(Ci.nsIWindowMediator);
>+        wm.getMostRecentWindow('navigator:browser').getBrowser().
>+           webNavigation.loadURI(UNSUPP_REDIRECT_URL, 0, null, null, null);
>+        throw Cr.NS_ERROR_ABORT;
>+        return null;
>+    }
>+    throw Cr.NS_ERROR_NOT_IMPLEMENTED;
>+    return null;
>+  }
>+
>+};
>+
>+/* Make our factory. */
>+var components = [ GopherProtocol ];
>+var NSGetFactory = XPCOMUtils.generateNSGetFactory(components);
>diff --git a/browser/locales/en-US/chrome/browser/browser.properties b/browser/locales/en-US/chrome/browser/browser.properties
>--- a/browser/locales/en-US/chrome/browser/browser.properties
>+++ b/browser/locales/en-US/chrome/browser/browser.properties
>@@ -271,10 +271,16 @@ ctrlTab.showAll.label=;Show all #1 tabs
> 
> # LOCALIZATION NOTE (addKeywordTitleAutoFill): %S will be replaced by the page's title
> # Used as the bookmark name when saving a keyword for a search field.
> addKeywordTitleAutoFill=Search %S
> 
> # TabView
> tabView2.title=%S - Group Your Tabs
> 
>+# Unsupported protocols, including gopher://
>+# LOCALIZATION NOTE: In both strings below, %S will be replaced by the
>+# name of the protocol.
>+unsupportedProtocol.promptTitle=%S URLs now require an add-on
>+unsupportedProtocol.promptMessage=An add-on is now required to access %S URLs. Would you like to go to the add-on's download page at Mozilla Add-ons?
>+
> extensions.{972ce4c6-7e08-4474-a285-3208198ce6fd}.name=Default
> extensions.{972ce4c6-7e08-4474-a285-3208198ce6fd}.description=The default theme.
Attachment #468910 - Flags: review?(gavin.sharp)
Attachment #468910 - Flags: review?(dolske)
Attachment #468910 - Flags: review?
(Assignee)

Comment 12

7 years ago
(In reply to comment #10)
> I'm temporarily moving this bug to Firefox as all the files the latest patch
> touches are in /browser/. But if this is put in /toolkit/ then SeaMonkey won't
> have to fork these files hopefully.

Is that preferable? I can certainly generate a second patch for that (or do so pending review of this one).
I see no real reason for this to live in /browser. Seems to me like this could live in netwerk/ and be usable by other gecko users.

Comment 14

7 years ago
So back to Core::Networking? Who would be a suitable peer to review this over there?
Assignee: nobody → spectre
Component: General → Networking
Product: Firefox → Core
QA Contact: general → networking
Version: unspecified → Trunk
Comment on attachment 468910 [details] [diff] [review]
Gopher protocol stub handler in JavaScript (v5)

What gavin said. Plus, adding new modal dialogs is a non-starter. TBH, I'm not sure why this should be treated differently from any other unknown protocol.
Attachment #468910 - Flags: ui-review?
Attachment #468910 - Flags: review?(gavin.sharp)
Attachment #468910 - Flags: review?(dolske)
Attachment #468910 - Flags: review-
(Assignee)

Comment 16

7 years ago
This was based on Robert Sayre's suggestion in bug 388195 comment 126 (which I asked to work on, and this seemed okay at the time based on bug 388195 comment 135 -- please advise if I misunderstood).

W/r/t the modal dialogue, I can rewrite it to use an XUL error page instead. Would that be acceptable?

Comment 17

7 years ago
(In reply to comment #16)
> W/r/t the modal dialogue, I can rewrite it to use an XUL error page instead.

That would definitely be much better than this approach if it's doable.
(Assignee)

Comment 18

7 years ago
Created attachment 469349 [details] [diff] [review]
Gopher protocol stub handler in JavaScript (v6: netwerk/, notification based)

I couldn't get netError.xhtml to cooperate, so this is a different tack -- it uses notifications instead. If there is no notification box available, it falls back on NS_ERROR_UNKNOWN_PROTOCOL so that the default handlers are called. This version is also simpler and runs from netwerk/base/src.

Boris, would you be the right one to ask for a review, or should I ask Darin or Jason?
Attachment #468910 - Attachment is obsolete: true
Attachment #469349 - Flags: review?

Comment 19

7 years ago
Jason might be a better reviewer in terms of how this will work with e10s.  If he wants me to take a look, though, I can.  Darin's probably not a good choice; he is not really active anymore.
(Assignee)

Comment 20

7 years ago
Comment on attachment 469349 [details] [diff] [review]
Gopher protocol stub handler in JavaScript (v6: netwerk/, notification based)

Okay. Jason, if you are also not the right one for this, please let me know.
Attachment #469349 - Flags: review? → review?(jduell.mcbugs)
(Assignee)

Comment 21

7 years ago
Created attachment 469434 [details] [diff] [review]
Gopher protocol stub handler in JavaScript (v7)

A couple small tweaks.
Attachment #469349 - Attachment is obsolete: true
Attachment #469434 - Flags: review?(jduell.mcbugs)
Attachment #469349 - Flags: review?(jduell.mcbugs)
(Assignee)

Comment 22

7 years ago
Created attachment 471392 [details] [diff] [review]
Gopher stub handler in JavaScript (v8)

Unbitrotted.
Attachment #469434 - Attachment is obsolete: true
Attachment #471392 - Flags: review?(jduell.mcbugs)
Attachment #469434 - Flags: review?(jduell.mcbugs)
(Assignee)

Updated

7 years ago
Whiteboard: [strings]
Comment on attachment 471392 [details] [diff] [review]
Gopher stub handler in JavaScript (v8)

This patch looks fine from the necko side of things (bz: one minor issue, below).

I don't know the browser.getNotificationBox stuff, so I'd like review on that logic from Gavin (or whoever). 

And I assume this needs UI review.  Note that the current logic causes the notificationBox to show up with 

    Gopher URLs now require an add-on. Would you like to go to its Mozilla Add-on page?

once and only once, the first time you click on a gopher:// link.  From my little soapbox, it seems like we might as well display the message every time a gopher link is clicked?   Seems more useful to users than the "Protocol unsupported" dialog.


>+  protocolFlags: Ci.nsIProtocolHandler.URI_NORELATIVE
>+    | Ci.nsIProtocolHandler.URI_NOAUTH
>+    | Ci.nsIProtocolHandler.URI_LOADABLE_BY_ANYONE,

bz: do we want more restrictive access than URI_LOADABLE_BY_ANYONE?  I suppose the worst-case scenario here is that mischievous scripts or whatnot could cause the notification box to pop up at some time other than when a user clicks a gopher link.  Would also happens if a server redirects to a gopher:// URI?  Doesn't seem like a big deal.

Re: electrolysis.  This patch just points users at a Gopher add-on, so it's fine for e10s.  The tricky part will be getting a gopher e10s add-on working, but that's a separate bug (Cameron, email me the bug # for that so I can keep you posted).
Attachment #471392 - Flags: ui-review?(limi)
Attachment #471392 - Flags: review?(jduell.mcbugs)
Attachment #471392 - Flags: review?(gavin.sharp)
Attachment #471392 - Flags: review+
Do we want for FF4?  Otherwise we have no "soft" transition for gopher://

If so, this needs to land soon--it has localization strings and UI interface change.
blocking2.0: --- → ?
Depends on: 388195
(Assignee)

Comment 25

7 years ago
Actually, it does show up each time a gopher URL is clicked, at least in my testing: I don't know if there is a change afoot, though my internal repo should be current as of last night. If I click a gopher link, the notification box drops down; if I dismiss it and click the link again, it reappears. It won't spawn multiple simultaneous ones, however (I presumed that would be annoying).

The bug I was originally working on for gopher e10s was a C++ version in bug 562320, which I presume is WONTFIX at this point, but I don't know how much of that will be relevant for JavaScript (I can ask you about it in E-mail).

Comment 26

7 years ago
Comment on attachment 471392 [details] [diff] [review]
Gopher stub handler in JavaScript (v8)

I'm not so fond of making a gopher-specific string generic, from an l10n point of view. Starts with the gopher vs Gopher being hard-coded in the code now.
Attachment #471392 - Flags: feedback-
(Assignee)

Comment 27

7 years ago
Created attachment 472432 [details] [diff] [review]
Gopher stub handler in JavaScript (v9)

Strings altered/fixed as requested.
Attachment #471392 - Attachment is obsolete: true
Attachment #472432 - Flags: ui-review?(gavin.sharp)
Attachment #472432 - Flags: review?(jduell.mcbugs)
Attachment #471392 - Flags: ui-review?(limi)
Attachment #471392 - Flags: review?(gavin.sharp)
(Assignee)

Updated

7 years ago
Attachment #472432 - Flags: review?(l10n)

Comment 28

7 years ago
Comment on attachment 472432 [details] [diff] [review]
Gopher stub handler in JavaScript (v9)

feedback+ with a few nits:

+AddOnGopher=Gopher URLs now require an add-on. Would you like to go to its Mozilla Add-on page?

"now" seems like it's bound to today, I'd rephrase that message to be future-proof. Something more like "You can view Goher URLs with an add-on. ...". Also, I'm not sure if "Mozilla Add-on page" is the right term, it sounds a lot like this would be a Mozilla add-on, instead of a page hosted on the Add-ons site.
Attachment #472432 - Flags: review?(l10n) → feedback+
(Assignee)

Comment 29

7 years ago
I put the Mozilla part on there to reassure the user that the site they were going to is a Mozilla site. Would just yanking the "now" be okay with you? Or is there a more standard way to express that the user is going to a Mozilla-maintained server?
Comment on attachment 472432 [details] [diff] [review]
Gopher stub handler in JavaScript (v9)

> Actually, it does show up each time a gopher URL is clicked

Right you are.

Carrying forward +r on necko bits.

It's be nice to land this before Friday.  Gavin, any chance you can get to it or reassign to someone else?
Attachment #472432 - Flags: review?(jduell.mcbugs) → review+
blocking2.0: ? → -
Comment on attachment 472432 [details] [diff] [review]
Gopher stub handler in JavaScript (v9)

There's no need to "return" after throwing, since throwing halts code execution.

Using getMostRecentWindow() might cover the common case, but it's not really ideal, since it won't necessarily be related to whatever is triggering the gopher:// load (possible for that to be a background tab, an addon, whatever). Better would be to return a channel for a custom page that contains the message, I think - similar to what services/sync/Weave.js's newChannel() does, but with a chrome URI pointing to the page containing the message rather than a file:// URI (and with the privilege-stripping stuff that nsAboutRedirector::NewChannel does).

The general approach (and the strings specifically) should be ui-reviewed, per http://www.mozilla.org/projects/firefox/review.html .

The strings would need to land for beta 7 (i.e. ASAP) if we're going to do this for Firefox 4... We can land strings-only ahead of the rest of the implementation if it comes down to that, but that depends on the ui-review.
Attachment #472432 - Flags: ui-review?(gavin.sharp) → review-

Comment 32

7 years ago
This is blocking2.0- per :bs' triage, so we shouldn't add it to the game anymore, IMHO.

Comment 33

7 years ago
Hold on.  blocking- doesn't mean "won't take the patch if it's ready"...

Comment 34

7 years ago
I strongly advocate against pre-landing strings in any case. And for things that are non-blocking, even stronger. Comment 31 sounds like that'd only depend on ui-review, and I disagree.
Landing a complete XHTML page with DTD that can be redirected to by the protocol implementation handler doesn't seem like it would be particularly difficult for localizers to deal with. Much simpler than most other strings-only patches have have landed in the past.

In any case, I was just mentioning that as one of the possible options. We can also just forget about this for 4.0, or get the whole thing done for b7. I don't really have any strong opinions either way.
(Assignee)

Comment 36

7 years ago
Okay, I'll work up something with that approach. Would netError.xhtml be a good basis to model after? Also, where would the best place in the tree be to put it?
(Assignee)

Comment 37

7 years ago
Created attachment 481149 [details]
Feedback: channel approach

Before I get too deep in the weeds, let me make sure that this is the basic notion Gavin was talking about. This just redirects gopher:// to netError and (hopefully correctly) sets the principal as he advised. Is this an acceptable approach?

Also, should I make a content/ area for netwerk/, or do I need to put the error page in docshell/ or toolkit/? I plan to base it on netError.xhtml, unless there's a way I can (ab)use netError itself to display a custom message -- I don't think I can from here, however.
Attachment #472432 - Attachment is obsolete: true
Attachment #481149 - Flags: feedback?
(Assignee)

Comment 38

7 years ago
Created attachment 481896 [details] [diff] [review]
Gopher stub protocol handler in JavaScript (v10)

I went ahead and created a content area for Necko, and put gopherAddon.xhtml in it. That will make it easier to remove being only in one module, since I know this stub will have a finite lifetime.

This uses the redirection method Gavin suggested and has a small DTD for l10n.
Attachment #481149 - Attachment is obsolete: true
Attachment #481896 - Flags: ui-review?(gavin.sharp)
Attachment #481896 - Flags: review?(jduell.mcbugs)
Attachment #481149 - Flags: feedback?
(Assignee)

Updated

7 years ago
Attachment #481896 - Flags: review?(l10n)

Comment 39

7 years ago
Comment on attachment 481896 [details] [diff] [review]
Gopher stub protocol handler in JavaScript (v10)

>+++ b/netwerk/base/src/nsGopherProtocolStubHandler.js

>+const UNSUPP_REDIRECT_URL =
>+  "https://addons.mozilla.org/addon/7685/";

This isn't used anymore.

>+++ b/netwerk/resources/content/gopherAddon.xhtml

>+      <!-- Go To Add-On Button -->
>+      <button id="errorTryAgain" onclick="document.location='http://addon.mozilla.org/addon/7685/';">&goToAddOn.label;</button>

You're linking directly to it here instead.

You should either just remove the unused UNSUPP_REDIRECT_URL line or move it such that the xhtml page fetches the URL from the file.

Comment 40

7 years ago
Comment on attachment 481896 [details] [diff] [review]
Gopher stub protocol handler in JavaScript (v10)

>+++ b/netwerk/resources/content/gopherAddon.xhtml

>+      <!-- Go To Add-On Button -->
>+      <button id="errorTryAgain" onclick="document.location='http://addon.mozilla.org/addon/7685/';">&goToAddOn.label;</button>

This should use oncommand not onclick, by the way.
(Assignee)

Comment 41

7 years ago
Created attachment 481956 [details] [diff] [review]
Gopher stub protocol handler in JavaScript (v11)

Thanks for the spot; I just pulled the consts out entirely since they serve no purpose at this point.

However, oncommand doesn't work within this context (in fact, netError.xhtml also uses onclick, not oncommand). onclick is the only event handler that functions for some reason. I'm not sure if that's a bug.
Attachment #481896 - Attachment is obsolete: true
Attachment #481956 - Flags: ui-review?(gavin.sharp)
Attachment #481956 - Flags: review?(jduell.mcbugs)
Attachment #481896 - Flags: ui-review?(gavin.sharp)
Attachment #481896 - Flags: review?(l10n)
Attachment #481896 - Flags: review?(jduell.mcbugs)
(Assignee)

Updated

7 years ago
Attachment #481956 - Flags: review?(l10n)

Comment 42

7 years ago
(In reply to comment #41)
> However, oncommand doesn't work within this context (in fact, netError.xhtml
> also uses onclick, not oncommand). onclick is the only event handler that
> functions for some reason. I'm not sure if that's a bug.

XHTML may not support it then. In XUL oncommand works and is the preferred event.

Comment 43

7 years ago
Comment on attachment 481956 [details] [diff] [review]
Gopher stub protocol handler in JavaScript (v11)

From an l10n-point of view this looks OK.

I tripped over the somewhat inconsistent language of gopher server vs url, something to watch out for in the ui-review.
Attachment #481956 - Flags: review?(l10n) → feedback+
(Assignee)

Comment 44

7 years ago
Okay. I'll normalize them all to say "URL" for consistency.
Comment on attachment 481956 [details] [diff] [review]
Gopher stub protocol handler in JavaScript (v11)

I'm not a ui-reviewer. You want faaborg or beltzner for that. I can code review the frontend parts
Attachment #481956 - Flags: ui-review?(gavin.sharp) → review?(gavin.sharp)
(Assignee)

Comment 46

7 years ago
Comment on attachment 481956 [details] [diff] [review]
Gopher stub protocol handler in JavaScript (v11)

Okay, I will ask Mike Beltzner if he can do the ui-review, since the patch would need his approval to land at this point anyway (if I read Tree Rules correctly). However, Gavin, if you have any time to let me know if there are any glaring errors I would still appreciate it. (And thanks, Axel, for the l10n feedback)
Attachment #481956 - Flags: ui-review?(beltzner)
(Assignee)

Comment 47

7 years ago
Would it be possible for this to make beta 8?
(Assignee)

Comment 48

7 years ago
I landed this in TenFourFox beta 11 in the meantime.

To the SeaMonkey devs, do you want this for SeaMonkey even if it is not taken in Firefox? I can work up a SM patch simultaneously, although I suspect it probably will apply as is.

Comment 49

7 years ago
> To the SeaMonkey devs, do you want this for SeaMonkey even if it is not taken
> in Firefox? I can work up a SM patch simultaneously, although I suspect it
> probably will apply as is.
Looking at the patch, it's all in  core (mozilla-central/netwerk) which means it's in shared code. Land it there and we'll pick it up automatically.

Of course it's possible to rewrite the patch to comm-central/suite but we'd rather not have forked code except as a last resort.
Comment on attachment 481956 [details] [diff] [review]
Gopher stub protocol handler in JavaScript (v11)

We shipped Firefox 4 with Gopher removed and so far I haven't heard a peep - I don't think we want/need to do this.
Attachment #481956 - Flags: ui-review?(mbeltzner)
Attachment #481956 - Flags: review?(jduell.mcbugs)
Attachment #481956 - Flags: review?(gavin.sharp)
Attachment #481956 - Flags: review-
I'm going to call this WONTFIX.
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → WONTFIX

Comment 52

6 years ago
(In reply to comment #50)
> We shipped Firefox 4 with Gopher removed and so far I haven't heard a peep -
> I don't think we want/need to do this.

My guess is that a population die-hard enough to still actually use Gopher is fine with just figuring out that they need an addon now themselves. Even if Firefox 4 managed to have gotten this, we'd probably be talking about removing it for Firefox 5 or 6 by now; we're definitely pass the window where this would've possibly been helpful. WONTFIX sounds good to me.

Comment 53

6 years ago
Another Gopher user here. I use SeaMonkey and just encountered gopher being removed in SeaMonkey version 2.1. I submitted bug 665708 since something just broke but it wasn't clear what happened. Is it possible to adapt this patch to SeaMonkey for bug 665708? Thanks.
You need to log in before you can comment on or make changes to this bug.