102812 - file: enable html directory listing mode

Status: VERIFIED FIXED

(Core :: Networking: File, defect, P1)

Description

[Bradley Baetz (:bbaetz)]

I checked in bug 78148, which has the backend work, but I had to disable it for
file urls because non-ascii filenames didn't display correctly. I need to poke
arround a bit and try to work out whats broken, and how to fix it.

Its surprising that the urls work, but since they do, all this involves is
probably moving the code to get the filesystem charset out of the #ifdef
THREADSAFE_I18N block, and proxying the call to get the FS charset over to the
UI thread.

I doubt I'll have time to do this in the next week, though.

Comment 1

[Bradley Baetz (:bbaetz)]

Out of time, -> 0.9.7

Comment 2

[Bradley Baetz (:bbaetz)]

This has missed 0.9.7, and so moving to 0.9.9 (I'm not here for 0.9.8). Bug
102812, bug 102826, and bug 110760 will probably be fixed by the same fix, and
so nominating for 1.0, since I expect that embedders will want bug 110760 fixed,
at least.

Comment 3

[John Levon]

Will this fix bug 49944 too or not ?

Comment 4

[Bradley Baetz (:bbaetz)]

Well, it won't fix it, but since it won't be using the xul viewer you won't be
able to notice it.

Comment 5

[Bradley Baetz (:bbaetz)]

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

Comment 6

[Bradley Baetz (:bbaetz)]

-> 1.0

Comment 7

[benc]

So, is the plan to hardcode this change, or have it selectable?

Comment 8

[Bradley Baetz (:bbaetz)]

A pref.

Comment 9

[Bradley Baetz (:bbaetz)]

Attachment: patch

This needs i18n testing to check that it works.

Because of other bugs (nspr ones wrt file system encoding), this can only be
tested on a non-english w2k version. Please compare file listings with a
non-english file both before and after. Check that you can follow the link as
well as see the name.

If there is a problem which applies with the patch but not without, please
save-as the file:/// directory listing page, and attach it + a screenshot to
this bug.

Comment 10

[Bradley Baetz (:bbaetz)]

momoi, can I please get you to test the attached patch? See my earlier comment
for testing instructions.

Comment 11

[Masaki Katakai]

I'm not able to do in today. Sorry for that. I'll try tomorrow.

Comment 12

[Masaki Katakai]

Hmm, the patch does not work on my environment...
I'm using NT4.0 JA.

I built the binary and tried just c:\, it does not work,
nothing happened. Actually before I applied the patch,
the binary worked to display directory listing...

Comment 13

[Bradley Baetz (:bbaetz)]

Do you get no display, or empty files, or something else when you do file:///c|/
? Does it just hang, or something else?

Comment 14

[Masaki Katakai]

No action happens for me. Same results for file:///c|/.

When I back out the patches under netwerk/, the behavior
changed. I type "c:\" on url field and return, it changed
to "file:///c:/" on url field. But nothing is displayed.

Comment 15

[Bradley Baetz (:bbaetz)]

Hmm. So it breaks even without my patch? This broke a while ago, but it was
fixed, I thought - when was your source updated?

Comment 16

[Masaki Katakai]

I pick up the tar image yesterday. Without patch,
it displays directory tree properly. 

Comment 17

[Bradley Baetz (:bbaetz)]

Hmm. Was this a debug or a release build? With the patch, can you try running
regxpcom before starting up?

Also, what is the setting in prefs->debug->networking? The direcroty format
should be html, but can you test with XUL also? (With my patch)

Comment 18

[Masaki Katakai]

I tried both debug and optimize build.
Row and XUL seems to work for me.
Only HTML mode does not work.

Comment 19

[Masaki Katakai]

Hotta-san,

Can your team also try the patch on Windows JA?

Comment 20

[Bradley Baetz (:bbaetz)]

This works on linux - can I get you to step through the code I added in
nsFileChannel.cpp and see which line causes the failure? 

Comment 21

[Roy Yokoyama]

bbaetz: nhotta asked me to try the patch, so I quickly looked over the patch
        before I go ahead and apply it to my local tree.  
        I now see netwerk is depending on Pref.  Hmmm, we should check with 
        darin or alecf (dependencies keeper).

darin/alecf: what do you guys think?

Comment 22

[Bradley Baetz (:bbaetz)]

Netowrk (http/ftp) has always depended on prefs (user agent, http version, and
so on). File didn't need it before, but this patch doesn't change what
dependancies the main necko library needs.

Comment 23

[Bradley Baetz (:bbaetz)]

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

Comment 24

[Bradley Baetz (:bbaetz)]

This patch fails on windows, apparently because of a return value of
NS_ERROR_FILE_UNRECOGNIZED_PATH being returned somewhere.

Can someone with a windows machine apply this and try to work out where this is
coming from?

/me smiles at dougt and darin

Comment 25

[Bradley Baetz (:bbaetz)]

Attachment: resolve cvs conflicts

This just resolves some conflicts.

I'm still looking for windows people to help debug this. I can walk people
through what to debug, if they are familiar with msvc and have a debug build to
apply this patch on.

Comment 26

[Bradley Baetz (:bbaetz)]

OK, steve morse helped me debug this partly. The first problem is that GetParent
appears not to work. Ignoring the return value at line 172 of nsIndexedToHTML,
and changing the if (parent) to if (NS_SUCCEEDED(rv) && parent) makes this get
further. (Its failing to find a \ in a path which does have a \ in it, somehow).
Steve only had an opt tree, so we couldn't work out more info.

Since the current file:/// viewer doens't let you go to the parent directory,
I'm happy to change this call to ignore errors.

However, there is still a problem, in that no file actually displays. It looks
like the correct data is coming in to the parser, but it dies somewhere after
that. Steve had to go, so we didn't get any further - I don't know where it is
dying.

Any volunteers, with a debug windows tree?

Comment 27

[Darin Fisher]

bbaetz: if you find me today, i can help.

Comment 28

[Peter Trudelle]

nsbeta1+, let me know if you guys need anyone else.

Comment 29

[Darin Fisher]

Attachment: patch for nsLocalFileWin.cpp

ok, so GetParent fails when called on a nsLocalFile w/ path = "c:"

w/ this patch and bbaetz's patch, i can view "file:///c:/" under win2k.

Comment 30

[Bradley Baetz (:bbaetz)]

Steve was having problems with a subdir, though - that was teh first thing I
tried after c: failed. I'm wondering if he just had an old tree, or something.

Given the windows breakage, this probably needs mac checking, too - I'll see if
I can find someone to handle that, and then get i18n testing, too.

When I get home I'll attach a patch which is less strict about getparent
succeeding, though.

Comment 31

[Bradley Baetz (:bbaetz)]

Attachment: v3

OK, now we don't fail if GetParent fails. We will assert, though.

Comment 32

[Darin Fisher]

Comment on attachment 76148 [details] [diff] [review]
v3

>Index: netwerk/protocol/file/src/nsFileChannel.cpp

>+        PRInt32 sFormat;
>+        rv = prefs->GetIntPref("network.dir.format", &sFormat);

nit: sFormat.. why the s-prefix?  the s-prefix is typically used for static
data or strings.


>+nsFileChannel::GetListFormat(PRUint32 *format) {

nit: inconsistent brace style


i think you should include the nsLocalFileWin.cpp patch.  w/o it, GetParent is
not following
the documentation for nsIFile::parent.

sr=darin

Comment 33

[Bradley Baetz (:bbaetz)]

K, changed locally. Your patch is a separate bug. r=bbaetz on that.

Comment 34

[Alec Flett]

Comment on attachment 76115 [details] [diff] [review]
patch for nsLocalFileWin.cpp

sr=alecf

Comment 35

[Darin Fisher]

Comment on attachment 76148 [details] [diff] [review]
v3

r=darin w/ the changes i mentioned in comment #32

Comment 36

[Bradley Baetz (:bbaetz)]

Attachment: v4

[darin: you gave both r and sr to that last patch - alecf's sr was for teh
separate windows one]

Mac behaves differently, so I had to copy code from the rdf:file stuff, so that
size includes the resource fork, and we handle package files correctly. I also
needed to inlcude the charset, since mac uses a different format to the default
one I assumed. See the comments for why this is done where it is.

Comment 37

[Bradley Baetz (:bbaetz)]

Attachment: v5

Now compiles on mac! And fixes a thinko with getting file sizes on non-mac.

Comment 38

[Bradley Baetz (:bbaetz)]

Comment on attachment 76359 [details] [diff] [review]
v5

Apparently this still doesn't get the japanese file stuff working correctly. I
need to try to work out what is hapening, and where this is getting lost.

Comment 39

[Bradley Baetz (:bbaetz)]

OK, one more random guess: In nsIndexedToHTML.cpp, try changing the
NS_ConvertASCIItoUCS2 to NS_ConvertUTF8toUCS2, in both places.

This is a random guess, and is very likely to break something.

If this doesn't work, I have one more guess (use nsITextToSubURI + the charset
convertor).

Comment 40

[Masaki Katakai]

It seems that

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

and

        mDateTime->FormatPRTime(nsnull,
                                kDateFormatNone,
                                kTimeFormatSeconds,
                                t,
                                formatted);
        pushBuffer.Append(formatted);

are not working properly but title string is encoded as Shift_JIS
properly and is displayed OK.

    nsXPIDLString title;
    nsAutoString uniSpec; uniSpec.AssignWithConversion(spec);

    const PRUnichar* formatTitle[] = {
        uniSpec.get()
    };

    rv = mBundle->FormatStringFromName(NS_LITERAL_STRING("DirTitle").get(),
                                       formatTitle,
                                       sizeof(formatTitle)/sizeof(PRUnichar*),
                                       getter_Copies(title));
    if (NS_FAILED(rv)) return rv;
    buffer.Append(title);

Comment 41

[Bradley Baetz (:bbaetz)]

Was that with or without the UTF8 change?

This is odd - the prtime stuff should be safe, unless its confused by the
description being wrong. If you remove the escaping, what happens?

Comment 42

[Masaki Katakai]

The changes of UTF-8 did not fix the problem. Yes, I tried
not to use escaping, but it did not fix the problem.

Comment 43

[Bradley Baetz (:bbaetz)]

Ok... I'll have to try to debug this, then, I guess.

Comment 44

[Bradley Baetz (:bbaetz)]

Hmm

OK, how about this. In nsDirIndexParser.cpp, change line 277 from:

if (gTextToSubURI) {

to:

if (0 && gTextToSubURI) {

That will then bypass the convertions, which is what I think is causing
problems. If you do this, then spaces will display as %20. Thats OK; if this
works then I can tidy this up to just unescape normally until i18n is threadsafe.

Comment 45

[Bradley Baetz (:bbaetz)]

OK, that didn't work. I could try a few more things, but the fact that the
datetime is also wrong suggests that this is probably a deeper problem.

In bug 118179, yokoyama noticed that the string input stream stuff does
ToNewCString. This could be the cause.

katakai has been of great help trying to debug this - I can't do this from here.
Unless we can work this out, or get a work arround, then this will have to be
pushed off past 1.0

Comment 46

[Masaki Katakai]

Not really sure for me what we should do here... but here are the results
while I'm debuging on UNIX platform,

- nsIndexedToHTML::OnStartRequest()

    char* spec = nsCRT::strdup(baseUri.get());
    char* escaped = nsEscapeHTML(spec);

    spec and escaped are native encoding e.g. Shift_JIS or EUC.
    So I think we need to convert these to UCS2.

- nsIndexedToHTML::OnIndexAvailable()
  nsIndexedToHTML::OnStartRequest()

  Who will/should convert buffer and pushBuffer to native encoding finally?
  Append() is doing as UCS2, but who will convert them to native encoding
  that specified by "charset=" field?

  Currently, it seems that \u304b\u3044 is converted to 0x4b0x44 at the
  page.

I'll put sample codes for nsIndexedToHTML.cpp, it's *not* a patch,
just an example codes.

- convert spec and escaped to native encoding to UCS2

- convert buffer and pushBuffer to native encoding
  Not sure where is the correct place to do conversion

It's working for me on Linux. Page format is not good on Window
build. but the character conversion is correct for me.

Comment 47

[Masaki Katakai]

Attachment: example: add conversion to UNICODE, to native encoding

Comment 48

[Bradley Baetz (:bbaetz)]

Thanks a lot for this! What do you mean by 'page format is not good' - does this
means that it doesn't work on windows? In other words if I made the patch a real
patch (removing calls to exit, and so on), would that fix the problem?

Presumably if there is a charset bit in the content type, layout handles it.
Thats what I've been assuming, but I may be wrong.

Comment 49

[Masaki Katakai]

My example codes around Convert() seem to be wrong...
The code conversion itself is correct and it works fine
on Linux. But source len or dest len for Convert() should
be corrected because the layout of table is broken by
the extra '<' on Windows. I'm sorry I just gathered the
codes from others. I understand if we could re-write
codes around Convert() correctly, it should work on both
Linux and Windows, I believe.

Comment 50

[Roy Yokoyama]

Isn't this very close to http://bugzilla.mozilla.org/show_bug.cgi?id=118179 ?
darin suggests that we should change 
nsString pushBuffer 
...
NS_NewStringInputStream() 

to 
nsCString pushBuffer
...
NS_NewCStringInputStream() 

see comment http://bugzilla.mozilla.org/show_bug.cgi?id=118179#c47
We may need to work together on this; because we could potentially collide.

Comment 51

[Bradley Baetz (:bbaetz)]

right - I think we're looking at the same problem here, except that in my case
we know the charset we have, and in yours we need to guess.

This bug is wanted for 1.0, since there are usablility/perf issues with the
current xul view which were pushed off to 1.1. I can't debug this though, I
don't think.

Comment 52

[Peter Trudelle]

Mass moving nsbeta1+/adt3 bugs assigned to Navigator team engineers out to
target milestone 1.0.1.  Please confine your attentions to driving down our list
of TM 1.0 bugs for beta.  Better to help, debug, or test one of them than fix
one of these.

Comment 53

[scottputterman]

Changing nsbeta1+ [adt3] bugs to nsbeta1- on behalf of the adt.  If you have any
questions about this, please email [e-mail].  You can search for
"changing adt3 bugs" to quickly find and delete these bug mails.

Comment 54

[scottputterman]

Changing nsbeta1+ [adt3] bugs to nsbeta1- on behalf of the adt.  If you have any
questions about this, please email [e-mail].  You can search for
"changing adt3 bugs" to quickly find and delete these bug mails.

Comment 55

[Roy Yokoyama]

Bradly/Katakai: 
I have a patch for 118179.
Can you try the patch in Win/Linux/Mac if it fixes this problem?
http://bugzilla.mozilla.org/attachment.cgi?id=80510&action=view

Comment 56

[Roy Yokoyama]

>because non-ascii filenames didn't display correctly.
Well, Ja filenames in my c drive ( W2K-Ja ) show correctly
with and without my patch (118179). :(

I observed two issues here:
1) Ja _Folder Name_ diplays as a blank folder
2) None-locale filename (eg Korean filename) is vanished from the list!!

Comment 57

[Bradley Baetz (:bbaetz)]

This will  apparently work on windows-ja system, but no windows-en systems,
because of the particular api calls nspr makes.

So you mean that with the patch in this bug (v5), the jp file listings work
without problems, and appear in html mode?

Comment 58

[Roy Yokoyama]

Sorry; but I am little confused about this bug.  The current plain vanilla trunk
build 
displays Ja filenames correctly in W2K-ja with Pref/Networking/HTML mode 
except two problems I described in #56.

Comment 59

[Bradley Baetz (:bbaetz)]

I suspect the nsifile cleanups may make this easier, since I can now get stuff
whic is definately-honestly-really utf8, I believe.

I hope to have a new patch this week.

Comment 60

[Bradley Baetz (:bbaetz)]

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

Comment 61

[Bradley Baetz (:bbaetz)]

dougt checked in something along these lines for bug 141247. It has the same
problem my patch does with non-ascii names, plus it didn't carry forward some
platform specific changes (eg for package stuff for the mac).

I hope to have this patch merged + working with the new nsIFile stuff this
weekend, if I find time. Then someone will need to test it....

Comment 62

[Roy Yokoyama]

Bradley: I am more than happy to help out.  Just give me a shout.

Comment 63

[benc]

nsbeta1- -> nsbeta1: renominating. File is the only protocol handler using
XUL-tree. Gopher and FTP have both migrated, so the existence of file w/ tree is
actually somewhat inconsistent with the rest of the application.

Comment 64

[Peter Trudelle]

We would prefer that all of these use HTML, not tree or outliner, but are way
past fixing 'inconsistency' bugs.  What is the impact of this on target users?

Comment 65

[Bradley Baetz (:bbaetz)]

-> default owner. I'mnot going to get to this any time soon, but I'll review
patches...

Comment 66

[nhottanscp]

I think this is fixed by bug 162377.

Comment 67

[Doug Turner (:dougt)]

per last comment.

Comment 68

[James Paige]

Are you sure? I think bug 162377 just fixed html-ftp listings. I still can't get
anything but the xul listing for file://

Comment 69

[nhottanscp]

right, it is working for HTML listing mode only
please re-open if this bug is about XUL listing

Comment 70

[Doug Turner (:dougt)]

the XUL viewer is unsupported.  If there is a bug in it, please move to XPAPPs.

Comment 71

[Bradley Baetz (:bbaetz)]

This bug is _NOT_ fixed. file:/// still only loads the XUL listing. Bug 162377
may make fixing this easier, but its definatley not fixed - for a start,
nsDirectoryViewer hardcodes file to the XUL format, and the patches in this bug
have not been applied.

Comment 72

[Doug Turner (:dougt)]

never mind my comment.  It was for another bug. 

Comment 73

[Doug Turner (:dougt)]

Naoki may have time next week to look at this.

Comment 74

[nhottanscp]

I looked at this with Roy and we saw that file:// shows Chinese file name on
Chinese system (WinXP). 
Katakai san, what is the remaining problem?

Comment 75

[nhottanscp]

I tried Japanese case also, it works fine as long as the system default is set
to Japanese. 
The current patch uses nsIPlatformCharset which depends on the system default.
So it cannot support path names in non system default. 

Change topembed+ to topembed for re-evaluation.

Comment 76

[nhottanscp]

Change qa contact to ylong.
So far, I saw that the listing works for the default locale.
Yuying, please check if that is the case. What I tried was like file:///C:/

Comment 77

[Yuying Long]

Confirm it's works fine with locale non-ascii path name on all platforms with
10-09 trunk build.

Comment 78

[nhottanscp]

Mark as worksforme.
Please reopen or file a separate bug if any remaining problems.

Comment 79

[Yuying Long]

Mark as verified as such.

Comment 80

[sairuh (rarely reading bugmail)]

this is still a problem for ascii filepaths --but i thought this bug covered
both ascii and non-ascii.

reopening. unless there's another bug somewhere for ascii filepaths.

Comment 81

[Doug Turner (:dougt)]

sairuh, what exactly is the problem?

Comment 82

[sairuh (rarely reading bugmail)]

when i try to do a local file listing, i get the XUL-formatted display, not the
HTML-formatted display.

this is what i do: for example, on linux i enter /home/halo into the urlbar,
then hit enter.

what results is the XUL listing, not an html listing. the urlbar resets what i
had entered as file:///home/halo/

Comment 83

[sairuh (rarely reading bugmail)]

comment 71 still stands, i believe. it looks like the summary morphed a bit on
the way, since it used to be something more generic, "file: enable html
directory listing mode".

Comment 84

[nhottanscp]

Then it would be less confusing either change the summary or file a new bug.
I prefer new bug. Doug, what do you think?

Comment 85

[Bradley Baetz (:bbaetz)]

Well, if this does work, then you can juat grab patch 5 and clean it up, so from
that POV I think using this bug (and remorphing back) is a good idea.

Comment 86

[Doug Turner (:dougt)]

it doesn't matter: new bug or not.  all we have to do is make sure that the
default pref is set.  nhotta, can you write up a patch that does this?

Comment 87

[Bradley Baetz (:bbaetz)]

dougt: Yeah, but the same pref should be used for all the directiry viewers. I
guess thats a separate bug, though.

Comment 88

[nhottanscp]

Change the summary to "file: enable html directory listing mode" and back to
dougt for now. I can help with i18n issues.

Comment 89

[Doug Turner (:dougt)]

bradley, I do not understand your comment #85.  If we just enable the
preference, what are you suggesting will break without patch 5?

Comment 90

[Bradley Baetz (:bbaetz)]

dougt: I'm not sure. You'd be using a different pref, so people would have ot
set both prefs. It used to be that this method required xpfe/components to do
the intermediate conversion, but that seems to have been fixed.

Comment 91

[Doug Turner (:dougt)]

Attachment: makes file: use the same pref as ftp and gopher

Comment 92

[Doug Turner (:dougt)]

keep in mind that if I check in that last patch, the default behavior of file:
directory listings will be changed to a html listing.  

Comment 93

[Bradley Baetz (:bbaetz)]

Comment on attachment 102876 [details] [diff] [review]
makes file: use the same pref as ftp and gopher

r=bbaetz

We should file a (futured) followup bug to use the same infrastructure though,
I suppose.

Comment 94

[nhottanscp]

I will create a test build with the patch so IQA can do some i18n verification.

Comment 95

[Darin Fisher]

Comment on attachment 102876 [details] [diff] [review]
makes file: use the same pref as ftp and gopher

sr=darin

Comment 96

[sairuh (rarely reading bugmail)]

many thanks to nhotta for fwd'ing info on a test mozilla build. i ran some quick
tests on win2k, and http, ftp and file listings looked fine on win2k --all are
displayed in html. (the file listing looked pretty much like an ftp listing.)

Comment 97

[Doug Turner (:dougt)]

I am going to let 1.2 ship without this fix.  I do not think that the risk is
worth it to flip the switch now.

Comment 98

[Doug Turner (:dougt)]

does anyone require this for 1.2?

Comment 99

[sairuh (rarely reading bugmail)]

bryner / smfr, would this be needed if chimera moves to 1.2? (see bug 159051)
or, could we just pick out this fix and land on the branch (whether it's 1.0 or
1.2 based)?

Comment 100

[Simon Fraser [no longer active]]

chimera can cherry-pick this fix.

Comment 101

[Yuying Long]

Naoki's test build:
Tested on WinXP-SC and WinME-JA, the local file is listing as html format.  If I
set browser default charset matches with OS charset, then the non-ascii path
name will diplay properly which seems like expected behavior. (before this fix,
the non-ascii path name will always display fine with UTF-8 marked, don't need
change the browser default charset).  In this test build, if I set browser
default charset as UTF-8, then non-ascii path names will disappear, but this
rarely happenes in real life though.

There is no change with ftp and http server display by this change.

Comment 102

[nhottanscp]

>before this fix,
>the non-ascii path name will always display fine with UTF-8 marked, don't need
>change the browser default charset
the xul listing uses the system's default but not Unicode (e.g. I cannot show
Japanese file name on my English Windows2000 using the XUL listing).
For local file list, it is better to use the system's default instead of the
browser's default. Or if possible we want to use Unicode file name.
Please file that issue as a separate bug (and assign to nhotta) after the patch
is checked in.

Comment 103

[Simon Fraser [no longer active]]

I checked the patch into chimera's branch. Note that this required fixing bug
177059, which MachO will need for this to work.

Comment 104

[Doug Turner (:dougt)]

checked in.  if regressions occur, please file new bugs.

Comment 105

[Jeremy M. Dolan]

filed bug 179093 - file doesn't link to or load eight bit files

this did work before the checkin.

Comment 106

[Yuying Long]

11-08 trunk build with all platforms, the file list in html mode.

However, there are some problems were introduced:
bug 179140 filed for non-ascii folder/file names can not be display/opened in Mac.
bug 179143 filed for non-ascii folders can not be opened in windows and linux.
bug 179146 filed for use system default charset instead of browser default charset.

I'm marking this bug as verified.

Comment 107

[Bradley Baetz (:bbaetz)]

I did say to test non-ascii stuff...

Comment 108

[nhottanscp]

>bug 179140 filed for non-ascii folder/file names can not be display/opened in Mac.
>bug 179143 filed for non-ascii folders can not be opened in windows and linux.
above two bugs are the same problem caused by bug 169902 recently, now it is
fixed on the trunk

>bug 179146 filed for use system default charset instead of browser 
>default charset.
this is the expected issue (see comment #102)

Comment 109

[benc]

This should be on by default in Mozilla trunk for 1.3.a and later, and I'll be
testing that it behaves correctly.