[RFE] Implement gopher

VERIFIED FIXED in mozilla0.8.1

Status

()

P3
enhancement
VERIFIED FIXED
19 years ago
18 years ago

People

(Reporter: jgoerzen, Assigned: bbaetz)

Tracking

Trunk
mozilla0.8.1
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: Fix in hand, Waiting for approval, URL)

Attachments

(29 attachments)

4.06 KB, patch
Details | Diff | Splinter Review
5.61 KB, application/octet-stream
Details
6.19 KB, application/octet-stream
Details
4.06 KB, patch
Details | Diff | Splinter Review
20.60 KB, patch
Details | Diff | Splinter Review
16.58 KB, patch
Details | Diff | Splinter Review
17.89 KB, patch
Details | Diff | Splinter Review
6.95 KB, application/octet-stream
Details
7.73 KB, patch
Details | Diff | Splinter Review
49.16 KB, patch
Details | Diff | Splinter Review
10.48 KB, application/octet-stream
Details
49.43 KB, patch
Details | Diff | Splinter Review
10.40 KB, patch
Details | Diff | Splinter Review
10.40 KB, application/octet-stream
Details
351.58 KB, application/zip
Details
17.80 KB, application/zip
Details
671 bytes, patch
Details | Diff | Splinter Review
18.15 KB, application/zip
Details
49.35 KB, patch
Details | Diff | Splinter Review
12.50 KB, application/zip
Details
18.14 KB, application/zip
Details
51.47 KB, patch
Details | Diff | Splinter Review
18.12 KB, application/zip
Details
53.16 KB, patch
Details | Diff | Splinter Review
17.83 KB, application/zip
Details
53.77 KB, patch
Details | Diff | Splinter Review
49.15 KB, patch
Details | Diff | Splinter Review
17.83 KB, application/zip
Details
54.62 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

19 years ago
From Bugzilla Helper:
User-Agent: Mozilla/4.72 [en] (X11; U; Linux 2.2.17pre10 i686)
BuildID:    2000072311

Whenever I try to access a gopherURL, I get
a message saying "gopher is not a registered protocol"

Reproducible: Always
Steps to Reproduce:
1. Type in gopher://blah and hit enter
2. Read error message
3. Apply sledgehammer to computer

Actual Results:  Got error message.

Expected Results:  Display gopher directory.

Gopher is still used, is still useful, and its lack will be sorely missed if not
added or fixed soon.

Comment 1

19 years ago
gopher is not implemented in Mozilla.
Changing severity to enhancement.
Modifying summary from "All gopher access fail" to "[RFE] Implement gopher"
Severity: major → enhancement
Summary: All gopher accesses fail → [RFE] Implement gopher

Comment 2

19 years ago
->nobody
Assignee: gagan → nobody
Keywords: helpwanted
Target Milestone: --- → Future

Comment 3

19 years ago
Confirming. Gopher is/was a nice protocol in it's time, and even if it's nearly
dead, it would be nice to have support for it. Adding 4xp keyword since both IE
and NS do this. Changing plat/os to All since this isn't linux/pc specific.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: 4xp
OS: Linux → All
Hardware: PC → All

Comment 4

19 years ago
Date: Sat, 26 Aug 2000 02:15:45 -0500
To: darkskyz@cyberspace.org From:
John Goerzen <jgoerzen@complete.org>
Subject: Gopher in Mozilla (Bug 49334)

Hi,

I don't know really how to work the bug system (feel free to forward this
post to it).  I'd like to make a few observations regarding gopher support.

First, yes, HTTP has really eclipsed it but it's not dead.  A lot of
universities are still using it for data, especially things generated from
mainframe databases.  I am actually developing a site around gopher
(quux.org) just for kicks (which is how I noticed this.)

Now then, I'd like to give some comments on implementation.  First,
supporting base gopher is incredibly easy.  This is what netscape does.  It
would be better to do a bit more than that, though.  Note that UMN has
recently GPL'd gopher/gopherd so you may be able to use parts of that code.

There is gopher+ which supports MIME types, language negotiation, and a
feature that HTTP does not have: content type negotiation.  (For example, if
I provide a resume in HTML, PDF, PS, ASCII, and DVI versions, the client can
request it in its preferred version and language).  It would be nice to
support this.

You may also want to compare the way Netscape does gopher to the way IE does
it.  IE appears to use gopher+ and provides more information about it.
Neither one do a particularly great job of providing a title, using instead
just "Gopher menu".  For any document that does not have a specific title
(this would be everything in gopher except HTML, most likely), the title
actually comes from the directory entry (link text) that pointed to it!  So
if I have a link to gopher://micro.tc.umn.edu/ and the text is "UMN Gopher
Server", set the title to that.  This may be hard as the URL spec doesn't
appear to provide for carrying that in the URL, so I realize that a web
browser may not handle it well.  Don't worry if it's hard, it's a minor nit.

You can find gopher specs and protocol information at:



gopher://quux.org:70/11/Archives/mirrors/boombox.micro.umn.edu/pub/gopher/gopher_protocol

or:

  ftp://boombox.micro.umn.edu/pub/gopher/gopher_protocol/  You can find the UMN
gopher server plus my bugfixes at:

  gopher://gopher.quux.org:70/11/Software/Gopher%20and%20Web/debgopher

Thanks!

-- John

Comment 5

18 years ago
Gopher is still widely used for listserv and listproc archives.  It isn't much,
and it's a feature that was present in all previous versions of Netscape.  It
would be nice to see it come back.

Comment 6

18 years ago
cc self, etc.
Assignee: nobody → gagan
Keywords: relnoteRTM
Whiteboard: relnote-user
Even if Gopher support isn't added any time soon, most proxies support Gopher,
so if there is a proxy set, it should at least try to request the Gopher URL
from the proxy. Currently, it doesn't even try if it sees a gopher:// URL.

Comment 8

18 years ago
I was suprised to find that anyone wanted Gopher support, I went surfing for
Gopher on Yahoo, and most of the links turned out to be dead ends...

If there is no gopher support, then it's time to remove it from the Proxy settings.

What might be better is if someone was encouraged to write a gopher module or
helper app, rather than build support into the core code. I'd like to see
Mozilla be narrowly focused on core specifications and be extensible to these
other protocols.

NOTE: Even if we do what Eric suggests above, it still requires implementation
of gopher, because we have to register the URL scheme (gopher) and add code that
supports following the proxy setting and the reception and handling of the content.

(Assignee)

Comment 9

18 years ago
Well, I'm going on an exchange program next year to McGill uni, and all the
timetabling and stuff in in a gopher system. So I want it :)

I've actually written some of it (starting from the datetime and finger
modules), and got it to the stage where it will download a file if you put in
the full path. It doesn't do directories yet though.

IT also only works when using the TestProtocol test program - it doesn't do
anything when using the browser, and stracting a server shows that its getting
SIGPIPE. I have no idea why. Using TestProtocol, it throws up XPCOM assertions
about missing queryinterfaces on every document load - I've got exams this week,
and then I'll try to track that down, and get it working.

The directoryservice has exceptions for ftp all over it, and I'll have to add
extra exceptions for gopher, so it won't be able to be fully modularised.
(Assignee)

Comment 10

18 years ago
Of course, the reason it doesn't work copy-and=pasting the datetime handler is
that the datetime protocol won't actually display anything in mozilla either.
And nsFingerHandler::OnStopRequest has mResponseContext and aContext reversed,
so errors don't get reported properly. Which is why gopher won't report errors.

Does that look right to the necko people? If it does, I'll file bugs on those two.
(Assignee)

Comment 11

18 years ago
Created attachment 19379 [details] [diff] [review]
patch to support gopher
(Assignee)

Comment 12

18 years ago
Created attachment 19380 [details]
gopher directory - unpack in netwerk/protocol
(Assignee)

Comment 13

18 years ago
Ok, the attached patches sort of support gopher. It won't do the directories
(which is the main part of gopher), but will happily get files if you type them
in to the URL bar correctly (remembering the leading type for gopher - just add
the first digit on the directory listing to the front of the URL)

It doesn't support directories - I'm not sure of the best way to do this.
It doesn't strip the trailing dot off, or give an error if the connection broke
before it got that - whats the best way of doing this?
It doesn't report errors - see bug 60421.
It probably doesn't handle ? or # in the url, but I haven't checked that.
Its a lot of cut and paste from the finger and datetime handler, but I can't get
datetime to work at the moment, so there are probably breakages.

Comment 14

18 years ago
We cannot access these URL:

gopher://simon.wharton.upenn.edu/11/birding
gopher://apa.oxy.edu
gopher://ernest.ccs.carleton.ca 419
...
...
 etc.
and more...

Telnet does not seem to be linked.

Should someone develop those modules ?
We hope so, soon !

K. and F.

(Assignee)

Comment 15

18 years ago
Gopher hasn't been hooked up yet - its just a patch. As a status update, I've
now got directories going to the fancy dir browser, but not working yet. The dir
browser code needs ursl to be realtive, but gopher urls are absolute by
definition. Maybe I'll have some more patches later today or early this week.
(Assignee)

Comment 16

18 years ago
I was about to submit my final patches.

Then I discovered what happens if two xul treenodes have the same id (browser
lockup - not nice). I got this because gopher uris are all absolute, and they
can reference each other. I think I have an idea how to fix this, but I'm going
to have to very carefully test that it doesn't break anything.

Except for that, everything in the core gopher spec appears to works except
proxies, which aren't implemented yet, and searches, which aren't done either.
Once I get the basics working, it shouldn't be too much effort to add those.

Clearing helpwanted, and assigning to me, since I've almost finished. (If I'm
not meant to do that, someone change it back)
Status: NEW → ASSIGNED
Keywords: helpwanted
(Assignee)

Comment 17

18 years ago
Assigning to me this time...
Assignee: gagan → bbaetz
Status: ASSIGNED → NEW

Comment 18

18 years ago
Adding to Mozilla1.0 radar because by that point, we should handle all of the
protocols that other browsers could typically handle. (telnet, gopher, etc).

bbaetz: If a final patch is forthcoming, please remember to add the "patch" and
"review" keywords to this bug once you attach it.
Keywords: mozilla1.0
(Assignee)

Comment 19

18 years ago
Created attachment 19609 [details]
gopher protocol version 2
(Assignee)

Comment 20

18 years ago
Created attachment 19610 [details] [diff] [review]
diff to add makefiles/module registration for gopher
(Assignee)

Comment 21

18 years ago
Created attachment 19611 [details] [diff] [review]
gopher directory converter
(Assignee)

Comment 22

18 years ago
Created attachment 19612 [details] [diff] [review]
diff to make nsDirectoryViewer work with gopher
(Assignee)

Comment 23

18 years ago
OK, the four patches above implement all the gopher functionality 4.x has,
except for proxies and searches. It integrates with the directory viewer. I
cleaned some of that code up - several instance variables weren't used, and
duplicated what could be got out of the context. Because xul ids (and thus uris)
ahve to be unique, I separated "real" gopher urls from each other using \n in
the same way ftp/file path componants are separated by /, and the current uri
optained when needed.

I implimented onMouseOver, so that the real uri can be seen in the status bar. I
added a description attribute, which is preferred over the name - see the source
for details.

Reviewers? Comments? 
Keywords: patch, review

Comment 24

18 years ago
Bradley: Did you e-mail those patches to reviewers@mozilla.org ?
(Assignee)

Comment 25

18 years ago
jce2@po.cwru.edu: I have now, thanks.
(Assignee)

Comment 26

18 years ago
Created attachment 19685 [details] [diff] [review]
Updated directory viewer patch
(Assignee)

Comment 27

18 years ago
Created attachment 19686 [details]
gopher protocol - supporting proxies
(Assignee)

Comment 28

18 years ago
Created attachment 19687 [details] [diff] [review]
diff for gopher proxy support
(Assignee)

Comment 29

18 years ago
OK, I now support proxies. Patch 19685 replaces patch 19612 (adding support for
open link in new windwo to the dir viewer), and patch 19686 replaces patch
19609. Patch 19687 is a new patch which adds pref support.

This support is subject to bug 61167, so searches won't work through a proxy
(they don't work without a proxy either yet) These patches can be checked in
without that though (hint, hint....)
(Assignee)

Comment 30

18 years ago
For some reason my mailer didn't send the message to reviewers@netscape.com to
the people I cc'd. No idea why, so I'm ccing the netwerk and xpfe reviewers
listed at http://www.mozilla.org/hacking/reviewers.html so that I can get these
patches reviewed.

Comment 31

18 years ago
Hey Bradley, can you tell us which of these patches you need reviewed to
actually get checked in? There are a lot of patches here and iw asn't sure which
ones count and which one's don't. Thanks =).
(Assignee)

Comment 32

18 years ago
The patches I need reviewed are:

Makefile patches:
http://bugzilla.mozilla.org/showattachment.cgi?attach_id=19610

Directory convertor:
http://bugzilla.mozilla.org/showattachment.cgi?attach_id=19611

Diff for directory viewer:
http://bugzilla.mozilla.org/showattachment.cgi?attach_id=19685

Gopher protocol (tgz file - unpack in netwerk/protocols):
http://bugzilla.mozilla.org/showattachment.cgi?attach_id=19686

Diffs for prefs/ui for proxy support:
http://bugzilla.mozilla.org/showattachment.cgi?attach_id=19687

Windows has support for registering schemes (in winhooks) - I don't have a
windows machine, so I can't test this. When this gets checked in, I'll open a
new rfe for that.

Bug 61167 stops queries working through a proxy, and I haven't implimented
searching without a proxy. That shouldn't stop this patch going in.

Thanks

Comment 33

18 years ago
*** Bug 61704 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 34

18 years ago
After some comments from jag and timeless on IRC, I've got an updated patch I'll
probably finish off tomorrow. Its much nicer, and avoid the '\n' hack.

I won't be able to attach a diff until cvs-mirror comes back up though.
(Assignee)

Comment 35

18 years ago
Created attachment 20515 [details] [diff] [review]
patch - latest version
(Assignee)

Comment 36

18 years ago
Created attachment 20516 [details]
new files for gopher - latest version
(Assignee)

Comment 37

18 years ago
OK, here are some updated patches. Changes:

- whitespace/linelength changes jag/timeless wanted, and a few cleanup patches
(using NS_LITERAL_STRING, etc), long lines, etc
- don't display information items - I can't stop them being selected and sorted
out of order.
- gopher:// won't be autocompleted
- remove a few printfs I left in by mistake
- Most importantly, remove the \n hack, since \n isn't valid in an XML id.
Instead, use a Target RDF attribute, pushed in by nsDirectoryViewer.cc. Because
file rdfs are done differently, the contextmenu js handles both cases.

cvs diff -N will only diffs cvs added files. Apply the diff, and then untar the
tgz in the mozilla directory.

I've tested this with gopher, ftp and file uris - they're the only ones which
use the directory viewer AFAIK.

Comments? Review?
(Assignee)

Comment 38

18 years ago
Jag's latest appcore removal gives conflicts in directory.js. I'm going to
attach an updated patch, which is the same as before but without the conflicts,
and I updated the emacs modeline to not produce tabs, because thats what the
file had anyway. This replaces patch 20515, and isn't different in content.

I'm recompiling to test though, and its doing a full rebuild for some reason, so
give it an hour or so (I'm part way through)

I thought I'd accepted this a while ago.
Status: NEW → ASSIGNED
Target Milestone: Future → mozilla0.8
(Assignee)

Comment 39

18 years ago
Created attachment 20593 [details] [diff] [review]
Updated patch without conflicts - replaces 20515
(Assignee)

Comment 40

18 years ago
scc suggested a small string change in nsGopherChannel.cpp - this replaces
20516. Can I please get a review on this? I'm going overseas in a week, so if
there are problems after that, it won't be happning til late January at the
earliest.
(Assignee)

Comment 41

18 years ago
Created attachment 20838 [details] [diff] [review]
small string change - replaces 20516
(Assignee)

Comment 42

18 years ago
Created attachment 20839 [details]
small string change - replaces 20516
(Assignee)

Comment 43

18 years ago
Ignore the first one - I selected the wrong mime type, and pressed stop too
late. Sorry.

Please review patch 20593 and 20839

Comment 44

18 years ago
Bradley, make sure you mail reviewers@mozilla.org to get someone to review this.
 We wouldn't want this to slip through the cracks as some patches have.
You need module owner OK first.

mscott, gagan, could you please review the netwerk parts?
rjc, mcafee, could you please review the directory viewer parts?

Comment 46

18 years ago
I can't read the 12/17/00 14:06 patch (with id = 20839). It thinks it's binary
(similar problem to the first string change patch which you already mentioned
was corrupted)

Comment 47

18 years ago
Couple comments,
1) can you tell me more about the changes you made to nsContextMenu.js? I didn't
understand what those had to do with gopher. Is it related to the fact that you
are adding a new property to the directory viewer?

Everything else looks pretty good to me. We need to get another reviewer though
for these things. Preferably someone more familiar with the directory viewer
code. (rjc, mcafee, etc. like you suggested)
(Assignee)

Comment 48

18 years ago
mscott: Its a tgz, sorry (I can't get cvs diff to diff against un cvsadded
files). winzip can read them.
(Assignee)

Comment 49

18 years ago
Created attachment 21008 [details]
zip version of patch 20839
(Assignee)

Comment 50

18 years ago
OK, unix zip doesn't overwrite existing files like I thought it did, so the
attached patch has the binary files as well (the zip I make before doing a make
clean). Oops. I'll attach another (much smaller) one.

Yes, the context menu patches are because the target is nowin a different rdf
attribute. I can't just replace id with target, because all the file:// uris
aren't handled in nsDirectoryViewer.cpp, and so they'd have to be changed
individually.
(Assignee)

Comment 51

18 years ago
Created attachment 21009 [details]
zip version without binary files. I'm really not having a good day...

Comment 52

18 years ago
cvs add the file (you don't need special permission)
now do a cvs diff -N

alternatively, do a diff, attach that, then attach the new files individually.
(Assignee)

Comment 53

18 years ago
~/src/mozilla/netwerk/streamconv/converters$ cvs diff -N
nsGopherDirListingConv.cpp
cvs server: I know nothing about nsGopherDirListingConv.cpp
~/src/mozilla/netwerk/streamconv/converters$ cvs add nsGopherDirListingConv.cpp
cvs [server aborted]: "add" requires write access to the repository

I don't have a cvs account.

Anyway, there are 12 new files all up (including makefiles and .cvsignores). I
could individually make diffs for all of them (and attaching them separately),
but that strikes me as a waste of time. I'll do so if someone wants me to
though.

Comment 54

18 years ago
oh, sorry about that. I guess cvs is dumber than I thought :)

"cvs add" does not actually make any changes on the server so I didn't realize
you'd need an account for that.

Updated

18 years ago
Whiteboard: relnote-user → Fix in hand, Waiting for approval
Bradley - are you back from holiday yet? :-) What's the score with gopher - 
still waiting for reviewers?

Gerv
(Assignee)

Comment 56

18 years ago
I've moved to Canada now, and am still looking for reviewers. I don't have a
computer at home yet though (its meant to arrive at the end of next week), then
I have to get net access at home, and the disk quota at uni isn't really big
enough to build mozilla, so it'll be a few weeks. I compiled xchat yesterday,
but that put me over my disk quota, so I had to delete it. I'll start nagging
again soon, although I won't be able to test any changes unti lmy computer
arrives.
Cc'ing darin for r= and moral support, if possible.

/be

Comment 58

18 years ago
r=darin on the stuff under netwerk/protocol/gopher (looks good)

I am, however, less familiar with the way stream converters work... dougt would
probably be the right person to contact for a review on that section.  He has
been working on FTP and making some changes (to the stream converters I think),
so he might have some useful comments for you.
I will review the directory viewer and stream convert stuff tomorrow.
The stream converter looks fine.

I am going to defer the changes to the html indexer to waterson or rjc as the
changes are pretty big and they initally wrote the code.

(Assignee)

Comment 61

18 years ago
I won't have a net connection on my new computer til the middle of next week,
but I suspect that the fixes for bug 62566 broke gopher, and changes similar to
nsDateTimeChannel's changes need to be made. I really haven't been following
mozilla development closly latly though, so there may be other changes.

The directory viewer changes can still be reviewed though (hint, hint)

Comment 62

18 years ago
I am attaching a fix to help you migrate to the new AsyncWrite signature.

Comment 63

18 years ago
Created attachment 23320 [details] [diff] [review]
Revision to nsGopherChannel.cpp per changes in bug 62566

Comment 64

18 years ago
Waterson? Rjc? We just need one last r= here before we get can the sr= and this
code can actually go in.
(Assignee)

Comment 65

18 years ago
The new patches just merges Darin's changes (+ a missing #include) with the
existing patch so that gopher compiles. I still need r= from waterson or rjc for
the directoryviewer changes.
Someone also needs to review the one line change to
caps/src/nsScriptSecurityManager.cpp. The prefs and urlbar changes need to be
reviewed, but they're a simple cut and paste from the other protocols. Can
someone do those please?

One thing I noticed which manually reviewing the patch again is the pref UI:

Currently, the port hotkeys use P, O, R, T for FTP, HTTP, SSL, and SOCKS. I
added gopher between FTP and HTTP (Thats the order 4.x has it, and its sort of
alphabetical), and gave the port a hot key of ":" (anything else results in the
hot key being printed in brackets afterwards, and it looked ugly). So its now P,
:, O, R, T. I could reorder them so that either gopher, or the ':' comes last.
(I don't think thats a problem), but then the next protocol handler will have
problems.

mpt suggested a slightly improved UI on IRC, involving removing the access keys,
and some general tidying up. I don't think that this should block the gopher
checkin. If theres not a bug filed on it, I'll file one, and this will be
cleaned up with that.

Compilable patches attached.
(Assignee)

Comment 66

18 years ago
Created attachment 23767 [details]
zip of new files
(Assignee)

Comment 67

18 years ago
Created attachment 23768 [details] [diff] [review]
diffs

Comment 68

18 years ago
bbaetz: this stuff looks pretty good! If you could take a stab at fixing some of
the problems we talked about on IRC last night, that'd be great...

- GetDest() could probably have a better name, and leaks whenever it has to
  do a ToNewCString()

- Can we get rid of GetDest() altogether by parsing the Gopher `target'
  as an RDF resource? (Would require adding a ParseResource() method?)
(Assignee)

Comment 69

18 years ago
> - GetDest() could probably have a better name, and leaks whenever it has to
>   do a ToNewCString()

I fixed the leak by making all the uris nsIRDFResources instead of literals
(they're uris - they have to be ascii), and adding a comment noting that all the
bits which use CStrings (inclusing the application/http-index which we start
with) would have to be changed if file uris are supported through this
interface.

GetDestination()? I don't want to call it GetTarget(), because the target is an
RDF attribute, but it may not be present. Basically the changes to the directory
viewer remove the assumption that the thing to display is the same as the place
to go to. But file uris don't go through nsDirectoryViewer.cpp, and I can't
write XUL to use one attribute if its present, otherwise use another one.

So:

> - Can we get rid of GetDest() altogether by parsing the Gopher `target'
>   as an RDF resource? (Would require adding a ParseResource() method?)

No. I didn't explain this last night (it was late...)

This uri is never parsed (my comments to you on IRC were wrong). The important
thing (which is why this is more complex than just adding another field) is that
you can't get the parent entry from child, like you can from an ftp uri. And
these uris aren't unique, so the uri from XUL's point of view (which must be
unique) is starturi/childuri/child2uri/etc. The only characters not valid from
gopher's point of view (\n, \r, and \t only) aren't valid XML ids (I initially
separated componants with \n) (I think it was jag who pointed that out to me).
So the target is simply the end componant. nsDirectoryViewer.cpp adds the target
as another attribute for this reason. For ftp, the attributes are the same, but
there's no reason for it to be present. (I could conditionalise adding this
entry to make it only occur for gopher, I suppose. Its also not present for the
initial url entered in the urlbar). file:// rdf attributes don't have the target
stuff, but they never see nsDirectoryViewer.cpp anyway.

This is also the reason that the context menu has to be changed. The context
menu is broken though - see bug 67013. I'll patch that up tonight as well, and
check that "open in new window" still works for gopher, file, and ftp.
law@netscape.com mentioned (in that bug) that other places will have the same
bug. If I can find them, I'll fix them as well. If there is charset conversion
stuff happening, I may need to rethink this. The previous code uses CStrings for
this though, so I don't think there's a problem.

I'll attach a new patch later this evening (fixing one other minor bug - I need
an OnMouseExit routine as well, and the mouseover stuff doesn't work for file
uris). Could you also please look at the comment I added arround line 1266 - why
is it done this way?

mpt: I'll get rid of the accessor key in the prefs (so the port just won't have
one), and file a bug for the pref proxy window redesign like we discussed.
> they're uris - they have to be ascii

No, URIs don't have to be ASCII. Not even hostnames have to be.


(Assignee)

Comment 71

18 years ago
> No, URIs don't have to be ASCII. Not even hostnames have to be.

But they're stored escaped, so they do. Note that I'm not changing anything -
the uri is an RDFResource, and so is currently (without this patch) ascii. Also,
gopher paths are definately ascii, and I'd be surprised if ftp paths were
allowed to be anything else. (a gopher hostname is transmitted in ascii from the
server)

file uris aren't handled here, but I'd be interested in knowing if non-ASCII
filenames work currently (probably after applying the fix to bug 67013 first,
which sets the default charset up) Noone on IRC last night had a setup to test
that though.

I didn't upload my patch last night, because I found a way to crash gopher. I'll
fix it, and upload tonight.
(Assignee)

Comment 72

18 years ago
Created attachment 24028 [details]
fix for crash: nsGopherChannel.cpp:97
(Assignee)

Comment 73

18 years ago
Created attachment 24029 [details]
left the streamconverters out - this is the zip
(Assignee)

Comment 74

18 years ago
Created attachment 24030 [details] [diff] [review]
...and the diff

Comment 75

18 years ago
r=darin for the crash fix

Comment 76

18 years ago
Comments on nsDirectoryViewer.[h|cpp] below. I've cited some bug numbers below 
that I'm worried you might regress: please check 'em.

Index: xpfe/components/directory/nsDirectoryViewer.cpp
===================================================================
RCS file: /cvsroot/mozilla/xpfe/components/directory/nsDirectoryViewer.cpp,v
retrieving revision 1.61
diff -u -r1.61 nsDirectoryViewer.cpp
--- xpfe/components/directory/nsDirectoryViewer.cpp     2000/11/02 07:34:43     
1.61
+++ xpfe/components/directory/nsDirectoryViewer.cpp     2001/02/01 01:02:23
@@ -57,6 +61,7 @@
 #include "nsIInterfaceRequestor.h"
 #include "iostream.h"
 #include "nsITextToSubURI.h"
+#include "nsMemory.h"
 
 //----------------------------------------------------------------------
 //

Why do you need nsMemory.h explicitly?

@@ -88,20 +97,26 @@
   static nsITextToSubURI* gTextToSubURI;
   static nsIRDFResource* kHTTPIndex_Comment;
   static nsIRDFResource* kHTTPIndex_Filename;
+  static nsIRDFResource* kHTTPIndex_Desc;

                                     ^^^^
XXX `Description', please!

   static nsIRDFResource* kHTTPIndex_Filetype;
   static nsIRDFResource* kHTTPIndex_Loading;
+  static nsIRDFResource* kHTTPIndex_Target;
   static nsIRDFResource* kNC_Child;
   static nsIRDFLiteral*  kTrueLiteral;
   static nsIRDFLiteral*  kFalseLiteral;
 
-  static nsresult ParseLiteral(nsIRDFResource *arc, const nsString& aValue, 
nsIRDFNode** aResult);
-  static nsresult ParseDate(nsIRDFResource *arc, const nsString& aValue, 
nsIRDFNode** aResult);
-  static nsresult ParseInt(nsIRDFResource *arc, const nsString& aValue, 
nsIRDFNode** aResult);
+  static nsresult ParseLiteral(nsIRDFResource *arc, const nsString& aValue,
+                               nsIRDFNode** aResult);
+  static nsresult ParseDate(nsIRDFResource *arc, const nsString& aValue,
+                            nsIRDFNode** aResult);
+  static nsresult ParseInt(nsIRDFResource *arc, const nsString& aValue,
+                           nsIRDFNode** aResult);


XXX Please shoot whoever told you to waste your time making these
whitespace changes. In the future, if you want to clean up whitespace
then please put it in another patch.
 
   struct Field {
     const char      *mName;
     const char      *mResName;
-    nsresult        (*mParse)(nsIRDFResource *arc, const nsString& aValue, 
nsIRDFNode** aResult);
+    nsresult        (*mParse)(nsIRDFResource *arc, const nsString& aValue,
+                              nsIRDFNode** aResult);
     nsIRDFResource* mProperty;
   };
 
@@ -111,14 +126,15 @@
 
   nsCOMPtr<nsIRDFDataSource> mDataSource;
   nsCOMPtr<nsIRDFResource> mDirectory;
-
-  nsCString mURI;
+  
   nsCString mBuf;
   PRInt32   mLineStart;
-
+  PRBool    mHasDesc; // Is there a description entry?
+  

XXX mHasDescription?

   nsresult ProcessData(nsISupports *context);
   nsresult ParseFormat(const char* aFormatStr);
-  nsresult ParseData(nsString* values, const char *baseStr, const char 
*encodingStr, char* aDataStr, nsIRDFResource *parentRes);
+  nsresult ParseData(nsString* values, const char *encodingStr, char* aDataStr,
+                     nsIRDFResource *parentRes);
 
   nsAutoString mComment;
 
@@ -152,30 +168,42 @@
 nsITextToSubURI *nsHTTPIndexParser::gTextToSubURI;
 nsIRDFResource* nsHTTPIndexParser::kHTTPIndex_Comment;
 nsIRDFResource* nsHTTPIndexParser::kHTTPIndex_Filename;
+nsIRDFResource* nsHTTPIndexParser::kHTTPIndex_Desc;


XXX kHTTPIndex_Description?

 nsIRDFResource* nsHTTPIndexParser::kHTTPIndex_Filetype;
 nsIRDFResource* nsHTTPIndexParser::kHTTPIndex_Loading;
+nsIRDFResource* nsHTTPIndexParser::kHTTPIndex_Target;
 nsIRDFResource* nsHTTPIndexParser::kNC_Child;
 nsIRDFLiteral*  nsHTTPIndexParser::kTrueLiteral;
 nsIRDFLiteral*  nsHTTPIndexParser::kFalseLiteral;
 
 
-
 // This table tells us how to parse the fields in the HTTP-index
 // stream into an RDF graph.
 nsHTTPIndexParser::Field
 nsHTTPIndexParser::gFieldTable[] = {
-  { "Filename",       "http://home.netscape.com/NC-rdf#Name",              
nsHTTPIndexParser::ParseLiteral, nsnull },
-  { "Content-Length", "http://home.netscape.com/NC-rdf#Content-Length",    
nsHTTPIndexParser::ParseInt,     nsnull },
-  { "Last-Modified",  "http://home.netscape.com/WEB-rdf#LastModifiedDate", 
nsHTTPIndexParser::ParseDate,    nsnull },
-  { "Content-Type",   "http://home.netscape.com/NC-rdf#Content-Type",      
nsHTTPIndexParser::ParseLiteral, nsnull },
-  { "File-Type",      "http://home.netscape.com/NC-rdf#File-Type",         
nsHTTPIndexParser::ParseLiteral, nsnull },
-  { "Permissions",    "http://home.netscape.com/NC-rdf#Permissions",       
nsHTTPIndexParser::ParseLiteral, nsnull },
-  { nsnull,           "",                                                  
nsHTTPIndexParser::ParseLiteral, nsnull },
+  { "Filename",       "http://home.netscape.com/NC-rdf#Name",
+    nsHTTPIndexParser::ParseLiteral, nsnull },
+  { "Description",    "http://home.netscape.com/NC-rdf#Description",
+    nsHTTPIndexParser::ParseLiteral, nsnull },
+  { "Content-Length", "http://home.netscape.com/NC-rdf#Content-Length",
+    nsHTTPIndexParser::ParseInt,     nsnull },
+  { "Last-Modified",  "http://home.netscape.com/WEB-rdf#LastModifiedDate",
+    nsHTTPIndexParser::ParseDate,    nsnull },
+  { "Content-Type",   "http://home.netscape.com/NC-rdf#Content-Type",
+    nsHTTPIndexParser::ParseLiteral, nsnull },
+  { "File-Type",      "http://home.netscape.com/NC-rdf#File-Type",
+    nsHTTPIndexParser::ParseLiteral, nsnull },
+  { "Permissions",    "http://home.netscape.com/NC-rdf#Permissions",
+    nsHTTPIndexParser::ParseLiteral, nsnull },
+  { nsnull,           "",
+    nsHTTPIndexParser::ParseLiteral, nsnull },
 };
 
-nsHTTPIndexParser::nsHTTPIndexParser(nsHTTPIndex* aHTTPIndex, nsISupports* 
aContainer)
+nsHTTPIndexParser::nsHTTPIndexParser(nsHTTPIndex* aHTTPIndex,
+                                     nsISupports* aContainer)
   : mHTTPIndex(aHTTPIndex),
     mLineStart(0),
+    mHasDesc(PR_FALSE),
     mContainer(aContainer)
 {
   NS_INIT_REFCNT();
@@ -209,24 +237,39 @@
                                       NS_REINTERPRET_CAST(nsISupports**, 
&gTextToSubURI));
     if (NS_FAILED(rv)) return rv;
 
-    rv = gRDF->GetResource(HTTPINDEX_NAMESPACE_URI "Comment",  
&kHTTPIndex_Comment);
+    rv = gRDF->GetResource(HTTPINDEX_NAMESPACE_URI "Comment",
+                           &kHTTPIndex_Comment);
     if (NS_FAILED(rv)) return rv;
 
-    rv = gRDF->GetResource(HTTPINDEX_NAMESPACE_URI "Name", 
&kHTTPIndex_Filename);
+    rv = gRDF->GetResource(HTTPINDEX_NAMESPACE_URI "Name",
+                           &kHTTPIndex_Filename);
     if (NS_FAILED(rv)) return rv;
+    
+    rv = gRDF->GetResource(HTTPINDEX_NAMESPACE_URI "Description",
+                           &kHTTPIndex_Desc);

XXX Please `kHTTPIndex_Desc' to `kHTTPIndex_Description'. The
convention is to name the constant as the namespace prefix, then an
underscore, then the property value.

+    if (NS_FAILED(rv)) return rv;    
 
-    rv = gRDF->GetResource(HTTPINDEX_NAMESPACE_URI "File-Type", 
&kHTTPIndex_Filetype);
+    rv = gRDF->GetResource(HTTPINDEX_NAMESPACE_URI "File-Type",
+                           &kHTTPIndex_Filetype);
     if (NS_FAILED(rv)) return rv;
 
-    rv = gRDF->GetResource(HTTPINDEX_NAMESPACE_URI "loading",  
&kHTTPIndex_Loading);
+    rv = gRDF->GetResource(HTTPINDEX_NAMESPACE_URI "loading",
+                           &kHTTPIndex_Loading);
+    if (NS_FAILED(rv)) return rv;
+    
+    rv = gRDF->GetResource(HTTPINDEX_NAMESPACE_URI "Target",
+                           &kHTTPIndex_Target);
     if (NS_FAILED(rv)) return rv;
 
-       rv = gRDF->GetResource(NC_NAMESPACE_URI "child",   &kNC_Child);
+    rv = gRDF->GetResource(NC_NAMESPACE_URI "child",
+                           &kNC_Child);
     if (NS_FAILED(rv)) return rv;
 
-    rv = gRDF->GetLiteral(NS_ConvertASCIItoUCS2("true").GetUnicode(), 
&kTrueLiteral);
+    rv = gRDF->GetLiteral(NS_LITERAL_STRING("true"),
+                          &kTrueLiteral);
     if (NS_FAILED(rv)) return rv;
-    rv = gRDF->GetLiteral(NS_ConvertASCIItoUCS2("false").GetUnicode(), 
&kFalseLiteral);
+    rv = gRDF->GetLiteral(NS_LITERAL_STRING("false"),
+                          &kFalseLiteral);
     if (NS_FAILED(rv)) return rv;
 
     for (Field* field = gFieldTable; field->mName; ++field) {
@@ -356,29 +404,16 @@
     if (! ok)
       return NS_ERROR_FAILURE;
   }  
-
-  // Save off some information about the stream we're about to parse.
-  nsCOMPtr<nsIURI> mDirectoryURI;
-  rv = aChannel->GetURI(getter_AddRefs(mDirectoryURI));
-  if (NS_FAILED(rv)) return rv;
-
-  nsXPIDLCString uristr;
-  rv = mDirectoryURI->GetSpec(getter_Copies(uristr));
-  if (NS_FAILED(rv)) return rv;
 
-  // we know that this is a directory (due to being a HTTP-INDEX response)
-  // so ensure it ends with a slash if its a FTP URL
-  mURI.Assign(uristr);
-  if ((mURI.Find("ftp://", PR_TRUE) == 0) && (mURI.Last() != (PRUnichar('/'))))
-  {
-       mURI.Append('/');
+  // Get the directory from the context
+  mDirectory = do_QueryInterface(aContext);
+  if (!mDirectory) {
+    return(NS_ERROR_UNEXPECTED);
   }

XXX I'm not convinced that you don't need mURI anymore. Did you test
this with FTP URLs, both with and without trailing '/'? How about
filesystem URLs? See for example bug 31586 and bug 51016.


-  rv = gRDF->GetResource(mURI, getter_AddRefs(mDirectory));
-  if (NS_FAILED(rv)) return rv;
-
   // Mark the directory as "loading"
-  rv = mDataSource->Assert(mDirectory, kHTTPIndex_Loading, kTrueLiteral, 
PR_TRUE);
+  rv = mDataSource->Assert(mDirectory, kHTTPIndex_Loading,
+                           kTrueLiteral, PR_TRUE);
   if (NS_FAILED(rv)) return rv;
 
   return NS_OK;
@@ -609,6 +622,9 @@
     // Okay, we're gonna monkey with the nsStr. Bold!
     name.mLength = nsUnescapeCount(name.mStr);
 
+    if (name.EqualsIgnoreCase("description"))
+        mHasDesc = PR_TRUE;
+

XXX Why EqualsIgnoreCase? Isn't the back-end code that you wrote the
stuff that'll generate this string?

     Field* field = nsnull;
     for (Field* i = gFieldTable; i->mName; ++i) {
       if (name.EqualsIgnoreCase(i->mName)) {
@@ -671,6 +679,13 @@
   nsCAutoString        filename;
   PRBool    isDirType = PR_FALSE;
 
+  const char* baseStr=nsnull;
+  parentRes->GetValueConst(&baseStr);
+  if (!baseStr) {
+    NS_WARNING("Could not reconstruct base uri\n");
+    return NS_ERROR_UNEXPECTED;
+  }
+

XXX Convention in this file is to use spaces in assignment (baseStr =
nsnull) and after bang (! baseStr). You're in Rome, be happy.

XXX If you're going to return NS_ERROR_UNEXPECTED, then you're saying
this quite the hard error. Use NS_ERROR, not NS_WARNING to force a
debugger trap in debug build.

   for (PRInt32 i = 0; i < numFormats; ++i) {
     // If we've exhausted the data before we run out of fields, just
     // bail.
@@ -755,40 +770,81 @@
         }
     }
   }
-
-      // we found the filename; construct a resource for its entry
-      nsCAutoString entryuriC(baseStr);
-      entryuriC.Append(filename);
-
-      // if its a directory, make sure it ends with a trailing slash
-      if (isDirType == PR_TRUE)
-      {
-            entryuriC.Append('/');
-      }
 
-      nsCOMPtr<nsIRDFResource> entry;
-      rv = gRDF->GetResource(entryuriC, getter_AddRefs(entry));
+  // we found the filename; construct a resource for its entry
+  nsCAutoString entryuriC(baseStr);
+  
+  // gopher resource don't point to an entry in the same directory
+  // like ftp uris. So the entryuriC is just a unique string, while
+  // the target attribute is the destination of this element
+  entryuriC.Append(filename);

XXX I don't get why you added this comment, but then didn't change any
of the logic.

+  
+  // if its a directory, make sure it ends with a trailing slash.
+  // This doesn't matter for gopher, (where directories don't have
+  // to end in a trailing /), because the filename is used for the target
+  // attribute.
+  if (isDirType == PR_TRUE)
+    {
+      entryuriC.Append('/');
+    }

XXX Ibid.

+  
+  nsCOMPtr<nsIRDFResource> entry;
+  rv = gRDF->GetResource(entryuriC, getter_AddRefs(entry));
 
   // At this point, we'll (hopefully) have found the filename and
   // constructed a resource for it, stored in entry. So now take a
   // second pass through the values and add as statements to the RDF
   // datasource.
+
   if (entry && NS_SUCCEEDED(rv)) {
-    for (PRInt32 indx = 0; indx < numFormats; ++indx) {
-      Field* field = NS_STATIC_CAST(Field*, mFormat.ElementAt(indx));
-//      Field* field = NS_REINTERPRET_CAST(Field*, mFormat.ElementAt(indx));
-      if (! field)
-        continue;
+
+    // assert the target, where this entry will lead to
+    /* This should be an nsIRDFResource, not an nsIRDFLiteral, because:
+     * 1) The filename is ASCII to start with
+     * 2) Items which don't have a target use the nsIRDFResource (for GetDest),
+     * and we need a shared const char*, not a const PRUnicode*
+     *
+     * If this ever handles file:// uris, this may have to change if local 
unicode
+     * file names aren't escaped in some way first. I don't even know if that's 
supported.
+     */

XXX You're in a C++ file. Use C++ comments.

+    nsCOMPtr<nsIRDFResource> targetVal;
+    nsCString target;
 
-      nsCOMPtr<nsIRDFNode> nodeValue;
-      rv = (*field->mParse)(field->mProperty, values[indx], 
getter_AddRefs(nodeValue));
-      if (NS_FAILED(rv)) break;
-      if (nodeValue)
-      {
-        rv = mDataSource->Assert(entry, field->mProperty, nodeValue, PR_TRUE);
+    if (!strncmp(entryuriC,kGopherProtocol, sizeof(kGopherProtocol)-1))
+      target.Assign(filename);
+    else
+      target.Assign(entryuriC);

XXX Ah, I see now. Can we factor this logic better, so that we don't
have to do all the string munging into entryuriC if we're just going
to throw it away?

+
+    rv = gRDF->GetResource(target.get(), getter_AddRefs(targetVal));

XXX I suspect that this is going to break FTP servers that have I18n
files. See bug 10373.

+
+    if (NS_SUCCEEDED(rv)) {
+      mDataSource->Assert(entry, kHTTPIndex_Target, targetVal, PR_TRUE);
+
+      for (PRInt32 indx = 0; indx < numFormats; ++indx) {
+        Field* field = NS_STATIC_CAST(Field*, mFormat.ElementAt(indx));
+//      Field* field = NS_REINTERPRET_CAST(Field*, mFormat.ElementAt(indx));

XXX Go ahead and nuke the commented-out code.

+        if (! field)
+          continue;
+        
+        if (mHasDesc && field->mProperty==kHTTPIndex_Filename)
+          continue;

XXX Whitespace around `==' in Rome.

+        
+        nsCOMPtr<nsIRDFNode> nodeValue;
+        rv = (*field->mParse)(field->mProperty, values[indx], 
getter_AddRefs(nodeValue));
         if (NS_FAILED(rv)) break;
+        if (nodeValue) {
+          // If there is a description entry, prefer that over the filename
+          // XUL doesn't appear to be able to select a ruleset based on whether 
an
+          // rdf attribute is present, so I can't do that there - 
bbaetz@student.usyd.edu.au

XXX I'm not sure what problem you're trying to solve here?

+          if (mHasDesc && field->mProperty==kHTTPIndex_Desc)

XXX Whitespace around `=='

+            rv = mDataSource->Assert(entry, kHTTPIndex_Filename, nodeValue, 
PR_TRUE);

XXX Do you care about `rv'? If not, don't bother assigning.

+          
+          rv = mDataSource->Assert(entry, field->mProperty, nodeValue, 
PR_TRUE);
+          if (NS_FAILED(rv)) break;
+        }
       }
     }
+    
 //   instead of
 //       rv = mDataSource->Assert(parentRes, kNC_Child, entry, PR_TRUE);
 //       if (NS_FAILED(rv)) return rv;
@@ -813,7 +869,7 @@
         PRInt32        len = aValue.Length();
         if (len > 0)
         {
-            if (aValue[len - 1] == '/')
+          if (aValue[len - 1] == '/' && 
aValue.CompareWithConversion(kGopherProtocol,PR_TRUE,sizeof(kGopherProtocol)-1))

XXX CompareWithConversion() almost always means you're doing something
wrong. I think you want to do

  aValue.Compare(NS_LITERAL_STRING("gopher://"))

             {
                 nsAutoString  temp(aValue);
                 temp.SetLength(len - 1);
@@ -907,6 +963,7 @@
 
        NS_IF_RELEASE(kNC_Child);
        NS_IF_RELEASE(kNC_loading);
+        NS_IF_RELEASE(kNC_Target);

XXX What's with the funky whitespace? These should line up.

        NS_IF_RELEASE(kTrueLiteral);
        NS_IF_RELEASE(kFalseLiteral);
 
@@ -953,6 +1010,7 @@
 
        mDirRDF->GetResource(NC_NAMESPACE_URI "child",   &kNC_Child);
        mDirRDF->GetResource(NC_NAMESPACE_URI "loading", &kNC_loading);
+        mDirRDF->GetResource(NC_NAMESPACE_URI "Target", &kNC_Target);

XXX Ibid.

 
     rv = mDirRDF->GetLiteral(NS_ConvertASCIItoUCS2("true").GetUnicode(), 
&kTrueLiteral);
     if (NS_FAILED(rv)) return(rv);
@@ -1077,7 +1135,28 @@
   return NS_OK;
 }
 
+const char* nsHTTPIndex::GetDest(nsIRDFResource* r) {

XXX Please, let's think of something better to call this than
`GetDest'. How about `GetRealDestination' or `GetRealLocation'? Also,
some commentary on why this magical function does what it does would
help!

+  // First try the target attribute
+  nsCOMPtr<nsIRDFNode> node;
+  
+  // use mInner, because GetTarget calls isWellknownContainerURI, and so we end 
up in a loop

XXX Er, unterminated recursion, right?

+  nsresult rv = mInner->GetTarget(r, kNC_Target, PR_TRUE, 
getter_AddRefs(node));
+  nsCOMPtr<nsIRDFResource> target;
+  
+  if (NS_SUCCEEDED(rv)) {

XXX mInner->GetTarget() will always `succeed' (modulo catastrophic
failure). I think what you mean is,

  if (node)
   target = do_QueryInterface(node);

+    target = do_QueryInterface(node);
+  }
 
+  const char* uri = nsnull;
+  // Don't use the target if we don't have it

XXX Obviously! A comment should say what the code is doing: the code
says that. The comment should say *why* the code is doing what it's
doing.

+  if (!target) {
+    r->GetValueConst(&uri);
+  } else {
+    target->GetValueConst(&uri);
+  }
+
+  return uri;
+}
 
 // rjc: isWellknownContainerURI() decides whether a URI is a container for 
which,
 // when asked (say, by the template builder), we'll make a network connection
@@ -1091,15 +1170,19 @@
 //            get double the # of answers we really want... also, "rdf:file" is
 //            less expensive in terms of both memory usage as well as speed
 
-static const char              kFTPProtocol[] = "ftp://";
+// New - we also handle gopher URLs.
+// This logic (and the gopher-specific exceptions cattered about the source
+// should be moved out somewhere (nsIURI and descendents?). That way pluggable
+// protocols could take advantage of this as well - bbaetz@student.usyd.edu.au
 
 PRBool
 nsHTTPIndex::isWellknownContainerURI(nsIRDFResource *r)
 {
        PRBool          isContainerFlag = PR_FALSE;
        const char      *uri = nsnull;
-       
-       r->GetValueConst(&uri);
+
+        uri = GetDest(r);

XXX Funky spacing. Make sure this lines up. Also, add a comment here;
e.g., ``for gopher URLs, we need to follow blah blah blah to get the
real location of the file.''

+
        if ((uri) && (!strncmp(uri, kFTPProtocol, sizeof(kFTPProtocol) - 1)))

XXX If we're already looking at the protocol, then why did we waste
time grovelling around the datasource?

        {
                if (uri[strlen(uri)-1] == '/')
@@ -1107,6 +1190,13 @@
                        isContainerFlag = PR_TRUE;
                }
        }
+        if ((uri) && (!strncmp(uri,kGopherProtocol, 
sizeof(kGopherProtocol)-1))) {
+          char* pos = PL_strchr(uri+sizeof(kGopherProtocol)-1,'/');
+          if (pos) {
+            if (pos==NULL || *(pos+1)=='\0' || *(pos+1)=='1')

XXX Use `nsnull', not `NULL'. Also, spaces between assignments.

+              isContainerFlag = PR_TRUE;
+          }
+        } 
        return(isContainerFlag);
 }
 
@@ -1167,6 +1257,8 @@
 
        *_retval = nsnull;
 
+        // Why is the isWellknownContainerURI test first? Its more expensive 
than the others,
+        // and leads to infinite recursion when that calls GetTarget.

XXX Probably no good reason. If you change it, what happens?

        if (isWellknownContainerURI(aSource) && (aProperty == kNC_Child) && 
(aTruthValue))
        {
                // fake out the generic builder (i.e. return anything in this 
case)
@@ -1218,7 +1310,7 @@
         // by using a global connection list and an immediately-firing timer
 
                if ((doNetworkRequest == PR_TRUE) && (mConnectionList))
-               {
+               {                  
                    PRInt32 connectionIndex = mConnectionList->IndexOf(aSource);
                    if (connectionIndex < 0)
                    {
@@ -1306,28 +1398,30 @@
 
                 nsCOMPtr<nsIRDFResource>    aSource;
                 if (isupports)  aSource = do_QueryInterface(isupports);
-                       const char      *uri = nsnull;
-                if (aSource)    aSource->GetValueConst(&uri);
+                const char     *uri = nsnull;
+                if (aSource) uri = httpIndex->GetDest(aSource);

XXX Again, more commentary to explain why you need extra dereference
step.

+
                 nsresult            rv = NS_OK;
-                       nsCOMPtr<nsIURI>        url;
+                nsCOMPtr<nsIURI>       url;
+                
                 if (uri)
-                {
-                               rv = NS_NewURI(getter_AddRefs(url), uri);
-                       }
-                               nsCOMPtr<nsIChannel>    channel;
-                       if (NS_SUCCEEDED(rv) && (url))
-                       {
-                               rv = NS_OpenURI(getter_AddRefs(channel), url, 
nsnull, nsnull);
-                       }
-                               nsCOMPtr<nsIStreamListener>     listener;
-                       if (NS_SUCCEEDED(rv) && (channel))
-                       {
-                                       rv = 
httpIndex->CreateListener(getter_AddRefs(listener));
-                       }
-                       if (NS_SUCCEEDED(rv) && (listener))
-                       {
-                                       rv = channel->AsyncRead(listener, 
aSource);
-                       }
+                  {
+                    rv = NS_NewURI(getter_AddRefs(url), uri);
+                  }
+                nsCOMPtr<nsIChannel>   channel;
+                if (NS_SUCCEEDED(rv) && (url))
+                  {
+                    rv = NS_OpenURI(getter_AddRefs(channel), url, nsnull, 
nsnull);
+                  }
+                nsCOMPtr<nsIStreamListener>    listener;
+                if (NS_SUCCEEDED(rv) && (channel))
+                  {
+                    rv = httpIndex->CreateListener(getter_AddRefs(listener));
+                  }
+                if (NS_SUCCEEDED(rv) && (listener))
+                  {
+                    rv = channel->AsyncRead(listener, aSource);
+                  }
             }
         }
         httpIndex->mConnectionList->Clear();
Index: xpfe/components/directory/nsDirectoryViewer.h
===================================================================
RCS file: /cvsroot/mozilla/xpfe/components/directory/nsDirectoryViewer.h,v
retrieving revision 1.4
diff -u -r1.4 nsDirectoryViewer.h
--- xpfe/components/directory/nsDirectoryViewer.h       2000/11/02 07:34:46     
1.4
+++ xpfe/components/directory/nsDirectoryViewer.h       2001/02/01 01:02:23
@@ -76,6 +76,7 @@
 
        nsIRDFResource          *kNC_Child;
        nsIRDFResource          *kNC_loading;
+    nsIRDFResource      *kNC_Target;

XXX Urgh. Why the broken whitespace here?

        nsIRDFLiteral           *kTrueLiteral;
        nsIRDFLiteral           *kFalseLiteral;
 
@@ -100,6 +101,7 @@
        nsresult        CommonInit(void);
        nsresult        Init(nsIURI* aBaseURL);
        PRBool          isWellknownContainerURI(nsIRDFResource *r);
+    const char* GetDest(nsIRDFResource *r); // Get the destination of the 
nsIRDFResource

XXX Ibid.

 static void    FireTimer(nsITimer* aTimer, void* aClosure);
 
waterson,
- IMO, "Desc" for "Description" and "Dest" for "Destination" are common  and
obvious abbreviations and should be used for brevity
- Adding a comment, even if nothing of the code changed, is an improvement, not?
I guess, he wondered, what this function does, figured it out and decided to
save the next guy that work. This is admirable, not to critizise.
- C comments are valid in C++ and are very useful for long comments. (You can
still use |#if 0|, if you want to temporarily skip some code.) Doesn't JavaDoc
even require them?
(Assignee)

Comment 78

18 years ago
This isn't going to make 0.8. I won't get to it before the weekend at the
earliest.
Target Milestone: mozilla0.8 → mozilla0.9
(Assignee)

Comment 79

18 years ago
I updated gopher to work with dougt's landing, but that's been backed out now.
I'll wait for it to be recommitted before I attach any more changes. (There's
one minor problem I had with the conversion that I need to ask someone about) I
have midterms this week, but if that hasn't happened by the end I'll attach the
directory viewer patches anyway for review.

I've fixed up most of waterson's problems with the directory viewer. I tried to
remove the string comparisons in the GetDestination routine (replacing it with
an attribute created using the parser, which is told whether its a dir or not,
and so should be better), but that didn't work, because somehow every uri in the
personal toolbar ends up in that at startup (including the NC: one). I also got
a whole lot of xpcom wrapper assertions (over a thousand - my xterm has a 2000
line buffer, and they flowed off the top), every so often, nonrepeatably, which
was followed by a UI lockup. It did make the code a bit nicer, except for the
small fact that it didn't actually work properly.

waterson: - The strange whitespace stuff is where the original code has tabs,
and I added a new line, which uses spaces. Diff pushes the spaced line one char
in, but it doesn't affect the tab. It lines up fine when patched.
- A couple of the comments were additional explanations added, but I did find a
few which described how the code used to work in an earlier revision of the
patch.
- And re my comment to you on IRC about why I couldn't use a <rule foo="...">
... </rule> <rule> ... </rule> pair for the description - I don't think I tried
it, because of the sorting bits, which have to appear outside the <template> bit
to be any use, but depend on an RDF uri rather than an attribute.

At some point in the next couple of days, I'll also file RFE bugs (depending on
this) for stuff which isn't needed to get this commited, but that I either can't
test myself (winhooks integration), and extra features (gopher queries, which
aren't in this patch).
> I updated gopher to work with dougt's landing

Woa! You must be very patient. I would have gone up the wall already. These
patches are waiting since 3 months!

Comment 81

18 years ago
Ok, r=waterson on the dougt-mangled patch you sent me for the directory viewer 
stuff. Thanks for the good work!
(Assignee)

Comment 82

18 years ago
Created attachment 25219 [details]
new files
(Assignee)

Comment 83

18 years ago
Created attachment 25221 [details] [diff] [review]
patch
(Assignee)

Comment 84

18 years ago
The attached patches are the final version, against the current build. I'll mail
dougt my changes which made it work on Saturday so that he can integrate them
into the branch.

I have:

r=darin for netwerk/*
r=dougt for netwerk/streamconv/*
r=mstoltz (via email) for caps/src/nsScriptSecurityManager.cpp
r=waterson for xpfe/components/directory/*

I need r= for prewindows, and the one line change to the contextmenu and urlbar
history. I've mailed blake (his DSL is having problems), and I'll nag people on
IRC after my midterms.

I also need an sr= - who do I get to do that?

Comment 85

18 years ago
i'd treat those as sr='s and get blake/jag/me to do r='s if possible.  I'll 
look at this tonight.
timeless: only waterson among the names on that list gives sr's.

http://www.mozilla.org/hacking/reviewers.html lists areas and super-reviewers,
indexed both by name and by area.

/be
(Assignee)

Comment 87

18 years ago
Brendan: I think timeless was suggesting that we ask waterson to do the sr, and
get someone else to do the r. Or do I need two sr='s? (netwerk + xpfe)

timeless: Thanks. I won't get to it before late Thursday.
bbaetz: timeless's words do not match your interpretation of his suggestion.  My
point about area super-reviewers does indeed suggest that you get mscott or a
delegate he names to look at the necko changes.

/be

Comment 89

18 years ago
At present (Mozilla 0.8) typing gopher://[anything] in Mozilla makes Navigator
4.* open the link. Is this the final and desired result or just a stopgap? 

Comment 90

18 years ago
+        return (false);
=> return false;
+    if (!uri || uri=="" )
=> if (!uri)

+              <!-- gopher port doesn't have an accesskey because the window 
needs a redesign - bbaetz -->
+              <text class="label" value="&port.label;" 
for="networkProxyGopher_Port"/>
-
+              <text class="label" value="&port.label;" 
accesskey="&gopherPort.accesskey;" for="networkProxyGopher_Port"/>

+<!-- No accesskey for gopher (':' doesn't go well) - mpt's going to redesign 
the window
+     <!ENTITY  Gopherport.accesskey          ":">
+-->
--
+<!-- No accesskey for gopher (':' doesn't go well) - mpt's going to redesign 
the window +-->
+     <!ENTITY  gopherPort.accesskey          "">

asside from that, r=timeless for the chrome changes.
mapping "" to accesskey is perfectly harmless, return (true) is bad style, 
js: !uri ~ uri==""
wrt gopherPort, js interCaps is the prefered naming convention.  I know that 
ftp and the other protocols don't follow it, but that's probably worth fixing 
not emulating.

thanks again for the work

piskozub@iopan.gda.pl: if we launch nc4 right now that's great. however this 
patch is very close to landing, and afterwards, we'd default to using this 
implementation of gopher instead of asking the nc4 for help.
(Assignee)

Comment 91

18 years ago
Final patches, with timeless' changes, and a renaming of target->URL to match
the bookmark manager's standard, with r=waterson on those changes.

One other change is for the URL attribute to be an nsIRDFLiteral, because thats
what the bookmarks code expects. waterson's on vacation for a week, so he hasn't
reviewed those changes, but scc gave me permission to blame him for the ugly
string manipulations (which he plans to fix as part of the big string cleanup
stuff)

I'm going to mail for sr's now, cause I want this in :)
(Assignee)

Comment 92

18 years ago
Created attachment 26183 [details]
final zip
(Assignee)

Comment 93

18 years ago
Created attachment 26184 [details] [diff] [review]
final diff
(Assignee)

Comment 94

18 years ago
Created attachment 26188 [details] [diff] [review]
diff -bwu for alecf
(Assignee)

Updated

18 years ago
Blocks: 70158
(Assignee)

Updated

18 years ago
Blocks: 70264
(Assignee)

Updated

18 years ago
Blocks: 70267
(Assignee)

Updated

18 years ago
No longer blocks: 70158

Comment 95

18 years ago
ok, so the xpfe/ stuff gets my sr=alecf
at some point it sure would be nice to break this out into a seperate module -
how much do we really need gopher in the core product?
(Assignee)

Comment 96

18 years ago
mscott: sr=?
(Reporter)

Comment 97

18 years ago
It's quite useful, and it should be fairly small.  If Mozilla includes an IRC
client, surely it can include gopher protocol support, which even Mosaic has :-)
jgoerzen, ChatZilla *is* a separate module and can easily be skipped during
compile time (this is even the default, IIRC). OTOH, the gopher support is
currently implemented in the core modules, like Netlib and the dir viewer. This
was alecf's point. Nobody is suggesting to reject the patch.
(Assignee)

Updated

18 years ago
Keywords: approval
Target Milestone: mozilla0.9 → mozilla0.8.1
(Assignee)

Comment 99

18 years ago
Created attachment 27464 [details]
new zip file
(Assignee)

Comment 100

18 years ago
Created attachment 27465 [details] [diff] [review]
new diffs
(Assignee)

Updated

18 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 18 years ago
Resolution: --- → FIXED
(Assignee)

Comment 101

18 years ago
blake checked this in, sr=rpotts for the necko changes

Comment 102

18 years ago
The bug is the longest I've seen, so with Asa's blessing, I'm going to encourage 
it's termination by creating bugs for some of the post "GOPHER WORKS NOW, 
but..." issues:

bug 72312:
Proxy support - UI, SOCKS, order of server usage issues here.

bug 72310:
Gopher as a module - Embedding or focused developers might want a gopher-less 
mozilla build.



Comment 103

18 years ago
Well, gopher://blah still doesn't work, but gopher seems to :)
Status: RESOLVED → VERIFIED

Comment 104

18 years ago
-relntoteRTM
Keywords: relnoteRTM
You need to log in before you can comment on or make changes to this bug.