325177 - Make the unknowndecoder implement nsIContentSniffer

Status: RESOLVED FIXED

(Core :: Networking, defect, P3)

Description

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

...and make basechannel use that interface to get a content type.

this will be necessary once basechannel implements stuff like nsIEncodedChannel, because applyDecoding can only be set during onStartRequest, and the stream converter in the middle has its own concept of onStartRequest.

Comment 1

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

Attachment: patch

had to change nsIInputStreamPump for this... see comments in the patch as for why.

Comment 2

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

Attachment: patch

er, this time, the right file

Comment 3

[Darin Fisher]

Comment on attachment 210101 [details] [diff] [review]
patch

Perhaps nsIInputStreamPump::stream should just return nsIAsyncInputStream.  The comments on nsIInputStreamPump talk about nsIAsyncInputStream, and  that's what mAsyncStream implements, so we might as well reveal that here.

Instead of hard-coding 1024, you might want to use the #define from nsNetSegmentUtils.h (NET_DEFAULT_SEGMENT_SIZE).

Comment 4

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

Attachment: with those changes

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 210284 [details] [diff] [review]
with those changes

>Index: netwerk/base/public/nsIContentSniffer.idl

>  * Content sniffer interface. Components implementing this interface can
>  * determine a MIME type from a chunk of bytes.

Should this mention whether aRequest can be used for the sniffing?

>+   * @param aRequest The request where this data came from. May be null.

Or would that be better to mention here?

In particular, should we talk about whether aData should "take precedence" over aRequest, whatever that means?  Or at least document that it's undefined?

>Index: netwerk/base/public/nsIInputStreamPump.idl

>+    readonly attribute nsIAsyncInputStream stream;

This should probably document that callers are not allowed to actually read data from the stream, right?  Or otherwise change the state of the stream (e.g. seek, setEOF, etc)?  In general, exposing the actual async stream here worries me a little bit; would it be safer to just return a shim object here that would disallow stream state changes on the underlying stream while allowing what you want?  Or are we ok with just documenting this well?

>Index: netwerk/streamconv/converters/nsUnknownDecoder.cpp
>+// nsIStreamListener methods...

You mean nsIContentSniffer methods, right?

sr=bzbarsky with those nits fixed

Comment 6

[Darin Fisher]

Perhaps what we really want is a way to peek at the next chunk of data to be pumped.

Comment 7

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

mmm... we could make the pump implement nsIInputStream maybe?

Comment 8

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

(and implement it in a way that doesn't consume the data)

Comment 9

[Darin Fisher]

Hmm... maybe a better answer is to expose a private interface that is used internally by Necko to communicate this detail.  We could even have the nsBaseChannel QI the nsIInputStreamPump to "nsInputStreamPump" (e.g., make nsInputStreamPump::QueryInterface return a pointer to nsInputStreamPump when aIID equals NS_INPUTSTREAMPUMP_CID).  Then, we could put a friendly method on nsInputStreamPump that reveals the first chunk of data.

  class nsInputStreamPump : ... {
  public:
    ...
    typedef void (*PeekSegmentFun)(void *closure, const char *buf,
                                   PRUint32 bufLen);
    void PeekStream(PeekSegmentFun callback, void *closure);
  };

We could move this construct out onto an actual interface, but I'm not sure that that is really worth it since it would be non-scriptable.

Comment 10

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

hm... should baseChannel then maybe also create the pump using "new nsInputStreamPump" so that it can be sure that that function will be available? (It'd also avoid the qi'ing)

Comment 11

[Darin Fisher]

Yes, that sounds like a fine idea.

Comment 12

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

Attachment: the new approach

Comment 13

[Boris Zbarsky [:bzbarsky]]

So how would someone implementing a channel class (in an extension, say) do something similar?

Comment 14

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

I'd like to fix that by exposing basechannel to extensions and JS.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Sounds like a plan!

Comment 16

[Darin Fisher]

Comment on attachment 211626 [details] [diff] [review]
the new approach

>Index: netwerk/base/src/nsBaseChannel.cpp

> #include "nsIChannelEventSink.h"
> #include "nsIStreamConverterService.h"
> 
>+#include "nsIContentSniffer.h"

nit: please kill the newline here.


>+static void
>+CallTypeSniffers(void *aClosure, const PRUint8 *aData, PRUint32 aCount)
>+{
>+  nsIChannel* chan = NS_STATIC_CAST(nsIChannel*, aClosure);
>+
>+  nsCOMPtr<nsIContentSniffer> sniffer(do_CreateInstance(NS_GENERIC_CONTENT_SNIFFER));

nit: please break lines at 80 chars.

nit: use a consistent style for the "*" ... this code uses "nsIChannel *chan"


>+#ifdef DEBUG_chb
>+  printf("### Detecting content type %s\n", detected.get());
>+#endif

nit: prefer DEBUG_BASECHANNEL instead of user specific debug output.
I would even prefer no #ifdef's scatter through the code.  Maybe just
declare DEBUG_PRINTF as a macro at the top of the file.


>Index: netwerk/base/src/nsInputStreamPump.cpp

>+CallPeekFunc(nsIInputStream *aInStream, void *aClosure,
...
>+}
>+
>+

nit: kill extra newline


r=darin

Comment 17

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

Attachment: nits fixed

I just removed the debug printfs

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 211754 [details] [diff] [review]
nits fixed

>diff --git browser/components/bookmarks/content/bookmarks.js browser/components/bookmarks/content/bookmarks.js

Hey, I'm all cool with you using git, but please do have more context and some equiv of the -p diff option, ok?  ;)

Code itself's ok.  sr=bzbarsky

Comment 19

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

Comment on attachment 211754 [details] [diff] [review]
nits fixed

er, this does use the -p option (which is in fact the default with git)

sorry about the context, will fix.

Comment 20

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

fixed on trunk. leaving open because I want to do a branch version of this, for bug 324985

Comment 21

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

this caused bug 328038

Comment 22

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

Attachment: branch patch

I'm using nsIContentSniffer_MOZILLA_1_8_BRANCH for the interface name, to make it clear that it's branch-only

Comment 23

[Darin Fisher]

> I'm using nsIContentSniffer_MOZILLA_1_8_BRANCH for the interface name, to make
> it clear that it's branch-only

I don't understand.  The interface is exactly the same on the trunk.

Comment 24

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

yeah, true. But I can't name it nsIContentSniffer on branch because it exists there already. I don't really want to name it nsIContentSniffer2 because that'd sound like trunk is behind branch.

Comment 25

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

so yeah, "branch-only" was a bad word to use.

Comment 26

[Darin Fisher]

OK, my apologies for being dense.  I forgot that we were changing this interface.  We should get consensus on naming convention for situations like this.  Can you post a question to a mailing list or at least to a wide enough audience to make sure that everyone is on the same page with this solution?

Comment 27

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

ok, sent to bonecho + m.d.general

Comment 28

[Darin Fisher]

Comment on attachment 212777 [details] [diff] [review]
branch patch

a=darin given that there is precidence for the MOZILLA_1_8_BRANCH interface name suffix.  If your posting results in any changes to this policy, then we'd want to make a blanket change, and that could include changing this post-facto.

Comment 29

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

fixed on branch.