The default bug view has changed. See this FAQ.

Remove gopher references from SeaMonkey code

RESOLVED FIXED in seamonkey2.1a3

Status

SeaMonkey
UI Design
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: Robert Kaiser, Assigned: ewong)

Tracking

Trunk
seamonkey2.1a3
x86
Linux

Firefox Tracking Flags

(blocking-seamonkey2.1 final+)

Details

(Whiteboard: [good first bug], URL)

Attachments

(2 attachments, 7 obsolete attachments)

(Reporter)

Description

7 years ago
Bug 388195 removed gopher from Firefox and the core platform, we should remove the references we have in SeaMonkey as well.

Comment 1

7 years ago
This was truly a sad day. :(
(Reporter)

Comment 2

7 years ago
(In reply to comment #1)
> This was truly a sad day. :(

Well, it should be provided through an add-on still, but the built-in support is gone and won't come back, so we need to react.
Can be done in stages, adding "good first bug"

At least blocking final for installer-related changes, and pref-UI.
blocking-seamonkey2.1: --- → final+
Whiteboard: [good first bug]
(Assignee)

Updated

7 years ago
Assignee: nobody → ewong
Status: NEW → ASSIGNED

Comment 4

7 years ago
We could port over Firefox [Bug 572000] Stub for Gopher redirecting to OverbiteFF add-on. But first we would have to ask Cameron Kaiser to make his extension compatible with SeaMonkey 2.0+.

We could also reimplement gopher in javascript either by shipping the OverbiteFF extension or borging the code there into suite/
(Assignee)

Comment 5

7 years ago
Created attachment 454237 [details] [diff] [review]
Removed gopher reference from Seamonkey (patch 1) v1

Removed gopher references from directory.js and utilityOverlay.js.
Attachment #454237 - Flags: review?(bugspam.Callek)
Comment on attachment 454237 [details] [diff] [review]
Removed gopher reference from Seamonkey (patch 1) v1

Cannot land as is, (for example needs pref and migration updates before I'm comfortable dropping its actual use)

But looks like this is on the right track. I think we can let me do the suite/ review and a final once-over by Neil as an sr.
Attachment #454237 - Flags: review?(bugspam.Callek)
Attachment #454237 - Flags: review-
Attachment #454237 - Flags: feedback+
(Assignee)

Comment 7

7 years ago
Created attachment 454775 [details] [diff] [review]
Removed gopher references from Seamonkey
Attachment #454237 - Attachment is obsolete: true
Attachment #454775 - Flags: feedback?(bugspam.Callek)
Comment on attachment 454775 [details] [diff] [review]
Removed gopher references from Seamonkey

>+++ b/calendar/installer/removed-files.in
>@@ -373,28 +373,16 @@ xpicleanup.exe
>-# bug 431775 (remove unused gopher images)
>-res/html/gopher-audio.gif
>-res/html/gopher-binary.gif
>-res/html/gopher-find.gif
>-res/html/gopher-image.gif
>-res/html/gopher-menu.gif
>-res/html/gopher-movie.gif
>-res/html/gopher-sound.gif
>-res/html/gopher-telnet.gif
>-res/html/gopher-text.gif
>-res/html/gopher-unknown.gif

These have to stay in this file; same with the mail/installer/removed-files.in.

Also *ADD* these lines to the suite/ removed-files.in as well please. [removed-files.in tells the installer/updater that these files need to be DELETED on a new update/install].

I'm about to look at the rest of this, but this alone is enough for a feedback-
Attachment #454775 - Flags: feedback?(bugspam.Callek) → feedback-
Comment on attachment 454775 [details] [diff] [review]
Removed gopher references from Seamonkey

>+++ b/editor/ui/composer/content/editorUtilities.js
> function TextIsURI(selectedText)
>-    ^ldaps:|^gopher:|^finger:|^javascript:/i.test(selectedText);
>+    ^ldaps:|^finger:|^javascript:/i.test(selectedText);

Thinking about this use, I don't think removing it here is a good idea; we shouldn't stop people from linkifying gopher: urls (via this easy case where we test for it; removing it wouldn't prevent it entirely) 


>+++ b/mailnews/base/src/nsMsgContentPolicy.cpp
>-  // Protocols like ftp, gopher are always blocked.
>+  // Protocols like ftp is always blocked.

Grammar nit: are always blocked

>+++ b/suite/app/macbuild/Contents/Info.plist.in

I don't grasp this files actual use; but it "LOOKS" like it should be correct.

>+++ b/suite/common/directory/directory.js
>-    // for file URLs, only do it for FTP/Gopher/etc URLs; the "rdf:files"
>+    // for file URLs, only do it for FTP/etc URLs; the "rdf:files"

grammar nit: "Only do it for other URLs;"

>+++ b/suite/installer/windows/nsis/shared.nsh

We also need to add some logic here for removing gopher stuff if present. See: http://hg.mozilla.org/mozilla-central/diff/6bfc738eb742/browser/installer/windows/nsis/shared.nsh

...

Every file not commented on above looks good
(Assignee)

Comment 10

7 years ago
Created attachment 456528 [details] [diff] [review]
Removed gopher reference from Seamonkey (patch 1) v3
Attachment #454775 - Attachment is obsolete: true
Attachment #456528 - Flags: superreview?(neil)
Attachment #456528 - Flags: review?(bugspam.Callek)
(Reporter)

Comment 11

7 years ago
(In reply to comment #9)
> >+++ b/suite/app/macbuild/Contents/Info.plist.in
> 
> I don't grasp this files actual use


It tells Mac OS about the application's metadata, i.e. name, capabilities, etc.

Updated

7 years ago
Attachment #456528 - Flags: review?(bugspam.Callek) → review-
(In reply to comment #8)
> (From update of attachment 454775 [details] [diff] [review])
> >+++ b/calendar/installer/removed-files.in
> Also *ADD* these lines to the suite/ removed-files.in as well please.
> [removed-files.in tells the installer/updater that these files need to be
> DELETED on a new update/install].

Missed this part, (we need to add the gopher lines to suite's removed-files.in so that updates remove them.

(In reply to comment #9)
> (From update of attachment 454775 [details] [diff] [review])
> >+++ b/suite/installer/windows/nsis/shared.nsh
> 
> We also need to add some logic here for removing gopher stuff if present. See:
> http://hg.mozilla.org/mozilla-central/diff/6bfc738eb742/browser/installer/windows/nsis/shared.nsh

You missed some of the logic: "; Delete gopher from Capabilities\URLAssociations if it is present."
"; Delete gopher from the user's UrlAssociations if it points to FirefoxURL." (of course for us this would be "SeaMonkeyURL" instead of "FirefoxURL")

Also:
>  ; Remove support for launching gopher urls from the shell during 
>  ; install or update if the DefaultIcon is from seamonkey.exe.
and
>; Remove the gopher key if the DefaultIcon is from the EXE file.

are inconsistent, I'd rather you use the same comment as firefox modulo s/firefox.exe/seamonkey.exe/

For this, I only verified that my previous other nits were addressed, did not interdiff though.
(Assignee)

Comment 13

7 years ago
Created attachment 457734 [details] [diff] [review]
Removed gopher reference from Seamonkey v4
Attachment #456528 - Attachment is obsolete: true
Attachment #457734 - Flags: review?(bugspam.Callek)
Attachment #456528 - Flags: superreview?(neil)
Comment on attachment 457734 [details] [diff] [review]
Removed gopher reference from Seamonkey v4

> function DefaultForShareSettingsPref()
> {
>   return gHTTP.value == gSSL.value &&
>          gHTTP.value == gFTP.value &&
>          gHTTP.value == gGopher.value &&
You forgot to remove this line.

>-
>+  ${RemoveDeprecatedKeys}
>+  
Nit: trailing spaces.

>+  
>+
Why the extra lines? Also, more trailing spaces (9 lines in total).

>+  ; Remove support for launching gopher urls from the shell during install or
>+  ; update if the DefaultIcon is from firefox.exe.
You forgot to rename this one to seamonkey.exe

>   // we know that http://, https://, ftp://, file://, chrome://, 
>-  // resource://, about:, and gopher:// (as if), 
>-  // should load in a browser.  but if we don't have one of those
>-  // (examples are mailto, imap, news, mailbox, snews, nntp, ldap, 
>-  // and externally handled schemes like aim)
>-  // we may or may not want a browser window, in which case we return here
>-  // and let the normal code handle it
>+  // resource://, and about, should load in a browser.  but if 
>+  // we don't have one of those (examples are mailto, imap, news, mailbox, snews, 
>+  // nntp, ldap, and externally handled schemes like aim) we may or may not 
>+  // want a browser window, in which case we return here and let the normal code 
>+  // handle it
>   var needABrowser = /(^http(s)?:|^ftp:|^file:|^gopher:|^chrome:|^resource:|^about:)/i;
>   if (href.search(needABrowser) == -1) 
>     return true;
So, just to confirm, if you receive a message with a gopher: URL, then clicking the URL doesn't open a browser window?
(Assignee)

Comment 15

7 years ago
(In reply to comment #14)
> Comment on attachment 457734 [details] [diff] [review]
> Removed gopher reference from Seamonkey v4
> 
> > function DefaultForShareSettingsPref()
> > {
> >   return gHTTP.value == gSSL.value &&
> >          gHTTP.value == gFTP.value &&
> >          gHTTP.value == gGopher.value &&
> You forgot to remove this line.

This is the problem that i seem to have. I have this
dumb tendency when redoing a patch to forget some
details.  Well, that and lack of attention to detail
I'll get this fixed.  
> 
> >-
> >+  ${RemoveDeprecatedKeys}
> >+  
> Nit: trailing spaces.
> 
> >+  
> >+
> Why the extra lines? Also, more trailing spaces (9 lines in total).
Will also get this fixed.

> 
> >+  ; Remove support for launching gopher urls from the shell during install or
> >+  ; update if the DefaultIcon is from firefox.exe.
> You forgot to rename this one to seamonkey.exe

Copy&paste error.  Noted and will get it fixed.
> 
> >   // we know that http://, https://, ftp://, file://, chrome://, 
> >-  // resource://, about:, and gopher:// (as if), 
> >-  // should load in a browser.  but if we don't have one of those
> >-  // (examples are mailto, imap, news, mailbox, snews, nntp, ldap, 
> >-  // and externally handled schemes like aim)
> >-  // we may or may not want a browser window, in which case we return here
> >-  // and let the normal code handle it
> >+  // resource://, and about, should load in a browser.  but if 
> >+  // we don't have one of those (examples are mailto, imap, news, mailbox, snews, 
> >+  // nntp, ldap, and externally handled schemes like aim) we may or may not 
> >+  // want a browser window, in which case we return here and let the normal code 
> >+  // handle it
> >   var needABrowser = /(^http(s)?:|^ftp:|^file:|^gopher:|^chrome:|^resource:|^about:)/i;
> >   if (href.search(needABrowser) == -1) 
> >     return true;
> So, just to confirm, if you receive a message with a gopher: URL, then clicking
> the URL doesn't open a browser window?

Reading that, it theoretically will.  Which shouldn't be the case.
I believe this is a missed patch.
(Assignee)

Comment 16

7 years ago
Created attachment 458285 [details] [diff] [review]
Removed gopher reference from Seamonkey v5
Attachment #457734 - Attachment is obsolete: true
Attachment #458285 - Flags: review?(bugspam.Callek)
Attachment #457734 - Flags: review?(bugspam.Callek)
Comment on attachment 458285 [details] [diff] [review]
Removed gopher reference from Seamonkey v5

NSIS only review for now (as it requires more concentration:

>diff --git a/suite/installer/windows/nsis/shared.nsh b/suite/installer/windows/nsis/shared.nsh
>@@ -40,16 +40,17 @@
> 
>   ; Remove registry entries for non-existent apps and for apps that point to our
>   ; install location in the Software\Mozilla key and uninstall registry entries
>   ; that point to our install location for both HKCU and HKLM.
>   SetShellVarContext current  ; Set SHCTX to the current user (e.g. HKCU) 
>   ${RegCleanMain} "Software\Mozilla"
>   ${RegCleanUninstall}
>   ${UpdateProtocolHandlers}
>+  ${RemoveDeprecatedKeys}
> 
>   ; Upgrade the copies of the MAPI DLLs

Can you drop this down to right above the |; Add Software\Mozilla\ registry entries| slightly further down.

>@@ -58,16 +59,17 @@
>     ${SetUninstallKeys}
>     ${UpdateProtocolHandlers}
>+    ${RemoveDeprecatedKeys}
> 

With the move above, this is not necessary. Also *ADD* an invoke of this macro just below the START of our |!macro SetAsDefaultAppGlobal|

[There are other spots I think we need to mimic Firefox's installer for, but for this bug I only want gopher to be removed from needed spots, we should probably plan to cleanup our installer in another bug]

>+!insertmacro RegCleanAppHandler
>+
>+!macro RemoveDeprecatedKeys

Please stick |; Removes various registry entries for reasons noted below (does not use SHCTX).| above this macro as a comment.

>+  ; Remove the gopher key if the DefaultIcon is from seamonkey.exe.
>+  ReadRegStr $2 SHCTX "$0\gopher\DefaultIcon" ""
>+  ClearErrors
>+  ${WordFind} "$2" "${FileMainEXE}" "E+1{" $R1
>+  ${Unless} ${Errors}
>+    DeleteRegKey SHCTX "$0\gopher"
>+  ${EndUnless}

Isn't this covered by the |+  ${RegCleanAppHandler} "gopher"| above?


I'm also requesting :rs to take a look at this, strictly for the nsh stuff, to see if any of my comments were wrong, or if you missed anything that I don't see. I will not block the patch landing on his comments though.
Attachment #458285 - Flags: review?(robert.bugzilla)
Comment on attachment 458285 [details] [diff] [review]
Removed gopher reference from Seamonkey v5

>+  ; Remove the gopher key if the DefaultIcon is from seamonkey.exe.
>+  ReadRegStr $2 SHCTX "$0\gopher\DefaultIcon" ""
>+  ClearErrors
>+  ${WordFind} "$2" "${FileMainEXE}" "E+1{" $R1
>+  ${Unless} ${Errors}
>+    DeleteRegKey SHCTX "$0\gopher"
>+  ${EndUnless}
This was added as an extra measure due to the gopher security bug. This removes gopher if gopher was associated to seamonkey.exe even when it isn't for this install location.
http://larholm.com/2007/06/12/safari-for-windows-0day-exploit-in-2-hours/
Attachment #458285 - Flags: review?(robert.bugzilla) → review+
(In reply to comment #18)
> Comment on attachment 458285 [details] [diff] [review]
> Removed gopher reference from Seamonkey v5
> 
> >+  ; Remove the gopher key if the DefaultIcon is from seamonkey.exe.
> >+  ReadRegStr $2 SHCTX "$0\gopher\DefaultIcon" ""
> >+  ClearErrors
> >+  ${WordFind} "$2" "${FileMainEXE}" "E+1{" $R1
> >+  ${Unless} ${Errors}
> >+    DeleteRegKey SHCTX "$0\gopher"
> >+  ${EndUnless}
> This was added as an extra measure due to the gopher security bug. This removes
> gopher if gopher was associated to seamonkey.exe even when it isn't for this
> install location.
> http://larholm.com/2007/06/12/safari-for-windows-0day-exploit-in-2-hours/

It was removed in the newer Firefox shared.nsh though (Which added RegCleanAppHandler)
It is safe to remove it
(Assignee)

Comment 21

7 years ago
Created attachment 458920 [details] [diff] [review]
Removed gopher reference from Seamonkey v6

Cleaned up the .nsh code as per comments 17-20
Attachment #458285 - Attachment is obsolete: true
Attachment #458920 - Flags: review?
Attachment #458285 - Flags: review?(bugspam.Callek)
(Assignee)

Comment 22

7 years ago
Created attachment 458965 [details] [diff] [review]
Removed gopher reference from Seamonkey v7

Fixed a syntax error in shared.nsh.
Attachment #458920 - Attachment is obsolete: true
Attachment #458965 - Flags: review?(bugspam.Callek)
Attachment #458920 - Flags: review?
Comment on attachment 458965 [details] [diff] [review]
Removed gopher reference from Seamonkey v7

suite/browser/pageinfo/pageInfo.js change? (Was changed in v3 patch; I may have told you not to change that, but can't find any record of it being discussed. So I'm not sure)

>diff --git a/suite/common/directory/directory.js b/suite/common/directory/directory.js
>-    // for file URLs, only do it for FTP/Gopher/etc URLs; the "rdf:files"
>+    // for file URLs, only do it for FTP/etc URLs; the "rdf:files"

re-nit: |// for file URLs, only do it for other URLs; the "rdf:files"| (as you had in v3)

>diff --git a/suite/installer/removed-files.in b/suite/installer/removed-files.in

Probably useful to apply the changes here to calendar/ but I will not force that on you, since Sunbird is officially dead anyway. So no [real] need.

>diff --git a/suite/installer/windows/nsis/shared.nsh b/suite/installer/windows/nsis/shared.nsh
>   ${UpdateProtocolHandlers}
>-
>+  

Nit, added whitespace.
>     ${AddHandlerValues} "SOFTWARE\Classes\nntp" "$2" "$8,0" "" "" ""
>   ${EndIf}
>+  
>+
> !macroend

Nit more whitespace additions.

>+  ${EndIf}
>+
>+!macroend
>+!define RemoveDeprecatedKeys "!insertmacro RemoveDeprecatedKeys"

Nit: kill the blankline above !macroend

>diff --git a/suite/locales/en-US/chrome/common/help/nav_help.xhtml b/suite/locales/en-US/chrome/common/help/nav_help.xhtml
>             <a href="glossary.xhtml#ftp">FTP</a>, Chrome,
>-            <a href="glossary.xhtml#gopher">Gopher</a></li>

Kill the ',' on previous line end, and be sure to close the li too.

r+=me with those addressed. And an answer on pageinfo.
Attachment #458965 - Flags: review?(bugspam.Callek) → review+
(Assignee)

Comment 24

7 years ago
Created attachment 460469 [details] [diff] [review]
Removed gopher reference from Seamonkey v8

Updated patch in accordance to comment #23.
Attachment #458965 - Attachment is obsolete: true
Attachment #460469 - Flags: review+
http://hg.mozilla.org/comm-central/rev/73a538b2e0e1
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
(In reply to comment #23)
>>diff --git a/suite/locales/en-US/chrome/common/help/nav_help.xhtml b/suite/locales/en-US/chrome/common/help/nav_help.xhtml
>>             <a href="glossary.xhtml#ftp">FTP</a>, Chrome,
>>-            <a href="glossary.xhtml#gopher">Gopher</a></li>
> Kill the ',' on previous line end, and be sure to close the li too.
Good catch!
(In reply to comment #15)
> (In reply to comment #14)
> > Comment on attachment 457734 [details] [diff] [review] [details]
> > Removed gopher reference from Seamonkey v4
> > 
> > > function DefaultForShareSettingsPref()
> > > {
> > >   return gHTTP.value == gSSL.value &&
> > >          gHTTP.value == gFTP.value &&
> > >          gHTTP.value == gGopher.value &&
> > You forgot to remove this line.
> 
> This is the problem that i seem to have. I have this
> dumb tendency when redoing a patch to forget some
> details.  Well, that and lack of attention to detail
> I'll get this fixed.  

Well, you fixed it in attachment 458285 [details] [diff] [review] but forgot about it again later so this is still there after check-in:

Error: gGopher is not defined
Source File: chrome://communicator/content/pref/pref-proxies.js
Line: 107
Version: unspecified → Trunk
(Assignee)

Comment 28

7 years ago
(In reply to comment #27)
> (In reply to comment #15)
> > (In reply to comment #14)
> > > Comment on attachment 457734 [details] [diff] [review] [details] [details]
> > > Removed gopher reference from Seamonkey v4
> > > 
> > > > function DefaultForShareSettingsPref()
> > > > {
> > > >   return gHTTP.value == gSSL.value &&
> > > >          gHTTP.value == gFTP.value &&
> > > >          gHTTP.value == gGopher.value &&
> > > You forgot to remove this line.
> > 
> > This is the problem that i seem to have. I have this
> > dumb tendency when redoing a patch to forget some
> > details.  Well, that and lack of attention to detail
> > I'll get this fixed.  
> 
> Well, you fixed it in attachment 458285 [details] [diff] [review] but forgot about it again later so this
> is still there after check-in:
> 
> Error: gGopher is not defined
> Source File: chrome://communicator/content/pref/pref-proxies.js
> Line: 107

I do profusely apologize for this mistake.  Will get that fixed.  
I've reopened this bug.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 29

7 years ago
Created attachment 464269 [details] [diff] [review]
Removed gopher reference from Seamonkey Follow-up [Checkin: comment 30]

Missed a gopher reference in pref-proxies.js.
Attachment #464269 - Flags: review?(bugspam.Callek)

Updated

7 years ago
Attachment #464269 - Flags: review?(bugspam.Callek) → review+
(Assignee)

Updated

7 years ago
Keywords: checkin-needed
Comment on attachment 464269 [details] [diff] [review]
Removed gopher reference from Seamonkey Follow-up [Checkin: comment 30]

http://hg.mozilla.org/comm-central/rev/96f2c2db44d0
Attachment #464269 - Attachment description: Removed gopher reference from Seamonkey v9 → Removed gopher reference from Seamonkey Follow-up [Checkin: comment 29]
Status: REOPENED → RESOLVED
Last Resolved: 7 years ago7 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.1a3
Attachment #464269 - Attachment description: Removed gopher reference from Seamonkey Follow-up [Checkin: comment 29] → Removed gopher reference from Seamonkey Follow-up [Checkin: comment 30]
You need to log in before you can comment on or make changes to this bug.