Gopher 'information' tag not supported.

RESOLVED FIXED in Future

Status

()

Core
Networking
P4
enhancement
RESOLVED FIXED
16 years ago
6 years ago

People

(Reporter: Neil Fraser, Assigned: Jan Ruzicka)

Tracking

({helpwanted})

Trunk
Future
helpwanted
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.7 -
blocking-aviary1.5 -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: needed-aviary1.0, URL)

Attachments

(4 attachments, 14 obsolete attachments)

1.87 KB, application/octet-stream
Details
3.67 KB, text/html
Details
125.60 KB, image/jpeg
Details
7.79 KB, patch
Biesinger
: review+
Darin Fisher
: superreview+
chris hofmann
: approval1.7-
Details | Diff | Splinter Review
(Reporter)

Description

16 years ago
All other browsers I've tested (Netscape 2/3/4, IE 4/5, Mosaic 3, Lynx) supports
Gopher's 'i' type.  In the above URL these other browsers list extensive
comments, whereas Mozilla (2002010303) ignores them and merely prints the
directory links.

Ideally Mozilla should do what previous versions of Netscape do: print these
lines without linking them to anything.  Comment #37 in Bug 49334 says,
>  - don't display information items - I can't stop them
>  being selected and sorted out of order.
However this doesn't seem to be an issue any more since bug 83881 appears to
have fixed the ordering problem.
->bbaetz (gopher)
Assignee: neeti → bbaetz

Comment 2

16 years ago
Platform and OS should be cahnged to all. Reproductible with macos X build ID
2002010208.
btw reporter, thanks for pointing out a working gopher server !
changing platform => all, and os => all (based on Ludovic's comment)
OS: Windows 98 → All
Hardware: PC → All
Yeah, this is true.

->moz1.1, P4, +helpwanted.

You'd need to change the backend code, and it would probably be easier to ignore
this for XUL, given the sorting which occurs.
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Keywords: helpwanted
Priority: -- → P4
Target Milestone: --- → mozilla1.1

Comment 5

15 years ago
Does this depend on bug 83881? Info lines aren't much use if the client's going 
to rearrange the menu.
-> future
Target Milestone: mozilla1.1alpha → Future

Updated

15 years ago
QA Contact: benc → gopherqa

Comment 7

15 years ago
Is anyone working on this? I'd like to do some work on improving Mozilla's gopher ability (especially since IE has now ditched gopher ability).

Comment 8

14 years ago
Please fix this bug. with this bug existing, mozillas's gopher abilities are
completely useless. even lynx can show gopher sites properly. (sigh, everything
worked so well with netscape navigator 4.x)
we have been waiting for the fix for such a long time. and with gopher support
ditched by microsoft, this could be a real feature that may convince many to
switch over to mozilla/firebird.

Comment 9

14 years ago
Created attachment 132639 [details]
A png screenshot that shows how gopher sites should be displayed correctly

Comment 10

14 years ago
Created attachment 132640 [details]
A png screenshot that shows the gecko rendering bug

Note that the whole information on this gopher site is missing ("Welcome to
the...")! Just some links are displayed.

Comment 11

14 years ago
I found some information on the current state of mozilla's gopher rendering
abillities here in gopherspace: gopher://gopher.floodgap.com/0/gopher/wbgopher

[Quote]Mozilla's current gopher support has improved as of 1.4RC1. It no longer
resorts selectors as earlier versions have done, which allows search engines
and so on to present salient link order. However, it still does not support
i itemtype at all, and you will see NO informational text from servers you
access, which is a bummer (Bugzilla bug 118438; there has been no activity
on this for some time). Nevertheless, you can use it to get and download
files, at least, and it works stably. (Why Netscape 4's gopher engine was
not used in Mozilla is still a mystery to me.)[/Quote]

They also mentioned the gopher abillities of other browsers. The part on
Netscape 4 could be of some interest:

[Quote] Netscape 4 is not spectacular but certainly functional as a gopher
client. It
doesn't know how to understand Gopher+ but it's tolerant, and also, like
Lynx, supports the gamut of Gopher features. It properly (or at least
sensibly) formats gopher menus, accepts i itemtype, and doesn't try to
mess with selectors. It is also very quick, simple and painless to use.
While it isn't as seamless as Lynx, it gets the job done. Even though it is
aging, it is a decent choice as a gopher browser, and probably the best
overall solution for most users.

This is not true of later versions; versions *after* 4.8 (not inclusive),
until 6.2.3 (not inclusive) do not support Gopher AT ALL. It has been reported
to me that Gopher support is back in as of version 6.2.3, but being based on
Mozilla (see below), stick with 4.x. [/Quote]
(Assignee)

Comment 12

13 years ago
Created attachment 146070 [details]
gopher-test data (raw dump of 'gopher://gopher.floodgap.com/')

This attachment is men as test source for testing gopher randering.
It contains data as they were sent from gopher.floodgap.com port 70.
(Assignee)

Comment 13

13 years ago
Created attachment 146071 [details]
XHTML - of expected result

This is my view how the page in question (gopher://gopher.floodgap.com/) should
be randered.

see the information 'i' tags as text and the 'h' tag as the hypertext.

Also the redundant <td></td> are dropped.
(Assignee)

Updated

13 years ago
Blocks: 194220

Comment 14

13 years ago
@Bradley Baetz:

Can you comment on this bug? Can there be any improvement without anyone helping
you? What kind of help are you looking for?

The best person I can think of (knowing gopher very well) would be Cameron
Kaiser himself ( ckaiser@floodgap.com ), who wrote the gopher gateway that was
mentioned before in this thread.

Shall I send him a mail, pointing him to this thread?

It would be great if this bug could be fixed before the release of firefox 1.0.
Then firefox would be an excellent html AND gopher browser.
(Assignee)

Comment 15

13 years ago
Is this a correct place to make the fix?

http://lxr.mozilla.org/seamonkey/source/netwerk/streamconv/converters/nsGopherDirListingConv.cpp


this looks like it is dealing with the compisition of the page and the links.
(Assignee)

Comment 16

13 years ago
or is it more likely the following?

http://lxr.mozilla.org/seamonkey/source/netwerk/streamconv/converters/nsIndexedToHTML.cpp
I'm not involvd in doing gopher stuff anymore.

The fix for this would need to change nsIndexedToHTML to read in a descriptive
text from gopher, and then display it. The extra API change should also be
useful enough to display the FTP welcome message.

You'd also have to change nsGopherDirListingConv.cpp to send the extra textual data.
Assignee: bbaetz → nobody
Status: ASSIGNED → NEW
(Assignee)

Comment 18

13 years ago
It looks like nsIDirIndex.idl will have to have one more type added.

http://lxr.mozilla.org/seamonkey/source/netwerk/streamconv/public/nsIDirIndex.idl

Is it safe to change this file?

How can I unit test the Index group of protocols?
Is there any unit test harness?

Comment 19

13 years ago
Created attachment 146160 [details]
Another XHTML sheet to show how the site should be rendered

Comment 20

13 years ago
Regarding the xhtml sheet of comment #13:

Shall the bold "Index of gopher://gopher.floodgap.com/" line be rendered, too?

This address can already be seen in the browser address bar, therefore wasting
space for rendering it doesn'make sense. The same applies to the ftp protocol
where this line is displayed without any need.
You should be able to safely add additional types without problems, since its
just an enum declaration.
(Assignee)

Comment 22

13 years ago
(In reply to comment #20)
> Regarding the xhtml sheet of comment #13:
> 
> Shall the bold "Index of gopher://gopher.floodgap.com/" line be rendered, too?
> 
> This address can already be seen in the browser address bar, therefore wasting
> space for rendering it doesn'make sense. The same applies to the ftp protocol
> where this line is displayed without any need.

On the other hand what about too long addres when you are printing the "page"?

Is there no case for leaving it there?
What would be a reson to leave it there?
(Assignee)

Comment 23

13 years ago
(In reply to comment #19)
> Created an attachment (id=146160)
> Another XHTML sheet to show how the site should be rendered
> 

On this rendering I miss indication of link type.

I like to know if I'am going to see some contents or will be presented with
another selection.
(Assignee)

Comment 24

13 years ago
Created attachment 146390 [details] [diff] [review]
patch adding the information tag

This patch adds info as shown on:

http://bugzilla.mozilla.org/attachment.cgi?id=146071

pluses:
+ adds a type "text" to the nsIDirIndex
+ adds parsing of the "text" type
+ adds emition if the "text"

minuses:
- the patch does not remove the excesive 'td' pairs.


note: 
Somehow the patch bloated differences in nsIndexedToHTML.cpp
Changes were only:
- putting the anchor creation to 'if' blocks.

----------------------------------------------
Index: nsIndexedToHTML.cpp
===================================================================
RCS file: /cvsroot/mozilla/netwerk/streamconv/converters/nsIndexedToHTML.cpp,v
retrieving revision 1.55
diff -b -w -r1.55 nsIndexedToHTML.cpp
477c477
<     pushBuffer.Append(NS_LITERAL_STRING("<tr>\n <td><a"));
---
>     pushBuffer.Append(NS_LITERAL_STRING("<tr>\n <td>"));
480a481,482
>     if (type != nsIDirIndex::TYPE_TEXT){
>	  pushBuffer.Append(NS_LITERAL_STRING("<a"));
539a542
>     }
547c550,553
<     pushBuffer.Append(NS_LITERAL_STRING("</a></td>\n <td>"));
---
>     if (type != nsIDirIndex::TYPE_TEXT){
>	  pushBuffer.Append(NS_LITERAL_STRING("</a>"));
>     }
>     pushBuffer.Append(NS_LITERAL_STRING("</td>\n <td>"));
Jan, you'll want to request review on that patch, if it works to your
satisfaction... I don't know whether Bradley has time for reviews; if he does
not, darin@meer.net is the networking module owner and may suggest reasonable
reviewers.
Comment on attachment 146390 [details] [diff] [review]
patch adding the information tag

Will these text links still get passed through the text to HTML converter, so
that they get linkified?

I won't have time to do a poper review for a few days. If thats OK, request one
from me and I'll get to it then
(Assignee)

Comment 27

13 years ago
how do I request review?
r=?
s-r=?

is this correct?
r=bbaetz@acm.org

Comment 28

13 years ago
Go to the patch, click on "Edit", and type in the e-mail address of the requestee.
(Assignee)

Updated

13 years ago
Attachment #146390 - Flags: review?(bbaetz)

Comment 29

13 years ago
Maybe ask Boris Zbarsky (bzbarsky@mit.edu) for the superreview. I saw that he
did some superreviews on other gopher related stuff.

Updated

13 years ago
Attachment #146390 - Flags: superreview?(darin)

Comment 30

13 years ago
Comment on attachment 146390 [details] [diff] [review]
patch adding the information tag

looks good to me sr=darin
Attachment #146390 - Flags: superreview?(darin) → superreview+
(Assignee)

Comment 31

13 years ago
does the review have to be from a domain expert?

Comment 32

13 years ago
@Jan:
1) Thanks a lot for the effort writing this patch.

2) A review is usually done by people comfortable with the materia. The protocol
people review protocol patches and so on...

3.) One question: Could you set the approval1.7 flag to "?". It would be great
if this patch could make it into the 1.7 branch. Mozilla 1.7, Firebird 0.9,
Firebird 1.0 and probably Netscape 7.2 will be based off this branch. As you can
see in comment #11 this bug is the criteria for the gopher people to determine
if the browser is usable or not. If this patch could make it into the branch, we
would have 3 new gopher browsers (beside Netscape 4 and Lynx). Otherwise we
would have  to wait for Mozilla 1.8, or Fx 1.x. Now let's wait for the review.

Updated

13 years ago
Attachment #146390 - Flags: approval1.7?
(Assignee)

Updated

13 years ago
Attachment #146390 - Flags: review?(bbaetz) → review?
(Assignee)

Comment 33

13 years ago
Does anybody interested in this bug have a time for review?
I don't want to impose new work on anybody already overloaded with other issues.

Volunteers one step forward!

(not all of you ;-) ) 

Comment 34

13 years ago
Created attachment 146957 [details]
Manually Patched Firefox - See the glitch...

I applied Jan's patch manually to latest firefox source code. This is the
result when viewing gopher sites with the information tag. The size comment on
the left side should not been shown.

Maybe someone can hunt the bug down. But the fault can be on my side as I had
to apply the patch manually and copy and paste errors are possible.
(Assignee)

Comment 35

13 years ago
This glitch is possible because I did not put condition on top of the
presentation of the rest of fields within the directory entry.
Give me a second.
(Assignee)

Comment 36

13 years ago
Created attachment 146963 [details] [diff] [review]
patch - adding the info tag (+ignoring size and date)

This patch patch should ignore size and modification date attributes of the
gopher info tags.
I am just compiling the source, but I have not finished it yet.
I am going to sleep. If somebody has an urge to try it, you are welcome.

Comment 37

13 years ago
Thanks for the update.

You will have to set the r/sr/approval1.7 flags for the updated patch again.
(Maybe ask bzbarsky for the review instead of bbaetz if he is too busy).

I am checking out CVS at the moment and will do a custom build again later
today. I will  only report back if new problems arise.

Comment 38

13 years ago
Created attachment 146981 [details]
New manually patched Firefox - a bit of the glitch remained

Built Firefox again. Only links to files expose show the size value.

Once again I copied and pasted the source files manually. Maybe I missed a
line.

Updated

13 years ago
Attachment #146957 - Attachment is obsolete: true

Updated

13 years ago
Attachment #146981 - Attachment mime type: text/plain → text/html
(Assignee)

Comment 39

13 years ago
(In reply to comment #38)
> Created an attachment (id=146981)
> New manually patched Firefox - a bit of the glitch remained
> 
> Built Firefox again. Only links to files expose show the size value.
> 
> Once again I copied and pasted the source files manually. Maybe I missed a
> line.


The problem seems to be introduced outside the original patch.
It was introduced recently. 
The dir entries are returned with a size parameter set to some value other then
(-1 ~ no Data available). 
(Assignee)

Comment 40

13 years ago
Created attachment 147002 [details] [diff] [review]
corrected - patch - adding the info tag (+ignoring size and date)

sorry for the previous one, it included some changes to make files done by the
config for OSX. The changes are still the same ones as for the patch 146963.
Attachment #146963 - Attachment is obsolete: true
(Assignee)

Updated

13 years ago
Depends on: 240257
(Assignee)

Comment 41

13 years ago
how can be a RESOLVED FIXED bug reopened?
the 240257 broke the directory index entry interpretation and needs to be fixed 
What can I do? 
(Assignee)

Comment 42

13 years ago
(In reply to comment #39)
> (In reply to comment #38)
> > Created an attachment (id=146981)
> > New manually patched Firefox - a bit of the glitch remained
> > 
> > Built Firefox again. Only links to files expose show the size value.
> > 
> > Once again I copied and pasted the source files manually. Maybe I missed a
> > line.
> 
> 
> The problem seems to be introduced outside the original patch.
> It was introduced recently. 
> The dir entries are returned with a size parameter set to some value other then
> (-1 ~ no Data available). 
> 

see http://bugzilla.mozilla.org/show_bug.cgi?id=240257 . 

Comment 43

13 years ago
Yes, timeless' updated fix for bug 240257 solved the problem (my third Firefox
today ;) ). Ask for r/sr/a again.
(Assignee)

Comment 44

13 years ago
Comment on attachment 147002 [details] [diff] [review]
corrected - patch - adding the info tag (+ignoring size and date)

Reverting back to the original patch because
this one is clutering code.

It would prevent legitimate use of the last modified entry if there ever was
one.

Example can be made with a text with annotations.
Attachment #147002 - Attachment is obsolete: true

Comment 45

13 years ago
Created attachment 147095 [details]
Strange Links above and below the information tag.

@Jan:

Maybe you will hate me. But can you investigate this? I found a gopher server
where strange links appear below and above the information tag. I attached one
page that shows that.

I compiled your second patch (the "corrected" and now "obsoleted" one) to test
this out.
Attachment #146981 - Attachment is obsolete: true
(Assignee)

Comment 46

13 years ago
(In reply to comment #45)
> Created an attachment (id=147095)
> Strange Links above and below the information tag.
> 
Ok the issue here seems to be an interpretation of an empty information line.
Not only that it generates strange link to wrong URL it also eats the following
line.

If you go to that site you can see information line after the first empty line
and a 0 link after the second one.

problem is likely to be in:
http://lxr.mozilla.org/seamonkey/source/netwerk/streamconv/converters/nsGopherDirListingConv.cpp

(Assignee)

Comment 47

13 years ago
Created attachment 147100 [details] [diff] [review]
patch - adding info tag even the empty one

This patch fixes the empty description of the info tag as pointed out earlier.
This patch should be use together with patch for bug 240257 which fixes
initialization of a nsDirIndex.
(http://bugzilla.mozilla.org/attachment.cgi?id=147008&action=view)

Added fix substitutes space for an empty description.
Attachment #146390 - Attachment is obsolete: true
(Assignee)

Updated

13 years ago
Attachment #147100 - Flags: superreview?
Attachment #147100 - Flags: review?
Attachment #147100 - Flags: approval1.7?

Comment 48

13 years ago
Created attachment 147146 [details]
All glitches fixed

Thanks Jan. Built again and now everything works now as it should. Here is a
nice screenshot of Mozillas new rendering  capability.

Note that the folder/files artwork was exchanged manually as suggested in Bug #
	 240463.
Attachment #132639 - Attachment is obsolete: true
Attachment #132640 - Attachment is obsolete: true
Attachment #146160 - Attachment is obsolete: true
Attachment #147095 - Attachment is obsolete: true

Updated

13 years ago
Attachment #146390 - Flags: approval1.7?

Comment 49

13 years ago
Comment on attachment 147100 [details] [diff] [review]
patch - adding info tag even the empty one

please don't request approval until you've actually got the reviews. thanks.
Attachment #147100 - Flags: approval1.7?
(Assignee)

Updated

13 years ago
Attachment #147100 - Flags: superreview?(darin)
Attachment #147100 - Flags: superreview?
Attachment #147100 - Flags: review?(darin)
Attachment #147100 - Flags: review?

Comment 50

13 years ago
@Asa:
A big sorry from me (since I suggested it).

If someone is interested in my Firefox gopher test build. GlennRP was so kind to
host it on mngzilla (because it is bundled with a mng plugin, too).
Get it here: http://prdownloads.sourceforge.net/mngzilla/Fx20040427MNG.exe?download 

Comment 51

13 years ago
I'm sorry, but it will be a few more days before I have time to properly review
this patch.  Gopher is just not high priority.
(Assignee)

Updated

13 years ago
Attachment #147100 - Flags: superreview?(darin)
Attachment #147100 - Flags: superreview?
Attachment #147100 - Flags: review?(darin)
Attachment #147100 - Flags: review?
(Assignee)

Comment 52

13 years ago
Is there anybody available for review/superreview ?
Comment on attachment 147100 [details] [diff] [review]
patch - adding info tag even the empty one

I'll try to look at this soon...

note to self, http://www.faqs.org/rfcs/rfc1436.html
Attachment #147100 - Flags: review? → review?(cbiesinger)
Comment on attachment 146390 [details] [diff] [review]
patch adding the information tag

(clearing review reqest on obsolete patch)
Attachment #146390 - Flags: review?
Comment on attachment 147100 [details] [diff] [review]
patch - adding info tag even the empty one

+		 char* descStr = PL_strndup(" ",1);
+		 char* escName = nsEscape(descStr,url_Path);
+		 desc = escName;
+		 nsMemory::Free(escName);
+		 nsMemory::Free(descStr);

you are escaping a constant string? what for?

hm... in fact, what is this entire if-branch used for? is it a problem to call
PL_strndup with zero length? like, what was wrong with the previous block here?


the comment here:
	if (tabPos) {
	     /* Don't display error messages or informative messages

seems to be lying now (informative messages are displayed)


ah, dir indexes. a twisty maste of classes, all alike.

a diff -w would've been nice...


+		 case 'i':
+		     aString.Append("TEXT");

I'm also not yet sure this is the best idea...
consider
http://web.archive.org/web/20010603010339/http://www.area.com/~roeber/file_form
at.html
preformatted text should use 101, not 201

Following that, I think I'd prefer an additional method on nsIDirIndexListener
http://lxr.mozilla.org/seamonkey/source/netwerk/streamconv/public/nsIDirIndexLi
stener.idl like onInformationAvailable, which passes the text to the listener.
nsIDirIndex seems very file-specific.
(Assignee)

Comment 56

13 years ago
(In reply to comment #55)
> (From update of attachment 147100 [details] [diff] [review])
> +		 char* descStr = PL_strndup(" ",1);
> +		 char* escName = nsEscape(descStr,url_Path);
> +		 desc = escName;
> +		 nsMemory::Free(escName);
> +		 nsMemory::Free(descStr);
> 
> you are escaping a constant string? what for?

the escaping is to ensure the space is passed around and gets safely to the
nsIndexedToHTML::OnIndexAvailable().


> 
> hm... in fact, what is this entire if-branch used for? is it a problem to call
> PL_strndup with zero length? like, what was wrong with the previous block here?
the proble is that a space is used as separator of fields within the file type
index entry as it is passes around.

> 
> 
> the comment here:
> 	if (tabPos) {
> 	     /* Don't display error messages or informative messages
> 
> seems to be lying now (informative messages are displayed)
> 
> 
> ah, dir indexes. a twisty maste of classes, all alike.
> 
> a diff -w would've been nice...

yes, I agree.
I attached in one of the comments. 
The problem is how to include the indentation changes.
If there is a way how to extract patch from cvs that would do that, I would like
to resubmit the patch.

> 
> 
> +		 case 'i':
> +		     aString.Append("TEXT");
> 
> I'm also not yet sure this is the best idea...
> consider
> http://web.archive.org/web/20010603010339/http://www.area.com/~roeber/file_form
> at.html
> preformatted text should use 101, not 201
> 
> Following that, I think I'd prefer an additional method on nsIDirIndexListener
> http://lxr.mozilla.org/seamonkey/source/netwerk/streamconv/public/nsIDirIndexLi
> stener.idl 

> like onInformationAvailable, which passes the text to the listener.
> nsIDirIndex seems very file-specific.
> 

I done this because I did not feel comfortable with nsDirIndexParser.cpp.
It appends the "101:" messages to the mComment in the
nsDirIndexParser::ProcessData function.
File like entries are treated differently (lines 453 - 457).

Your idea of additional function in nsIDirIndexListener is a lot cleaner
solution. As I was trying to be lazy and change minimum of code and have it
working soon.

Thanks a lot for looking at my code.
I will try to look into your solution.

(In reply to comment #56)
> I done this because I did not feel comfortable with nsDirIndexParser.cpp.
> It appends the "101:" messages to the mComment in the
> nsDirIndexParser::ProcessData function.
> File like entries are treated differently (lines 453 - 457).

Oh - I wasn't aware that the dirindexparser already supported 101 lines...

Well, I dislike how it implements them anyway :)

> Thanks a lot for looking at my code.
> I will try to look into your solution.

Thank you!
(Assignee)

Comment 58

13 years ago
(In reply to comment #57)
> 
> Oh - I wasn't aware that the dirindexparser already supported 101 lines...
> 
> Well, I dislike how it implements them anyway :)
> 
They are not handeled properly, mixed presentation breaks everything. ;-)

> > Thanks a lot for looking at my code.
> > I will try to look into your solution.
> 
> Thank you!
> 

For interim period could be this solution aceptable?
It can be later cleaned up by moving info only parts into onInformationAvailable.

I don't have much free time to go on big coding adventure.

Yesterday while poking around I was trying to add the onInformationAvailable
I was unable to find an example of proper escaping of normal sting.
And I got confused what is "the correct" string.
Is it char*, PRUnichar* or some of the typedefs?

Code in question is:

    nsXPIDLString tmp;
    aIndex->GetDescription(getter_Copies(tmp));
    PRUnichar* escaped = nsEscapeHTML2(tmp.get(), tmp.Length());
    pushBuffer.Append(escaped);
    nsMemory::Free(escaped);


the onInformationAvailable would be passed an information as 'char *'.
How can be 'char *' converted to 'nsXPIDLString' ?
Or am I missing something?
where should I look?
(In reply to comment #58)
> For interim period could be this solution aceptable?

experience shows that interim solutions will stay unmodified for a very long time...

> Yesterday while poking around I was trying to add the onInformationAvailable
> I was unable to find an example of proper escaping of normal sting.
> And I got confused what is "the correct" string.
> Is it char*, PRUnichar* or some of the typedefs?

Hmm. char* I guess, in this case... but then you need to specify the encoding
somewhere. Or PRUnichar*, but then you need to convert to UTF-16 yourself
(PRUnichar strings are UTF-16 encoded)

> Code in question is:
> 
>     nsXPIDLString tmp;

nsXPIDLString is really just a wrapper around PRUnichar*, that automatically
frees memory. it is used when an interface wants a PRUnichar** (i.e. an out param)

> How can be 'char *' converted to 'nsXPIDLString' ?

Depends on the charset :) (and you'd want nsString or nsAutoSTring or
something... also see http://www.mozilla.org/projects/xpcom/string-guide.html)
(Assignee)

Comment 60

13 years ago
Created attachment 148636 [details] [diff] [review]
patch with OnInformationAvailable

This patch is first stab at implementation of OnInformationAvailable idea as it
was mentioned earlier.

For reviewers:
I am not sure if I am using the strings properly (correct types and
allocation/freeing).

Changes made:

nsDirIndexParser.cpp - I was not sure if the removal of appneding to mComment
would break anything, therfor I left it there.

nsGopherDirListingConv.cpp - escaping of empty info was left in place.
info was converted to type '101:' - "Pre formatted human readable information
line" (instead of the previous special file format TEXT - patch 147100 or
ignoring completely - current CVS head)

nsIndexedToHTML.cpp - left separation of anchor tag (personal coding
preference);
implemented OnInformationAvailable it has empty TDs to match the file entries.
Which in turn have them as to e compatible with FTP.

nsIDirIndexListener.idl - This is the interface change.

xpfe/components/directory/nsDirectoryViewer.cpp - This had to bechanged as it
is  implementing nsIDirIndexListener interface. Implementation is done by
returning NS_ERROR_NOT_IMPLEMENTED.

Sorry for this long pause.
(Assignee)

Updated

13 years ago
Attachment #147100 - Attachment is obsolete: true
(Assignee)

Comment 61

13 years ago
Comment on attachment 148636 [details] [diff] [review]
patch with OnInformationAvailable

please review this patch.

Thank you in advance.
Attachment #148636 - Flags: superreview?
Attachment #148636 - Flags: review?(cbiesinger)
Comment on attachment 148636 [details] [diff] [review]
patch with OnInformationAvailable

nsDirIndexParser.cpp:
+	     mListener->OnInformationAvailable(aRequest,
aCtxt,NS_ConvertUTF8toUCS2(value));

add a space after aCtxt,
I'm also not happy with the unescaping (see below)

I'd really much prefer it if you removed mComment, and the comment attribute of
nsIDirIndexParser

nsGopherDirListingConv.cpp:
+		 char* descStr = PL_strndup(" ",1);

see, my poing was: if the string is constant, you know what the escaped string
will look like. no need to waste cpu cycles and codesize to do it at runtime.

But actually... why are you escaping at all? ah... this is used for both
"links" and "information"... I don't think 101 lines should be escaped though.
it doesn't seem needed.


+		 aString.Append(char(nsCRT::LF));
+			}
wrong indentation at second line

nsIndexedTOHtml.cpp:
+//    TODO  conversion

hm... doesn't look like any conversion is needed here...

+    PRUnichar* escaped = nsEscapeHTML2(PromiseFlatString(aInfo).get());
you should nullcheck escaped here, and return if it is.

nsIDirIndexListener:
+   /**
+     * Called for each directory entry

isn't the style here:
/**
 * Called for ...
?

+			   in nsISupports aCtxt,
wrong indentatiob
Attachment #148636 - Flags: superreview?
Attachment #148636 - Flags: review?(cbiesinger)
Attachment #148636 - Flags: review-
Attachment #147100 - Flags: superreview?
Attachment #147100 - Flags: review?(cbiesinger)
(Assignee)

Comment 63

13 years ago
Created attachment 149129 [details] [diff] [review]
OnInformationAvailable patch cleaned

(In reply to comment #62)
> (From update of attachment 148636 [details] [diff] [review])

Thanks for all the comments.

> I'd really much prefer it if you removed mComment, and the comment attribute
of
> nsIDirIndexParser

Somewhere I have seen a remark that comment would be presented in case of
error.
I can't find the remark now.

> 
> nsGopherDirListingConv.cpp:
> +		 char* descStr = PL_strndup(" ",1);
> 
Sorry for this brain-****.

> see, my poing was: if the string is constant, you know what the escaped
string
> will look like. no need to waste cpu cycles and codesize to do it at runtime.


I understand this reason and agree from the performance standpoint.
My point is maintainabilty, in case the escaping ever changes.

> 
> But actually... why are you escaping at all? ah... this is used for both
> "links" and "information"... I don't think 101 lines should be escaped
though.
> it doesn't seem needed.

Let's do the information/description processing in a same consistant way.
Case can be made for any processing that can operate on the output.

> 
> +    PRUnichar* escaped = nsEscapeHTML2(PromiseFlatString(aInfo).get());
> you should nullcheck escaped here, and return if it is.

What would cause the escaped to be a null?
PromiseFlatString().get() ?

There is no nullcheck in the OnIndexAvailable().
(Assignee)

Updated

13 years ago
Attachment #148636 - Attachment is obsolete: true
(In reply to comment #63)
> > +    PRUnichar* escaped = nsEscapeHTML2(PromiseFlatString(aInfo).get());
> > you should nullcheck escaped here, and return if it is.
> 
> What would cause the escaped to be a null?

out of memory situation.
(Assignee)

Comment 65

13 years ago
Created attachment 149173 [details] [diff] [review]
OnInformationAvailable patch cleaned - 2nd round

the null check on escaped is added.
(There should be something similar in the OnIndexAvailable().)

Thanks for critical and constructive input.
Attachment #149129 - Attachment is obsolete: true
(Assignee)

Updated

13 years ago
Attachment #149173 - Flags: superreview?
Attachment #149173 - Flags: review?
Comment on attachment 149173 [details] [diff] [review]
OnInformationAvailable patch cleaned - 2nd round

> I'd really much prefer it if you removed mComment, and the comment attribute of
> nsIDirIndexParser

looks like this patch doesn't do that.

+		 char* escName = nsEscape(" ",url_Path);
+		 desc = escName;
+		 nsMemory::Free(escName);

why not just:
desc = "%20";
Attachment #149173 - Flags: superreview?
Attachment #149173 - Flags: review?
Attachment #149173 - Flags: review-
(Assignee)

Comment 67

13 years ago
(In reply to comment #66)
> (From update of attachment 149173 [details] [diff] [review])
> > I'd really much prefer it if you removed mComment, and the comment attribute of
> > nsIDirIndexParser
Ok great idea but does this justify change in 

 xpfe/components/directory/nsDirectoryViewer.cpp

  nsHTTPIndex::OnStopRequest ?

What should that be ?

> 
> looks like this patch doesn't do that.
> 
> +		 char* escName = nsEscape(" ",url_Path);
> +		 desc = escName;
> +		 nsMemory::Free(escName);
> 
> why not just:
> desc = "%20";
> 
mantainability (more boring details about this in private mail) 
(Assignee)

Comment 68

13 years ago
Created attachment 149466 [details] [diff] [review]
OnInformationAvailable patch cleaned - 3nd round

The "%20" made it.
Attachment #149173 - Attachment is obsolete: true
(Assignee)

Updated

13 years ago
Attachment #149466 - Flags: review?

Updated

13 years ago
Attachment #149466 - Flags: review? → review?(cbiesinger)
Comment on attachment 149466 [details] [diff] [review]
OnInformationAvailable patch cleaned - 3nd round

+	     }else if(type == 'i'){

missing space before |else|, but no need to attach a new patch just for this.

I guess leaving the comment attribute there is not all that bad... in case the
implementor wants all the info at once, instead of in chunks.

darin needs to sr this.
Attachment #149466 - Flags: superreview?(darin)
Attachment #149466 - Flags: review?(cbiesinger)
Attachment #149466 - Flags: review+

Comment 70

13 years ago
Comment on attachment 149466 [details] [diff] [review]
OnInformationAvailable patch cleaned - 3nd round

>Index: netwerk/streamconv/converters/nsDirIndexParser.cpp

>+            char    *value = ((char *)buf) + 4;
>+            nsUnescape(value);

nit: strange use of whitespace.


>+            mListener->OnInformationAvailable(aRequest, aCtxt, NS_ConvertUTF8toUCS2(value));

use NS_ConvertUTF8toUTF16 instead.


>Index: netwerk/streamconv/converters/nsGopherDirListingConv.cpp

>         if (tabPos) {
>+            /* if the description is not empty */
>+            if (tabPos != line)
>+            {

nit: use a consistent style for the opening brackets.  looks like
this file likes to start brackets on the same line as the branch
condition.


>Index: netwerk/streamconv/converters/nsIndexedToHTML.cpp

>+    pushBuffer.Append(NS_LITERAL_STRING("</a>"));
>+    pushBuffer.Append(NS_LITERAL_STRING("</td>\n <td>"));

these should be combined into one call to .Append


>+nsIndexedToHTML::OnInformationAvailable(nsIRequest *aRequest,
>+                                  nsISupports *aCtxt,
>+                                  const nsAString& aInfo) {

please make the parameters line up vertically... like this:

  nsIndexedToHTML::OnInformationAvailable(nsIRequest *aRequest,
					  nsISupports *aCtxt,
					  const nsAString& aInfo) {


>+    nsString pushBuffer;

use nsAutoString instead to avoid heap allocations for
small information strings.


>+    PRUnichar* escaped = nsEscapeHTML2(PromiseFlatString(aInfo).get());
>+    if (!escaped)
>+        return NS_ERROR_OUT_OF_MEMORY;
>+    pushBuffer.Append(NS_LITERAL_STRING("<tr>\n <td>"));
>+    pushBuffer.Append(escaped);
>+    nsMemory::Free(escaped);

>+    pushBuffer.Append(NS_LITERAL_STRING("</td>\n <td>"));
>+    pushBuffer.Append(NS_LITERAL_STRING("</td>\n <td>"));
>+    pushBuffer.Append(NS_LITERAL_STRING("</td>\n <td>"));
>+    pushBuffer.Append(NS_LITERAL_STRING("</td>\n</tr>\n"));

combine these into one .Append call to minimize codesize.


>Index: netwerk/streamconv/public/nsIDirIndexListener.idl


>+    /**
>+     * Called for each directory entry

can you elaborate on this comment?  what does this mean exactly?


>+     *
>+     * @param request - the request
>+     * @param ctxt - opaque parameter
>+     * @param info - new info to add

parameter names in the comment do not match actual parameter names.


looks good otherwise, but please post a revised patch.
Attachment #149466 - Flags: superreview?(darin) → superreview-
(Assignee)

Comment 71

13 years ago
(In reply to comment #70)
> (From update of attachment 149466 [details] [diff] [review])
> >Index: netwerk/streamconv/converters/nsDirIndexParser.cpp
> 
> >+            char    *value = ((char *)buf) + 4;
> >+            nsUnescape(value);
> 
> nit: strange use of whitespace.
Sorry, just copied what was few lines below
 for the '201:' case in the ParseData.

Do you have any suggestions?

>
> >Index: netwerk/streamconv/converters/nsGopherDirListingConv.cpp
>
> nit: use a consistent style for the opening brackets.  looks like
> this file likes to start brackets on the same line as the branch
> condition.
Sorry my dysgraphia. I will fix this momentarily.

> >Index: netwerk/streamconv/public/nsIDirIndexListener.idl
> parameter names in the comment do not match actual parameter names.

Same goes for the previous function onIndexAvailable which was there 
at least since (1.1 <bbaetz@cs.mcgill.ca> 2001-10-02 17:26)!!

Should I fix those too, introduce inconsistency or leave it as it is?

> looks good otherwise, but please post a revised patch.
Ok, as soon as you answer to this reply.
(The deleted parts of super review were applied.)

Thanks for super review

Comment 72

13 years ago
What matters most is that the code additions remain consistent with existing
code.  So, if the existing code has the odd style, then I guess it's fine to
stick to that style.  For bonus points: you could fix up the entire files :-)

But, I'm fine if you want to just make your additions match.  Sorry for not
noticing that you had!

sr=darin
(Assignee)

Comment 73

13 years ago
Created attachment 150402 [details] [diff] [review]
149466: OnInformationAvailable patch cleaned - 4nd round

The promissed patch.
Attachment #149466 - Attachment is obsolete: true
(Assignee)

Comment 74

13 years ago
Comment on attachment 150402 [details] [diff] [review]
149466: OnInformationAvailable patch cleaned - 4nd round

any other nitpick? :-P
Attachment #150402 - Flags: superreview?(darin)
Attachment #150402 - Flags: review?(cbiesinger)
Comment on attachment 150402 [details] [diff] [review]
149466: OnInformationAvailable patch cleaned - 4nd round

+	     } else if(type == 'i'){

hm, whoever checks this in should probably add a space before {
Attachment #150402 - Flags: review?(cbiesinger) → review+
(Assignee)

Comment 76

13 years ago
Comment on attachment 150402 [details] [diff] [review]
149466: OnInformationAvailable patch cleaned - 4nd round

>Index: netwerk/streamconv/converters/nsDirIndexParser.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/netwerk/streamconv/converters/nsDirIndexParser.cpp,v
>retrieving revision 1.14
>diff -u -8 -p -r1.14 nsDirIndexParser.cpp
>--- netwerk/streamconv/converters/nsDirIndexParser.cpp	10 May 2004 23:21:34 -0000	1.14
>+++ netwerk/streamconv/converters/nsDirIndexParser.cpp	10 Jun 2004 02:17:13 -0000
>@@ -425,16 +425,21 @@ nsDirIndexParser::ProcessData(nsIRequest
>       
>       if (buf[0] == '1') {
>         if (buf[1] == '0') {
>           if (buf[2] == '0' && buf[3] == ':') {
>             // 100. Human-readable comment line. Ignore
>           } else if (buf[2] == '1' && buf[3] == ':') {
>             // 101. Human-readable information line.
>             mComment.Append(buf + 4);
>+
>+            char    *value = ((char *)buf) + 4;
>+            nsUnescape(value);
>+            mListener->OnInformationAvailable(aRequest, aCtxt, NS_ConvertUTF8toUTF16(value));
>+
>           } else if (buf[2] == '2' && buf[3] == ':') {
>             // 102. Human-readable information line, HTML.
>             mComment.Append(buf + 4);
>           }
>         }
>       } else if (buf[0] == '2') {
>         if (buf[1] == '0') {
>           if (buf[2] == '0' && buf[3] == ':') {
>Index: netwerk/streamconv/converters/nsGopherDirListingConv.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/netwerk/streamconv/converters/nsGopherDirListingConv.cpp,v
>retrieving revision 1.14
>diff -u -8 -p -r1.14 nsGopherDirListingConv.cpp
>--- netwerk/streamconv/converters/nsGopherDirListingConv.cpp	18 Apr 2004 21:59:21 -0000	1.14
>+++ netwerk/streamconv/converters/nsGopherDirListingConv.cpp	10 Jun 2004 02:17:13 -0000
>@@ -298,22 +298,26 @@ nsGopherDirListingConv::DigestBufferLine
>         PRInt32 port = GOPHER_PORT;
> 
>         type = line[0];
>         line++;
>         char* tabPos = PL_strchr(line,'\t');
> 
>         /* Get the description */
>         if (tabPos) {
>-            char* descStr = PL_strndup(line,tabPos-line);
>-            char* escName = nsEscape(descStr,url_Path);
>-            desc = escName;
>-            nsMemory::Free(escName);
>-            nsMemory::Free(descStr);
>-
>+            /* if the description is not empty */
>+            if (tabPos != line) {
>+                char* descStr = PL_strndup(line,tabPos-line);
>+                char* escName = nsEscape(descStr,url_Path);
>+                desc = escName;
>+                nsMemory::Free(escName);
>+                nsMemory::Free(descStr);
>+            } else {
>+                desc = "%20";
>+            }
>             line = tabPos+1;
>             tabPos = PL_strchr(line,'\t');
>         }
> 
>         /* Get selector */
>         if (tabPos) {
>             char* sel = PL_strndup(line,tabPos-line);
>             char* escName = nsEscape(sel,url_Path);
>@@ -385,16 +389,20 @@ nsGopherDirListingConv::DigestBufferLine
>                 aString.Append(' ');
>                 aString.Append(filename);
>                 aString.Append(' ');
>                 if (type == '1')
>                     aString.Append("DIRECTORY");
>                 else
>                     aString.Append("FILE");
>                 aString.Append(char(nsCRT::LF));
>+            } else if(type == 'i') {
>+                aString.Append("101: ");
>+                aString.Append(desc);
>+                aString.Append(char(nsCRT::LF));
>             }
>         } else {
>             NS_WARNING("Error parsing gopher directory response.\n");
>             //printf("Got: %s\n",filename.get());
>         }
>         
>         if (cr)
>             line = eol+2;
>Index: netwerk/streamconv/converters/nsIndexedToHTML.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/netwerk/streamconv/converters/nsIndexedToHTML.cpp,v
>retrieving revision 1.58
>diff -u -8 -p -r1.58 nsIndexedToHTML.cpp
>--- netwerk/streamconv/converters/nsIndexedToHTML.cpp	18 Apr 2004 21:59:21 -0000	1.58
>+++ netwerk/streamconv/converters/nsIndexedToHTML.cpp	10 Jun 2004 02:17:14 -0000
>@@ -593,16 +593,38 @@ nsIndexedToHTML::OnIndexAvailable(nsIReq
>     if (++mRowCount > ROWS_PER_TABLE) {
>         pushBuffer.Append(NS_LITERAL_STRING("</table>\n<table>\n"));
>         mRowCount = 0;
>     }
>     
>     return FormatInputStream(aRequest, aCtxt, pushBuffer);
> }
> 
>+NS_IMETHODIMP
>+nsIndexedToHTML::OnInformationAvailable(nsIRequest *aRequest,
>+                                        nsISupports *aCtxt,
>+                                        const nsAString& aInfo) {
>+    nsAutoString pushBuffer;
>+    PRUnichar* escaped = nsEscapeHTML2(PromiseFlatString(aInfo).get());
>+    if (!escaped)
>+        return NS_ERROR_OUT_OF_MEMORY;
>+    pushBuffer.Append(NS_LITERAL_STRING("<tr>\n <td>"));
>+    pushBuffer.Append(escaped);
>+    nsMemory::Free(escaped);
>+    pushBuffer.Append(NS_LITERAL_STRING("</td>\n <td></td>\n <td></td>\n <td></td>\n</tr>\n"));
>+    
>+    // Split this up to avoid slow layout performance with large tables
>+    // - bug 85381
>+    if (++mRowCount > ROWS_PER_TABLE) {
>+        pushBuffer.Append(NS_LITERAL_STRING("</table>\n<table>\n"));
>+        mRowCount = 0;
>+    }   
>+    return FormatInputStream(aRequest, aCtxt, pushBuffer);
>+}
>+
> void nsIndexedToHTML::FormatSizeString(PRInt64 inSize, nsString& outSizeString)
> {
>     nsInt64 size(inSize);
>     outSizeString.Truncate();
>     if (size > nsInt64(0)) {
>         // round up to the nearest Kilobyte
>         PRInt64  upperSize = (size + nsInt64(1023)) / nsInt64(1024);
>         outSizeString.AppendInt(upperSize);
>Index: netwerk/streamconv/public/nsIDirIndexListener.idl
>===================================================================
>RCS file: /cvsroot/mozilla/netwerk/streamconv/public/nsIDirIndexListener.idl,v
>retrieving revision 1.2
>diff -u -8 -p -r1.2 nsIDirIndexListener.idl
>--- netwerk/streamconv/public/nsIDirIndexListener.idl	18 Apr 2004 21:59:22 -0000	1.2
>+++ netwerk/streamconv/public/nsIDirIndexListener.idl	10 Jun 2004 02:17:18 -0000
>@@ -52,16 +52,28 @@ interface nsIDirIndexListener : nsISuppo
>      *
>      * @param request - the request
>      * @param ctxt - opaque parameter
>      * @param index - new index to add
>      */
>     void onIndexAvailable(in nsIRequest aRequest,
>                           in nsISupports aCtxt,
>                           in nsIDirIndex aIndex);
>+
>+    /**
>+     * Called for each information line
>+     *
>+     * @param request - the request
>+     * @param ctxt - opaque parameter
>+     * @param info - new info to add
>+     */
>+    void onInformationAvailable(in nsIRequest aRequest,
>+                                in nsISupports aCtxt,
>+                                in AString aInfo);
>+
> };
> 
> %{C++
> #define NS_IDIRINDEXLISTENER_KEY         "@mozilla.org/dirIndexListener;1"
> %}
> 
> /**
>  * A parser for application/http-index-format
>Index: xpfe/components/directory/nsDirectoryViewer.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/xpfe/components/directory/nsDirectoryViewer.cpp,v
>retrieving revision 1.110
>diff -u -8 -p -r1.110 nsDirectoryViewer.cpp
>--- xpfe/components/directory/nsDirectoryViewer.cpp	10 May 2004 23:21:35 -0000	1.110
>+++ xpfe/components/directory/nsDirectoryViewer.cpp	10 Jun 2004 02:18:15 -0000
>@@ -538,16 +538,23 @@ nsHTTPIndex::OnIndexAvailable(nsIRequest
> //       if (NS_FAILED(rv)) return rv;
> //   defer insertion onto a timer so that the UI isn't starved
>     AddElement(parentRes, kNC_Child, entry);
>   }
> 
>   return rv;
> }
> 
>+nsresult
>+nsHTTPIndex::OnInformationAvailable(nsIRequest *aRequest,
>+                                  nsISupports *aCtxt,
>+                                  const nsAString& aInfo) {
>+  return NS_ERROR_NOT_IMPLEMENTED;
>+}
>+
> //----------------------------------------------------------------------
> //
> // nsHTTPIndex implementation
> //
> 
> nsHTTPIndex::nsHTTPIndex()
>   : mBindToGlobalObject(PR_TRUE),
>     mRequestor(nsnull)

Comment 77

13 years ago
Now with the review/superreview from Darin and Christian, could you set the
approval 1.7 flag again? If it made it into the new stable branch, then it would
make Firefox 1.0, which is based off the stable branch) a gopher friendly
browser. And promotion from the gopher community is certainly welcome ;)

Thanks to Jan Darin and Christian for all their effort put into getting this
done this way.

Comment 78

13 years ago
Comment on attachment 150402 [details] [diff] [review]
149466: OnInformationAvailable patch cleaned - 4nd round

sr=darin

1.7 is nearly complete.  i don't think drivers are likely to accept this patch.
 firefox is not on the 1.7 branch anymore.  they cut a special branch for
firefox and thunderbird off of the 1.7 branch.	it's known as the aviary 1.0
branch.  i also think it unlikely that they will accept this patch there since
they are getting close to release, but you might have a shot.  i don't know how
to request approval to land a change on the aviary 1.0 branch.	you'd probably
have to send mail to the firefox devs.

probably best to just land this on the trunk and wait for it to be included in
moz 1.8
Attachment #150402 - Flags: superreview?(darin) → superreview+
(Assignee)

Comment 79

13 years ago
(In reply to comment #78)
> (From update of attachment 150402 [details] [diff] [review])
> sr=darin
> probably best to just land this on the trunk and wait for it to be included in
> moz 1.8

Ok, it seems to be the best way.
 
Are there any plans to merge some code from the main trunk back to 1.7 or
Firefox?

I would love to have my patch in the 1.7, 
but I don't want to be like 
the "thin mint wafer" from the meaning of life (monty python).

Comment 80

13 years ago
Heck, it can't hurt to ask.... There's a patch and it seems harmless enough.
1.7?
Flags: blocking1.7?
[Netscape® Communicator 4.8 : en-20020722] (W98SE)

Late confirmation: adding '4xp' !
Keywords: 4xp
Comment on attachment 150402 [details] [diff] [review]
149466: OnInformationAvailable patch cleaned - 4nd round


'approval1.7=?':
Giving it a try, per '4xp' and comment 11.
I can't say more about this patch :-|
Attachment #150402 - Flags: approval1.7?

Comment 83

13 years ago
Comment on attachment 150402 [details] [diff] [review]
149466: OnInformationAvailable patch cleaned - 4nd round

we hope to have made the last builds for 1.7.  need to minus this.
Attachment #150402 - Flags: approval1.7? → approval1.7-

Comment 84

13 years ago
Well, bad luck then.

Who volunteers to check this into the trunk? Christian?

(I don't think that Jan has the necessary privilidges to do this)
Assignee: nobody → jan.ruzicka
checked in on trunk.
Status: NEW → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED

Updated

13 years ago
Flags: blocking1.7? → blocking1.7-

Comment 86

13 years ago
mike, people want this landed on aviary1.0, see bug 248230.
Whiteboard: needed-aviary1.0

Comment 87

13 years ago
Maybe someone should set the "approval-aviary" flag to "?" in the "Edit" menue
of patch 149466?
(Assignee)

Updated

13 years ago
Attachment #150402 - Flags: approval-aviary?

Comment 88

13 years ago
Since we still didn't get any response (neither negative nor positive) from the
firefox project management, I CC'ed two additional people: Asa Dotzler and Mike
Kaply (since he once helped out with a MNG bug).


To sum up the arguments for checking this into the aviary:

1) Without this bug fixed the gopher code is practically unusable and therefore
cruft

2) For this reason Mozilla and Firefox weren't recommended browsers for the
gopherspace (see comment #11). With this bug fixed Firefox 1.0 would probably
become a recommended browser for all gopher people with probably some people
converting from Netscape 4 (!) and Lynx to Mozilla/Firefox and getting some
marketing/popularity boost from gopher related websites and people.

3) According to the module owner Darin Fisher this patch isn't likely to break
anything.

4) Feature parity with Mozilla 1.8

5) WORKING gopher support can be listed to demonstrate that Firefox is
technically superior to the Internet Explorer (Gopher support > ditched Gopher
support in IE6, before it was broken gopher support = ditched support in IE).

Comment 89

13 years ago
Oh and please read Bugzilla Bug 248230, because it deals with checking this in
on the aviary branch. If this should be moved on to the branch this bug should
be marked "FIXED", if not it should me marked "WONTFIX".

Updated

13 years ago
Attachment #150402 - Flags: approval-aviary? → approval-aviary-
(Assignee)

Comment 90

13 years ago
Does gopher support qualify for the blocking-aviary1.1 ?
Flags: blocking-aviary1.1?
Flags: blocking-aviary1.1? → blocking-aviary1.1-
being fixed on trunk, this bug is already fixed for firefox 1.1
You need to log in before you can comment on or make changes to this bug.