49334 - [RFE] Implement gopher

Status: VERIFIED FIXED

(Core :: Networking, enhancement, P3)

Description

[jgoerzen]

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

[R.K.Aa.]

gopher is not implemented in Mozilla.
Changing severity to enhancement.
Modifying summary from "All gopher access fail" to "[RFE] Implement gopher"

Comment 2

[Gagan]

->nobody

Comment 3

[Tomer Brisker]

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.

Comment 4

[Tomer Brisker]

Date: Sat, 26 Aug 2000 02:15:45 -0500
To: [e-mail] From:
John Goerzen <[e-mail]>
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

[PTRourke]

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

[Kathleen :Brade]

cc self, etc.

Comment 7

[Eric Gillespie, Jr.]

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

[[e-mail]]

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.

Comment 9

[Bradley Baetz]

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.

Comment 10

[Bradley Baetz]

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.

Comment 11

[Bradley Baetz]

Attachment: patch to support gopher

Comment 12

[Bradley Baetz]

Attachment: gopher directory - unpack in netwerk/protocol

Comment 13

[Bradley Baetz]

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

[finecom]

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.

Comment 15

[Bradley Baetz]

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.

Comment 16

[Bradley Baetz]

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)

Comment 17

[Bradley Baetz]

Assigning to me this time...

Comment 18

[Jason Eager]

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.

Comment 19

[Bradley Baetz]

Attachment: gopher protocol version 2

Comment 20

[Bradley Baetz]

Attachment: diff to add makefiles/module registration for gopher

Comment 21

[Bradley Baetz]

Attachment: gopher directory converter

Comment 22

[Bradley Baetz]

Attachment: diff to make nsDirectoryViewer work with gopher

Comment 23

[Bradley Baetz]

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? 

Comment 24

[Jason Eager]

Bradley: Did you e-mail those patches to [e-mail] ?

Comment 25

[Bradley Baetz]

[e-mail]: I have now, thanks.

Comment 26

[Bradley Baetz]

Attachment: Updated directory viewer patch

Comment 27

[Bradley Baetz]

Attachment: gopher protocol - supporting proxies

Comment 28

[Bradley Baetz]

Attachment: diff for gopher proxy support

Comment 29

[Bradley Baetz]

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

Comment 30

[Bradley Baetz]

For some reason my mailer didn't send the message to [e-mail] 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

[Scott MacGregor]

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 =).

Comment 32

[Bradley Baetz]

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

[R.K.Aa.]

*** Bug 61704 has been marked as a duplicate of this bug. ***

Comment 34

[Bradley Baetz]

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.

Comment 35

[Bradley Baetz]

Attachment: patch - latest version

Comment 36

[Bradley Baetz]

Attachment: new files for gopher - latest version

Comment 37

[Bradley Baetz]

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?

Comment 38

[Bradley Baetz]

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.

Comment 39

[Bradley Baetz]

Attachment: Updated patch without conflicts - replaces 20515

Comment 40

[Bradley Baetz]

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.

Comment 41

[Bradley Baetz]

Attachment: small string change - replaces 20516

Comment 42

[Bradley Baetz]

Attachment: small string change - replaces 20516

Comment 43

[Bradley Baetz]

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

Please review patch 20593 and 20839

Comment 44

[David Krause]

Bradley, make sure you mail [e-mail] to get someone to review this.
 We wouldn't want this to slip through the cracks as some patches have.

Comment 45

[Ben Bucksch (:BenB)]

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

[Scott MacGregor]

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

[Scott MacGregor]

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)

Comment 48

[Bradley Baetz]

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

Comment 49

[Bradley Baetz]

Attachment: zip version of patch 20839

Comment 50

[Bradley Baetz]

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.

Comment 51

[Bradley Baetz]

Attachment: zip version without binary files. I'm really not having a good day...

Comment 52

[Alec Flett]

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.

Comment 53

[Bradley Baetz]

~/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

[Alec Flett]

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.

Comment 55

[Gervase Markham [:gerv]]

Bradley - are you back from holiday yet? :-) What's the score with gopher - 
still waiting for reviewers?

Gerv

Comment 56

[Bradley Baetz]

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.

Comment 57

[Brendan Eich [:brendan]]

Cc'ing darin for r= and moral support, if possible.

/be

Comment 58

[Darin Fisher]

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.

Comment 59

[Doug Turner (:dougt)]

I will review the directory viewer and stream convert stuff tomorrow.

Comment 60

[Doug Turner (:dougt)]

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.

Comment 61

[Bradley Baetz]

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

[Darin Fisher]

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

Comment 63

[Darin Fisher]

Attachment: Revision to nsGopherChannel.cpp per changes in bug 62566

Comment 64

[Jason Eager]

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

Comment 65

[Bradley Baetz]

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.

Comment 66

[Bradley Baetz]

Attachment: zip of new files

Comment 67

[Bradley Baetz]

Attachment: diffs

Comment 68

[Chris Waterson]

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

Comment 69

[Bradley Baetz]

> - 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.
[e-mail] 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.

Comment 70

[Ben Bucksch (:BenB)]

> they're uris - they have to be ascii

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

Comment 71

[Bradley Baetz]

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

Comment 72

[Bradley Baetz]

Attachment: fix for crash: nsGopherChannel.cpp:97

Comment 73

[Bradley Baetz]

Attachment: left the streamconverters out - this is the zip

Comment 74

[Bradley Baetz]

Attachment: ...and the diff

Comment 75

[Darin Fisher]

r=darin for the crash fix

Comment 76

[Chris Waterson]

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 - 
[e-mail]

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 - [e-mail]
 
 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);
 

Comment 77

[Ben Bucksch (:BenB)]

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?

Comment 78

[Bradley Baetz]

This isn't going to make 0.8. I won't get to it before the weekend at the
earliest.

Comment 79

[Bradley Baetz]

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

Comment 80

[Ben Bucksch (:BenB)]

> 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

[Chris Waterson]

Ok, r=waterson on the dougt-mangled patch you sent me for the directory viewer 
stuff. Thanks for the good work!

Comment 82

[Bradley Baetz]

Attachment: new files

Comment 83

[Bradley Baetz]

Attachment: patch

Comment 84

[Bradley Baetz]

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

[timeless]

i'd treat those as sr='s and get blake/jag/me to do r='s if possible.  I'll 
look at this tonight.

Comment 86

[Brendan Eich [:brendan]]

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

Comment 87

[Bradley Baetz]

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.

Comment 88

[Brendan Eich [:brendan]]

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

[Jacek Piskozub]

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

[timeless]

+        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

[e-mail]: 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.

Comment 91

[Bradley Baetz]

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

Comment 92

[Bradley Baetz]

Attachment: final zip

Comment 93

[Bradley Baetz]

Attachment: final diff

Comment 94

[Bradley Baetz]

Attachment: diff -bwu for alecf

Comment 95

[Alec Flett]

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?

Comment 96

[Bradley Baetz]

mscott: sr=?

Comment 97

[jgoerzen]

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

Comment 98

[Ben Bucksch (:BenB)]

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.

Comment 99

[Bradley Baetz]

Attachment: new zip file

Comment 100

[Bradley Baetz]

Attachment: new diffs

Comment 101

[Bradley Baetz]

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

Comment 102

[[e-mail]]

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

[benc]

Well, gopher://blah still doesn't work, but gopher seems to :)

Comment 104

[benc]

-relntoteRTM