110760 - directory listings shouldn't rely on libappcomps

Status: VERIFIED FIXED

(Core Graveyard :: Embedding: APIs, defect, P1)

Description

[amutch]

From Bugzilla Helper:

BuildID:    MFCEmbed - 11/19/2001 nightly

When browsing to any FTP site with MFCEmbed, instead of listing the site 
directory listing, a dialog box titled "Choose An Action" appears and indicates 
that "You have chosen to download a file of type: applicaton/http-index-
format". You have the option of "Open Using" and you can select and application 
or "Save this file to a disk".  Neither option will properly open the FTP 
site.  

Reproducible: Always

Expected Results:  MFCEmbed should list the contents of the FTP directory of 
the site that you have opened.

Comment 1

[Bradley Baetz (:bbaetz)]

Do you include  the appcomps lib?

Comment 2

[amutch]

OK, if I add in the appcomps.dll from the latest Mozilla nightly, the FTP 
directory site listing works. However, appcomps.dll is not normally included 
with embedded versions of Mozilla.  Are we going to have to include this file in 
our distributions to properly enable FTP directory listing? Should that be part 
of the "core" files for the embedded version of Mozilla?

Comment 3

[Bradley Baetz (:bbaetz)]

Not as such. The problem is that it can't go into necko, because it involves a
layout interface. I need to slightly refactor some of the code, I think, for
this and a couple of other bugs.

Comment 4

[Adam Lock]

Bradley are you taking this?

To explicitly state the problem, it appears that the directory viewer service
(the thing that displays FTP sites as HTML) lives in
mozilla/xpfe/components/directory and thus gets built into appcomps.dll.
Embedding doesn't want to include this file because it's a monster.
An alternative location might be mozilla/embedding/components but it might be
worth floating the idea on npm.embedding first.

Comment 5

[Adam Lock]

Sorry you already own it so ignore that bit :)

Comment 6

[Bradley Baetz (:bbaetz)]

My plan is to have necko get the pref status, and do the html conversion there
if we need to.

This means that embedders will have to set the pref to use the html view to on
explicitly if they want it to work.

Comment 7

[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 8

[Bradley Baetz (:bbaetz)]

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

Comment 9

[Judson Valeski]

cc'ing alec for more dependency thoughts.

Comment 10

[Alec Flett]

from the FAQ (http://www.mozilla.org/projects/embedding/faq.html#4)
-----
Why won't FTP work?

Make sure you are including the "necko2" DLL and XPT files. If you are able to
download ftp files, but cannot show directories, you may be using the wrong FTP
directory view. There are two different views to display FTP directories. One of
these views is XUL-based and requires the "appcomps" DLL. The other view is
HTML-based and requires only the necko2 DLL. The network.dir.generate_html
preference will make mozilla use HTML mode.

Note: there have been reports that appcomps.dll is still needed, even with HTML
displays... this is a bug that needs to be investigated further. In fact, we
should fallback to HTML if the directory viewer isn't found anyway. -alecf
----

Now, if we need appcomps just to display the HTML listing (not the XUL-based
one) then something is busted here, and it's something that we should be able to
fix without adding extra dependencies, etc. My guess is that it's just some
bogus early-failure. It's just a matter of debugging to find out where the
failure is occuring and handle it gracefully.

Comment 11

[Bradley Baetz (:bbaetz)]

No, the "what type do we use" stuff relies on appcomps, and it can't be moved to
necko w/o making necko depend on appcomps. I know what I have to do, and this
bug is next on my list after I get rid of some regression bugs I have/

Comment 12

[Bradley Baetz (:bbaetz)]

I'm hacking on a patch now, which works for ftp. I'm going to attach it for
comments. Its not complete (it doesn't touch gopher or file yet), but it works
for ftp. Note that I'd love to move the pref stuff into a separate mixin helper
class, but since the protocols are in different shared libs, that would either
require putting the entire class in the (exported) header file or linking
libnecko2.so to libnecko.so explicitly, and I don't imagine that either of those
would be appropriate. Instead, I'm going to duplicate code in three places.
Ain't XPCOM fun.

Note that the actual display of application/http-index-format files sent via a
webserver will still depend on libappcomps. Once bug 123991 is fixed, then I can
probably move that into layout. No promises though, but since thats no different
to how its always been, and I've never seen a webserver which actually serves
those, so I'm not too worried.

Comment 13

[Bradley Baetz (:bbaetz)]

Attachment: v0.9 - for comment

Darin, dougt, valeski, anyone else? Comments?

One thing I didn't mention above - I decided to ddefault to html because thats
the generic format. If you want me to default to the pref, then I need to fix
all the callers, and I'm worried that that may be a large number, and would
impose an additional requirement everywhere - its easier to localise here in
the one place all documents go through.

Comment 14

[Bradley Baetz (:bbaetz)]

darin, dougt - please see my above comments + the patch. Its not ready for
review, but if you hate the design concept, please let me know ;)

Comment 15

[Bradley Baetz (:bbaetz)]

Attachment: v1

Darin didn't have problems with the concept, so here we go. This doesn't change
file:///, becuase of the i18n issues - there's no point in adding the
infrastructure just to only support html.

I've included what is probably a patch to fix the i18n issues in here - after
this is in I'll get someone who can test to check, before making the file:///
changes for this interface in the other bug.

This includes a workarround for a streamconverter crash, and I changed the pref
stuff in the debug pref panel.

The old pref is unsupported - it was debug only anyway, so I don't see that as
an issue. If this goes in, I'll post to .netlib and .embedding describing the
change.

Also, caching the pref in the protocol handlers is not really a win, since they
aren't set up with a static ::get() method like http is, and the code to do
that + set up the observer is more than the code to check the pref each time.

I'd like to get this in today, if possible - can I get some review? I'll also
need the .idl added to the mac build. I'll find someone on irc to do that for
me.

Comment 16

[Darin Fisher]

Comment on attachment 70400 [details] [diff] [review]
v1

sr=darin

Comment 17

[Doug Turner (:dougt)]

Comment on attachment 70400 [details] [diff] [review]
v1

what darin said.

Also get a mac person to help you add your idl so that you get typelibs and
all.

Comment 18

[Bradley Baetz (:bbaetz)]

Checked in. ftp and gopher no longer requite libappcomps. File still does - see
bugs 102812 and 115135.

Comment 19

[Bradley Baetz (:bbaetz)]

And marking fixed, too

Comment 20

[Alec Flett]

awesome!
we need to updating the embedding FAQ then

Comment 21

[Bradley Baetz (:bbaetz)]

Yeah, probably. I'll wait til I have file hacked up. For various reasons that
won't be befor ethe weekend, then someone has to test it, and then we'll see.

Comment 22

[Tracy Walker [:tracy]]

verified fixed on mfcembed 2002-02-20-09-trunk and testgtkembed 
2002-02-20-06-trunk