572389 - Remove gopher references from SeaMonkey code

Status: RESOLVED FIXED

(SeaMonkey :: UI Design, defect)

Description

[Robert Kaiser]

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

Comment 1

[Karsten Düsterloh]

This was truly a sad day. :(

Comment 2

[Robert Kaiser]

(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.

Comment 3

[Justin Wood (:Callek)]

Can be done in stages, adding "good first bug"

At least blocking final for installer-related changes, and pref-UI.

Comment 4

[Philip Chee]

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/

Comment 5

[Edmund Wong (:ewong)]

Attachment: Removed gopher reference from Seamonkey (patch 1) v1

Removed gopher references from directory.js and utilityOverlay.js.

Comment 6

[Justin Wood (: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.

Comment 7

[Edmund Wong (:ewong)]

Attachment: Removed gopher references from Seamonkey

Comment 8

[Justin Wood (: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-

Comment 9

[Justin Wood (:Callek)]

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

Comment 10

[Edmund Wong (:ewong)]

Attachment: Removed gopher reference from Seamonkey (patch 1) v3

Comment 11

[Robert Kaiser]

(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.

Comment 12

[Justin Wood (:Callek)]

(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.

Comment 13

[Edmund Wong (:ewong)]

Attachment: Removed gopher reference from Seamonkey v4

Comment 14

[[e-mail]]

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?

Comment 15

[Edmund Wong (:ewong)]

(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.

Comment 16

[Edmund Wong (:ewong)]

Attachment: Removed gopher reference from Seamonkey v5

Comment 17

[Justin Wood (: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.

Comment 18

[Robert Strong (they/them - no direct email)]

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/

Comment 19

[Justin Wood (:Callek)]

(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)

Comment 20

[Robert Strong (they/them - no direct email)]

It is safe to remove it

Comment 21

[Edmund Wong (:ewong)]

Attachment: Removed gopher reference from Seamonkey v6

Cleaned up the .nsh code as per comments 17-20

Comment 22

[Edmund Wong (:ewong)]

Attachment: Removed gopher reference from Seamonkey v7

Fixed a syntax error in shared.nsh.

Comment 23

[Justin Wood (:Callek)]

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.

Comment 24

[Edmund Wong (:ewong)]

Attachment: Removed gopher reference from Seamonkey v8

Updated patch in accordance to comment #23.

Comment 25

[Justin Wood (:Callek)]

http://hg.mozilla.org/comm-central/rev/73a538b2e0e1

Comment 26

[[e-mail]]

(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!

Comment 27

[Jens Hatlak (:InvisibleSmiley)]

(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

Comment 28

[Edmund Wong (:ewong)]

(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.

Comment 29

[Edmund Wong (:ewong)]

Attachment: Removed gopher reference from Seamonkey Follow-up [Checkin: comment 30]

Missed a gopher reference in pref-proxies.js.

Comment 30

[Jens Hatlak (:InvisibleSmiley)]

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