Closed Bug 572389 Opened 14 years ago Closed 14 years ago

Remove gopher references from SeaMonkey code

Categories

(SeaMonkey :: UI Design, defect)

x86
Linux
defect
Not set
normal

Tracking

(blocking-seamonkey2.1 final+)

RESOLVED FIXED
seamonkey2.1a3
Tracking Status
blocking-seamonkey2.1 --- final+

People

(Reporter: kairo, Assigned: ewong)

References

()

Details

(Whiteboard: [good first bug])

Attachments

(2 files, 7 obsolete files)

Bug 388195 removed gopher from Firefox and the core platform, we should remove the references we have in SeaMonkey as well.
This was truly a sad day. :(
(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: nobody → ewong
Status: NEW → ASSIGNED
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/
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+
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
Attachment #454775 - Attachment is obsolete: true
Attachment #456528 - Flags: superreview?(neil)
Attachment #456528 - Flags: review?(bugspam.Callek)
(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.
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.
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?
(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.
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
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)
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+
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
Closed: 14 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
(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 → ---
Missed a gopher reference in pref-proxies.js.
Attachment #464269 - Flags: review?(bugspam.Callek)
Attachment #464269 - Flags: review?(bugspam.Callek) → review+
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
Closed: 14 years ago14 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.