315159 - Enable the Stream Converter graph code

Status: RESOLVED FIXED

(Core :: Networking, enhancement, P5)

Description

[Darin Fisher]

It turns out that we fail to populate the stream converter category because of a bug introduced back in 2002.

Note the computation of g_StreamConverterCount is bogus:
http://lxr.mozilla.org/mozilla/source/netwerk/build/nsNetModule.cpp#308

Moreover, if you take a look at the set of stream converters that we implement, there are hardly any useful "edges" in the would-be graph.

So, I think we should just remove the dead code.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Hmm.  This would basically remove most of the stuff in nsStreamConverterService?  As in, the chaining that it tries to implement just doesn't work and hasn't in a while?

I guess no one's really using this stuff, so perhaps it's just a tad over-engineered... ;)

Comment 2

[Boris Zbarsky [:bzbarsky]]

Although... For extensions or embeddors that implement stream converters and register with the category, the graph code would still work; this typo just affects our built-in list there.

Comment 3

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

I was actually considering making use of this: Removing the explicit stream converter calls in FTP and Gopher directory listings would save some code. (But it'd have downsides - it wouldn't consistently expose an http-index-format type for a directory listing. So that may not be a good idea; and that's why I didn't make a patch for that yet.)

Would trying to save an eml file (in SeaMonkey) as a plain text file trigger this code? hm... that probably just goes through a DOM serializer, so no.

Comment 4

[Darin Fisher]

The basechannel work touches the gopher (and shortly the ftp) code extensively.  In particular, I've made it so that any subclass of nsBaseChannel can simply call a method (PushStreamConverter) to add a stream converter into the listener chain.  It somewhat reduces the code involved for each protocol, which is nice.  I like the fact that protocols generate http-index-format for directory listings.

I think this stream converter graph business is overkill, and I'd rather just remove it.  I think stream converters could be simplified greatly.  In particular, I think that they should not have to implement anything more than nsIInputStream reading from another nsIInputStream, but that's a much more involved change.

Comment 5

[Darin Fisher]

biesi, boris: so do we want to fix and enable the stream converter graph code, or do we want to remove it?

Comment 6

[Boris Zbarsky [:bzbarsky]]

I think I'd prefer fixing it....

Comment 7

[Darin Fisher]

OK

Comment 8

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

yeah, I think I'd prefer that too.

Comment 9

[Darin Fisher]

Attachment: v1 patch

Comment 10

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

Comment on attachment 215849 [details] [diff] [review]
v1 patch

having a unit test for the graph code might be good...

Comment 11

[Darin Fisher]

fixed-on-trunk... yeah, a test case would be nice...