Loading stuff via <object> doesn't do any stream conversions, because it directly calls nsIURIContentListener::DoContent. This includes making nsObjectLoadingContent recognize types only supported by stream converters as valid document types.

Includes a necessary patch for rdf/chrome, so that IsPending works correctly (this is already fixed in chrome/).

I wonder if I should expose this on nsIWebNavigation, and call that...

Comment on attachment 202911 [details] [diff] [review] netwerk/ part >Index: netwerk/streamconv/src/nsStreamConverterService.cpp >+nsStreamConverterService::CanConvert(const char* >+ *_retval = NS_SUCCEEDED(rv) != PR_FALSE; Isn't that the same as: *_retval = NS_SUCCEEDED(rv); ? sr=bzbarsky

Comment on attachment 202912 [details] [diff] [review] uriloader/ part >Index: uriloader/base/nsIURILoader.idl >+ /* @{ */ I assume that indicates to something that these two constants belong together? >+ /** >+ * Loads data from a channel. This differs from openURI in that the channel >+ * may already be opened, and that it returns a stream listener into which the >+ * caller should pump data. I'm not sure I follow this. What does it mean for this to return a stream listener? What will this do if the channel is _not_ opened? Still return a stream listener? What data is the caller expected to pump into the listener this returns? >+ * Note: If the channel already has a loadgroup, it will be replaced with a >+ * different one. Which one? Or can we not document that in a reasonable way? >+ * If the current listener refuses the load immediately Which one is the "current listener"? What does it mean to refuse the load immediately? >, this method will >+ * return NS_ERROR_WONT_HANDLE_CONTENT. If desired, the caller should cancel >+ * the channel (this method won't). s/If desired etc/At that point, the caller should probably cancel the channel if it's already open (this method will not cancel the channel)./ Or something like that, perhaps? >+ * If flags include DONT_RETARGET, but the current listener refuses the load, Again, what is "current listener", and how does "refuses the load" differ from "refuses the load immediately"? >+ * @param aChannel >+ * The channel that should be loaded. The channel may already be >+ * opened. May it also already be closed? eg. a channel that's done loading? >+ * @param aIsContentPreferred aFlags, you mean? >+ * Should the content be displayed in a container that prefers the >+ * content-type, or will any container do. And fix this comment, please? >Index: uriloader/base/nsURILoader.cpp I think the diff to this file would be a lot more readable as a diff -b (or diff -w). Could you post a diff like that, please?

Comment on attachment 202913 [details] [diff] [review] content/base/src part I need to think about this a bit more, but comments off top of my head: 1) We don't really want to change nsIWebNavigation -- we want to freeze it more or less as-is. 2) >@@ -1007,15 +1003,30 @@ nsObjectLoadingContent::IsSupportedDocum >+ if (supported == nsIWebNavigationInfo::UNSUPPORTED) ... >+ rv = convServ->CanConvert(aMimeType.get(), "*/*", &canConvert); >+ return NS_SUCCEEDED(rv) && canConvert; I'm not sure I buy that... Just because we can convert the type to something doesn't mean the something will be a supported document, does it? Consider, eg, the case when the type is application/x-unknown-content-type and the unknown decoder detects it as PDF. If there's something I'm missing, it should probably be documented here with this code... ;)

(In reply to comment #5) > Isn't that the same as: > > *_retval = NS_SUCCEEDED(rv); Hm, yes, looks like it is for NS_SUCCEEDED (not for NS_FAILED though) (In reply to comment #6) > I assume that indicates to something that these two constants belong together? Yeah, doxygen will group them together with a "Flags for opening URIs." header. > What data is the caller expected to pump into the listener > this returns? Well, the point is that this stream listener will receive the data from the channel. I wanted to allow both doing something like: var chan = newChannel(...); var listener = uriLoader.openChannel(chan); return chan.asyncOpen(listener, nsnull); (which openURI now does) And the other approach, calling it inside a listener's onStartRequest method. I should probably clarify the comment. > Which one? Or can we not document that in a reasonable way? does anyone care which one it is? :-) > Which one is the "current listener"? The one from aWindowContext... I should document this better > What does it mean to refuse the load immediately? You mean I should document that this calls onStartURIOpen? >Again, what is "current listener", and how does "refuses the load" differ from >"refuses the load immediately"? It differs in that the caller only refuses it in onStartRequest :-) (canHandleContent etc)... hm.. probably needs rewording too. > May it also already be closed? eg. a channel that's done loading? ohh, that's an interesting question. I'm pretty sure the impl would screw this up, with that isPending check. so, no :-) (In reply to comment #7) > 1) We don't really want to change nsIWebNavigation -- we want to freeze it > more or less as-is. Yes, I meant as an nsIWebNavigation2 method or so. > I'm not sure I buy that... Just because we can convert the type to something > doesn't mean the something will be a supported document, does it? Consider, > eg, the case when the type is application/x-unknown-content-type and the > unknown decoder detects it as PDF. Yeah, hm, but we can only detect that in onStartRequest. At that point, uriloader will call canHandleContent, which fails, uriloader returns NS_ERROR_WONT_HANDLE_CONTENT, which means onStartRequest fails, which means we fall back. So I think we're ok here. I should probably test this.

> I should probably clarify the comment. Yes, exactly. > does anyone care which one it is? :-) I do, if we're putting this in an api comment. Why bother mentioning it at all otherwise? > The one from aWindowContext... I should document this better Yes, please. > You mean I should document that this calls onStartURIOpen? For example, yes. :) But seriously, I didn't have any idea what that comment was talking about. Note that "listener" in this comment can mean either nsIStreamListener or nsIURIContentListener depending on the use; it would be good to disambiguate... > probably needs rewording too. Right. > Yeah, hm, but we can only detect that in onStartRequest. At that point, > uriloader will call canHandleContent, which fails, uriloader returns > NS_ERROR_WONT_HANDLE_CONTENT, which means onStartRequest fails, which means we > fall back. So I think we're ok here. I should probably test this. Please. And document.

> Why bother mentioning it at all otherwise? The reason I mentioned is that the caller might assume that the loadgroup he set might be the one that's used. I wanted to make it clear that it will get replaced.

OK. But then I think it's worth saying what it gets replaced with...

Comment on attachment 203663 [details] [diff] [review] netwerk/ part, v2 r=darin

Comment on attachment 203672 [details] [diff] [review] uriloader/ part, v2 I'll sr this after boris has finished his review of it.

Comment on attachment 203672 [details] [diff] [review] uriloader/ part, v2 Thanks for the comment improvements! r=bzbarsky

Comment on attachment 202913 [details] [diff] [review] content/base/src part >Index: content/base/src/nsObjectLoadingContent.cpp >+ rv = uriLoader->OpenChannel(chan, nsIURILoader::IS_CONTENT_PREFERRED | >+ nsIURILoader::DONT_RETARGET, req, Why nsIURILoader::IS_CONTENT_PREFERRED ? That should only be set for link clicks, no? That's certainly what docshell does it passes mLoadType == LOAD_LINK for the aIsContentPreferred arg of nsIURILoader::LoadURI. r+sr=bzbarsky with that fixed.

Comment on attachment 203673 [details] [diff] [review] uriloader/ part, v2, as diff -b sr=darin

hm... I did use to pass true to the isContentPreferred argument: - rv = listener->DoContent(mContentType.get(), PR_TRUE, aRequest, I guess that was wrong. I'll remove that flag before checking in. (note to self: still need to do aforementioned testing)

Testcase for a stream converter that converts to something unsupported: data:text/html,<object%20data="data:application/x-unknown-content-type,FAIL%2500">PASS</object> (Verification that this does what one would expect: data:text/html,<object%20data="data:application/x-unknown-content-type,Text%20content">Alternate</object> ) Works fine with the patch: The first one shows "PASS", the second one "Text content" (as expected)

netwerk part: Checking in netwerk/streamconv/public/nsIStreamConverterService.idl; /cvsroot/mozilla/netwerk/streamconv/public/nsIStreamConverterService.idl,v <-- nsIStreamConverterService.idl new revision: 1.11; previous revision: 1.10 done Checking in netwerk/streamconv/src/nsStreamConverterService.cpp; /cvsroot/mozilla/netwerk/streamconv/src/nsStreamConverterService.cpp,v <-- nsStreamConverterService.cpp new revision: 1.67; previous revision: 1.66 done uriloader part: Checking in rdf/chrome/src/nsChromeProtocolHandler.cpp; /cvsroot/mozilla/rdf/chrome/src/nsChromeProtocolHandler.cpp,v <-- nsChromeProtocolHandler.cpp new revision: 1.108; previous revision: 1.107 done Checking in uriloader/base/nsIURILoader.idl; /cvsroot/mozilla/uriloader/base/nsIURILoader.idl,v <-- nsIURILoader.idl new revision: 1.34; previous revision: 1.33 done Checking in uriloader/base/nsURILoader.cpp; /cvsroot/mozilla/uriloader/base/nsURILoader.cpp,v <-- nsURILoader.cpp new revision: 1.138; previous revision: 1.137 done

the comment change in v2 was wrong... this one is right (also has the correct diff parameters)

content part: Checking in content/base/src/Makefile.in; /cvsroot/mozilla/content/base/src/Makefile.in,v <-- Makefile.in new revision: 1.84; previous revision: 1.83 done Checking in content/base/src/nsObjectLoadingContent.cpp; /cvsroot/mozilla/content/base/src/nsObjectLoadingContent.cpp,v <-- nsObjectLoadingContent.cpp new revision: 1.18; previous revision: 1.17 done

this seems interesting functionality. what it is supposed the eml testcase to do (i get FAIL on latest trunk)?

requires seamonkey with mailnews. you should see the mail message in the <object>.

I think this fix caused bug 320732 - perhaps mailnews is doing something wrong...investigating