288810 - File listing code should not split output into multiple tables

Status: RESOLVED FIXED

(Core :: Networking, enhancement)

Description

[bugzillaspambox]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7.6) Gecko/20050317 Firefox/1.0.2
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7.6) Gecko/20050317 Firefox/1.0.2

Screenshot: http://www.ontheserver.de/temp/Filelist.png

In long file lists mozilla shows the files in more than 1 table. This moves the
date and the time of the last editing of the files a little around. I guess
that's made that if there is one filename in the list that is really long, that
not the whole table has a huge size.

however that doesn't look very good to me. in my opinion there are 2 ways to
solve this problem:

a) move the date and the time in the first cols
b) display all files in 1 table

I would prefer solution b because if someone names his files like "This is a
letter to my boss, i wrote it on last Saturday, er no it was Sunday, but he
didn't answer jet.doc" then there is nothing else to expect than a wide table :-/

Reproducible: Always

Comment 1

[Tristor]

Attachment: Screenshot from reporter

Comment 2

[Tristor]

Added the reporter's screenshot as an attachment.

Reporter:  Can you possibly whip up a reduced testcase to demonstrate this problem?

Comment 3

[:Gavin Sharp [email: [e-mail]]]

This is defined at:

http://lxr.mozilla.org/mozilla/source/netwerk/streamconv/converters/nsIndexedToHTML.cpp#464

and was done this way intentionally so that layout would be quicker for long
file lists. Table code has since improved, and it may not be necessary anymore.
Bug 85381 is where the change was made.

Bug 103523 may also be of interest, its a bug about reverting Bugzilla's
behavior with long bug lists, and it deals with this same issue.

It's a valid RFE, but it's possible that for the reasons listed in bug 85381, 
that this bug will be marked WONTFIX.

Comment 4

[bugzillaspambox]

@Comment #2: What do you mean by that? Is the explaination text to long?

Comment 5

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #4)
> @Comment #2: What do you mean by that? Is the explaination text to long?

No. Very large tables don't render as quickly as multiple shorter tables. That's
why the decision was made to split the output into multiple tables.

Comment 6

[Tristor]

(In reply to comment #4)
> @Comment #2: What do you mean by that? Is the explaination text to long?

No, what I was wanting is some sort of testcase that would demonstrate the
split, but since Gavin posted a link to the bug that caused this behavior to be
added in the first place and a link to the lxr that shows the code related to
this, I don't think it is necessary any longer.

Comment 7

[bugzillaspambox]

@Comment #5: we are talking about something like 5/1,000 seconds, right?

Comment 8

[Christian :Biesinger (don't email me, ping me on IRC)]

not at all. bug 85381 talks about 2 minutes to render a large directory listing,
and a comment there indicates another directory took > 5 minutes.

Comment 9

[bugzillaspambox]

"minutes"?

sorry I can't confirm that. I rendered it with the splited tables and after it
with only 1 table. both took me 5 seconds. It only got a lot faster ... about 2
seconds for rendering ... when I used the microsoft internet explorer.

so I don't see any speed benefit in using more than one table. perhaps there is
one that you can calculate. but it isn't noticeable by me.

the list consists of about 1,300 entries, so that's quite a long list.

Comment 10

[Tristor]

(In reply to comment #9)

Quoting from comment #3 
"Table code has since improved, and it may not be necessary anymore." 

I believe it may be that it is fast enough with only one table now that it is
neglible in difference, I have yet to test it myself, since I have nothing on
which to test it.

Comment 11

[Olaf Flebbe]

Attachment: Patch to remove the additional </table><table> tags

The code removed seems to be inserted when back in the past the Netscape 3.x
and 4.x table rendering performance was lousy.

I did a short benchmark with mozilla 1.7.11, the diffence in time between
additional table breaks cannot be seen, even for lage directories (~ 2000
Files).

The side effect is that the dir listing looks much better.

Please apply for seamonkey & firefox. Tested on seamonkey 1.7.11; firefox 1.0.6
has the same code.

Comment 12

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 193056 [details] [diff] [review]
Patch to remove the additional </table><table> tags

please patch trunk, not old branches...

and you should also remove the mRowCount variable.

Comment 13

[Olaf Flebbe]

Attachment: Corrected Patch

: Forgot to include header as well.

The cvs update takes a long time.... I will update when it is ready.

Comment 14

[Olaf Flebbe]

O.k. I see, this file is indeed changed very much since 1.7.11. Have to modify
and test it. 

Comment 15

[Olaf Flebbe]

Forgive me asking silly questions: What CVS Tag do I have to use to check out
"Trunk"?

I had no luck compiling the HEAD.

Comment 16

[Olaf Flebbe]

Attachment: Patch relative to HEAD

Tested by me. Will submit installation problems later.

Comment 17

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #15)
> Forgive me asking silly questions: What CVS Tag do I have to use to check out
> "Trunk"?

None (i.e. HEAD)

> I had no luck compiling the HEAD.

hmm, usually HEAD should compile...

Comment 18

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 193146 [details] [diff] [review]
Patch relative to HEAD

test results:
dir with 10,000 entries, linux

without patch: ~45 sec
with patch:    ~47 sec

so essentially identical behaviour.

Comment 19

[Christian :Biesinger (don't email me, ping me on IRC)]

fixed on trunk; if you want this fixed on the branches (1.8, mostly (for firefox
1.5/seamonkey 1.0), please request approval on this patch.

thanks for the patch.
 

Checking in netwerk/streamconv/converters/nsIndexedToHTML.cpp;
/cvsroot/mozilla/netwerk/streamconv/converters/nsIndexedToHTML.cpp,v  <-- 
nsIndexedToHTML.cpp
new revision: 1.69; previous revision: 1.68
done
Checking in netwerk/streamconv/converters/nsIndexedToHTML.h;
/cvsroot/mozilla/netwerk/streamconv/converters/nsIndexedToHTML.h,v  <-- 
nsIndexedToHTML.h
new revision: 1.15; previous revision: 1.14
done